Record declaration names when they have a DW_AT_MIPS_linkage_name

Record declaration names when they have a DW_AT_MIPS_linkage_name

[jimb]

I think this patch needs to have some unit tests. Without them I'm not sure what
problem it addresses; are there cases where the specification has a linkage
name, but the definition does not? Or is it that something appears in an
enclosing context that has a linkage name? (Either one is surprising to me.) A
test would make it clear the situation we're trying to cope with.

It seems to me that passing the demangled name as the second argument to
MakeQualifiedName is not correct, as that argument should be an unqualified
name, whereas demangled names are always fully qualified. In general, storing a
qualified name in something called "unqualified_name", as this patch does with
the local variable and the Specification member, seems like bad news.

[rafael.espindola]

On 2012/10/05 18:15:58, jimb wrote:
> I think this patch needs to have some unit tests. Without them I'm not sure
what
> problem it addresses; are there cases where the specification has a linkage
> name, but the definition does not? Or is it that something appears in an
> enclosing context that has a linkage name? (Either one is surprising to me.) A
> test would make it clear the situation we're trying to cope with.

Yes, sorry. I had discussed this with ted. He asked for you review and I forgot
to provide some context. The problem is that declarations don't get marked as
such and when we get a definition we end up with the warning:

libxul.so: the DIE at offset 0x293ef30 has a DW_AT_specification attribute
referring to the die at offset 0x2918a81, which either was not marked as a
declaration, or comes later in the file

I asked Ted how to add a test showing that no warning was printed, but he was
not sure.

> It seems to me that passing the demangled name as the second argument to
> MakeQualifiedName is not correct, as that argument should be an unqualified
> name, whereas demangled names are always fully qualified. In general, storing
a
> qualified name in something called "unqualified_name", as this patch does with
> the local variable and the Specification member, seems like bad news.

What do you think should be done instead? I can see some options:

* Split on the last "top level" ::
* Add a flag to Specification (make it a tagged union?) saying that it has a
full name already.
* Try to only store the full name in the Specification. I have not actually
tried this one yet to see if there is some user that really needs just the
context or just the plain name.

[jimb]

On 2012/10/05 18:24:04, rafael.espindola wrote:
> I asked Ted how to add a test showing that no warning was printed, but he was
> not sure.

The tests should fail if an unexpected warning gets printed. In
dwarf_cu_to_module_unittest.cc, CUFixtureBase's constructor sets up default
expectations for reporter_: that no warnings will be issued. These are the
EXPECT_CALL(reporter_, ...) forms.

Is that not working? Are tests printing warnings but passing anyway?

[jimb]

On 2012/10/05 18:24:04, rafael.espindola wrote:
> On 2012/10/05 18:15:58, jimb wrote:
> > I think this patch needs to have some unit tests. Without them I'm not sure
> what
> > problem it addresses; are there cases where the specification has a linkage
> > name, but the definition does not? Or is it that something appears in an
> > enclosing context that has a linkage name? (Either one is surprising to me.)
A
> > test would make it clear the situation we're trying to cope with.
> 
> Yes, sorry. I had discussed this with ted. He asked for you review and I
forgot
> to provide some context. The problem is that declarations don't get marked as
> such and when we get a definition we end up with the warning:
> 
> libxul.so: the DIE at offset 0x293ef30 has a DW_AT_specification attribute
> referring to the die at offset 0x2918a81, which either was not marked as a
> declaration, or comes later in the file

What's the effect of this problem on the output?

If a DIE isn't marked as a definition_, we won't create an entry in
DwarfCUToModule::FilePrivate::specifications for it when we see it, and then any
other DIE using it as its specification is hosed. I can see how using the
linkage name will help here, but it seems like this patch is concerned with
populating Specification entries for DIEs that *are* marked, so I don't
understand why it helps.

[rafael.espindola]

> What's the effect of this problem on the output?

I added a test .so to http://people.mozilla.org/~respindola/test.so

With the current breakpad we get:

warning ...
...
FUNC 50c d 0 <name omitted>
....


With this patch we get

FUNC 50c d 0 C::f(int)

[rafael.espindola]

On 2012/10/05 18:35:40, jimb wrote:
> On 2012/10/05 18:24:04, rafael.espindola wrote:
> > I asked Ted how to add a test showing that no warning was printed, but he
was
> > not sure.
> 
> The tests should fail if an unexpected warning gets printed. In
> dwarf_cu_to_module_unittest.cc, CUFixtureBase's constructor sets up default
> expectations for reporter_: that no warnings will be issued. These are the
> EXPECT_CALL(reporter_, ...) forms.
> 
> Is that not working? Are tests printing warnings but passing anyway?

Cool, I didn't realize that the default was to fail if any warning was printed.
I will try to write a unit test.

[jimb]

Oh... I see the problem here. It's much simpler than I thought.

The declaration DIEs are indeed being marked as such; the DWARF is correct. It's
just that, with the original demangled name patch, we return before we reach the
code that populates the specifications table.

I think the right fix here is for Specification to have a third member,
'qualified_name'.

When a fully qualified name is available from the demangler,
ComputeQualifiedName should create an entry in the specifications table that has
only its 'qualified_name' member set to a non-empty string. It shouldn't compute
'unqualified_name' and 'enclosing_name' and then call 'MakeQualifiedName'.

Furthermore, if the DIE has a specification, and that specification has a
non-empty 'qualified_name', ComputeQualifiedName should return that immediately.

[jimb]

The test case for this is one where the specification DIE has a
DW_AT_MIPS_linkage_name attribute, and is cited by a DIE that has no naming
data, like the DIEs at offsets <37> and <5d> here:

Contents of the .debug_info section:

  Compilation Unit @ offset 0x0:
   Length:        0x97 (32-bit)
   Version:       2
   Abbrev Offset: 0
   Pointer Size:  8
 <0><b>: Abbrev Number: 1 (DW_TAG_compile_unit)
    <c>   DW_AT_producer    : (indirect string, offset: 0x0): GNU C++ 4.5.2	
    <10>   DW_AT_language    : 4	(C++)
    <11>   DW_AT_name        : (indirect string, offset: 0xe): test.cpp	
    <15>   DW_AT_comp_dir    : (indirect string, offset: 0x17):
/home/espindola/llvm	
    <19>   DW_AT_low_pc      : 0x50c	
    <21>   DW_AT_high_pc     : 0x519	
    <29>   DW_AT_stmt_list   : 0x0	
 <1><2d>: Abbrev Number: 2 (DW_TAG_class_type)
    <2e>   DW_AT_name        : C	
    <30>   DW_AT_byte_size   : 1	
    <31>   DW_AT_decl_file   : 1	
    <32>   DW_AT_decl_line   : 1	
    <33>   DW_AT_sibling     : <0x50>	
 <2><37>: Abbrev Number: 3 (DW_TAG_subprogram)
    <38>   DW_AT_external    : 1	
    <39>   DW_AT_name        : f	
    <3b>   DW_AT_decl_file   : 1	
    <3c>   DW_AT_decl_line   : 2	
    <3d>   DW_AT_MIPS_linkage_name: (indirect string, offset: 0x31): _ZN1C1fEi	
    <41>   DW_AT_accessibility: 3	(private)
    <42>   DW_AT_declaration : 1	
...
 <1><5d>: Abbrev Number: 8 (DW_TAG_subprogram)
    <5e>   DW_AT_specification: <0x37>	
    <62>   DW_AT_decl_line   : 5	
    <63>   DW_AT_low_pc      : 0x50c	
    <6b>   DW_AT_high_pc     : 0x519	
    <73>   DW_AT_frame_base  : 0x0	(location list)
    <77>   DW_AT_sibling     : <0x95>	
...

[rafael.espindola]

On 2012/10/05 20:00:54, jimb wrote:
> The test case for this is one where the specification DIE has a
> DW_AT_MIPS_linkage_name attribute, and is cited by a DIE that has no naming
> data, like the DIEs at offsets <37> and <5d> here:

That works, but currently the top of tree has a problem with any case where the
specification has a DW_AT_MIPS_linkage_name, even if the referring DIE has a
name too. The included testcase does fail with current trunk.

[jimb]

This looks much better. I had a few comments.

https://breakpad.appspot.com/478004/diff/6003/src/common/dwarf_cu_to_module.cc
File src/common/dwarf_cu_to_module.cc (right):

https://breakpad.appspot.com/478004/diff/6003/src/common/dwarf_cu_to_module.c...
src/common/dwarf_cu_to_module.cc:338: // Use DW_AT_MIPS_linkage_name if it is
available. It is already qualified.
This comment isn't quite right: the DW_AT_MIPS_linkage_name is mangled, not
qualified. It should say something like:

// Use the demangled name, if one is available. Demangled names are
// preferable to those inferred from the DWARF structure because they
// include argument types.

Also, it seems to me we shouldn't bother computing enclosing_name and
unqualified_name if we can get a qualified_name either here or in the
specification. Once we've got a qualified name, we don't need any of that stuff
any more.

https://breakpad.appspot.com/478004/diff/6003/src/common/dwarf_cu_to_module_u...
File src/common/dwarf_cu_to_module_unittest.cc (right):

https://breakpad.appspot.com/478004/diff/6003/src/common/dwarf_cu_to_module_u...
src/common/dwarf_cu_to_module_unittest.cc:213: const string &mangled_name);
Is it possible to declare the new parameter with a default value, so we don't
need to write "" in all the callers? Or does that not work for strings?

[jimb]

On 2012/10/05 20:08:25, rafael.espindola wrote:
> On 2012/10/05 20:00:54, jimb wrote:
> > The test case for this is one where the specification DIE has a
> > DW_AT_MIPS_linkage_name attribute, and is cited by a DIE that has no naming
> > data, like the DIEs at offsets <37> and <5d> here:
> 
> That works, but currently the top of tree has a problem with any case where
the
> specification has a DW_AT_MIPS_linkage_name, even if the referring DIE has a
> name too. The included testcase does fail with current trunk.

Yeah, we crossed paths; the test in Patch Set 3 (and also 2?) is fine.

[rafael.espindola]

> src/common/dwarf_cu_to_module.cc:338: // Use DW_AT_MIPS_linkage_name if it is
> available. It is already qualified.

True. Will update the comment.
 
> Also, it seems to me we shouldn't bother computing enclosing_name and
> unqualified_name if we can get a qualified_name either here or in the
> specification. Once we've got a qualified name, we don't need any of that
stuff
> any more.

ok.

>
https://breakpad.appspot.com/478004/diff/6003/src/common/dwarf_cu_to_module_u...
> File src/common/dwarf_cu_to_module_unittest.cc (right):
> 
>
https://breakpad.appspot.com/478004/diff/6003/src/common/dwarf_cu_to_module_u...
> src/common/dwarf_cu_to_module_unittest.cc:213: const string &mangled_name);
> Is it possible to declare the new parameter with a default value, so we don't
> need to write "" in all the callers? Or does that not work for strings?

The style guide
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Defaul...)
says:

We do not allow default function parameters, except in a few uncommon situations
explained below.

[jimb]

On 2012/10/05 20:26:33, rafael.espindola wrote:
> The style guide
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Defaul...)
> says:
> 
> We do not allow default function parameters, except in a few uncommon
situations
> explained below.

Ahh, right. I'm forgetting all this stuff.

[jimb]

Looks good to me --- thanks for the revisions.

[rafael.espindola]

On 2012/10/05 20:38:05, jimb wrote:
> Looks good to me --- thanks for the revisions.

I don't have write access. Con you commit it?

[jimb]

On 2012/10/05 20:41:03, rafael.espindola wrote:
> On 2012/10/05 20:38:05, jimb wrote:
> > Looks good to me --- thanks for the revisions.
> 
> I don't have write access. Con you commit it?

Committed as revision 1062.