[PATCH][gdb/symtab] Relocate call_site_htab

[PATCH][gdb/symtab] Relocate call_site_htab

[Tom de Vries]

[ 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?

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;
+
+  /* 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);
+    gdb_assert (slot != NULL);
+    gdb_assert (*slot == NULL);
+    *slot = s;
+  }
+}
+
 /* Relocate OBJFILE to NEW_OFFSETS.  There should be OBJFILE->NUM_SECTIONS
    entries in new_offsets.  SEPARATE_DEBUG_OBJFILE is not touched here.
    Return non-zero iff any change happened.  */
@@ -697,6 +751,10 @@ objfile_relocate1 (struct objfile *objfile,
 		relocate_one_symbol (sym, objfile, delta);
 	      }
 	  }
+
+	if (COMPUNIT_CALL_SITE_HTAB (cust) != nullptr)
+	  compunit_call_site_htab_relocate (COMPUNIT_CALL_SITE_HTAB (cust),
+					    delta[block_line_section]);
       }
   }
 

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

[Simon Marchi]

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

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

[Tom Tromey]

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

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

[Tom de Vries]

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
> 

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

[Simon Marchi]

> 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);
>             }
>         }

That makes sense.  This means that with my patch, the value stored in
the target was relocated, by interpreted as unrelocated?  Does that mean
that with your patch, that value ended up relocated twice?  Once here,
and once in objfile_relocate1?

Simon

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

[Tom de Vries]

n 10/1/21 3:15 AM, Simon Marchi wrote:
>> 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);
>>             }
>>         }
> 
> That makes sense.  This means that with my patch, the value stored in
> the target was relocated, by interpreted as unrelocated?

Yes.

> Does that mean
> that with your patch, that value ended up relocated twice?  Once here,
> and once in objfile_relocate1?

No, though it took me some debugging to realize why :)

There are two scenarios:
- read_call_site_scope is triggered with baseaddr == 0, so the
  addresses are unrelocated.
  Then call_site_relocate is triggered with non-zero baseaddr, and
  the addresses become relocated.
- read_call_site_scope is triggered with non-zero baseaddr, so the
  addresses are relocated.
  Then call_site_relocate is not triggered, so the addresses remain
  correct, and are not relocated twice.

Thanks,
- Tom

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

[Tom de Vries]

On 10/1/21 10:25 AM, Tom de Vries wrote:
> n 10/1/21 3:15 AM, Simon Marchi wrote:
>>> 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've split this up into a patch series:
- [gdb/symtab] Fix htab_find_slot call in read_call_site_scope
- [gdb/symtab] Remove COMPUNIT_CALL_SITE_HTAB
- [gdb/symtab] Add call_site::pc ()
- [gdb/symtab] Use unrelocated addresses in call_site

Patch series was submitted, first one at
https://sourceware.org/pipermail/gdb-patches/2021-October/182325.html .

Thanks,
- Tom