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

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

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?

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.

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
-- 
2.33.0