Re: [PATCH][gdb/symtab] Relocate call_site_htab

[Tom Tromey <tom@tromey.com>]

>> I've submitted an attempt at fixing this before, trying to handle this at all
>> points where the information is used (
>> https://sourceware.org/pipermail/gdb-patches/2019-August/159631.html ).

Simon> Instead of relocating the data structure, isn't the tendency now (to
Simon> make sharing between objfiles easier) to keep the data structure
Simon> unrelocated, and relocate things when looking things up?

Yeah.  That's what the earlier patch did but I guess I never re-reviewed
it.

Here's the final one

https://sourceware.org/pipermail/gdb-patches/2019-August/159822.html

That patch looks pretty reasonable to me.

-	    {
-	      lowpc = gdbarch_adjust_dwarf2_addr (gdbarch, lowpc + baseaddr);
-	      SET_FIELD_PHYSADDR (call_site->target, lowpc);
-	    }
+	    SET_FIELD_PHYSADDR (call_site->target, lowpc);

Normally I think the DWARF reader applies the offset, calls the gdbarch
method, then subtracts the offset.  It's unclear to me if this is truly
needed, it's maybe done just so I didn't need to audit all the gdbarch
implementations for some earlier patch.

Simon> I could imagine a compunit_symtab method called "find_call_site",
Simon> where you pass a relocated PC.  The method internally unrelocates the PC
Simon> to do the lookup in the htab.  call_site would store an unrelocated pc,
Simon> and a call_site::pc method would return the (computed) relocated pc.
Simon> Simiarly, the call_site's target would keep holding an unrelocated pc,
Simon> and it would be computed on the fly.

Simon> See crude patch implementing this at the bottom of this message.

This seemed pretty reasonable to me as well.

Simon>  - In call_site_to_target_addr, when the target is defined using a DWARF
Simon>    expression (FIELD_LOC_KIND_DWARF_BLOCK), is the result of computing
Simon>    that expression relocated or not?  I would presume that it's not
Simon>    relocated.  Do we need to relocate it at that point?  That looks like
Simon>    a bug to me, it would probably be caught by an appropriate test.

I don't know the answer here.

Simon>  - In read_call_site_scope, we do an htab lookup to look for
Simon>    duplicates.  We do:

Simon>      slot = htab_find_slot (cu->call_site_htab, &call_site_local, INSERT);

Simon>    where call_site_local is a call_site object.  However, the htab works
Simon>    using the address as index, it passes core_addr_hash and core_addr_eq
Simon>    as hash/eq functions.  And in call_site_for_pc, the lookup is done
Simon>    by passing a pointer to a CORE_ADDR.  Therefore, the lookup in
Simon>    read_call_site_scope looks wrong to me.  It just works because
Simon>    call_site::pc happens to be the first field of call_site.

IIUC, this is valid in C at least -- you can cast pointer-to-struct to
pointer-to-first-element.  However, it seems better for the code to be
less tricky.

Tom