Re: [PATCHv3] gdb: remove attempted type de-duplication when building gdb-index

[Tom Tromey <tom@tromey.com>]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> In v3:
Andrew>   - Fixed test failure in gdb.cp/stub-array-size.exp when using
Andrew>     cc-with-gdb-index, using more recent versions of gcc.  This did
Andrew>     require additional changes in dwarf2/read-gdb-index.c, and removal
Andrew>     of a test that is now out of date.  The commit message has been
Andrew>     updated with a full description.

Thanks.  I think this is ok.  See below for commentary but there's
nothing you need to change.

Approved-By: Tom Tromey <tom@tromey.com>

Andrew> NOTE: Obviously a program doing this (defining a type differently in
Andrew>   different CUs) would need to be mindful of the One Definition Rule,
Andrew>   but so long as the type doesn't escape outside of a single CU then
Andrew>   reusing a type name isn't, as I understand it, wrong.  And even if
Andrew>   it is, the fact that it compiles, and could be a source of bugs,
Andrew>   means (in my opinion) that GDB should handle this case to enable
Andrew>   debugging of it.

Also ODR only applies to C++ and in C it's ok to have different
definitions of an opaque type in different CUs.  gdb itself used to use
this technique.

Andrew> This test has two CUs, and a type 'A'.  The test description says:
Andrew>   Test size of arrays of stubbed types (structures where the full
Andrew>   definition is not immediately available).
Andrew> Which I don't really understand given the test's source code.

I think the relevant difference in the DWARF is:

Andrew>  <1><4a>: Abbrev Number: 8 (DW_TAG_structure_type)
Andrew>     <4b>   DW_AT_name        : A
Andrew>     <4d>   DW_AT_declaration : 1
Andrew>     <4d>   DW_AT_sibling     : <0x6d>

Andrew> while in the second CU, the type looks like this:

Andrew>  <1><178>: Abbrev Number: 4 (DW_TAG_structure_type)
Andrew>     <179>   DW_AT_name        : A
Andrew>     <17b>   DW_AT_byte_size   : 8

... that the second one has the size.

Andrew> So, for reasons that I don't understand, the type, despite (as far as
Andrew> I can see) having its full definition available, is recorded only as
Andrew> declared in one CU.

I think this is something the C++ compilers do, which is only emit the
full definition when the "key method" (in this case the destructor) is
also being emitted.

I don't really recall the details.  Basically it's a workaround /
optimization to avoid exploding the debuginfo due to the separate
compilation/header file model.

Andrew> To be clear; the performance issue mentioned in PR gold/15646 is now
Andrew> back again.  But my claim is that gold was right all along to include
Andrew> the duplicate index entries, and any performance hit we see as a
Andrew> result, though unfortunate, is just a consequence of doing it right.

Yes.  To some extent I think this was a workaround for the broken C
parser and best_symbol behavior.  I.e., addressing a symptom of
excessive CU expansion rather than the (admittedly much more difficult)
cause.

For the completion issue you pointed out:

Andrew> The problem specifically called out in the bug report is that
Andrew> namespaces can appear in multiple CUs, and that trying to complete
Andrew> 'ns::misspelled' would expand every CU containing namespace 'ns' due
Andrew> to the duplicate 'ns' type symbols.

... do we have a bug for this?

This also seems like something that could be fixed.  For instance
completion could be done from the index as first pass.

Tom