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

[Tom de Vries via Gdb-patches <gdb-patches@sourceware.org>]

On 9/30/21 4:26 PM, Simon Marchi wrote:
> On 2021-09-30 05:56, Tom de Vries via Gdb-patches wrote:
>> [ CC-ing maintainers that reviewed previous submission. ]
>>
>> Hi,
>>
>> When running test-case gdb.arch/amd64-entry-value-inline.exp with target board
>> unix/-no-pie/-fno-PIE we have:
>> ...
>> (gdb) continue^M
>> Continuing.^M
>> ^M
>> Breakpoint 2, fn2 (y=y@entry=25, x=x@entry=6) at \
>>   gdb.arch/amd64-entry-value-inline.c:32^M
>> 32            y = -2 + x;       /* break-here */^M
>> (gdb) PASS: gdb.arch/amd64-entry-value-inline.exp: \
>>   continue to breakpoint: break-here
>> p y^M
>> $1 = 25^M
>> (gdb) PASS: gdb.arch/amd64-entry-value-inline.exp: p y
>> ...
>>
>> But with target board unix/-pie/-fPIE we have instead:
>> ...
>> p y^M
>> $1 = <optimized out>^M
>> (gdb) FAIL: gdb.arch/amd64-entry-value-inline.exp: p y
>> ...
>>
>> The test-case uses a .S file, which was generated using gcc 4.8.0, but I can
>> reproduce the same problem using the original C file and gcc 4.8.5.
>>
>> The problem is that in order to access the value, call_site information is
>> accessed, which is both:
>> - unrelocated, and
>> - accessed as if it were relocated.
>>
>> 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 ).
>>
>> Instead, fix this more reliably by relocating the call_site information.
>>
>> This fixes for me all remaining regressions for unix/-pie/-fPIE vs
>> unix/-no-pie/-fno-PIE (not counting ada compilation FAILs).
>>
>> Tested on x86_64-linux.
>>
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=24892
>>
>> Any comments?
> 
> Instead of relocating the data structure, isn't the tendency now (to
> make sharing between objfiles easier) to keep the data structure
> unrelocated, and relocate things when looking things up?
> 

Hmm, that makes sense, I didn't think of that.  Anyway, I'm still happy
to have written this, since it gives me a working solution to compare to.

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

Ack, thanks.  I've put this through testing and ran into only two
regressions (so, this used to pass for me with unix/-fPIE/-pie):
...
FAIL: gdb.trace/entry-values.exp: bt (1) (pattern 1)
FAIL: gdb.trace/entry-values.exp: bt (2) (pattern 1)
...
while still fixing all the unix/-fno-PIE/-no-pie vs unix/-fPIE/-pie
regressions.

Fixed by:
...
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index bb5767aae67..fa775722afb 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -13517,7 +13517,8 @@ read_call_site_scope
                       sect_offset_str (die->sect_off), objfile_name
(objfile));
          else
            {
-             lowpc = gdbarch_adjust_dwarf2_addr (gdbarch, lowpc +
baseaddr);
+             lowpc = (gdbarch_adjust_dwarf2_addr (gdbarch, lowpc +
baseaddr)
+                      - baseaddr);
              SET_FIELD_PHYSADDR (call_site->target, lowpc);
            }
        }
...

I'll put this through some testing.

Thanks,
- Tom

> Some observations I made while reading about this.
> 
>  - In call_site_to_target_addr, when the target is defined using a DWARF
>    expression (FIELD_LOC_KIND_DWARF_BLOCK), is the result of computing
>    that expression relocated or not?  I would presume that it's not
>    relocated.  Do we need to relocate it at that point?  That looks like
>    a bug to me, it would probably be caught by an appropriate test.
> 
>  - In read_call_site_scope, we do an htab lookup to look for
>    duplicates.  We do:
> 
>      slot = htab_find_slot (cu->call_site_htab, &call_site_local, INSERT);
> 
>    where call_site_local is a call_site object.  However, the htab works
>    using the address as index, it passes core_addr_hash and core_addr_eq
>    as hash/eq functions.  And in call_site_for_pc, the lookup is done
>    by passing a pointer to a CORE_ADDR.  Therefore, the lookup in
>    read_call_site_scope looks wrong to me.  It just works because
>    call_site::pc happens to be the first field of call_site.
> 
> Otherwise, comments on this version:
> 
>>
>> Thanks,
>> - Tom
>>
>> [gdb/symtab] Relocate call_site_htab
>>
>> ---
>>  gdb/objfiles.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 58 insertions(+)
>>
>> diff --git a/gdb/objfiles.c b/gdb/objfiles.c
>> index b65fa8820ca..ceff0fcfba4 100644
>> --- a/gdb/objfiles.c
>> +++ b/gdb/objfiles.c
>> @@ -626,6 +626,60 @@ relocate_one_symbol (struct symbol *sym, struct objfile *objfile,
>>      }
>>  }
>>  
>> +/* Relocate call_site S using offset DELTA.  */
>> +
>> +static void
>> +call_site_relocate (struct call_site *s, CORE_ADDR delta)
>> +{
>> +  s->pc += delta;
>> +  if (FIELD_LOC_KIND (s->target) == FIELD_LOC_KIND_PHYSADDR)
>> +    FIELD_STATIC_PHYSADDR (s->target) += delta;
>> +}
>> +
>> +/* Relocate HTAB, which is a COMPUNIT_CALL_SITE_HTAB using offset DELTA.  */
>> +
>> +static void
>> +compunit_call_site_htab_relocate (htab_t htab, CORE_ADDR delta)
>> +{
>> +  /* Changing the pc field changes the hashcode, so we can't just update the
>> +     elements.  Instead, we move them to this var, and then reinsert them.  */
>> +  std::vector<struct call_site *> tmp;
> 
> Instead of using a temporary vector, would it be simpler to just create
> a new htab, fill it while traversing the original one, and move it in
> place of the original one?  The original symtab might still be
> referenced in dwarf2_cu, but it's probably not needed there anymore
> after we have moved it to compunit_symtab.  So in
> process_full_comp_unit, after moving the htab, we could set
> dwarf2_cu::call_site_htab to nullptr to make sure we don't ever use it
> again.
> 
>> +
>> +  /* Copy elements to tmp.  */
>> +  auto visitor_func
>> +    = [] (void **slot, void *info) -> int
>> +    {
>> +      /* Copy element to tmp.  */
>> +      struct call_site *s = (struct call_site *) *slot;
>> +      std::vector<struct call_site *> *tmp_ptr
>> +      = (std::vector<struct call_site *> *)info;
>> +      tmp_ptr->push_back (s);
>> +
>> +      /* Keep going.  */
>> +      return 1;
>> +    };
>> +  htab_traverse (htab, visitor_func, &tmp);
>> +
>> +  /* Make hashtable empty.  This does not destroy the elements because the
>> +     hashtable is created with del_f == nullptr.  */
>> +  htab_empty (htab);
>> +
>> +  /* Relocate and reinsert elements.  */
>> +  for (struct call_site *s : tmp) {
>> +    /* Relocate element.  */
>> +    call_site_relocate (s, delta);
>> +
>> +    /* Reinsert element.  */
>> +    struct call_site call_site_local;
>> +    call_site_local.pc = s->pc;
>> +    void **slot
>> +      = htab_find_slot (htab, &call_site_local, INSERT);
> 
> As explained above, since the htab functions are
> core_addr_eq/core_addr_hash, we should pass a pointer to a CORE_ADDR.
> 
>> +    gdb_assert (slot != NULL);
>> +    gdb_assert (*slot == NULL);
>> +    *slot = s;
>> +  }
> 
> Watch the formatting above.
> 
> 
> From 8e8b9679da453bd6e5f4573412da275038a945dc Mon Sep 17 00:00:00 2001
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Date: Thu, 30 Sep 2021 09:08:43 -0400
> Subject: [PATCH] blah
> 
> Change-Id: I6c73686653d6c4daad331f3536a257cbcea77c64
> ---
>  gdb/block.c                 | 13 ++++++-------
>  gdb/dwarf2/frame-tailcall.c |  4 ++--
>  gdb/dwarf2/loc.c            | 27 +++++++++++++++++----------
>  gdb/dwarf2/read.c           | 10 +++++-----
>  gdb/gdbtypes.c              | 10 ++++++++++
>  gdb/gdbtypes.h              |  7 ++++---
>  gdb/symtab.c                | 24 ++++++++++++++++++++++++
>  gdb/symtab.h                |  6 ++++--
>  8 files changed, 72 insertions(+), 29 deletions(-)
> 
> diff --git a/gdb/block.c b/gdb/block.c
> index 4cb957313960..426ccb630e25 100644
> --- a/gdb/block.c
> +++ b/gdb/block.c
> @@ -224,16 +224,15 @@ blockvector_contains_pc (const struct blockvector *bv, CORE_ADDR pc)
>  struct call_site *
>  call_site_for_pc (struct gdbarch *gdbarch, CORE_ADDR pc)
>  {
> -  struct compunit_symtab *cust;
> -  void **slot = NULL;
> +  call_site *cs = nullptr;
>  
>    /* -1 as tail call PC can be already after the compilation unit range.  */
> -  cust = find_pc_compunit_symtab (pc - 1);
> +  compunit_symtab *cust = find_pc_compunit_symtab (pc - 1);
>  
> -  if (cust != NULL && COMPUNIT_CALL_SITE_HTAB (cust) != NULL)
> -    slot = htab_find_slot (COMPUNIT_CALL_SITE_HTAB (cust), &pc, NO_INSERT);
> +  if (cust != nullptr)
> +    cs = cust->find_call_site (pc);
>  
> -  if (slot == NULL)
> +  if (cs == nullptr)
>      {
>        struct bound_minimal_symbol msym = lookup_minimal_symbol_by_pc (pc);
>  
> @@ -247,7 +246,7 @@ call_site_for_pc (struct gdbarch *gdbarch, CORE_ADDR pc)
>  		    : msym.minsym->print_name ()));
>      }
>  
> -  return (struct call_site *) *slot;
> +  return cs;
>  }
>  
>  /* Return the blockvector immediately containing the innermost lexical block
> diff --git a/gdb/dwarf2/frame-tailcall.c b/gdb/dwarf2/frame-tailcall.c
> index f112b4ecca48..9fe498b0924e 100644
> --- a/gdb/dwarf2/frame-tailcall.c
> +++ b/gdb/dwarf2/frame-tailcall.c
> @@ -240,14 +240,14 @@ pretend_pc (struct frame_info *this_frame, struct tailcall_cache *cache)
>    gdb_assert (next_levels >= 0);
>  
>    if (next_levels < chain->callees)
> -    return chain->call_site[chain->length - next_levels - 1]->pc;
> +    return chain->call_site[chain->length - next_levels - 1]->pc ();
>    next_levels -= chain->callees;
>  
>    /* Otherwise CHAIN->CALLEES are already covered by CHAIN->CALLERS.  */
>    if (chain->callees != chain->length)
>      {
>        if (next_levels < chain->callers)
> -	return chain->call_site[chain->callers - next_levels - 1]->pc;
> +	return chain->call_site[chain->callers - next_levels - 1]->pc ();
>        next_levels -= chain->callers;
>      }
>  
> diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c
> index 2322a01f396a..16e0be73d027 100644
> --- a/gdb/dwarf2/loc.c
> +++ b/gdb/dwarf2/loc.c
> @@ -654,10 +654,10 @@ call_site_to_target_addr (struct gdbarch *call_site_gdbarch,
>  	  {
>  	    struct bound_minimal_symbol msym;
>  	    
> -	    msym = lookup_minimal_symbol_by_pc (call_site->pc - 1);
> +	    msym = lookup_minimal_symbol_by_pc (call_site->pc () - 1);
>  	    throw_error (NO_ENTRY_VALUE_ERROR,
>  			 _("DW_AT_call_target is not specified at %s in %s"),
> -			 paddress (call_site_gdbarch, call_site->pc),
> +			 paddress (call_site_gdbarch, call_site->pc ()),
>  			 (msym.minsym == NULL ? "???"
>  			  : msym.minsym->print_name ()));
>  			
> @@ -666,12 +666,12 @@ call_site_to_target_addr (struct gdbarch *call_site_gdbarch,
>  	  {
>  	    struct bound_minimal_symbol msym;
>  	    
> -	    msym = lookup_minimal_symbol_by_pc (call_site->pc - 1);
> +	    msym = lookup_minimal_symbol_by_pc (call_site->pc () - 1);
>  	    throw_error (NO_ENTRY_VALUE_ERROR,
>  			 _("DW_AT_call_target DWARF block resolving "
>  			   "requires known frame which is currently not "
>  			   "available at %s in %s"),
> -			 paddress (call_site_gdbarch, call_site->pc),
> +			 paddress (call_site_gdbarch, call_site->pc ()),
>  			 (msym.minsym == NULL ? "???"
>  			  : msym.minsym->print_name ()));
>  			
> @@ -700,11 +700,11 @@ call_site_to_target_addr (struct gdbarch *call_site_gdbarch,
>  	msym = lookup_minimal_symbol (physname, NULL, NULL);
>  	if (msym.minsym == NULL)
>  	  {
> -	    msym = lookup_minimal_symbol_by_pc (call_site->pc - 1);
> +	    msym = lookup_minimal_symbol_by_pc (call_site->pc () - 1);
>  	    throw_error (NO_ENTRY_VALUE_ERROR,
>  			 _("Cannot find function \"%s\" for a call site target "
>  			   "at %s in %s"),
> -			 physname, paddress (call_site_gdbarch, call_site->pc),
> +			 physname, paddress (call_site_gdbarch, call_site->pc ()),
>  			 (msym.minsym == NULL ? "???"
>  			  : msym.minsym->print_name ()));
>  			
> @@ -713,7 +713,14 @@ call_site_to_target_addr (struct gdbarch *call_site_gdbarch,
>        }
>  
>      case FIELD_LOC_KIND_PHYSADDR:
> -      return FIELD_STATIC_PHYSADDR (call_site->target);
> +      {
> +	dwarf2_per_objfile *per_objfile = call_site->per_objfile;
> +	compunit_symtab *cust = per_objfile->get_symtab (call_site->per_cu);
> +	int sect_idx = COMPUNIT_BLOCK_LINE_SECTION (cust);
> +	CORE_ADDR delta = per_objfile->objfile->section_offsets[sect_idx];
> +
> +	return FIELD_STATIC_PHYSADDR (call_site->target) + delta;
> +      }
>  
>      default:
>        internal_error (__FILE__, __LINE__, _("invalid call site target kind"));
> @@ -810,7 +817,7 @@ func_verify_no_selftailcall (struct gdbarch *gdbarch, CORE_ADDR verify_addr)
>  static void
>  tailcall_dump (struct gdbarch *gdbarch, const struct call_site *call_site)
>  {
> -  CORE_ADDR addr = call_site->pc;
> +  CORE_ADDR addr = call_site->pc ();
>    struct bound_minimal_symbol msym = lookup_minimal_symbol_by_pc (addr - 1);
>  
>    fprintf_unfiltered (gdb_stdlog, " %s(%s)", paddress (gdbarch, addr),
> @@ -986,7 +993,7 @@ call_site_find_chain_1 (struct gdbarch *gdbarch, CORE_ADDR caller_pc,
>  
>  	  if (target_call_site)
>  	    {
> -	      if (addr_hash.insert (target_call_site->pc).second)
> +	      if (addr_hash.insert (target_call_site->pc ()).second)
>  		{
>  		  /* Successfully entered TARGET_CALL_SITE.  */
>  
> @@ -1005,7 +1012,7 @@ call_site_find_chain_1 (struct gdbarch *gdbarch, CORE_ADDR caller_pc,
>  	      call_site = chain.back ();
>  	      chain.pop_back ();
>  
> -	      size_t removed = addr_hash.erase (call_site->pc);
> +	      size_t removed = addr_hash.erase (call_site->pc ());
>  	      gdb_assert (removed == 1);
>  
>  	      target_call_site = call_site->tail_call_next;
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index 00aa64dd0abc..bb5767aae673 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -9510,7 +9510,7 @@ process_full_comp_unit (dwarf2_cu *cu, enum language pretend_language)
>        if (gcc_4_minor >= 5)
>  	cust->epilogue_unwind_valid = 1;
>  
> -      cust->call_site_htab = cu->call_site_htab;
> +      cust->set_call_site_htab (cu->call_site_htab);
>      }
>  
>    per_objfile->set_symtab (cu->per_cu, cust);
> @@ -13341,7 +13341,7 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
>    struct gdbarch *gdbarch = objfile->arch ();
>    CORE_ADDR pc, baseaddr;
>    struct attribute *attr;
> -  struct call_site *call_site, call_site_local;
> +  struct call_site *call_site;
>    void **slot;
>    int nparams;
>    struct die_info *child_die;
> @@ -13364,13 +13364,13 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
>      }
>    pc = attr->as_address () + baseaddr;
>    pc = gdbarch_adjust_dwarf2_addr (gdbarch, pc);
> +  pc -= baseaddr;
>  
>    if (cu->call_site_htab == NULL)
>      cu->call_site_htab = htab_create_alloc_ex (16, core_addr_hash, core_addr_eq,
>  					       NULL, &objfile->objfile_obstack,
>  					       hashtab_obstack_allocate, NULL);
> -  call_site_local.pc = pc;
> -  slot = htab_find_slot (cu->call_site_htab, &call_site_local, INSERT);
> +  slot = htab_find_slot (cu->call_site_htab, &pc, INSERT);
>    if (*slot != NULL)
>      {
>        complaint (_("Duplicate PC %s for DW_TAG_call_site "
> @@ -13406,7 +13406,7 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
>  		      + (sizeof (*call_site->parameter) * (nparams - 1))));
>    *slot = call_site;
>    memset (call_site, 0, sizeof (*call_site) - sizeof (*call_site->parameter));
> -  call_site->pc = pc;
> +  call_site->m_unrelocated_pc = pc;
>  
>    if (dwarf2_flag_true_p (die, DW_AT_call_tail_call, cu)
>        || dwarf2_flag_true_p (die, DW_AT_GNU_tail_call, cu))
> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index be7c74ac6cfd..2585fe03a138 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -37,6 +37,7 @@
>  #include "cp-support.h"
>  #include "bcache.h"
>  #include "dwarf2/loc.h"
> +#include "dwarf2/read.h"
>  #include "gdbcore.h"
>  #include "floatformat.h"
>  #include "f-lang.h"
> @@ -6309,6 +6310,15 @@ objfile_type (struct objfile *objfile)
>    return objfile_type;
>  }
>  
> +CORE_ADDR
> +call_site::pc () const
> +{
> +  compunit_symtab *cust = this->per_objfile->get_symtab (this->per_cu);
> +  CORE_ADDR delta
> +	= this->per_objfile->objfile->section_offsets[COMPUNIT_BLOCK_LINE_SECTION (cust)];
> +  return m_unrelocated_pc + delta;
> +}
> +
>  void _initialize_gdbtypes ();
>  void
>  _initialize_gdbtypes ()
> diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
> index 2a641122aec0..e47bc46374d3 100644
> --- a/gdb/gdbtypes.h
> +++ b/gdb/gdbtypes.h
> @@ -1783,11 +1783,12 @@ struct call_site_parameter
>  
>  struct call_site
>    {
> -    /* * Address of the first instruction after this call.  It must be
> +    CORE_ADDR pc () const;
> +
> +    /* Unrelocated address of the first instruction after this call.  It must be
>         the first field as we overload core_addr_hash and core_addr_eq
>         for it.  */
> -
> -    CORE_ADDR pc;
> +    CORE_ADDR m_unrelocated_pc;
>  
>      /* * List successor with head in FUNC_TYPE.TAIL_CALL_LIST.  */
>  
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index a30d900cf8d6..e536754db02e 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -329,6 +329,30 @@ search_domain_name (enum search_domain e)
>      }
>  }
>  
> +call_site *
> +compunit_symtab::find_call_site (CORE_ADDR pc) const
> +{
> +  if (m_call_site_htab == nullptr)
> +    return nullptr;
> +
> +  CORE_ADDR delta
> +    = this->objfile->section_offsets[COMPUNIT_BLOCK_LINE_SECTION (this)];
> +  CORE_ADDR unrelocated_pc = pc - delta;
> +
> +  void **slot = htab_find_slot (m_call_site_htab, &unrelocated_pc, NO_INSERT);
> +  if (slot == nullptr)
> +    return nullptr;
> +
> +  return (call_site *) *slot;
> +}
> +
> +void
> +compunit_symtab::set_call_site_htab (htab_t call_site_htab)
> +{
> +  gdb_assert (m_call_site_htab == nullptr);
> +  m_call_site_htab = call_site_htab;
> +}
> +
>  /* See symtab.h.  */
>  
>  struct symtab *
> diff --git a/gdb/symtab.h b/gdb/symtab.h
> index 5182f51672e3..748f49c42435 100644
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -1449,6 +1449,9 @@ struct symtab
>  
>  struct compunit_symtab
>  {
> +  call_site *find_call_site (CORE_ADDR pc) const;
> +  void set_call_site_htab (htab_t call_site_htab);
> +
>    /* Unordered chain of all compunit symtabs of this objfile.  */
>    struct compunit_symtab *next;
>  
> @@ -1503,7 +1506,7 @@ struct compunit_symtab
>    unsigned int epilogue_unwind_valid : 1;
>  
>    /* struct call_site entries for this compilation unit or NULL.  */
> -  htab_t call_site_htab;
> +  htab_t m_call_site_htab;
>  
>    /* The macro table for this symtab.  Like the blockvector, this
>       is shared between different symtabs in a given compilation unit.
> @@ -1538,7 +1541,6 @@ using compunit_symtab_range = next_range<compunit_symtab>;
>  #define COMPUNIT_BLOCK_LINE_SECTION(cust) ((cust)->block_line_section)
>  #define COMPUNIT_LOCATIONS_VALID(cust) ((cust)->locations_valid)
>  #define COMPUNIT_EPILOGUE_UNWIND_VALID(cust) ((cust)->epilogue_unwind_valid)
> -#define COMPUNIT_CALL_SITE_HTAB(cust) ((cust)->call_site_htab)
>  #define COMPUNIT_MACRO_TABLE(cust) ((cust)->macro_table)
>  
>  /* A range adapter to allowing iterating over all the file tables
>