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

[Andrew Burgess <aburgess@redhat.com>]

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.

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

To be clear, this isn't just a completion issue.  The example in PR
gold/15646 is for 'break namespace_foo::misspelled', where this is also
going to trigger expansion of many CUs.

But maybe there's information that we could track better that would
allow us to only expand a single CU in some cases?

Thanks,
Andrew