Re: [PATCH 1/5] Remove call to dwarf2_per_objfile::adjust from ranges readers

[Tom Tromey <tom@tromey.com>]

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

Andrew> So, I don't know the details of the DWARF reader too well, so my
Andrew> attempt to review this patch might be just wasting your time.

Not at all.  Thank you for looking at it.

>> dwarf2_per_objfile::adjust applies gdbarch_adjust_dwarf2_addr to an
>> address, leaving the result unrelocated.  However, this adjustment is
>> only needed for text-section symbols -- it isn't needed for any sort

Andrew> I didn't know if the use of the word 'symbols' here was significant.
Andrew> gdbarch_adjust_dwarf2_addr operates on addresses, and I guess symbol
Andrew> could be a synonym for address in some contexts.  But the addresses here
Andrew> do seem to be .text addresses, so clearly there's some important
Andrew> distinction that I'm not understanding.

The basic idea of this series is to note that gdbarch_adjust_dwarf2_addr
is only really needed when computing the address of a full symbol.  In
other cases, it is just extra work.

Part of this observation is from looking at the sole implementation of
this hook.  This is an abstraction violation, of course, and so maybe
the hook ought to be renamed.  Certainly other arches should not use it
-- in fact, even MIPS shouldn't use it, it's a giant hack, and probably
incorrectly implemented to boot (for example, is it intentional that
minsym lookups here examine all objfiles?  Because they do).

Anyway, the MIPS hook implementation looks to see if a given DWARF
symbol is really a MIPS16 (or MicroMIPS) symbol.

However, this information isn't relevant to the first round of DWARF
scanning.  For one thing, in the scanner, symbols don't have addresses.
So, fixing up the address doesn't matter.  Now, text addresses are
needed a little -- to map an address to a CU.  However, the "un-fixed"
address is perfectly suitable for this as well (and these mappings are
done by ranges anyway).

Andrew> A follow on question.  Looking through gdb/dwarf/ there seem to
Andrew> be several other places where the addrmap_mutable::set_empty is
Andrew> called, and in at least some of those places the addresses are
Andrew> still being adjusted.  E.g.:

Andrew>  dwarf2_ranges_read
Andrew>  cooked_indexer::check_bounds
Andrew>  cooked_indexer::scan_attributes

Andrew> Why do these not need changing?

These are all changed in patch #2.

Andrew> And an additional question.  Are lookups in these maps not done
Andrew> via the two ::find_per_cu functions?  Which are passed, and are
Andrew> documented to expected an un-adjusted (i.e. have
Andrew> text_section_offset removed) address.

Andrew> If we don't add text_section_offset in to begin with doesn't
Andrew> that cause problems?  Or maybe the bit I'm missing is that the
Andrew> two paths you've changed already are adjusted?

Yes, the lookups might be done via find_per_cu.  And yes, this takes an
unrelocated address.

The current code labors under the misapprehension that this gdbarch
transform is meaningful to the index.  IIRC the previous psymtab code
had this same incorrect idea, and so I preserved it in the cooked
indexer without looking too deeply into it.

Whether the addresses are relocated or not is a bit of a red herring.
The 'adjust' method took this into account, by applying the runtime
offset, calling the gdbarch method, and then un-applying the offset.

However, I believe there's just no need to call this arch hook at all in
the indexes -- only for full symbols, where it affects the MIPS ABI.


I hope this helps.  And if not, please let me know and I can try some
more.  I feel like I'm not explaining it very well.

thanks,
Tom