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

[Andrew Burgess <aburgess@redhat.com>]

Andrew Burgess <aburgess@redhat.com> writes:

> Tom Tromey <tom@tromey.com> writes:
>
>>>>>>> "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.
>
> Thank you!  That exactly explains what's going on.  The test does
> specifically put just the destructor into a separate .cc file, which
> corresponds to the CU where we get the full type, with size.
>
>>
>> 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?
>
> I don't know.  I'll check, and if not open a new bug report, adding a
> 'Bug:' tag to the commit message of this patch before pushing.
>

OK, related bugs:

1.  https://sourceware.org/bugzilla/show_bug.cgi?id=29398

    CU expansion is slow.

2. https://sourceware.org/bugzilla/show_bug.cgi?id=15035

   Best match, excessive CU expansion.

3. https://sourceware.org/bugzilla/show_bug.cgi?id=25435

   Completion is slow due to CU expansion in general.

I propose to link to the second of these with a 'Bug:' tag in this
commit, but I don't think opening another bug is necessary.  Does that
sound OK to you?

Thanks,
Andrew