Re: [PATCH v2 00/13] [gdb/symtab, cc-with-dwz] Fix gdb.cp/breakpoint-locs.exp

[Tom Tromey <tom@tromey.com>]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

[...]

Thank you for this series and the write-up.
You've done a great job on this.

Tom> IV. ALTERNATIVE APPROACH

Tom> I suppose it could be a matter of taste whether the current internal
Tom> representation is:
Tom> - a smart solution, or
Tom> - incorrect representation of the dwarf.

I'm not sure I totally understood this section, but the basic idea
behind this code -- and I think these decisions predate the cooked-index
rewrite -- was to try to exploit dwz compression by also using less
memory inside gdb.

Technically, I think, in DWARF an import can appear at any level and gdb
should probably just be re-reading a bunch of DIEs over and over.

However, this seemed super expensive back when I wrote the dwz support,
and since dwz is the only known compressor and it doesn't do some of the
weird stuff, it looked better to try to implement this optimization.

Shrug, maybe that was a mistake.  If every import caused a re-scan of
the PU, it would mean that only dwz users would pay for this feature.
Just maybe the price would be very high.

Tom> - I'm not sure if other maintainers are supportive of this approach.

I probably just don't understand.

Tom> And this is comparing the patch series with the base version:
Tom> ...
Tom> real: mean: 687.00    (100%) mean: 931.00    (135.52%)
Tom> user: mean: 2204.00   (100%) mean: 2938.00   (133.30%)
Tom> sys : mean: 102.00    (100%) mean: 137.00    (134.31%)
Tom> mem : mean: 345479.60 (100%) mean: 418396.00 (121.11%)
Tom> ...

Tom> In summary, the overall result is ~36% more real time and ~21% more memory.

Pretty heavy.  However I was curious about something:

Tom> The cumulative results of individual patches (leaving out the test-case
Tom> patches) are:

Tom> [gdb/symtab] Recurse into c++ DW_TAG_subprogram DIEs for cooked index

Tom> real: mean: 690.00    (100%) mean: 835.00    (121.01%)
Tom> user: mean: 2186.00   (100%) mean: 2693.00   (123.19%)
Tom> sys : mean: 106.00    (100%) mean: 130.00    (122.64%)
Tom> mem : mean: 345059.60 (100%) mean: 409780.00 (118.76%)

... the big performance jump appears here -- but is it really needed?

IIUC this is done because we want to detect inlined functions in C++.
However, in theory those should also be emitted at the top level with
DW_AT_inline, with a value of either DW_INL_inlined or
DW_INL_declared_inlined.

Would having entries for these be enough to provoke the desired CU
expansion?  If so it may be just a matter of marking some more abbrevs
as "interesting".

Actually, for the simple test case I tried, I already see it in the
index.  So I wonder what's going on there.

Anyway if we could drop this patch then it seems like the overall cost
would be alright.

Tom> Ideally, the impact of a patch series implementing dwz support on a non-dwz
Tom> test-case is none.

Tom> But fixing dwz support requires tracking more data, and there's no way of
Tom> knowing in advance whether the additional data will be used or not.

Yeah.  I loathe this part of DWARF but I have come around to accept that
we just have to handle it.

Tom> Of course this can be accommodated by optimistically assuming that the data is
Tom> unnecessary, and when it turns out it was actually needed, partially or
Tom> completely restart indexing.  My suspicion is that this approach itself is
Tom> going to be complex, so I think it's best avoided.

I agree.

Tom