Add a base class for PDB symbols.

Add a base class for PDB symbols.

Every PDB symbol has a type id and can be written to the
symbol record stream.

Types of symbols are defined in the Microsoft_Cci_Pdb::SYM enum.

BUG=
R=chrisha@chromium.org

Committed: https://code.google.com/p/sawbuck/source/detail?r=2223

[fdoray, 2014-07-31 18:19:43]

Could you review this CL?
Thanks.

[chrisha, 2014-07-31 18:41:21]

https://codereview.appspot.com/116500043/diff/40001/syzygy/experimental/pdb_w...
File syzygy/experimental/pdb_writer/pdb_symbol.cc (right):

https://codereview.appspot.com/116500043/diff/40001/syzygy/experimental/pdb_w...
syzygy/experimental/pdb_writer/pdb_symbol.cc:15: #include
"syzygy/experimental/pdb_writer/pdb_symbol.h"
Just call these files symbol.*? They define a class Symbol, after all, and not
PdbSymbol.

https://codereview.appspot.com/116500043/diff/40001/syzygy/experimental/pdb_w...
File syzygy/experimental/pdb_writer/pdb_symbol.h (right):

https://codereview.appspot.com/116500043/diff/40001/syzygy/experimental/pdb_w...
syzygy/experimental/pdb_writer/pdb_symbol.h:24: class Symbol {
Your mixing interface and implementation here.

class Symbol is likely best as a pure virtual base class, defining the interface
that symbols must have. Leave it to the derived symbol to deal with how things
are implemented.

Common things (like implementing GetType()) can be handled with 'impl' classes,
which can be stacked:

class SymbolInterface {
 public:
  virtual uint32 GetType() const = 0;
  virtual bool WritePayload(WritablePdbStream* stream) const = 0;
};

class TypedSymbolImpl : public Symbol {
 public:
  virtual uint32 GetType() OVERRIDE { return type_; }
 protected:
   uint32 type_;
};

Then things like ImageSymbol would derive from TypedSymbolImpl.

Follow my general meaning?

https://codereview.appspot.com/116500043/diff/40001/syzygy/experimental/pdb_w...
syzygy/experimental/pdb_writer/pdb_symbol.h:26: explicit Symbol(unsigned int
type);
I'd prefer use of uint16 or uint32, for consistency. In your IDA plugin you can
always make a facade or wrapper for the functionality you need...

[fdoray, 2014-07-31 20:09:57]

Could you take another look?
Thanks.

https://codereview.appspot.com/116500043/diff/40001/syzygy/experimental/pdb_w...
File syzygy/experimental/pdb_writer/pdb_symbol.cc (right):

https://codereview.appspot.com/116500043/diff/40001/syzygy/experimental/pdb_w...
syzygy/experimental/pdb_writer/pdb_symbol.cc:15: #include
"syzygy/experimental/pdb_writer/pdb_symbol.h"
On 2014/07/31 18:41:20, chrisha wrote:
> Just call these files symbol.*? They define a class Symbol, after all, and not
> PdbSymbol.

Done.

https://codereview.appspot.com/116500043/diff/40001/syzygy/experimental/pdb_w...
File syzygy/experimental/pdb_writer/pdb_symbol.h (right):

https://codereview.appspot.com/116500043/diff/40001/syzygy/experimental/pdb_w...
syzygy/experimental/pdb_writer/pdb_symbol.h:24: class Symbol {
On 2014/07/31 18:41:20, chrisha wrote:
> Your mixing interface and implementation here.
> 
> class Symbol is likely best as a pure virtual base class, defining the
interface
> that symbols must have. Leave it to the derived symbol to deal with how things
> are implemented.
> 
> Common things (like implementing GetType()) can be handled with 'impl'
classes,
> which can be stacked:
> 
> class SymbolInterface {
>  public:
>   virtual uint32 GetType() const = 0;
>   virtual bool WritePayload(WritablePdbStream* stream) const = 0;
> };
> 
> class TypedSymbolImpl : public Symbol {
>  public:
>   virtual uint32 GetType() OVERRIDE { return type_; }
>  protected:
>    uint32 type_;
> };
> 
> Then things like ImageSymbol would derive from TypedSymbolImpl.
> 
> Follow my general meaning?

Done.

Writing the header is a behavior common to all symbol records. Is the new way
that I've implemented this common behavior correct?

https://codereview.appspot.com/116500043/diff/40001/syzygy/experimental/pdb_w...
syzygy/experimental/pdb_writer/pdb_symbol.h:26: explicit Symbol(unsigned int
type);
On 2014/07/31 18:41:20, chrisha wrote:
> I'd prefer use of uint16 or uint32, for consistency. In your IDA plugin you
can
> always make a facade or wrapper for the functionality you need...

Done.

[Sébastien Marchand, 2014-08-04 14:37:59]

A few nits.

https://codereview.appspot.com/116500043/diff/100001/syzygy/experimental/pdb_...
File syzygy/experimental/pdb_writer/symbol.cc (right):

https://codereview.appspot.com/116500043/diff/100001/syzygy/experimental/pdb_...
syzygy/experimental/pdb_writer/symbol.cc:41: if (!stream->Align(4))
could this 4 be replaced by sizeof(something) or is it just 4-byte aligned ?

https://codereview.appspot.com/116500043/diff/100001/syzygy/experimental/pdb_...
syzygy/experimental/pdb_writer/symbol.cc:58:
TypedSymbolImpl::TypedSymbolImpl(uint16 type) : type_(type) {}
Add a space between the brackets.

https://codereview.appspot.com/116500043/diff/100001/syzygy/experimental/pdb_...
File syzygy/experimental/pdb_writer/symbol.h (right):

https://codereview.appspot.com/116500043/diff/100001/syzygy/experimental/pdb_...
syzygy/experimental/pdb_writer/symbol.h:29: virtual ~Symbol() {}
Add a space between the brackets.

https://codereview.appspot.com/116500043/diff/100001/syzygy/experimental/pdb_...
syzygy/experimental/pdb_writer/symbol.h:46: virtual bool
Write(WritablePdbStream* stream) const OVERRIDE;
We usually use doxygen style for these functions.

  // @name Symbol functions.
  // @{
  virtual bool Write();
  // @}

https://codereview.appspot.com/116500043/diff/100001/syzygy/experimental/pdb_...
syzygy/experimental/pdb_writer/symbol.h:62: virtual uint16 GetType() const
OVERRIDE;
ditto.

[fdoray, 2014-08-04 14:52:50]

All done.
Could you take another look?

https://codereview.appspot.com/116500043/diff/100001/syzygy/experimental/pdb_...
File syzygy/experimental/pdb_writer/symbol.cc (right):

https://codereview.appspot.com/116500043/diff/100001/syzygy/experimental/pdb_...
syzygy/experimental/pdb_writer/symbol.cc:41: if (!stream->Align(4))
On 2014/08/04 14:37:58, Sébastien Marchand wrote:
> could this 4 be replaced by sizeof(something) or is it just 4-byte aligned ?

Done.

https://codereview.appspot.com/116500043/diff/100001/syzygy/experimental/pdb_...
syzygy/experimental/pdb_writer/symbol.cc:58:
TypedSymbolImpl::TypedSymbolImpl(uint16 type) : type_(type) {}
On 2014/08/04 14:37:59, Sébastien Marchand wrote:
> Add a space between the brackets.

Done.

https://codereview.appspot.com/116500043/diff/100001/syzygy/experimental/pdb_...
File syzygy/experimental/pdb_writer/symbol.h (right):

https://codereview.appspot.com/116500043/diff/100001/syzygy/experimental/pdb_...
syzygy/experimental/pdb_writer/symbol.h:29: virtual ~Symbol() {}
On 2014/08/04 14:37:59, Sébastien Marchand wrote:
> Add a space between the brackets.

Done.

https://codereview.appspot.com/116500043/diff/100001/syzygy/experimental/pdb_...
syzygy/experimental/pdb_writer/symbol.h:46: virtual bool
Write(WritablePdbStream* stream) const OVERRIDE;
On 2014/08/04 14:37:59, Sébastien Marchand wrote:
> We usually use doxygen style for these functions.
> 
>   // @name Symbol functions.
>   // @{
>   virtual bool Write();
>   // @}

Done.

https://codereview.appspot.com/116500043/diff/100001/syzygy/experimental/pdb_...
syzygy/experimental/pdb_writer/symbol.h:62: virtual uint16 GetType() const
OVERRIDE;
On 2014/08/04 14:37:59, Sébastien Marchand wrote:
> ditto.

Done.

[chrisha, 2014-08-04 15:24:44]

lgtm with nits

https://codereview.appspot.com/116500043/diff/120001/syzygy/experimental/pdb_...
File syzygy/experimental/pdb_writer/symbol.h (right):

https://codereview.appspot.com/116500043/diff/120001/syzygy/experimental/pdb_...
syzygy/experimental/pdb_writer/symbol.h:42: // the header.
Note that this is a partial implementation, and leaves Symbol::GetType()
unimplemented.

https://codereview.appspot.com/116500043/diff/120001/syzygy/experimental/pdb_...
syzygy/experimental/pdb_writer/symbol.h:58: // Implements a symbol with a type
that is specified at construction time.
Again note that this is a partial implementation, and leaves
SymbolBaseImpl::WritePayload unimplemented.

[fdoray, 2014-08-04 17:20:03]

https://codereview.appspot.com/116500043/diff/120001/syzygy/experimental/pdb_...
File syzygy/experimental/pdb_writer/symbol.h (right):

https://codereview.appspot.com/116500043/diff/120001/syzygy/experimental/pdb_...
syzygy/experimental/pdb_writer/symbol.h:42: // the header.
On 2014/08/04 15:24:44, chrisha wrote:
> Note that this is a partial implementation, and leaves Symbol::GetType()
> unimplemented.

Done.

https://codereview.appspot.com/116500043/diff/120001/syzygy/experimental/pdb_...
syzygy/experimental/pdb_writer/symbol.h:58: // Implements a symbol with a type
that is specified at construction time.
On 2014/08/04 15:24:44, chrisha wrote:
> Again note that this is a partial implementation, and leaves
> SymbolBaseImpl::WritePayload unimplemented.

Done.

[fdoray, 2014-08-04 17:20:54]

Committed patchset #9 manually as 2223.