[PATCH 1/4] [gdb/symtab] Fix htab_find_slot call in read_call_site_scope

[PATCH 1/4] [gdb/symtab] Fix htab_find_slot call in read_call_site_scope

[Tom de Vries]

From: Simon Marchi <simon.marchi@polymtl.ca>

In read_call_site_scope we have:
...
  call_site_local.pc = pc;
  slot = htab_find_slot (cu->call_site_htab, &call_site_local, INSERT);
...

The call passes a call_site pointer as element.  OTOH, the hashtab is created
using hash_f == core_addr_hash and eq_f == core_addr_eq, so the element
will be accessed through a CORE_ADDR pointer.

This is not wrong (at least in C), given that pc is the first field in
call_site.

Nevertheless, as in call_site_for_pc, make the htab_find_slot call match the
used hash_f and eq_f by using &pc instead:
...
  slot = htab_find_slot (cu->call_site_htab, &pc, INSERT);
...

Tested on x86_64-linux.

Co-Authored-By: Tom de Vries <tdevries@suse.de>
---
 gdb/dwarf2/read.c | 5 ++---
 gdb/gdbtypes.h    | 4 +---
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 00aa64dd0ab..23870c04e74 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -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;
@@ -13369,8 +13369,7 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
     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 "
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index 2a641122aec..84b751e82e3 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -1783,9 +1783,7 @@ struct call_site_parameter
 
 struct call_site
   {
-    /* * 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.  */
+    /* Address of the first instruction after this call.  */
 
     CORE_ADDR pc;
 

base-commit: e4860c08f990675c9b3535ab18cc7ff21e2a5639
-- 
2.26.2

[PATCH 2/4] [gdb/symtab] Remove COMPUNIT_CALL_SITE_HTAB

[Tom de Vries]

From: Simon Marchi <simon.marchi@polymtl.ca>

Remove macro COMPUNIT_CALL_SITE_HTAB, and provide access to the htab using
member functions:
- compunit_symtab::find_call_site
- compunit_symtab::set_call_site_htab

Tested on x86_64-linux.

Co-Authored-By: Tom de Vries <tdevries@suse.de>
---
 gdb/block.c       | 10 +++++-----
 gdb/dwarf2/read.c |  2 +-
 gdb/symtab.c      | 24 ++++++++++++++++++++++++
 gdb/symtab.h      |  9 +++++++--
 4 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/gdb/block.c b/gdb/block.c
index 4cb95731396..90c0c5b3250 100644
--- a/gdb/block.c
+++ b/gdb/block.c
@@ -225,15 +225,15 @@ 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);
 
-  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 +247,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/read.c b/gdb/dwarf2/read.c
index 23870c04e74..ac8460df9a4 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);
diff --git a/gdb/symtab.c b/gdb/symtab.c
index a30d900cf8d..b7a00709d00 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -331,6 +331,30 @@ search_domain_name (enum search_domain e)
 
 /* See symtab.h.  */
 
+call_site *
+compunit_symtab::find_call_site (CORE_ADDR pc) const
+{
+  if (m_call_site_htab == nullptr)
+    return nullptr;
+
+  void **slot = htab_find_slot (m_call_site_htab, &pc, NO_INSERT);
+  if (slot == nullptr)
+    return nullptr;
+
+  return (call_site *) *slot;
+}
+
+/* See symtab.h.  */
+
+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 *
 compunit_primary_filetab (const struct compunit_symtab *cust)
 {
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 5182f51672e..6a6cf8201bc 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1449,6 +1449,12 @@ struct symtab
 
 struct compunit_symtab
 {
+  /* Set m_call_site_htab.  */
+  void set_call_site_htab (htab_t call_site_htab);
+
+  /* Find call_site info for PC.  */
+  call_site *find_call_site (CORE_ADDR pc) const;
+
   /* Unordered chain of all compunit symtabs of this objfile.  */
   struct compunit_symtab *next;
 
@@ -1503,7 +1509,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 +1544,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.26.2

Re: [PATCH 2/4] [gdb/symtab] Remove COMPUNIT_CALL_SITE_HTAB

[Simon Marchi]

On 2021-10-01 08:33, Tom de Vries via Gdb-patches wrote:
> From: Simon Marchi <simon.marchi@polymtl.ca>
> 
> Remove macro COMPUNIT_CALL_SITE_HTAB, and provide access to the htab using
> member functions:
> - compunit_symtab::find_call_site
> - compunit_symtab::set_call_site_htab
> 
> Tested on x86_64-linux.
> 
> Co-Authored-By: Tom de Vries <tdevries@suse.de>

LGTM, but I wrote some of it, so I'm not really impartial here.

Simon

[PATCH 3/4] [gdb/symtab] Add call_site::pc ()

[Tom de Vries]

From: Simon Marchi <simon.marchi@polymtl.ca>

Add member function call_site::pc () and update all uses.

Tested on x86_64-linux.

Co-Authored-By: Tom de Vries <tdevries@suse.de>
---
 gdb/dwarf2/frame-tailcall.c |  4 ++--
 gdb/dwarf2/loc.c            | 18 +++++++++---------
 gdb/dwarf2/read.c           |  2 +-
 gdb/gdbtypes.c              |  9 +++++++++
 gdb/gdbtypes.h              |  6 +++++-
 5 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/gdb/dwarf2/frame-tailcall.c b/gdb/dwarf2/frame-tailcall.c
index f112b4ecca4..9fe498b0924 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 2322a01f396..27a1c97682a 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 ()));
 			
@@ -810,7 +810,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 +986,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 +1005,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 ac8460df9a4..d0460674d10 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -13405,7 +13405,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_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 be7c74ac6cf..500488adece 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,14 @@ objfile_type (struct objfile *objfile)
   return objfile_type;
 }
 
+/* See gdbtypes.h.  */
+
+CORE_ADDR
+call_site::pc () const
+{
+  return m_pc;
+}
+
 void _initialize_gdbtypes ();
 void
 _initialize_gdbtypes ()
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index 84b751e82e3..7a17005229e 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -1783,9 +1783,13 @@ struct call_site_parameter
 
 struct call_site
   {
+    /* As m_pc.  */
+
+    CORE_ADDR pc () const;
+
     /* Address of the first instruction after this call.  */
 
-    CORE_ADDR pc;
+    CORE_ADDR m_pc;
 
     /* * List successor with head in FUNC_TYPE.TAIL_CALL_LIST.  */
 
-- 
2.26.2

Re: [PATCH 3/4] [gdb/symtab] Add call_site::pc ()

[Simon Marchi]

On 2021-10-01 8:33 a.m., Tom de Vries via Gdb-patches wrote:
> From: Simon Marchi <simon.marchi@polymtl.ca>
>
> Add member function call_site::pc () and update all uses.
>
> Tested on x86_64-linux.
>
> Co-Authored-By: Tom de Vries <tdevries@suse.de>
> ---
>  gdb/dwarf2/frame-tailcall.c |  4 ++--
>  gdb/dwarf2/loc.c            | 18 +++++++++---------
>  gdb/dwarf2/read.c           |  2 +-
>  gdb/gdbtypes.c              |  9 +++++++++
>  gdb/gdbtypes.h              |  6 +++++-
>  5 files changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/gdb/dwarf2/frame-tailcall.c b/gdb/dwarf2/frame-tailcall.c
> index f112b4ecca4..9fe498b0924 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 2322a01f396..27a1c97682a 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 ()));
>  			
> @@ -810,7 +810,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 +986,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 +1005,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 ac8460df9a4..d0460674d10 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -13405,7 +13405,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_pc = pc;

I did this as a quick hack in my proof of concept patch, but I think we
should make this a bit nicer than setting the (conceptually) private
field here.  We can have add a constructor to call_site and use
placement new, see patch below (only built-tested).


From b0515b5273d725d32bfdd0ec4eb9c346217d57a8 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@efficios.com>
Date: Fri, 1 Oct 2021 13:50:52 -0400
Subject: [PATCH] fixup for patch 3

Change-Id: I291008c07c17cca1f8c8cd08954c308eff4611d1
---
 gdb/dwarf2/read.c | 20 +++++++++-----------
 gdb/gdbtypes.h    | 28 +++++++++++++++++-----------
 2 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 2648faf6289..ec2f64c4014 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -13341,7 +13341,6 @@ 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;
   void **slot;
   int nparams;
   struct die_info *child_die;
@@ -13398,14 +13397,16 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
       nparams++;
     }

-  call_site
-    = ((struct call_site *)
-       obstack_alloc (&objfile->objfile_obstack,
-		      sizeof (*call_site)
-		      + (sizeof (*call_site->parameter) * (nparams - 1))));
+  struct call_site *call_site
+    = new (XOBNEWVAR (&objfile->objfile_obstack,
+		      struct call_site,
+		      sizeof (*call_site) + sizeof (call_site->parameter[0]) * nparams))
+    struct call_site (pc, cu->per_cu, per_objfile);
   *slot = call_site;
-  memset (call_site, 0, sizeof (*call_site) - sizeof (*call_site->parameter));
-  call_site->m_pc = pc;
+
+  /* We never call the destructor of call_site, so we must ensure it is
+     trivially destructible.  */
+  gdb_static_assert(std::is_trivially_destructible<struct call_site>::value);

   if (dwarf2_flag_true_p (die, DW_AT_call_tail_call, cu)
       || dwarf2_flag_true_p (die, DW_AT_GNU_tail_call, cu))
@@ -13526,9 +13527,6 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
 		 "block nor reference, for DIE %s [in module %s]"),
 	       sect_offset_str (die->sect_off), objfile_name (objfile));

-  call_site->per_cu = cu->per_cu;
-  call_site->per_objfile = per_objfile;
-
   for (child_die = die->child;
        child_die && child_die->tag;
        child_die = child_die->sibling)
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index ed4bc25afa1..e181a899981 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -1793,39 +1793,45 @@ struct call_site_parameter

 struct call_site
   {
-    /* As m_pc.  */
+    call_site (CORE_ADDR pc, dwarf2_per_cu_data *per_cu,
+	       dwarf2_per_objfile *per_objfile)
+      : per_cu (per_cu), per_objfile (per_objfile), m_pc (pc)
+    {}

-    CORE_ADDR pc () const;
-
-    /* Address of the first instruction after this call.  */
+    /* Return the address of the first instruction after this call.  */

-    CORE_ADDR m_pc;
+    CORE_ADDR pc () const;

     /* * List successor with head in FUNC_TYPE.TAIL_CALL_LIST.  */

-    struct call_site *tail_call_next;
+    struct call_site *tail_call_next = nullptr;

     /* * Describe DW_AT_call_target.  Missing attribute uses
        FIELD_LOC_KIND_DWARF_BLOCK with FIELD_DWARF_BLOCK == NULL.  */

-    struct call_site_target target;
+    struct call_site_target target {};

     /* * Size of the PARAMETER array.  */

-    unsigned parameter_count;
+    unsigned parameter_count = 0;

     /* * CU of the function where the call is located.  It gets used
        for DWARF blocks execution in the parameter array below.  */

-    dwarf2_per_cu_data *per_cu;
+    dwarf2_per_cu_data *const per_cu = nullptr;

     /* objfile of the function where the call is located.  */

-    dwarf2_per_objfile *per_objfile;
+    dwarf2_per_objfile *const per_objfile = nullptr;
+
+  private:
+    /* Address of the first instruction after this call.  */
+    const CORE_ADDR m_pc;

+  public:
     /* * Describe DW_TAG_call_site's DW_TAG_formal_parameter.  */

-    struct call_site_parameter parameter[1];
+    struct call_site_parameter parameter[];
   };

 /* The type-specific info for TYPE_CODE_FIXED_POINT types.  */
-- 
2.33.0

Re: [PATCH 3/4] [gdb/symtab] Add call_site::pc ()

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 5677 bytes --]

On 10/1/21 8:10 PM, Simon Marchi wrote:
> On 2021-10-01 8:33 a.m., Tom de Vries via Gdb-patches wrote:
>> From: Simon Marchi <simon.marchi@polymtl.ca>
>>
>> Add member function call_site::pc () and update all uses.
>>
>> Tested on x86_64-linux.
>>
>> Co-Authored-By: Tom de Vries <tdevries@suse.de>
>> ---
>>  gdb/dwarf2/frame-tailcall.c |  4 ++--
>>  gdb/dwarf2/loc.c            | 18 +++++++++---------
>>  gdb/dwarf2/read.c           |  2 +-
>>  gdb/gdbtypes.c              |  9 +++++++++
>>  gdb/gdbtypes.h              |  6 +++++-
>>  5 files changed, 26 insertions(+), 13 deletions(-)
>>
>> diff --git a/gdb/dwarf2/frame-tailcall.c b/gdb/dwarf2/frame-tailcall.c
>> index f112b4ecca4..9fe498b0924 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 2322a01f396..27a1c97682a 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 ()));
>>  			
>> @@ -810,7 +810,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 +986,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 +1005,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 ac8460df9a4..d0460674d10 100644
>> --- a/gdb/dwarf2/read.c
>> +++ b/gdb/dwarf2/read.c
>> @@ -13405,7 +13405,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_pc = pc;
> 
> I did this as a quick hack in my proof of concept patch, but I think we
> should make this a bit nicer than setting the (conceptually) private
> field here.  We can have add a constructor to call_site and use
> placement new, see patch below (only built-tested).
> 
> 

Ack, integrated in patch, and renamed $subject to indicate larger scope
of patch.

Also some trivial updates due to commit "[gdb/symtab] Add call_site_eq
and call_site_hash".

Committed as attached.

Thanks,
- Tom


[-- Attachment #2: 0002-gdb-symtab-C-ify-call_site.patch --]
[-- Type: text/x-patch, Size: 10646 bytes --]

[gdb/symtab] C++-ify call_site

- add constructor
- add member function call_site::pc ()

Tested on x86_64-linux.

Co-Authored-By: Tom de Vries <tdevries@suse.de>

---
 gdb/dwarf2/frame-tailcall.c |  4 +--
 gdb/dwarf2/loc.c            | 18 ++++++------
 gdb/dwarf2/read.c           | 27 +++++++++---------
 gdb/gdbtypes.c              |  9 ++++++
 gdb/gdbtypes.h              | 67 +++++++++++++++++++++++++++++----------------
 gdb/symtab.c                |  3 +-
 6 files changed, 77 insertions(+), 51 deletions(-)

diff --git a/gdb/dwarf2/frame-tailcall.c b/gdb/dwarf2/frame-tailcall.c
index f112b4ecca4..9fe498b0924 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 2322a01f396..27a1c97682a 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 ()));
 			
@@ -810,7 +810,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 +986,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 +1005,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 f8421aeb344..230eb6a04b1 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -13341,7 +13341,6 @@ 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;
   void **slot;
   int nparams;
   struct die_info *child_die;
@@ -13366,10 +13365,11 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
   pc = gdbarch_adjust_dwarf2_addr (gdbarch, pc);
 
   if (cu->call_site_htab == NULL)
-    cu->call_site_htab = htab_create_alloc_ex (16, call_site_hash, call_site_eq,
-					       NULL, &objfile->objfile_obstack,
+    cu->call_site_htab = htab_create_alloc_ex (16, call_site::hash,
+					       call_site::eq, NULL,
+					       &objfile->objfile_obstack,
 					       hashtab_obstack_allocate, NULL);
-  call_site_local.pc = pc;
+  struct call_site call_site_local (pc, nullptr, nullptr);
   slot = htab_find_slot (cu->call_site_htab, &call_site_local, INSERT);
   if (*slot != NULL)
     {
@@ -13399,14 +13399,16 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
       nparams++;
     }
 
-  call_site
-    = ((struct call_site *)
-       obstack_alloc (&objfile->objfile_obstack,
-		      sizeof (*call_site)
-		      + (sizeof (*call_site->parameter) * (nparams - 1))));
+  struct call_site *call_site
+    = new (XOBNEWVAR (&objfile->objfile_obstack,
+		      struct call_site,
+		      sizeof (*call_site) + sizeof (call_site->parameter[0]) * nparams))
+    struct call_site (pc, cu->per_cu, per_objfile);
   *slot = call_site;
-  memset (call_site, 0, sizeof (*call_site) - sizeof (*call_site->parameter));
-  call_site->pc = pc;
+
+  /* We never call the destructor of call_site, so we must ensure it is
+     trivially destructible.  */
+  gdb_static_assert(std::is_trivially_destructible<struct call_site>::value);
 
   if (dwarf2_flag_true_p (die, DW_AT_call_tail_call, cu)
       || dwarf2_flag_true_p (die, DW_AT_GNU_tail_call, cu))
@@ -13527,9 +13529,6 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
 		 "block nor reference, for DIE %s [in module %s]"),
 	       sect_offset_str (die->sect_off), objfile_name (objfile));
 
-  call_site->per_cu = cu->per_cu;
-  call_site->per_objfile = per_objfile;
-
   for (child_die = die->child;
        child_die && child_die->tag;
        child_die = child_die->sibling)
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 97f016fccd1..8954c370b7b 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"
@@ -6308,6 +6309,14 @@ objfile_type (struct objfile *objfile)
   return objfile_type;
 }
 
+/* See gdbtypes.h.  */
+
+CORE_ADDR
+call_site::pc () const
+{
+  return m_pc;
+}
+
 void _initialize_gdbtypes ();
 void
 _initialize_gdbtypes ()
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index 8021cb21ecc..5fe10cb7f1e 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -1793,52 +1793,71 @@ struct call_site_parameter
 
 struct call_site
   {
-    /* Address of the first instruction after this call.  */
-
-    CORE_ADDR pc;
+    call_site (CORE_ADDR pc, dwarf2_per_cu_data *per_cu,
+	       dwarf2_per_objfile *per_objfile)
+      : per_cu (per_cu), per_objfile (per_objfile), m_pc (pc)
+    {}
+
+    static int
+    eq (const call_site *a, const call_site *b)
+    {
+      return core_addr_eq (&a->m_pc, &b->m_pc);
+    }
+
+    static hashval_t
+    hash (const call_site *a)
+    {
+      return core_addr_hash (&a->m_pc);
+    }
+
+    static int
+    eq (const void *a, const void *b)
+    {
+      return eq ((const call_site *)a, (const call_site *)b);
+    }
+
+    static hashval_t
+    hash (const void *a)
+    {
+      return hash ((const call_site *)a);
+    }
+
+    /* Return the address of the first instruction after this call.  */
+
+    CORE_ADDR pc () const;
 
     /* * List successor with head in FUNC_TYPE.TAIL_CALL_LIST.  */
 
-    struct call_site *tail_call_next;
+    struct call_site *tail_call_next = nullptr;
 
     /* * Describe DW_AT_call_target.  Missing attribute uses
        FIELD_LOC_KIND_DWARF_BLOCK with FIELD_DWARF_BLOCK == NULL.  */
 
-    struct call_site_target target;
+    struct call_site_target target {};
 
     /* * Size of the PARAMETER array.  */
 
-    unsigned parameter_count;
+    unsigned parameter_count = 0;
 
     /* * CU of the function where the call is located.  It gets used
        for DWARF blocks execution in the parameter array below.  */
 
-    dwarf2_per_cu_data *per_cu;
+    dwarf2_per_cu_data *const per_cu = nullptr;
 
     /* objfile of the function where the call is located.  */
 
-    dwarf2_per_objfile *per_objfile;
+    dwarf2_per_objfile *const per_objfile = nullptr;
 
+  private:
+    /* Address of the first instruction after this call.  */
+    const CORE_ADDR m_pc;
+
+  public:
     /* * Describe DW_TAG_call_site's DW_TAG_formal_parameter.  */
 
-    struct call_site_parameter parameter[1];
+    struct call_site_parameter parameter[];
   };
 
-static inline int
-call_site_eq (const void *a_, const void *b_)
-{
-  const struct call_site *a = (const call_site *)a_;
-  const struct call_site *b = (const call_site *)b_;
-  return core_addr_eq (&a->pc, &b->pc);
-}
-
-static inline hashval_t
-call_site_hash (const void *a_)
-{
-  const struct call_site *a = (const call_site *)a_;
-  return core_addr_hash (&a->pc);
-}
-
 /* The type-specific info for TYPE_CODE_FIXED_POINT types.  */
 
 struct fixed_point_type_info
diff --git a/gdb/symtab.c b/gdb/symtab.c
index bd1eb429b94..cb583846efa 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -334,11 +334,10 @@ search_domain_name (enum search_domain e)
 call_site *
 compunit_symtab::find_call_site (CORE_ADDR pc) const
 {
-  struct call_site call_site_local;
   if (m_call_site_htab == nullptr)
     return nullptr;
 
-  call_site_local.pc = pc;
+  struct call_site call_site_local (pc, nullptr, nullptr);
   void **slot
     = htab_find_slot (m_call_site_htab, &call_site_local, NO_INSERT);
   if (slot == nullptr)

[PATCH 4/4] [gdb/symtab] Use unrelocated addresses in call_site

[Tom de Vries]

From: Simon Marchi <simon.marchi@polymtl.ca>

Consider test-case gdb.trace/entry-values.exp with target board
unix/-fPIE/-pie.

Using this command we have an abbreviated version, and can see the correct
@entry values for foo:
...
$ gdb -q -batch outputs/gdb.trace/entry-values/entry-values \
  -ex start \
  -ex "break foo" \
  -ex "set print entry-values both" \
  -ex continue
Temporary breakpoint 1 at 0x679

Temporary breakpoint 1, 0x0000555555554679 in main ()
Breakpoint 2 at 0x55555555463e

Breakpoint 2, 0x000055555555463e in foo (i=0, i@entry=2, j=2, j@entry=3)
...

Now, let's try the same again, but run directly to foo rather than stopping at
main:
...
$ gdb -q -batch outputs/gdb.trace/entry-values/entry-values \
  -ex "break foo" \
  -ex "set print entry-values both" \
  -ex run
Breakpoint 1 at 0x63e

Breakpoint 1, 0x000055555555463e in foo (i=0, i@entry=<optimized out>, \
  j=2, j@entry=<optimized out>)
...

So, what explains the difference?  Noteworthy, this is a dwarf assembly
test-case, with debug info for foo and bar, but not for main.

In the first case:
- we run to main
- this does not trigger expanding debug info, because there's none for main
- we set a breakpoint at foo
- this triggers expanding debug info.  Relocated addresses are used in
  call_site info (because the exec is started)
- we continue to foo, and manage to find the call_site info

In the second case:
- we set a breakpoint at foo
- this triggers expanding debug info.  Unrelocated addresses are used in
  call_site info (because the exec is not started)
- we run to foo
- this triggers objfile_relocate1, but it doesn't update the call_site
  info addresses
- we don't manage to find the call_site info

We could fix this by adding the missing call_site relocation in
objfile_relocate1.

This solution however is counter-trend in the sense that we're trying to
work towards the situation where when starting two instances of an executable,
we need only one instance of debug information, implying the use of
unrelocated addresses.

So, fix this instead by using unrelocated addresses in call_site info.

Tested on x86_64-linux.

This fixes all remaining unix/-fno-PIE/-no-pie vs unix/-fPIE/-pie
regressions, like f.i. PR24892.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=24892

Co-Authored-By: Tom de Vries <tdevries@suse.de>
---
 gdb/dwarf2/loc.c  | 9 ++++++++-
 gdb/dwarf2/read.c | 6 ++++--
 gdb/gdbtypes.c    | 5 ++++-
 gdb/gdbtypes.h    | 6 +++---
 gdb/symtab.c      | 6 +++++-
 5 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c
index 27a1c97682a..16e0be73d02 100644
--- a/gdb/dwarf2/loc.c
+++ b/gdb/dwarf2/loc.c
@@ -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"));
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index d0460674d10..fa775722afb 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -13364,6 +13364,7 @@ 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,
@@ -13405,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->m_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))
@@ -13516,7 +13517,8 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
 		       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);
 	    }
 	}
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 500488adece..54a99af9c2e 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -6315,7 +6315,10 @@ objfile_type (struct objfile *objfile)
 CORE_ADDR
 call_site::pc () const
 {
-  return m_pc;
+  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 ();
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index 7a17005229e..1d15916d4e5 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -1783,13 +1783,13 @@ struct call_site_parameter
 
 struct call_site
   {
-    /* As m_pc.  */
+    /* As m_unrelocated_pc, but relocated.  */
 
     CORE_ADDR pc () const;
 
-    /* Address of the first instruction after this call.  */
+    /* Unrelocated address of the first instruction after this call.  */
 
-    CORE_ADDR m_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 b7a00709d00..65247869913 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -337,7 +337,11 @@ compunit_symtab::find_call_site (CORE_ADDR pc) const
   if (m_call_site_htab == nullptr)
     return nullptr;
 
-  void **slot = htab_find_slot (m_call_site_htab, &pc, NO_INSERT);
+  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;
 
-- 
2.26.2

Re: [PATCH 4/4] [gdb/symtab] Use unrelocated addresses in call_site

[Simon Marchi]

On 2021-10-01 08:33, Tom de Vries via Gdb-patches wrote:
> From: Simon Marchi <simon.marchi@polymtl.ca>
> 
> Consider test-case gdb.trace/entry-values.exp with target board
> unix/-fPIE/-pie.
> 
> Using this command we have an abbreviated version, and can see the 
> correct
> @entry values for foo:
> ...
> $ gdb -q -batch outputs/gdb.trace/entry-values/entry-values \
>   -ex start \
>   -ex "break foo" \
>   -ex "set print entry-values both" \
>   -ex continue
> Temporary breakpoint 1 at 0x679
> 
> Temporary breakpoint 1, 0x0000555555554679 in main ()
> Breakpoint 2 at 0x55555555463e
> 
> Breakpoint 2, 0x000055555555463e in foo (i=0, i@entry=2, j=2, 
> j@entry=3)
> ...
> 
> Now, let's try the same again, but run directly to foo rather than 
> stopping at
> main:
> ...
> $ gdb -q -batch outputs/gdb.trace/entry-values/entry-values \
>   -ex "break foo" \
>   -ex "set print entry-values both" \
>   -ex run
> Breakpoint 1 at 0x63e
> 
> Breakpoint 1, 0x000055555555463e in foo (i=0, i@entry=<optimized out>, 
> \
>   j=2, j@entry=<optimized out>)
> ...
> 
> So, what explains the difference?  Noteworthy, this is a dwarf assembly
> test-case, with debug info for foo and bar, but not for main.
> 
> In the first case:
> - we run to main
> - this does not trigger expanding debug info, because there's none for 
> main
> - we set a breakpoint at foo
> - this triggers expanding debug info.  Relocated addresses are used in
>   call_site info (because the exec is started)
> - we continue to foo, and manage to find the call_site info
> 
> In the second case:
> - we set a breakpoint at foo
> - this triggers expanding debug info.  Unrelocated addresses are used 
> in
>   call_site info (because the exec is not started)
> - we run to foo
> - this triggers objfile_relocate1, but it doesn't update the call_site
>   info addresses
> - we don't manage to find the call_site info

Thanks for this explanation, I had not realized the difference in 
behavior here.

The patch LGTM.

Simon

Re: [PATCH 4/4] [gdb/symtab] Use unrelocated addresses in call_site

[Tom de Vries]

On 10/1/21 10:56 PM, Simon Marchi wrote:
> On 2021-10-01 08:33, Tom de Vries via Gdb-patches wrote:
>> From: Simon Marchi <simon.marchi@polymtl.ca>
>>
>> Consider test-case gdb.trace/entry-values.exp with target board
>> unix/-fPIE/-pie.
>>
>> Using this command we have an abbreviated version, and can see the
>> correct
>> @entry values for foo:
>> ...
>> $ gdb -q -batch outputs/gdb.trace/entry-values/entry-values \
>>   -ex start \
>>   -ex "break foo" \
>>   -ex "set print entry-values both" \
>>   -ex continue
>> Temporary breakpoint 1 at 0x679
>>
>> Temporary breakpoint 1, 0x0000555555554679 in main ()
>> Breakpoint 2 at 0x55555555463e
>>
>> Breakpoint 2, 0x000055555555463e in foo (i=0, i@entry=2, j=2, j@entry=3)
>> ...
>>
>> Now, let's try the same again, but run directly to foo rather than
>> stopping at
>> main:
>> ...
>> $ gdb -q -batch outputs/gdb.trace/entry-values/entry-values \
>>   -ex "break foo" \
>>   -ex "set print entry-values both" \
>>   -ex run
>> Breakpoint 1 at 0x63e
>>
>> Breakpoint 1, 0x000055555555463e in foo (i=0, i@entry=<optimized out>, \
>>   j=2, j@entry=<optimized out>)
>> ...
>>
>> So, what explains the difference?  Noteworthy, this is a dwarf assembly
>> test-case, with debug info for foo and bar, but not for main.
>>
>> In the first case:
>> - we run to main
>> - this does not trigger expanding debug info, because there's none for
>> main
>> - we set a breakpoint at foo
>> - this triggers expanding debug info.  Relocated addresses are used in
>>   call_site info (because the exec is started)
>> - we continue to foo, and manage to find the call_site info
>>
>> In the second case:
>> - we set a breakpoint at foo
>> - this triggers expanding debug info.  Unrelocated addresses are used in
>>   call_site info (because the exec is not started)
>> - we run to foo
>> - this triggers objfile_relocate1, but it doesn't update the call_site
>>   info addresses
>> - we don't manage to find the call_site info
> 
> Thanks for this explanation, I had not realized the difference in
> behavior here.
> 
> The patch LGTM.

Thanks for the review.  Committed.

Thanks,
- Tom

Re: [PATCH 1/4] [gdb/symtab] Fix htab_find_slot call in read_call_site_scope

[Simon Marchi]

On 2021-10-01 08:33, Tom de Vries via Gdb-patches wrote:
> From: Simon Marchi <simon.marchi@polymtl.ca>
> 
> In read_call_site_scope we have:
> ...
>   call_site_local.pc = pc;
>   slot = htab_find_slot (cu->call_site_htab, &call_site_local, INSERT);
> ...
> 
> The call passes a call_site pointer as element.  OTOH, the hashtab is created
> using hash_f == core_addr_hash and eq_f == core_addr_eq, so the element
> will be accessed through a CORE_ADDR pointer.
> 
> This is not wrong (at least in C), given that pc is the first field in
> call_site.
> 
> Nevertheless, as in call_site_for_pc, make the htab_find_slot call match the
> used hash_f and eq_f by using &pc instead:
> ...
>   slot = htab_find_slot (cu->call_site_htab, &pc, INSERT);
> ...
> 
> Tested on x86_64-linux.
> 
> Co-Authored-By: Tom de Vries <tdevries@suse.de>
> ---
>  gdb/dwarf2/read.c | 5 ++---
>  gdb/gdbtypes.h    | 4 +---
>  2 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index 00aa64dd0ab..23870c04e74 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -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;
> @@ -13369,8 +13369,7 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
>      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 "
> diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
> index 2a641122aec..84b751e82e3 100644
> --- a/gdb/gdbtypes.h
> +++ b/gdb/gdbtypes.h
> @@ -1783,9 +1783,7 @@ struct call_site_parameter
>  
>  struct call_site
>    {
> -    /* * 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.  */

Ah, I had not seen this comment.  So it was on purpose.  Still, I think
that it makes it more confusing than anything.  The patch LGTM.

Simon

Re: [PATCH 1/4] [gdb/symtab] Fix htab_find_slot call in read_call_site_scope

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 2716 bytes --]

On 10/1/21 3:09 PM, Simon Marchi wrote:
> 
> 
> On 2021-10-01 08:33, Tom de Vries via Gdb-patches wrote:
>> From: Simon Marchi <simon.marchi@polymtl.ca>
>>
>> In read_call_site_scope we have:
>> ...
>>   call_site_local.pc = pc;
>>   slot = htab_find_slot (cu->call_site_htab, &call_site_local, INSERT);
>> ...
>>
>> The call passes a call_site pointer as element.  OTOH, the hashtab is created
>> using hash_f == core_addr_hash and eq_f == core_addr_eq, so the element
>> will be accessed through a CORE_ADDR pointer.
>>
>> This is not wrong (at least in C), given that pc is the first field in
>> call_site.
>>
>> Nevertheless, as in call_site_for_pc, make the htab_find_slot call match the
>> used hash_f and eq_f by using &pc instead:
>> ...
>>   slot = htab_find_slot (cu->call_site_htab, &pc, INSERT);
>> ...
>>
>> Tested on x86_64-linux.
>>
>> Co-Authored-By: Tom de Vries <tdevries@suse.de>
>> ---
>>  gdb/dwarf2/read.c | 5 ++---
>>  gdb/gdbtypes.h    | 4 +---
>>  2 files changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
>> index 00aa64dd0ab..23870c04e74 100644
>> --- a/gdb/dwarf2/read.c
>> +++ b/gdb/dwarf2/read.c
>> @@ -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;
>> @@ -13369,8 +13369,7 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
>>      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 "
>> diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
>> index 2a641122aec..84b751e82e3 100644
>> --- a/gdb/gdbtypes.h
>> +++ b/gdb/gdbtypes.h
>> @@ -1783,9 +1783,7 @@ struct call_site_parameter
>>  
>>  struct call_site
>>    {
>> -    /* * 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.  */
> 
> Ah, I had not seen this comment.  So it was on purpose.  Still, I think
> that it makes it more confusing than anything.  The patch LGTM.

And, this follow-up commit reverts everything except the comment.

Any comments?

Thanks,
- Tom


[-- Attachment #2: 0001-gdb-symtab-Add-call_site_eq-and-call_site_hash.patch --]
[-- Type: text/x-patch, Size: 3944 bytes --]

[gdb/symtab] Add call_site_eq and call_site_hash

In commit b4c919f7525 "[gdb/symtab] Fix htab_find_slot call in
read_call_site_scope" , I removed the comment:
...
It must be the first field as we overload core_addr_hash and core_addr_eq for
it.
...
for field pc of struct call_site.

However, this was not tested, and when indeed moving field pc to the second
location, we run into a testsuite failure in gdb.trace/entry-values.exp.

This is caused by core_addr_eq (the eq_f function for the htab) being
called with a pointer to the pc field (as passed into htab_find_slot) and a
pointer to a hash table element.  Now that pc is no longer the first field,
the pointer to hash table element no longer points to the pc field.

This could be fixed by simply reinstating the comment, but we're trying to
get rid of this kind of tricks that make refactoring more difficult.

Instead, fix this by:
- reverting commit b4c919f7525, apart from the comment removal, such that
  we're passing a pointer to element to htab_find_slot
- updating the htab_find_slot call in compunit_symtab::find_call_site
  in a similar manner
- adding a call_site_eq and call_site_hash, and using these in the hash table
  instead of core_addr_eq and core_addr_hash.

Tested on x86_64-linux, both with and without a trigger patch that moves pc to
the second location in struct call_site.

---
 gdb/dwarf2/read.c |  7 ++++---
 gdb/gdbtypes.h    | 15 +++++++++++++++
 gdb/symtab.c      |  5 ++++-
 3 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 2d4ca08b667..75d6853fd5b 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -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;
+  struct call_site *call_site, call_site_local;
   void **slot;
   int nparams;
   struct die_info *child_die;
@@ -13366,10 +13366,11 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
   pc = gdbarch_adjust_dwarf2_addr (gdbarch, pc);
 
   if (cu->call_site_htab == NULL)
-    cu->call_site_htab = htab_create_alloc_ex (16, core_addr_hash, core_addr_eq,
+    cu->call_site_htab = htab_create_alloc_ex (16, call_site_hash, call_site_eq,
 					       NULL, &objfile->objfile_obstack,
 					       hashtab_obstack_allocate, NULL);
-  slot = htab_find_slot (cu->call_site_htab, &pc, INSERT);
+  call_site_local.pc = pc;
+  slot = htab_find_slot (cu->call_site_htab, &call_site_local, INSERT);
   if (*slot != NULL)
     {
       complaint (_("Duplicate PC %s for DW_TAG_call_site "
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index 6d09576208d..8021cb21ecc 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -1824,6 +1824,21 @@ struct call_site
     struct call_site_parameter parameter[1];
   };
 
+static inline int
+call_site_eq (const void *a_, const void *b_)
+{
+  const struct call_site *a = (const call_site *)a_;
+  const struct call_site *b = (const call_site *)b_;
+  return core_addr_eq (&a->pc, &b->pc);
+}
+
+static inline hashval_t
+call_site_hash (const void *a_)
+{
+  const struct call_site *a = (const call_site *)a_;
+  return core_addr_hash (&a->pc);
+}
+
 /* The type-specific info for TYPE_CODE_FIXED_POINT types.  */
 
 struct fixed_point_type_info
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 6ec5d95401e..84193ddaae3 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -334,10 +334,13 @@ search_domain_name (enum search_domain e)
 call_site *
 compunit_symtab::find_call_site (CORE_ADDR pc) const
 {
+  struct call_site call_site_local;
   if (m_call_site_htab == nullptr)
     return nullptr;
 
-  void **slot = htab_find_slot (m_call_site_htab, &pc, NO_INSERT);
+  call_site_local.pc = pc;
+  void **slot
+    = htab_find_slot (m_call_site_htab, &call_site_local, NO_INSERT);
   if (slot == nullptr)
     return nullptr;
 

Re: [PATCH 1/4] [gdb/symtab] Fix htab_find_slot call in read_call_site_scope

[Simon Marchi]

>> Ah, I had not seen this comment.  So it was on purpose.  Still, I think
>> that it makes it more confusing than anything.  The patch LGTM.
> 
> And, this follow-up commit reverts everything except the comment.
> 
> Any comments?
> 
> Thanks,
> - Tom
> 

Is there a problem with having the lookups done with just the pc?  If we
were to replace this with some C++ hash table, say std::unordered_map, it
would be std::unordered_map<CORE_ADDR /* pc */, call_site>.  So doing
the lookups using just the pc in the htab makes sense to me.

Simon

Re: [PATCH 1/4] [gdb/symtab] Fix htab_find_slot call in read_call_site_scope

[Tom de Vries]

On 10/4/21 2:05 PM, Simon Marchi wrote:
>>> Ah, I had not seen this comment.  So it was on purpose.  Still, I think
>>> that it makes it more confusing than anything.  The patch LGTM.
>>
>> And, this follow-up commit reverts everything except the comment.
>>
>> Any comments?
>>
>> Thanks,
>> - Tom
>>
> 
> Is there a problem with having the lookups done with just the pc? 

Well, there's the problem that I describe in the commit message.  I
don't known of any other problem.

> If we
> were to replace this with some C++ hash table, say std::unordered_map, it
> would be std::unordered_map<CORE_ADDR /* pc */, call_site>. 

Right, because there's a separation between key and element.

> So doing
> the lookups using just the pc in the htab makes sense to me.

I'm sorry, I don't really understand what you're trying to say here.

Do you agree with the patch?  Do you disagree with the patch.  Are you
suggesting an alternative solution?

Thanks,
- Tom

Re: [PATCH 1/4] [gdb/symtab] Fix htab_find_slot call in read_call_site_scope

[Simon Marchi]

On 2021-10-04 08:46, Tom de Vries wrote:
> On 10/4/21 2:05 PM, Simon Marchi wrote:
>>>> Ah, I had not seen this comment.  So it was on purpose.  Still, I think
>>>> that it makes it more confusing than anything.  The patch LGTM.
>>>
>>> And, this follow-up commit reverts everything except the comment.
>>>
>>> Any comments?
>>>
>>> Thanks,
>>> - Tom
>>>
>>
>> Is there a problem with having the lookups done with just the pc? 
> 
> Well, there's the problem that I describe in the commit message.  I
> don't known of any other problem.
> 
>> If we
>> were to replace this with some C++ hash table, say std::unordered_map, it
>> would be std::unordered_map<CORE_ADDR /* pc */, call_site>. 
> 
> Right, because there's a separation between key and element.
> 
>> So doing
>> the lookups using just the pc in the htab makes sense to me.
> 
> I'm sorry, I don't really understand what you're trying to say here.
> 
> Do you agree with the patch?  Do you disagree with the patch.  Are you
> suggesting an alternative solution?

My thinking was: why not keep core_addr_eq and core_addr_hash, and pass
a pointer to a CORE_ADDR when calling htab_find_slot.  And drop the
requirement for call_site::pc to be the first member of call_site.  But
I now realize that this may not be a correct use of htab: htab_find
passes entries to the eq func.  So if we have core_addr_eq as the eq
func and htab passes a call_site* entry to core_addr_eq, it won't work.
Now, we don't actually use htab_find, so it would still work for us.
But that would be like setting up a trap for whoever tries to use
htab_find in the future.

So, the patche LGTM.

Simon

Re: [PATCH 1/4] [gdb/symtab] Fix htab_find_slot call in read_call_site_scope

[Tom de Vries]

On 10/4/21 5:41 PM, Simon Marchi wrote:
> On 2021-10-04 08:46, Tom de Vries wrote:
>> On 10/4/21 2:05 PM, Simon Marchi wrote:
>>>>> Ah, I had not seen this comment.  So it was on purpose.  Still, I think
>>>>> that it makes it more confusing than anything.  The patch LGTM.
>>>>
>>>> And, this follow-up commit reverts everything except the comment.
>>>>
>>>> Any comments?
>>>>
>>>> Thanks,
>>>> - Tom
>>>>
>>>
>>> Is there a problem with having the lookups done with just the pc? 
>>
>> Well, there's the problem that I describe in the commit message.  I
>> don't known of any other problem.
>>
>>> If we
>>> were to replace this with some C++ hash table, say std::unordered_map, it
>>> would be std::unordered_map<CORE_ADDR /* pc */, call_site>. 
>>
>> Right, because there's a separation between key and element.
>>
>>> So doing
>>> the lookups using just the pc in the htab makes sense to me.
>>
>> I'm sorry, I don't really understand what you're trying to say here.
>>
>> Do you agree with the patch?  Do you disagree with the patch.  Are you
>> suggesting an alternative solution?
> 
> My thinking was: why not keep core_addr_eq and core_addr_hash, and pass
> a pointer to a CORE_ADDR when calling htab_find_slot.  And drop the
> requirement for call_site::pc to be the first member of call_site.  But
> I now realize that this may not be a correct use of htab: htab_find
> passes entries to the eq func.  So if we have core_addr_eq as the eq
> func and htab passes a call_site* entry to core_addr_eq, it won't work.
> Now, we don't actually use htab_find, so it would still work for us.
> But that would be like setting up a trap for whoever tries to use
> htab_find in the future.
> 

All find variants (with the explicit exception of
find_empty_slot_for_expand) call eq_f on both an element argument and a
hash table element.  Consequently, if we break first-field-type
compatibility between the two, the element argument must be of the same
type as the hash table element, and the eq function must use hash table
element type.

This is not a future problem, this is an actual problem I ran into when
I tried it out moving the pc to the second field position, and the
problem is described in the commit log.

> So, the patche LGTM.

Ack, thanks for the review, I'll commit.

Thanks,
- Tom

Re: [PATCH 1/4] [gdb/symtab] Fix htab_find_slot call in read_call_site_scope

[Simon Marchi]

> All find variants (with the explicit exception of
> find_empty_slot_for_expand) call eq_f on both an element argument and a
> hash table element.  Consequently, if we break first-field-type
> compatibility between the two, the element argument must be of the same
> type as the hash table element, and the eq function must use hash table
> element type.
> 
> This is not a future problem, this is an actual problem I ran into when
> I tried it out moving the pc to the second field position, and the
> problem is described in the commit log.

That makes sense.  I read the hashtab code wrong.

Simon