This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH] Eliminate dwarf2_frame_cache recursion (move dwarf2_tailcall_sniffer_first elsewhere)


TL;DR:

dwarf2_frame_cache recursion is evil.  dwarf2_frame_cache calls
dwarf2_tailcall_sniffer_first which then recurses into
dwarf2_frame_cache.  Avoid that by deferring
dwarf2_tailcall_sniffer_first until it's really necessary.

Rationale:

As an experiment, I tried making dwarf2-frame.c:read_addr_from_reg use
address_from_register.  That caused a bunch of regressions, but it
actually took me a long while to figure out what was going on.  Turns
out dwarf2-frame.c:read_addr_from_reg is called while computing the
frame's CFA, from within dwarf2_frame_cache.  address_from_register
wants to create a register with frame_id set to the frame being
constructed.  To create the frame id, we again call dwarf2_frame_cache,
which given:

static struct dwarf2_frame_cache *
dwarf2_frame_cache (struct frame_info *this_frame, void **this_cache)
{
...
  if (*this_cache)
    return *this_cache;

returns an incomplete object to the caller:
static void
dwarf2_frame_this_id (struct frame_info *this_frame, void **this_cache,
		      struct frame_id *this_id)
{
  struct dwarf2_frame_cache *cache =
    dwarf2_frame_cache (this_frame, this_cache);
...
 (*this_id) = frame_id_build (cache->cfa, get_frame_func (this_frame));
}

As cache->cfa is still 0 (we were trying to compute it!), and
get_frame_id recalls this id from here on, we end up with a broken
frame id in recorded for this frame.  Later, when inspecting locals,
the dwarf machinery needs to know the selected frame's base, which
calls get_frame_base:

CORE_ADDR
get_frame_base (struct frame_info *fi)
{
  return get_frame_id (fi).stack_addr;
}

which as seen above then returns 0 ...

So I gave up using address_from_register.

But, the pain of investigating this made me want to have GDB itself
assert that recursion never happens here.  So I wrote a patch to do
that.  But, it triggers on current mainline, because
dwarf2_tailcall_sniffer_first, called from dwarf2_frame_cache, unwinds
the this_frame.

Now, I think that as design principle, a sniffer shouldn't be trying
to unwind, exactly because of this sort of tricky issue.  So the patch
defers calling dwarf2_tailcall_sniffer_first until it's really
necessary, in dwarf2_frame_prev_register (thus actually outside the
sniffer path).  As this makes the call to dwarf2_frame_sniffer in
dwarf2_frame_cache unnecessary again, the patch removes that too.

Tested on x86_64 Fedora 17.

gdb/
2013-11-20  Pedro Alves  <palves@redhat.com>

	* dwarf2-frame.c (struct dwarf2_frame_cache)
	<checked_tailcall_bottom, entry_cfa_sp_offset,
	entry_cfa_sp_offset_p>: New fields.
	(dwarf2_frame_cache): Adjust to use the new cache fields instead
	of locals.  Don't call dwarf2_tailcall_sniffer_first here.
	(dwarf2_frame_prev_register): Call it here, but only once.
---
 gdb/dwarf2-frame.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index 91d8802..c4f8771 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -993,12 +993,22 @@ struct dwarf2_frame_cache
   /* The .text offset.  */
   CORE_ADDR text_offset;
 
+  /* True if we already checked whether this frame is the bottom frame
+     of a virtual tail call frame chain.  */
+  int checked_tailcall_bottom;
+
   /* If not NULL then this frame is the bottom frame of a TAILCALL_FRAME
      sequence.  If NULL then it is a normal case with no TAILCALL_FRAME
      involved.  Non-bottom frames of a virtual tail call frames chain use
      dwarf2_tailcall_frame_unwind unwinder so this field does not apply for
      them.  */
   void *tailcall_cache;
+
+  /* The number of bytes to subtract from TAILCALL_FRAME frames frame
+     base to get the SP, to simulate the return address pushed on the
+     stack.  */
+  LONGEST entry_cfa_sp_offset;
+  int entry_cfa_sp_offset_p;
 };
 
 /* A cleanup that sets a pointer to NULL.  */
@@ -1023,8 +1033,6 @@ dwarf2_frame_cache (struct frame_info *this_frame, void **this_cache)
   struct dwarf2_fde *fde;
   volatile struct gdb_exception ex;
   CORE_ADDR entry_pc;
-  LONGEST entry_cfa_sp_offset;
-  int entry_cfa_sp_offset_p = 0;
   const gdb_byte *instr;
 
   if (*this_cache)
@@ -1089,8 +1097,8 @@ dwarf2_frame_cache (struct frame_info *this_frame, void **this_cache)
 	  && (gdbarch_dwarf2_reg_to_regnum (gdbarch, fs->regs.cfa_reg)
 	      == gdbarch_sp_regnum (gdbarch)))
 	{
-	  entry_cfa_sp_offset = fs->regs.cfa_offset;
-	  entry_cfa_sp_offset_p = 1;
+	  cache->entry_cfa_sp_offset = fs->regs.cfa_offset;
+	  cache->entry_cfa_sp_offset_p = 1;
 	}
     }
   else
@@ -1239,13 +1247,6 @@ incomplete CFI data; unspecified registers (e.g., %s) at %s"),
     cache->undefined_retaddr = 1;
 
   do_cleanups (old_chain);
-
-  /* Try to find a virtual tail call frames chain with bottom (callee) frame
-     starting at THIS_FRAME.  */
-  dwarf2_tailcall_sniffer_first (this_frame, &cache->tailcall_cache,
-				 (entry_cfa_sp_offset_p
-				  ? &entry_cfa_sp_offset : NULL));
-
   discard_cleanups (reset_cache_cleanup);
   return cache;
 }
@@ -1292,6 +1293,16 @@ dwarf2_frame_prev_register (struct frame_info *this_frame, void **this_cache,
   CORE_ADDR addr;
   int realnum;
 
+  /* Check whether THIS_FRAME is the bottom frame of a virtual tail
+     call frame chain.  */
+  if (!cache->checked_tailcall_bottom)
+    {
+      cache->checked_tailcall_bottom = 1;
+      dwarf2_tailcall_sniffer_first (this_frame, &cache->tailcall_cache,
+				     (cache->entry_cfa_sp_offset_p
+				      ? &cache->entry_cfa_sp_offset : NULL));
+    }
+
   /* Non-bottom frames of a virtual tail call frames chain use
      dwarf2_tailcall_frame_unwind unwinder so this code does not apply for
      them.  If dwarf2_tailcall_prev_register_first does not have specific value
@@ -1418,10 +1429,6 @@ dwarf2_frame_sniffer (const struct frame_unwind *self,
   if (self->type != NORMAL_FRAME)
     return 0;
 
-  /* Preinitializa the cache so that TAILCALL_FRAME can find the record by
-     dwarf2_tailcall_sniffer_first.  */
-  dwarf2_frame_cache (this_frame, this_cache);
-
   return 1;
 }
 
-- 
1.7.11.7


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]