[PATCH 0/6] Handle split functions in call site chains

[PATCH 0/6] Handle split functions in call site chains

[Tom Tromey]

This series fixes a bug in DW_OP_entry_value handling.

A large customer program that is compiled with optimization has a
function that is split into hot and cold parts.  The DWARF uses
DW_AT_ranges to represent this.

However, the call site code only looks at the low and high PCs.  This
means that the entry value computation fails to find the correct outer
frame, and so cannot evaluate a certain local variable.

This series changes the code to record multiple addresses in a call
site target.  This change lets this particular case work.

Some of this series is refactoring to make it simpler to deal with
call_site_target.

Patch #4 could use an extra examination, both because I convert
explicit state management to recursion (IMO ok because these call
chains tend to be short); but also because I think I found a bug in
the current code.

Regression tested on x86-64 Fedora 34.
Let me know what you think.

Tom

[PATCH 1/6] Change call_site_target to use custom type and enum

[Tom Tromey]

call_site_target reuses field_loc_kind and field_location.  However,
it has never used the full range of the field_loc_kind enum.  In a
subsequent patch, I plan to add a new 'kind' here, so it seemed best
to avoid this reuse and instead introduce new types here.
---
 gdb/dwarf2/loc.c |  6 +++---
 gdb/gdbtypes.h   | 38 ++++++++++++++++++++++++++++----------
 2 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c
index 182f15e7077..1d49e7960f1 100644
--- a/gdb/dwarf2/loc.c
+++ b/gdb/dwarf2/loc.c
@@ -643,7 +643,7 @@ call_site_to_target_addr (struct gdbarch *call_site_gdbarch,
 {
   switch (call_site->target.loc_kind ())
     {
-    case FIELD_LOC_KIND_DWARF_BLOCK:
+    case call_site_target::DWARF_BLOCK:
       {
 	struct dwarf2_locexpr_baton *dwarf_block;
 	struct value *val;
@@ -690,7 +690,7 @@ call_site_to_target_addr (struct gdbarch *call_site_gdbarch,
 	  return value_as_address (val);
       }
 
-    case FIELD_LOC_KIND_PHYSNAME:
+    case call_site_target::PHYSNAME:
       {
 	const char *physname;
 	struct bound_minimal_symbol msym;
@@ -713,7 +713,7 @@ call_site_to_target_addr (struct gdbarch *call_site_gdbarch,
 	return BMSYMBOL_VALUE_ADDRESS (msym);
       }
 
-    case FIELD_LOC_KIND_PHYSADDR:
+    case call_site_target::PHYSADDR:
       {
 	dwarf2_per_objfile *per_objfile = call_site->per_objfile;
 	compunit_symtab *cust = per_objfile->get_symtab (call_site->per_cu);
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index 5284a4c3a03..48d1658613f 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -1810,52 +1810,70 @@ enum call_site_parameter_kind
 
 struct call_site_target
 {
-  field_loc_kind loc_kind () const
+  /* The kind of location held by this call site target.  */
+  enum kind
+  {
+    /* An address.  */
+    PHYSADDR,
+    /* A name.  */
+    PHYSNAME,
+    /* A DWARF block.  */
+    DWARF_BLOCK,
+  };
+
+  kind loc_kind () const
   {
     return m_loc_kind;
   }
 
   CORE_ADDR loc_physaddr () const
   {
-    gdb_assert (m_loc_kind == FIELD_LOC_KIND_PHYSADDR);
+    gdb_assert (m_loc_kind == PHYSADDR);
     return m_loc.physaddr;
   }
 
   void set_loc_physaddr (CORE_ADDR physaddr)
   {
-    m_loc_kind = FIELD_LOC_KIND_PHYSADDR;
+    m_loc_kind = PHYSADDR;
     m_loc.physaddr = physaddr;
   }
 
   const char *loc_physname () const
   {
-    gdb_assert (m_loc_kind == FIELD_LOC_KIND_PHYSNAME);
+    gdb_assert (m_loc_kind == PHYSNAME);
     return m_loc.physname;
   }
 
   void set_loc_physname (const char *physname)
     {
-      m_loc_kind = FIELD_LOC_KIND_PHYSNAME;
+      m_loc_kind = PHYSNAME;
       m_loc.physname = physname;
     }
 
   dwarf2_locexpr_baton *loc_dwarf_block () const
   {
-    gdb_assert (m_loc_kind == FIELD_LOC_KIND_DWARF_BLOCK);
+    gdb_assert (m_loc_kind == DWARF_BLOCK);
     return m_loc.dwarf_block;
   }
 
   void set_loc_dwarf_block (dwarf2_locexpr_baton *dwarf_block)
     {
-      m_loc_kind = FIELD_LOC_KIND_DWARF_BLOCK;
+      m_loc_kind = DWARF_BLOCK;
       m_loc.dwarf_block = dwarf_block;
     }
 
-  union field_location m_loc;
+  union
+  {
+    /* Address.  */
+    CORE_ADDR physaddr;
+    /* Mangled name.  */
+    const char *physname;
+    /* DWARF block.  */
+    struct dwarf2_locexpr_baton *dwarf_block;
+  } m_loc;
 
   /* * Discriminant for union field_location.  */
-
-  ENUM_BITFIELD(field_loc_kind) m_loc_kind : 3;
+  enum kind m_loc_kind;
 };
 
 union call_site_parameter_u
-- 
2.31.1

[PATCH 2/6] Make call_site_target members private

[Tom Tromey]

This makes the data members of call_site_target 'private'.  This lets
us remove most of its public API.  call_site_to_target_addr is changed
to be a method of this type.  This is a preparatory refactoring for
the fix at the end of this series.
---
 gdb/dwarf2/loc.c | 26 ++++++++++++--------------
 gdb/gdbtypes.h   | 44 +++++++++++++++++++++-----------------------
 2 files changed, 33 insertions(+), 37 deletions(-)

diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c
index 1d49e7960f1..7a3fda5eac1 100644
--- a/gdb/dwarf2/loc.c
+++ b/gdb/dwarf2/loc.c
@@ -632,16 +632,14 @@ show_entry_values_debug (struct ui_file *file, int from_tty,
 		    value);
 }
 
-/* Find DW_TAG_call_site's DW_AT_call_target address.
-   CALLER_FRAME (for registers) can be NULL if it is not known.  This function
-   always returns valid address or it throws NO_ENTRY_VALUE_ERROR.  */
+/* See gdbtypes.h.  */
 
-static CORE_ADDR
-call_site_to_target_addr (struct gdbarch *call_site_gdbarch,
-			  struct call_site *call_site,
-			  struct frame_info *caller_frame)
+CORE_ADDR
+call_site_target::address (struct gdbarch *call_site_gdbarch,
+			   const struct call_site *call_site,
+			   struct frame_info *caller_frame) const
 {
-  switch (call_site->target.loc_kind ())
+  switch (m_loc_kind)
     {
     case call_site_target::DWARF_BLOCK:
       {
@@ -650,7 +648,7 @@ call_site_to_target_addr (struct gdbarch *call_site_gdbarch,
 	struct type *caller_core_addr_type;
 	struct gdbarch *caller_arch;
 
-	dwarf_block = call_site->target.loc_dwarf_block ();
+	dwarf_block = m_loc.dwarf_block;
 	if (dwarf_block == NULL)
 	  {
 	    struct bound_minimal_symbol msym;
@@ -695,7 +693,7 @@ call_site_to_target_addr (struct gdbarch *call_site_gdbarch,
 	const char *physname;
 	struct bound_minimal_symbol msym;
 
-	physname = call_site->target.loc_physname ();
+	physname = m_loc.physname;
 
 	/* Handle both the mangled and demangled PHYSNAME.  */
 	msym = lookup_minimal_symbol (physname, NULL, NULL);
@@ -720,7 +718,7 @@ call_site_to_target_addr (struct gdbarch *call_site_gdbarch,
 	int sect_idx = COMPUNIT_BLOCK_LINE_SECTION (cust);
 	CORE_ADDR delta = per_objfile->objfile->section_offsets[sect_idx];
 
-	return call_site->target.loc_physaddr () + delta;
+	return m_loc.physaddr + delta;
       }
 
     default:
@@ -790,7 +788,7 @@ func_verify_no_selftailcall (struct gdbarch *gdbarch, CORE_ADDR verify_addr)
 
 	  /* CALLER_FRAME with registers is not available for tail-call jumped
 	     frames.  */
-	  target_addr = call_site_to_target_addr (gdbarch, call_site, NULL);
+	  target_addr = call_site->address (gdbarch, nullptr);
 
 	  if (target_addr == verify_addr)
 	    {
@@ -968,7 +966,7 @@ call_site_find_chain_1 (struct gdbarch *gdbarch, CORE_ADDR caller_pc,
 
       /* CALLER_FRAME with registers is not available for tail-call jumped
 	 frames.  */
-      target_func_addr = call_site_to_target_addr (gdbarch, call_site, NULL);
+      target_func_addr = call_site->address (gdbarch, nullptr);
 
       if (target_func_addr == callee_pc)
 	{
@@ -1159,7 +1157,7 @@ dwarf_expr_reg_to_entry_parameter (struct frame_info *frame,
   caller_pc = get_frame_pc (caller_frame);
   call_site = call_site_for_pc (gdbarch, caller_pc);
 
-  target_addr = call_site_to_target_addr (gdbarch, call_site, caller_frame);
+  target_addr = call_site->address (gdbarch, caller_frame);
   if (target_addr != func_addr)
     {
       struct minimal_symbol *target_msym, *func_msym;
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index 48d1658613f..d2b5483c893 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -1821,47 +1821,35 @@ struct call_site_target
     DWARF_BLOCK,
   };
 
-  kind loc_kind () const
-  {
-    return m_loc_kind;
-  }
-
-  CORE_ADDR loc_physaddr () const
-  {
-    gdb_assert (m_loc_kind == PHYSADDR);
-    return m_loc.physaddr;
-  }
-
   void set_loc_physaddr (CORE_ADDR physaddr)
   {
     m_loc_kind = PHYSADDR;
     m_loc.physaddr = physaddr;
   }
 
-  const char *loc_physname () const
-  {
-    gdb_assert (m_loc_kind == PHYSNAME);
-    return m_loc.physname;
-  }
-
   void set_loc_physname (const char *physname)
     {
       m_loc_kind = PHYSNAME;
       m_loc.physname = physname;
     }
 
-  dwarf2_locexpr_baton *loc_dwarf_block () const
-  {
-    gdb_assert (m_loc_kind == DWARF_BLOCK);
-    return m_loc.dwarf_block;
-  }
-
   void set_loc_dwarf_block (dwarf2_locexpr_baton *dwarf_block)
     {
       m_loc_kind = DWARF_BLOCK;
       m_loc.dwarf_block = dwarf_block;
     }
 
+  /* Find DW_TAG_call_site's DW_AT_call_target address.  CALLER_FRAME
+     (for registers) can be NULL if it is not known.  This function
+     always returns valid address or it throws
+     NO_ENTRY_VALUE_ERROR.  */
+
+  CORE_ADDR address (struct gdbarch *call_site_gdbarch,
+		     const struct call_site *call_site,
+		     struct frame_info *caller_frame) const;
+
+private:
+
   union
   {
     /* Address.  */
@@ -1953,6 +1941,16 @@ struct call_site
 
     CORE_ADDR pc () const;
 
+    /* Find the target address.  CALLER_FRAME (for registers) can be
+       NULL if it is not known.  This function always returns valid
+       address or it throws NO_ENTRY_VALUE_ERROR.  */
+
+    CORE_ADDR address (struct gdbarch *call_site_gdbarch,
+		       struct frame_info *caller_frame) const
+    {
+      return target.address (call_site_gdbarch, this, caller_frame);
+    }
+
     /* * List successor with head in FUNC_TYPE.TAIL_CALL_LIST.  */
 
     struct call_site *tail_call_next = nullptr;
-- 
2.31.1

[PATCH 3/6] Constify chain_candidate

[Tom Tromey]

While investigating this bug, I wasn't sure if chain_candidate might
update 'chain'.  I changed it to accept a const reference, making it
clear that it cannot.  This simplifies the code a tiny bit as well.
---
 gdb/dwarf2/loc.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c
index 7a3fda5eac1..e793bbffd05 100644
--- a/gdb/dwarf2/loc.c
+++ b/gdb/dwarf2/loc.c
@@ -835,9 +835,9 @@ tailcall_dump (struct gdbarch *gdbarch, const struct call_site *call_site)
 static void
 chain_candidate (struct gdbarch *gdbarch,
 		 gdb::unique_xmalloc_ptr<struct call_site_chain> *resultp,
-		 std::vector<struct call_site *> *chain)
+		 const std::vector<struct call_site *> &chain)
 {
-  long length = chain->size ();
+  long length = chain.size ();
   int callers, callees, idx;
 
   if (*resultp == NULL)
@@ -850,8 +850,8 @@ chain_candidate (struct gdbarch *gdbarch,
 		    + sizeof (*result->call_site) * (length - 1)));
       result->length = length;
       result->callers = result->callees = length;
-      if (!chain->empty ())
-	memcpy (result->call_site, chain->data (),
+      if (!chain.empty ())
+	memcpy (result->call_site, chain.data (),
 		sizeof (*result->call_site) * length);
       resultp->reset (result);
 
@@ -870,7 +870,7 @@ chain_candidate (struct gdbarch *gdbarch,
     {
       fprintf_unfiltered (gdb_stdlog, "tailcall: compare:");
       for (idx = 0; idx < length; idx++)
-	tailcall_dump (gdbarch, chain->at (idx));
+	tailcall_dump (gdbarch, chain[idx]);
       fputc_unfiltered ('\n', gdb_stdlog);
     }
 
@@ -878,7 +878,7 @@ chain_candidate (struct gdbarch *gdbarch,
 
   callers = std::min ((long) (*resultp)->callers, length);
   for (idx = 0; idx < callers; idx++)
-    if ((*resultp)->call_site[idx] != chain->at (idx))
+    if ((*resultp)->call_site[idx] != chain[idx])
       {
 	(*resultp)->callers = idx;
 	break;
@@ -889,7 +889,7 @@ chain_candidate (struct gdbarch *gdbarch,
   callees = std::min ((long) (*resultp)->callees, length);
   for (idx = 0; idx < callees; idx++)
     if ((*resultp)->call_site[(*resultp)->length - 1 - idx]
-	!= chain->at (length - 1 - idx))
+	!= chain[length - 1 - idx])
       {
 	(*resultp)->callees = idx;
 	break;
@@ -970,7 +970,7 @@ call_site_find_chain_1 (struct gdbarch *gdbarch, CORE_ADDR caller_pc,
 
       if (target_func_addr == callee_pc)
 	{
-	  chain_candidate (gdbarch, &retval, &chain);
+	  chain_candidate (gdbarch, &retval, chain);
 	  if (retval == NULL)
 	    break;
 
-- 
2.31.1

[PATCH 4/6] Change call_site_find_chain_1 to work recursively

[Tom Tromey]

call_site_find_chain_1 has a comment claiming that recursive calls
would be too expensive.  However, I doubt this is so expensive; and
furthermore the explicit state management approach here is difficult
both to understand and to modify.  This patch changes this code to use
explicit recursion, so that a subsequent patch can generalize this
code without undue trauma.

Additionally, I think this patch detects a latent bug in the recursion
code.  (It's hard for me to be completely certain.)  The bug is that
when a new target_call_site is entered, the code does:

	  if (target_call_site)
	    {
	      if (addr_hash.insert (target_call_site->pc ()).second)
		{
		  /* Successfully entered TARGET_CALL_SITE.  */

		  chain.push_back (target_call_site);
		  break;
		}
	    }

Here, if entering the target_call_site fails, then any tail_call_next
elements in this call site are not visited.  However, if this code
does happen to enter a call site, then the tail_call_next elements
will be visited during backtracking.  This applies when doing the
backtracking as well -- it will only continue through a given chain as
long as each element in the chain can successfully be visited.

I'd appreciate some review of this.  If this behavior is intentional,
it can be added to the new implementation.
---
 gdb/dwarf2/loc.c                             | 136 +++++++++----------
 gdb/testsuite/gdb.arch/amd64-entry-value.exp |   2 +-
 2 files changed, 64 insertions(+), 74 deletions(-)

diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c
index e793bbffd05..398538b54f8 100644
--- a/gdb/dwarf2/loc.c
+++ b/gdb/dwarf2/loc.c
@@ -924,11 +924,65 @@ chain_candidate (struct gdbarch *gdbarch,
   gdb_assert ((*resultp)->callers + (*resultp)->callees <= (*resultp)->length);
 }
 
-/* Create and return call_site_chain for CALLER_PC and CALLEE_PC.  All the
-   assumed frames between them use GDBARCH.  Use depth first search so we can
-   keep single CHAIN of call_site's back to CALLER_PC.  Function recursion
-   would have needless GDB stack overhead.  Any unreliability results
-   in thrown NO_ENTRY_VALUE_ERROR.  */
+/* Recursively try to construct the call chain.  GDBARCH, RESULTP, and
+   CHAIN are passed to chain_candidate.  ADDR_HASH tracks which
+   addresses have already been seen along the current chain.
+   CALL_SITE is the call site to visit, and CALLEE_PC is the PC we're
+   trying to "reach".  Returns false if an error has already been
+   detected and so an early return can be done.  If it makes sense to
+   keep trying (even if no answer has yet been found), returns
+   true.  */
+
+static bool
+call_site_find_chain_2
+     (struct gdbarch *gdbarch,
+      gdb::unique_xmalloc_ptr<struct call_site_chain> *resultp,
+      std::vector<struct call_site *> &chain,
+      std::unordered_set<CORE_ADDR> &addr_hash,
+      struct call_site *call_site,
+      CORE_ADDR callee_pc)
+{
+  /* CALLER_FRAME with registers is not available for tail-call jumped
+     frames.  */
+  CORE_ADDR target_func_addr = call_site->address (gdbarch, nullptr);
+
+  if (target_func_addr == callee_pc)
+    {
+      chain_candidate (gdbarch, resultp, chain);
+      /* If RESULTP was reset, then chain_candidate failed, and so we
+	 can tell our callers to early-return.  */
+      return *resultp != nullptr;
+    }
+
+  struct symbol *target_func
+    = func_addr_to_tail_call_list (gdbarch, target_func_addr);
+  for (struct call_site *target_call_site
+	 = TYPE_TAIL_CALL_LIST (SYMBOL_TYPE (target_func));
+       target_call_site != nullptr;
+       target_call_site = target_call_site->tail_call_next)
+    {
+      if (addr_hash.insert (target_call_site->pc ()).second)
+	{
+	  /* Successfully entered TARGET_CALL_SITE.  */
+	  chain.push_back (target_call_site);
+
+	  if (!call_site_find_chain_2 (gdbarch, resultp, chain,
+				       addr_hash, target_call_site,
+				       callee_pc))
+	    return false;
+
+	  size_t removed = addr_hash.erase (target_call_site->pc ());
+	  gdb_assert (removed == 1);
+	  chain.pop_back ();
+	}
+    }
+
+  return true;
+}
+
+/* Create and return call_site_chain for CALLER_PC and CALLEE_PC.  All
+   the assumed frames between them use GDBARCH.  Any unreliability
+   results in thrown NO_ENTRY_VALUE_ERROR.  */
 
 static gdb::unique_xmalloc_ptr<call_site_chain>
 call_site_find_chain_1 (struct gdbarch *gdbarch, CORE_ADDR caller_pc,
@@ -958,74 +1012,10 @@ call_site_find_chain_1 (struct gdbarch *gdbarch, CORE_ADDR caller_pc,
      target's function will get iterated as already pushed into CHAIN via their
      TAIL_CALL_NEXT.  */
   call_site = call_site_for_pc (gdbarch, caller_pc);
-
-  while (call_site)
-    {
-      CORE_ADDR target_func_addr;
-      struct call_site *target_call_site;
-
-      /* CALLER_FRAME with registers is not available for tail-call jumped
-	 frames.  */
-      target_func_addr = call_site->address (gdbarch, nullptr);
-
-      if (target_func_addr == callee_pc)
-	{
-	  chain_candidate (gdbarch, &retval, chain);
-	  if (retval == NULL)
-	    break;
-
-	  /* There is no way to reach CALLEE_PC again as we would prevent
-	     entering it twice as being already marked in ADDR_HASH.  */
-	  target_call_site = NULL;
-	}
-      else
-	{
-	  struct symbol *target_func;
-
-	  target_func = func_addr_to_tail_call_list (gdbarch, target_func_addr);
-	  target_call_site = TYPE_TAIL_CALL_LIST (SYMBOL_TYPE (target_func));
-	}
-
-      do
-	{
-	  /* Attempt to visit TARGET_CALL_SITE.  */
-
-	  if (target_call_site)
-	    {
-	      if (addr_hash.insert (target_call_site->pc ()).second)
-		{
-		  /* Successfully entered TARGET_CALL_SITE.  */
-
-		  chain.push_back (target_call_site);
-		  break;
-		}
-	    }
-
-	  /* Backtrack (without revisiting the originating call_site).  Try the
-	     callers's sibling; if there isn't any try the callers's callers's
-	     sibling etc.  */
-
-	  target_call_site = NULL;
-	  while (!chain.empty ())
-	    {
-	      call_site = chain.back ();
-	      chain.pop_back ();
-
-	      size_t removed = addr_hash.erase (call_site->pc ());
-	      gdb_assert (removed == 1);
-
-	      target_call_site = call_site->tail_call_next;
-	      if (target_call_site)
-		break;
-	    }
-	}
-      while (target_call_site);
-
-      if (chain.empty ())
-	call_site = NULL;
-      else
-	call_site = chain.back ();
-    }
+  /* No need to check the return value here, because we no longer care
+     about possible early returns.  */
+  call_site_find_chain_2 (gdbarch, &retval, chain, addr_hash, call_site,
+			  callee_pc);
 
   if (retval == NULL)
     {
diff --git a/gdb/testsuite/gdb.arch/amd64-entry-value.exp b/gdb/testsuite/gdb.arch/amd64-entry-value.exp
index fdfa4a01b58..386388d71b4 100644
--- a/gdb/testsuite/gdb.arch/amd64-entry-value.exp
+++ b/gdb/testsuite/gdb.arch/amd64-entry-value.exp
@@ -253,7 +253,7 @@ gdb_test "bt" "^bt\r\n#0 +d \\(i=<optimized out>, j=<optimized out>\\)\[^\r\n\]*
 
 gdb_continue_to_breakpoint "self: breakhere"
 
-gdb_test "bt" "^bt\r\n#0 +d \\(i=<optimized out>, j=<optimized out>\\)\[^\r\n\]*\r\n#1 +0x\[0-9a-f\]+ in self \\(i=<optimized out>\\)\[^\r\n\]*\r\n#2 +0x\[0-9a-f\]+ in main \\(\\)\[^\r\n\]*" \
+gdb_test "bt" "^bt\r\n#0 +d \\(i=<optimized out>, j=<optimized out>\\)\[^\r\n\]*\r\n#1 +0x\[0-9a-f\]+ in self \\(i=<optimized out>\\)\[^\r\n\]*\r\n#2 +0x\[0-9a-f\]+ in self2 \\(i=<optimized out>\\)\[^\r\n\]*\r\n#3 +0x\[0-9a-f\]+ in self \\(i=<optimized out>\\)\[^\r\n\]*\r\n#4 +0x\[0-9a-f\]+ in main \\(\\)\[^\r\n\]*" \
 	 "self: bt"
 
 gdb_test_no_output "set debug entry-values 1"
-- 
2.31.1

[PATCH 5/6] Change call_site_target to iterate over addresses

[Tom Tromey]

In order to handle the case where a call site target might refer to
multiple addresses, we change the code to use a callback style.  Any
spot using call_site_target::address now passes in a callback function
that may be called multiple times.
---
 gdb/dwarf2/loc.c | 134 +++++++++++++++++++++++++++++------------------
 gdb/gdbtypes.h   |  34 +++++++-----
 2 files changed, 105 insertions(+), 63 deletions(-)

diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c
index 398538b54f8..e1001eb7adb 100644
--- a/gdb/dwarf2/loc.c
+++ b/gdb/dwarf2/loc.c
@@ -634,10 +634,12 @@ show_entry_values_debug (struct ui_file *file, int from_tty,
 
 /* See gdbtypes.h.  */
 
-CORE_ADDR
-call_site_target::address (struct gdbarch *call_site_gdbarch,
-			   const struct call_site *call_site,
-			   struct frame_info *caller_frame) const
+void
+call_site_target::iterate_over_addresses
+     (struct gdbarch *call_site_gdbarch,
+      const struct call_site *call_site,
+      struct frame_info *caller_frame,
+      iterate_ftype callback) const
 {
   switch (m_loc_kind)
     {
@@ -683,10 +685,11 @@ call_site_target::address (struct gdbarch *call_site_gdbarch,
 					dwarf_block->per_objfile);
 	/* DW_AT_call_target is a DWARF expression, not a DWARF location.  */
 	if (VALUE_LVAL (val) == lval_memory)
-	  return value_address (val);
+	  callback (value_address (val));
 	else
-	  return value_as_address (val);
+	  callback (value_as_address (val));
       }
+      break;
 
     case call_site_target::PHYSNAME:
       {
@@ -708,8 +711,9 @@ call_site_target::address (struct gdbarch *call_site_gdbarch,
 			  : msym.minsym->print_name ()));
 			
 	  }
-	return BMSYMBOL_VALUE_ADDRESS (msym);
+	callback (BMSYMBOL_VALUE_ADDRESS (msym));
       }
+      break;
 
     case call_site_target::PHYSADDR:
       {
@@ -718,8 +722,9 @@ call_site_target::address (struct gdbarch *call_site_gdbarch,
 	int sect_idx = COMPUNIT_BLOCK_LINE_SECTION (cust);
 	CORE_ADDR delta = per_objfile->objfile->section_offsets[sect_idx];
 
-	return m_loc.physaddr + delta;
+	callback (m_loc.physaddr + delta);
       }
+      break;
 
     default:
       internal_error (__FILE__, __LINE__, _("invalid call site target kind"));
@@ -784,28 +789,28 @@ func_verify_no_selftailcall (struct gdbarch *gdbarch, CORE_ADDR verify_addr)
       for (call_site = TYPE_TAIL_CALL_LIST (SYMBOL_TYPE (func_sym));
 	   call_site; call_site = call_site->tail_call_next)
 	{
-	  CORE_ADDR target_addr;
-
 	  /* CALLER_FRAME with registers is not available for tail-call jumped
 	     frames.  */
-	  target_addr = call_site->address (gdbarch, nullptr);
-
-	  if (target_addr == verify_addr)
+	  call_site->iterate_over_addresses (gdbarch, nullptr,
+					     [&] (CORE_ADDR target_addr)
 	    {
-	      struct bound_minimal_symbol msym;
-	      
-	      msym = lookup_minimal_symbol_by_pc (verify_addr);
-	      throw_error (NO_ENTRY_VALUE_ERROR,
-			   _("DW_OP_entry_value resolving has found "
-			     "function \"%s\" at %s can call itself via tail "
-			     "calls"),
-			   (msym.minsym == NULL ? "???"
-			    : msym.minsym->print_name ()),
-			   paddress (gdbarch, verify_addr));
-	    }
-
-	  if (addr_hash.insert (target_addr).second)
-	    todo.push_back (target_addr);
+	      if (target_addr == verify_addr)
+		{
+		  struct bound_minimal_symbol msym;
+
+		  msym = lookup_minimal_symbol_by_pc (verify_addr);
+		  throw_error (NO_ENTRY_VALUE_ERROR,
+			       _("DW_OP_entry_value resolving has found "
+				 "function \"%s\" at %s can call itself via tail "
+				 "calls"),
+			       (msym.minsym == NULL ? "???"
+				: msym.minsym->print_name ()),
+			       paddress (gdbarch, verify_addr));
+		}
+
+	      if (addr_hash.insert (target_addr).second)
+		todo.push_back (target_addr);
+	    });
 	}
     }
 }
@@ -942,11 +947,18 @@ call_site_find_chain_2
       struct call_site *call_site,
       CORE_ADDR callee_pc)
 {
-  /* CALLER_FRAME with registers is not available for tail-call jumped
-     frames.  */
-  CORE_ADDR target_func_addr = call_site->address (gdbarch, nullptr);
+  std::vector<CORE_ADDR> addresses;
+  bool found_exact = false;
+  call_site->iterate_over_addresses (gdbarch, nullptr,
+				     [&] (CORE_ADDR addr)
+    {
+      if (addr == callee_pc)
+	found_exact = true;
+      else
+	addresses.push_back (addr);
+    });
 
-  if (target_func_addr == callee_pc)
+  if (found_exact)
     {
       chain_candidate (gdbarch, resultp, chain);
       /* If RESULTP was reset, then chain_candidate failed, and so we
@@ -954,26 +966,29 @@ call_site_find_chain_2
       return *resultp != nullptr;
     }
 
-  struct symbol *target_func
-    = func_addr_to_tail_call_list (gdbarch, target_func_addr);
-  for (struct call_site *target_call_site
-	 = TYPE_TAIL_CALL_LIST (SYMBOL_TYPE (target_func));
-       target_call_site != nullptr;
-       target_call_site = target_call_site->tail_call_next)
+  for (CORE_ADDR target_func_addr : addresses)
     {
-      if (addr_hash.insert (target_call_site->pc ()).second)
+      struct symbol *target_func
+	= func_addr_to_tail_call_list (gdbarch, target_func_addr);
+      for (struct call_site *target_call_site
+	     = TYPE_TAIL_CALL_LIST (SYMBOL_TYPE (target_func));
+	   target_call_site != nullptr;
+	   target_call_site = target_call_site->tail_call_next)
 	{
-	  /* Successfully entered TARGET_CALL_SITE.  */
-	  chain.push_back (target_call_site);
+	  if (addr_hash.insert (target_call_site->pc ()).second)
+	    {
+	      /* Successfully entered TARGET_CALL_SITE.  */
+	      chain.push_back (target_call_site);
 
-	  if (!call_site_find_chain_2 (gdbarch, resultp, chain,
-				       addr_hash, target_call_site,
-				       callee_pc))
-	    return false;
+	      if (!call_site_find_chain_2 (gdbarch, resultp, chain,
+					   addr_hash, target_call_site,
+					   callee_pc))
+		return false;
 
-	  size_t removed = addr_hash.erase (target_call_site->pc ());
-	  gdb_assert (removed == 1);
-	  chain.pop_back ();
+	      size_t removed = addr_hash.erase (target_call_site->pc ());
+	      gdb_assert (removed == 1);
+	      chain.pop_back ();
+	    }
 	}
     }
 
@@ -998,6 +1013,12 @@ call_site_find_chain_1 (struct gdbarch *gdbarch, CORE_ADDR caller_pc,
      TAIL_CALL_NEXT.  This is inappropriate for CALLER_PC's call_site.  */
   std::vector<struct call_site *> chain;
 
+  /* A given call site may have multiple associated addresses.  This
+     can happen if, e.g., the caller is split by hot/cold
+     partitioning.  This vector tracks the ones we haven't visited
+     yet.  */
+  std::vector<std::vector<CORE_ADDR>> unvisited_addresses;
+
   /* We are not interested in the specific PC inside the callee function.  */
   callee_pc = get_pc_function_start (callee_pc);
   if (callee_pc == 0)
@@ -1147,19 +1168,32 @@ dwarf_expr_reg_to_entry_parameter (struct frame_info *frame,
   caller_pc = get_frame_pc (caller_frame);
   call_site = call_site_for_pc (gdbarch, caller_pc);
 
-  target_addr = call_site->address (gdbarch, caller_frame);
-  if (target_addr != func_addr)
+  bool found = false;
+  unsigned count = 0;
+  call_site->iterate_over_addresses (gdbarch, caller_frame,
+				     [&] (CORE_ADDR addr)
+    {
+      /* Preserve any address.  */
+      target_addr = addr;
+      ++count;
+      if (addr == func_addr)
+	found = true;
+    });
+  if (!found)
     {
       struct minimal_symbol *target_msym, *func_msym;
 
       target_msym = lookup_minimal_symbol_by_pc (target_addr).minsym;
       func_msym = lookup_minimal_symbol_by_pc (func_addr).minsym;
       throw_error (NO_ENTRY_VALUE_ERROR,
-		   _("DW_OP_entry_value resolving expects callee %s at %s "
+		   _("DW_OP_entry_value resolving expects callee %s at %s %s"
 		     "but the called frame is for %s at %s"),
 		   (target_msym == NULL ? "???"
 					: target_msym->print_name ()),
 		   paddress (gdbarch, target_addr),
+		   (count > 0
+		    ? _("(but note there are multiple addresses not listed)")
+		    : ""),
 		   func_msym == NULL ? "???" : func_msym->print_name (),
 		   paddress (gdbarch, func_addr));
     }
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index d2b5483c893..eaa3cd0dcb7 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -51,6 +51,7 @@
 #include "gdbsupport/enum-flags.h"
 #include "gdbsupport/underlying.h"
 #include "gdbsupport/print-utils.h"
+#include "gdbsupport/function-view.h"
 #include "dwarf2.h"
 #include "gdb_obstack.h"
 #include "gmp-utils.h"
@@ -1839,14 +1840,18 @@ struct call_site_target
       m_loc.dwarf_block = dwarf_block;
     }
 
-  /* Find DW_TAG_call_site's DW_AT_call_target address.  CALLER_FRAME
-     (for registers) can be NULL if it is not known.  This function
-     always returns valid address or it throws
-     NO_ENTRY_VALUE_ERROR.  */
+  /* Callback type for iterate_over_addresses.  */
 
-  CORE_ADDR address (struct gdbarch *call_site_gdbarch,
-		     const struct call_site *call_site,
-		     struct frame_info *caller_frame) const;
+  using iterate_ftype = gdb::function_view<void (CORE_ADDR)>;
+
+  /* Call CALLBACK for each DW_TAG_call_site's DW_AT_call_target
+     address.  CALLER_FRAME (for registers) can be NULL if it is not
+     known.  This function always may throw NO_ENTRY_VALUE_ERROR.  */
+
+  void iterate_over_addresses (struct gdbarch *call_site_gdbarch,
+			       const struct call_site *call_site,
+			       struct frame_info *caller_frame,
+			       iterate_ftype callback) const;
 
 private:
 
@@ -1941,14 +1946,17 @@ struct call_site
 
     CORE_ADDR pc () const;
 
-    /* Find the target address.  CALLER_FRAME (for registers) can be
-       NULL if it is not known.  This function always returns valid
-       address or it throws NO_ENTRY_VALUE_ERROR.  */
+    /* Call CALLBACK for each target address.  CALLER_FRAME (for
+       registers) can be NULL if it is not known.  This function may
+       throw NO_ENTRY_VALUE_ERROR.  */
 
-    CORE_ADDR address (struct gdbarch *call_site_gdbarch,
-		       struct frame_info *caller_frame) const
+    void iterate_over_addresses (struct gdbarch *call_site_gdbarch,
+				 struct frame_info *caller_frame,
+				 call_site_target::iterate_ftype callback)
+      const
     {
-      return target.address (call_site_gdbarch, this, caller_frame);
+      return target.iterate_over_addresses (call_site_gdbarch, this,
+					    caller_frame, callback);
     }
 
     /* * List successor with head in FUNC_TYPE.TAIL_CALL_LIST.  */
-- 
2.31.1

[PATCH 6/6] Handle multiple addresses in call_site_target

[Tom Tromey]

A large customer program has a function that is partitioned into hot
and cold parts.  A variable in a callee of this function is described
using DW_OP_GNU_entry_value, but gdb gets confused when trying to find
the caller.  I tracked this down to dwarf2_get_pc_bounds interpreting
the function's changes so that the returned low PC is the "wrong"
function.

Intead, when processing DW_TAG_call_site, the low PC of each range in
DW_AT_ranges should be preserved in the call_site_target.  This fixes
the variable lookup in the test case I have.

I didn't write a standalone test for this as it seemed excessively
complicated.
---
 gdb/dwarf2/loc.c  | 12 ++++++++++++
 gdb/dwarf2/read.c | 35 +++++++++++++++++++++++++++++++++++
 gdb/gdbtypes.h    | 15 +++++++++++++++
 3 files changed, 62 insertions(+)

diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c
index e1001eb7adb..82508c78cc7 100644
--- a/gdb/dwarf2/loc.c
+++ b/gdb/dwarf2/loc.c
@@ -726,6 +726,18 @@ call_site_target::iterate_over_addresses
       }
       break;
 
+    case call_site_target::ADDRESSES:
+      {
+	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];
+
+	for (unsigned i = 0; i < m_loc.addresses.length; ++i)
+	  callback (m_loc.addresses.values[i] + delta);
+      }
+      break;
+
     default:
       internal_error (__FILE__, __LINE__, _("invalid call site target kind"));
     }
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 57538fc0adf..f40606216d1 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -13335,6 +13335,11 @@ read_lexical_block_scope (struct die_info *die, struct dwarf2_cu *cu)
   cu->get_builder ()->set_local_using_directives (cstk.local_using_directives);
 }
 
+static void dwarf2_ranges_read_low_addrs (unsigned offset,
+					  struct dwarf2_cu *cu,
+					  dwarf_tag tag,
+					  std::vector<CORE_ADDR> &result);
+
 /* Read in DW_TAG_call_site and insert it to CU->call_site_htab.  */
 
 static void
@@ -13498,6 +13503,10 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
 
       target_die = follow_die_ref (die, attr, &target_cu);
       gdb_assert (target_cu->per_objfile->objfile == objfile);
+
+      struct attribute *ranges_attr
+	= dwarf2_attr (target_die, DW_AT_ranges, target_cu);
+
       if (die_is_declaration (target_die, target_cu))
 	{
 	  const char *target_physname;
@@ -13513,6 +13522,18 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
 	  else
 	    call_site->target.set_loc_physname (target_physname);
 	}
+      else if (ranges_attr != nullptr && ranges_attr->form_is_unsigned ())
+	{
+	  ULONGEST ranges_offset = (ranges_attr->as_unsigned ()
+				    + target_cu->gnu_ranges_base);
+	  std::vector<CORE_ADDR> addresses;
+	  dwarf2_ranges_read_low_addrs (ranges_offset, target_cu,
+					target_die->tag, addresses);
+	  CORE_ADDR *saved = XOBNEWVAR (&objfile->objfile_obstack, CORE_ADDR,
+					addresses.size ());
+	  std::copy (addresses.begin (), addresses.end (), saved);
+	  call_site->target.set_loc_array (addresses.size (), saved);
+	}
       else
 	{
 	  CORE_ADDR lowpc;
@@ -14086,6 +14107,20 @@ dwarf2_ranges_read (unsigned offset, CORE_ADDR *low_return,
   return 1;
 }
 
+/* Process ranges and fill in a vector of the low PC values only.  */
+
+static void
+dwarf2_ranges_read_low_addrs (unsigned offset, struct dwarf2_cu *cu,
+			      dwarf_tag tag,
+			      std::vector<CORE_ADDR> &result)
+{
+  dwarf2_ranges_process (offset, cu, tag,
+			 [&] (CORE_ADDR start, CORE_ADDR end)
+    {
+      result.push_back (start);
+    });
+}
+
 /* Get low and high pc attributes from a die.  See enum pc_bounds_kind
    definition for the return value.  *LOWPC and *HIGHPC are set iff
    neither PC_BOUNDS_NOT_PRESENT nor PC_BOUNDS_INVALID are returned.  */
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index eaa3cd0dcb7..c89521c4318 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -1820,6 +1820,8 @@ struct call_site_target
     PHYSNAME,
     /* A DWARF block.  */
     DWARF_BLOCK,
+    /* An array of addresses.  */
+    ADDRESSES,
   };
 
   void set_loc_physaddr (CORE_ADDR physaddr)
@@ -1840,6 +1842,13 @@ struct call_site_target
       m_loc.dwarf_block = dwarf_block;
     }
 
+  void set_loc_array (unsigned length, const CORE_ADDR *data)
+  {
+    m_loc_kind = ADDRESSES;
+    m_loc.addresses.length = length;
+    m_loc.addresses.values = data;
+  }
+
   /* Callback type for iterate_over_addresses.  */
 
   using iterate_ftype = gdb::function_view<void (CORE_ADDR)>;
@@ -1863,6 +1872,12 @@ struct call_site_target
     const char *physname;
     /* DWARF block.  */
     struct dwarf2_locexpr_baton *dwarf_block;
+    /* Array of addresses.  */
+    struct
+    {
+      unsigned length;
+      const CORE_ADDR *values;
+    } addresses;
   } m_loc;
 
   /* * Discriminant for union field_location.  */
-- 
2.31.1

Re: [PATCH 0/6] Handle split functions in call site chains

[Tom Tromey]

>>>>> "Tom" == Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> This series fixes a bug in DW_OP_entry_value handling.
Tom> A large customer program that is compiled with optimization has a
Tom> function that is split into hot and cold parts.  The DWARF uses
Tom> DW_AT_ranges to represent this.
[...]
Tom> Patch #4 could use an extra examination, both because I convert
Tom> explicit state management to recursion (IMO ok because these call
Tom> chains tend to be short); but also because I think I found a bug in
Tom> the current code.

I think at this point I will delay this until after the 12.1 branch is
made.  Then I will probably check it in.

Tom

Re: [PATCH 0/6] Handle split functions in call site chains

[Tom Tromey]

Tom> This series fixes a bug in DW_OP_entry_value handling.
Tom> A large customer program that is compiled with optimization has a
Tom> function that is split into hot and cold parts.  The DWARF uses
Tom> DW_AT_ranges to represent this.
Tom> [...]
Tom> Patch #4 could use an extra examination, both because I convert
Tom> explicit state management to recursion (IMO ok because these call
Tom> chains tend to be short); but also because I think I found a bug in
Tom> the current code.

> I think at this point I will delay this until after the 12.1 branch is
> made.  Then I will probably check it in.

I'm checking this in now.  I've rebased it, which only required minor
changes.  I've also regression tested it on x86-64 Fedora 34.

Tom