Add parameter types to function names (DWARF)

[jimb]

This is great stuff --- thanks for doing this!

I made some comments, but I understand this is in-progress; don't let them break
your stride.

I didn't see anything to deal with forward references to type DIEs in general;
did I miss it?

http://breakpad.appspot.com/67001/diff/1/3
File common/linux/dwarf_cu_to_module.cc (right):

http://breakpad.appspot.com/67001/diff/1/3#newcode108
Line 108: struct DwarfCUToModule::TypeInfo {
The fact that this structure has a 'tag' member, and contains members that will
be unused except for certain kinds of types, and that GetName is a big switch on
the tag, suggests that you might want an abstract base class and per-type
subclasses, with GetName being virtual.

The Google C++ style guide admonishes against doing what you have done here: "Do
not hand-implement an RTTI-like workaround. The arguments against RTTI apply
just as much to workarounds like class hierarchies with type tags."

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Run-Ti...

http://breakpad.appspot.com/67001/diff/1/3#newcode192
Line 192: case dwarf2reader::DW_TAG_volatile_type: qualifier = "volatile";
break;
Oh, so this is pulling qualifiers off 'this' and putting them after the
paremeter list. Isn't C++ wonderful.

Note that 'this' can be both const- and volatile-qualified.

http://breakpad.appspot.com/67001/diff/1/3#newcode216
Line 216: struct DwarfCUToModule::AbstractOrigin {
I didn't even catch that AbstractOrigin wasn't nested within DwarfCUToModule
when I first reviewed the patch.  This is better.

http://breakpad.appspot.com/67001/diff/1/3#newcode315
Line 315: // record_declaration is true, record its enclosing name and
unqualified
record_declaration doesn't seem to be present any more.

http://breakpad.appspot.com/67001/diff/1/3#newcode541
Line 541: containing_type_ = &cu_context_->types[data];
Missing 'break'?

http://breakpad.appspot.com/67001/diff/1/3#newcode583
Line 583: case dwarf2reader::DW_TAG_union_type:
We shouldn't be repeating this list of type tags in each FindChildHandler that
might be interested in type children.  It would probably be best to have a
handler factory function, and call it from the 'default:' cases where
appropriate.

http://breakpad.appspot.com/67001/diff/1/3#newcode756
Line 756: // additional const qualifier to the parameters for the acutal
functions.
"actual"

http://breakpad.appspot.com/67001/diff/1/4
File common/linux/dwarf_cu_to_module.h (right):

http://breakpad.appspot.com/67001/diff/1/4#newcode206
Line 206: struct ParameterInfo;
Nit: It would be nice to avoid type names ending in "Info"; it doesn't convey
much. Just "Type" and "Parameter" would probably be fine.

[jimb]

I didn't even look at the unit tests...

http://breakpad.appspot.com/67001/diff/1/2
File common/linux/dwarf_cu_to_module_unittest.cc (right):

http://breakpad.appspot.com/67001/diff/1/2#newcode1040
Line 1040: ProcessCU(
This is very beautiful.

[jimb]

Some initial thoughts.

I gather that the client for most of those non-intuitive member functions in
Type is the GCC pointer-to-member-function kludge; I'm trying to think of a
better way to do that. I really don't like having Type with all these extraneous
methods, but pattern-matching really doesn't play well with opaque,
"object-oriented" type representations... pattern matching is all about writing
a single pattern that can look inside whatever it encounters.

Also... is there any reason we're having this conversation in private?  It seems
to me like it's exactly the sort of thing google-breakpad-dev is meant for.

http://breakpad.appspot.com/67001/diff/9001/10002
File common/linux/dwarf_cu_to_module.cc (right):

http://breakpad.appspot.com/67001/diff/9001/10002#newcode119
Line 119: struct DwarfCUToModule::Signature : public DIEContext {
I wonder, since we need to treat 'this' specially in so many ways, would it
simplify things to have a separate 'this_type' member here, instead of placing
it first in the parameter list?  If it's not a net simplification, then it's not
worth it...

http://breakpad.appspot.com/67001/diff/9001/10002#newcode130
Line 130: virtual string GetName(const string& modifiers) const { return ""; }
Every concrete class should override this, right?  Shouldn't we use "= 0" here,
then?  What about the others?

I see GetName(modifiers) more as:

// Return the source code form of a declaration giving
// DECLARATOR the type represented by this.
string FormatDeclaration(const string &declarator);

http://breakpad.appspot.com/67001/diff/9001/10002#newcode207
Line 207: Type(), modifier_(modifier), type_(type) {}
I kind of like to omit things like Type(); I only include them for types that
wouldn't be initialized otherwise, like pointers: it's nicer to write
pointer_member() than pointer_member(NULL).

http://breakpad.appspot.com/67001/diff/9001/10002#newcode257
Line 257: class DwarfCUToModule::FunctionType : public Type {
Would it work for FunctionType to have a member function like this?

// Return the source code form of a declarator (not a
// declaration) specifying that DECLARATOR is a function
// taking the same argument types as this function type.
// 
// As C++ requires, if this function type is a member
// function (i.e. its first argument is an artificial
// 'this', whose type is a qualified pointer to some
// class type), the declarator includes the 'this'
// qualifiers after the parameter declaration clause.
//
// This is the same as FormatDeclaration, except that it
// omits the return types. Thus, if DECLARATOR is the
// function's name, this returns the function's name
// with its argument types, identifying a particular
// overloading of the name as users expect to see it.
//
// For example, if this FunctionType represents the type
// 'function accepting a char * and returning an int',
// then this->FormatDeclarator("foo") is "foo(char *)".
string FormatDeclarationNoReturnType(const string &declarator);

I *think* this could then be used as a subroutine for
FormatDeclaration.  Then, we could delete FormatFunction, which definitely seems
out of place.

[jimb]

I found some problems in what I'd written.

Also --- since all this is language-specific, it really belongs in
google_breakpad::Language. That should probably have a bunch of factories for
type nodes. But we can leave that for a subsequent patch; no need to take care
of it now, in my opinion.

http://breakpad.appspot.com/67001/diff/9001/10002
File common/linux/dwarf_cu_to_module.cc (right):

http://breakpad.appspot.com/67001/diff/9001/10002#newcode257
Line 257: class DwarfCUToModule::FunctionType : public Type {
I made some mistakes here:

On 2010/03/13 20:03:47, jimb wrote:
> // omits the return types. Thus, if DECLARATOR is the

"return type"

> string FormatDeclarationNoReturnType(const string &declarator);

This should be called FormatDeclarator, not FormatDeclarationNoReturnType. 
(Figuring out how to relate the functionality we need to the language spec was
the whole point, so it'd be foolish not to use the spec language.)

[dmuir]

On 2010/03/14 04:37:32, jimb wrote:
> I found some problems in what I'd written.
> 
> Also --- since all this is language-specific, it really belongs in
> google_breakpad::Language. That should probably have a bunch of factories for
> type nodes. But we can leave that for a subsequent patch; no need to take care
> of it now, in my opinion.
> 
> http://breakpad.appspot.com/67001/diff/9001/10002
> File common/linux/dwarf_cu_to_module.cc (right):
> 
> http://breakpad.appspot.com/67001/diff/9001/10002#newcode257
> Line 257: class DwarfCUToModule::FunctionType : public Type {
> I made some mistakes here:
> 
> On 2010/03/13 20:03:47, jimb wrote:
> > // omits the return types. Thus, if DECLARATOR is the
> 
> "return type"
> 
> > string FormatDeclarationNoReturnType(const string &declarator);
> 
> This should be called FormatDeclarator, not FormatDeclarationNoReturnType. 
> (Figuring out how to relate the functionality we need to the language spec was
> the whole point, so it'd be foolish not to use the spec language.)

Incorporated some more changes.  Let me know what else you'd like to see.

[jimb]

Here are some initial comments; there's still a lot of the code I don't
understand yet.

The DIEHandler arrangement doesn't really lead to the most legible code, does
it?  I have a hard time reading it, at least, which is not a good sign.

http://breakpad.appspot.com/67001/diff/14001/15002
File common/linux/dwarf_cu_to_module.cc (right):

http://breakpad.appspot.com/67001/diff/14001/15002#newcode63
Line 63: T* operator->() const { return *referent_; }
In the rest of this code I've been using T *foo, not T* foo. Similarly for &.

http://breakpad.appspot.com/67001/diff/14001/15002#newcode146
Line 146: virtual string FormatDeclaration(const string& declarator) const = 0;
You didn't like my nice long comments for these that I put in the other
reviews?!?  *snif*

I kind of think FormatDeclarator should be = 0 in the base class.  The
definition here is only correct for named types.

http://breakpad.appspot.com/67001/diff/14001/15002#newcode169
Line 169: NamedType(const string& name) : Type(), base_name_(name) {}
Please leave out BASECLASS() and member() when the initialization would happen
anyway.

http://breakpad.appspot.com/67001/diff/14001/15002#newcode191
Line 191: 
I think this class should override FormatDeclarator to affix the modifier:

string FormatDeclarator(const string &declarator) const {
  return modifier_ + declarator;
}

and FormatDeclaration should simply be:

return type_->FormatDeclaration(FormatDeclarator(declarator));

In fact, I think that latter should be the definition in the Type base class,
and FormatDeclarator should be = 0.

http://breakpad.appspot.com/67001/diff/14001/15002#newcode235
Line 235: ArrayType(const Type::Ref& type) : ModifiedType("[]", type) {}
Hm?  There's no way ArrayType (postfix []) and ReferenceType (prefix &) can
differ only in the modifier string used. Doesn't ArrayType need its own
definition of FormatDeclarator?

http://breakpad.appspot.com/67001/diff/14001/15002#newcode262
Line 262: // examining the this argument.
Well, it's not the function that is qualified, per se.  The comment should read
something like:

// If this is a member function, find the qualifiers on
// its 'this' argument, to include after the parameter list.

Also, let's move all the computation of qualifiers down after the '+= ")"',
where it'll be used.  We won't need the variable 'qualifiers'; we'll just append
to 'out' in the 'then' clause.

http://breakpad.appspot.com/67001/diff/14001/15002#newcode283
Line 283: }
indentation

http://breakpad.appspot.com/67001/diff/14001/15002#newcode301
Line 301: string FormatDeclaration(const string& declarator) const {
This, too, should inherit FormatDeclaration from Type and override
FormatDeclarator.

http://breakpad.appspot.com/67001/diff/14001/15002#newcode574
Line 574: // Type, Namespaces, and Functions have a cyclic dependency, so the
I think you mean: "ParameterHandler, FuncHandler, NamedScopeHandler, and the
various FooTypeHandlers"

http://breakpad.appspot.com/67001/diff/14001/15002#newcode685
Line 685: {
I don't think we should have braces around this.

http://breakpad.appspot.com/67001/diff/14001/15002#newcode1073
Line 1073: assert(spec != specifications->end());
If we know the entry exists, why not just declare spec to be Specification *,
initialized to &specifications[offset_]?

http://breakpad.appspot.com/67001/diff/14001/15002#newcode1079
Line 1079: // additional const qualifier to the parameters for the actutal
functions.
"actual"

http://breakpad.appspot.com/67001/diff/14001/15002#newcode1590
Line 1590: }
Would it be good to do cu_context_->types.clear() here, for good measure?

[dmuir]

Thought I'd cc'd you on my last revisions, but now I'm not so sure.  Have you
had a chance to look over the latest yet?


On 2010/03/31 00:03:38, jimb wrote:
> Here are some initial comments; there's still a lot of the code I don't
> understand yet.
> 
> The DIEHandler arrangement doesn't really lead to the most legible code, does
> it?  I have a hard time reading it, at least, which is not a good sign.
> 
> http://breakpad.appspot.com/67001/diff/14001/15002
> File common/linux/dwarf_cu_to_module.cc (right):
> 
> http://breakpad.appspot.com/67001/diff/14001/15002#newcode63
> Line 63: T* operator->() const { return *referent_; }
> In the rest of this code I've been using T *foo, not T* foo. Similarly for &.
> 
> http://breakpad.appspot.com/67001/diff/14001/15002#newcode146
> Line 146: virtual string FormatDeclaration(const string& declarator) const =
0;
> You didn't like my nice long comments for these that I put in the other
> reviews?!?  *snif*
> 
> I kind of think FormatDeclarator should be = 0 in the base class.  The
> definition here is only correct for named types.
> 
> http://breakpad.appspot.com/67001/diff/14001/15002#newcode169
> Line 169: NamedType(const string& name) : Type(), base_name_(name) {}
> Please leave out BASECLASS() and member() when the initialization would happen
> anyway.
> 
> http://breakpad.appspot.com/67001/diff/14001/15002#newcode191
> Line 191: 
> I think this class should override FormatDeclarator to affix the modifier:
> 
> string FormatDeclarator(const string &declarator) const {
>   return modifier_ + declarator;
> }
> 
> and FormatDeclaration should simply be:
> 
> return type_->FormatDeclaration(FormatDeclarator(declarator));
> 
> In fact, I think that latter should be the definition in the Type base class,
> and FormatDeclarator should be = 0.
> 
> http://breakpad.appspot.com/67001/diff/14001/15002#newcode235
> Line 235: ArrayType(const Type::Ref& type) : ModifiedType("[]", type) {}
> Hm?  There's no way ArrayType (postfix []) and ReferenceType (prefix &) can
> differ only in the modifier string used. Doesn't ArrayType need its own
> definition of FormatDeclarator?
> 
> http://breakpad.appspot.com/67001/diff/14001/15002#newcode262
> Line 262: // examining the this argument.
> Well, it's not the function that is qualified, per se.  The comment should
read
> something like:
> 
> // If this is a member function, find the qualifiers on
> // its 'this' argument, to include after the parameter list.
> 
> Also, let's move all the computation of qualifiers down after the '+= ")"',
> where it'll be used.  We won't need the variable 'qualifiers'; we'll just
append
> to 'out' in the 'then' clause.
> 
> http://breakpad.appspot.com/67001/diff/14001/15002#newcode283
> Line 283: }
> indentation
> 
> http://breakpad.appspot.com/67001/diff/14001/15002#newcode301
> Line 301: string FormatDeclaration(const string& declarator) const {
> This, too, should inherit FormatDeclaration from Type and override
> FormatDeclarator.
> 
> http://breakpad.appspot.com/67001/diff/14001/15002#newcode574
> Line 574: // Type, Namespaces, and Functions have a cyclic dependency, so the
> I think you mean: "ParameterHandler, FuncHandler, NamedScopeHandler, and the
> various FooTypeHandlers"
> 
> http://breakpad.appspot.com/67001/diff/14001/15002#newcode685
> Line 685: {
> I don't think we should have braces around this.
> 
> http://breakpad.appspot.com/67001/diff/14001/15002#newcode1073
> Line 1073: assert(spec != specifications->end());
> If we know the entry exists, why not just declare spec to be Specification *,
> initialized to &specifications[offset_]?
> 
> http://breakpad.appspot.com/67001/diff/14001/15002#newcode1079
> Line 1079: // additional const qualifier to the parameters for the actutal
> functions.
> "actual"
> 
> http://breakpad.appspot.com/67001/diff/14001/15002#newcode1590
> Line 1590: }
> Would it be good to do cu_context_->types.clear() here, for good measure?