[patch RFC] Re: Notes on a frame_unwind_address_in_block problem

[Daniel Jacobowitz <drow at false dot org>]

Now that the CFI issue in glibc is fixed, I'm back to this discussion
from July.  Quoted below for reference, since it's been a while.

Here's a patch, that I'm looking for comments on.  It's not the most
gorgeous code I've ever written, but it's the simplest solution I could
come up with for a complicated problem.  It fixes eight failures in the
x86-64 GNU/Linux testsuite and I believe it will fix those same
failures for i386 also.  Test results for this platform are starting to
look really good.  I hope I can get it to no failures during the next
two weeks, and then move on to do the same for another platform.

Problem
=======

The problem is a function which looks like this:

  <fde start> nop
  function_label: code
  <fde end> <function end>

Such functions are interesting to me because glibc and the Linux kernel
both use them to provide unwind information for signal handlers.  The
nop allows pc-1 to work, at least for libgcc's unwinder, even though
function_label is pushed directly onto the stack as if it were a return
address.

But GDB ends up subtracting one from the PC in some places it would be
better not to.  We decide that get_frame_func for that frame should
return the func associated with the fde start if the next frame is a
normal frame, but the func associated with the function_label if the
next frame is the sentinel frame.  This inconsistency breaks "finish"
out of a signal handler, because the unwound frame ID when the command
is issued doesn't match the current frame ID when we hit the temporary
breakpoint.  We have several tests for this case.

Solution
========

I spent a little while thinking about it today.  I want the decision to
be made by frame_unwind_address_in_block, because that function has a
bunch of callers that might rely on its result being in the right
block.  My previous nasty hack made that function right some of the
time, but not all of the time, for the same frame.  Here's a bigger
patch, which makes it right consistently.

After the patch, the unwound address in block is decremented only if
there are two consecutive normal frames.  If either the next or current
frame is a signal frame, we leave it alone.  This is basically the
same as my original nasty hack, except that there are better safety
measures in place: frame_unwind_address_in_block does not change
during unwinding, and if you use it too early you get a clear assertion
failure.

The complicated part is avoiding recursion. 

frame_unwind_address_in_block is no longer safe to use during sniffers.
I broke it apart into two functions.  The new one assumes that the
unwound frame will be normal.  This does the right thing when called in
sniffers for normal frames.  Then, I audited every single frame unwind
sniffer, and adjusted all the sniffers for normal frames to use it. 
Several of them should have been using address_in_block already, but
weren't.  Signal frame unwinders still use frame_pc_unwind directly.

There were surprisingly few changes necessary.  Of course, I might have
missed one, but I added an assertion and some comments that should make
the problem easy to recognize.

On Tue, Jul 18, 2006 at 02:39:10PM -0400, Daniel Jacobowitz wrote:
> > > Another solution would be to use the FDE start address as the code address
> > > for dwarf2 signal frame IDs, instead of the function.  This would work on
> > > the assumption that a single FDE would generally cover the entire trampoline
> > > - a reasonable assumption, I think, and the consequences for the frame ID
> > > changing while single-stepping are less disruptive here than the
> > > alternative.
> > > 
> > > Mark, what do you think of that idea?  It seems to work.  It looks like the
> > > patch at the end of this message.
> > 
> > In general, I think it's a bad idea to do this, but for the special
> > case of a signal frame, especially in the presence of the 'S"
> > augmentation, that might be a reasonable thing to do.  However, I
> > think we can do better than that.  What about checking whether the
> > address returned by frame_unwind_address_in_block() is equal to the
> > FDE start address and add one bytes if that's the case before looking
> > up the function corresponding to that address?
> 
> Unfortunately we can't readily; we'd have to add a bit of new code,
> since all that's there is a call to frame_func_unwind (next_frame)
> and that has no way to do the right thing.
> 
> I'd like to trigger off the signal augmentation.  If you'd prefer to
> check the FDE start address, could you show me exactly what you have in
> mind?
> 
> With that patch plus the fixed unwind information gdb.base/*.exp
> results are basically clean for x86_64 - best I've ever seen them.
> There is a corefile issue which I think is kernel related, and two
> annotation tests fail because they don't account for a glibc with debug
> information enabled.

-- 
Daniel Jacobowitz
CodeSourcery

2007-01-01  Daniel Jacobowitz  <dan@codesourcery.com>

	* frame-unwind.c (searching_for_unwind): New.
	(clear_searching_flag): New.
	(frame_unwind_find_by_frame): Assert that this function is not
	entered recursively.
	* frame.c (normal_frame_unwind_address_in_block): Renamed from
	frame_unwind_address_in_block.  Doc update.
	(frame_unwind_address_in_block): Use the new function.  Also
	check the previous frame's type.
	* frame.h (normal_frame_unwind_address_in_block): New prototype.

	* alpha-mdebug-tdep.c (alpha_mdebug_frame_sniffer): Use
	normal_frame_unwind_address_in_block.
	(alpha_mdebug_frame_base_sniffer): Likewise.
	* arm-tdep.c (arm_stub_unwind_sniffer): Likewise.
	* dwarf2-frame.c (dwarf2_frame_sniffer): Likewise.
	(dwarf2_frame_base_sniffer): Likewise.
	* hppa-tdep.c (hppa_find_unwind_entry_in_block): Likewise.
	(hppa_stub_unwind_sniffer): Likewise.
	* mips-mdebug-tdep.c (mips_mdebug_frame_sniffer): Likewise.
	* mips-tdep.c (mips_insn16_frame_sniffer, mips_insn32_frame_sniffer)
	(mips_stub_frame_sniffer): Likewise.
	* s390-tdep.c (s390_stub_frame_sniffer): Likewise.
	* sparc64obsd-tdep.c (sparc64obsd_trapframe_sniffer): Likewise.
	(sparc64obsd_trapframe_cache): Whitespace fix.

Index: alpha-mdebug-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/alpha-mdebug-tdep.c,v
retrieving revision 1.11
diff -u -p -r1.11 alpha-mdebug-tdep.c
--- alpha-mdebug-tdep.c	17 Dec 2005 22:33:59 -0000	1.11
+++ alpha-mdebug-tdep.c	1 Jan 2007 19:06:21 -0000
@@ -307,7 +307,7 @@ static const struct frame_unwind alpha_m
 const struct frame_unwind *
 alpha_mdebug_frame_sniffer (struct frame_info *next_frame)
 {
-  CORE_ADDR pc = frame_pc_unwind (next_frame);
+  CORE_ADDR pc = normal_frame_unwind_address_in_block (next_frame);
   struct mdebug_extra_func_info *proc_desc;
 
   /* If this PC does not map to a PDR, then clearly this isn't an
@@ -364,7 +364,7 @@ static const struct frame_base alpha_mde
 static const struct frame_base *
 alpha_mdebug_frame_base_sniffer (struct frame_info *next_frame)
 {
-  CORE_ADDR pc = frame_pc_unwind (next_frame);
+  CORE_ADDR pc = normal_frame_unwind_address_in_block (next_frame);
   struct mdebug_extra_func_info *proc_desc;
 
   /* If this PC does not map to a PDR, then clearly this isn't an
Index: arm-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/arm-tdep.c,v
retrieving revision 1.217
diff -u -p -r1.217 arm-tdep.c
--- arm-tdep.c	22 Nov 2006 18:51:58 -0000	1.217
+++ arm-tdep.c	1 Jan 2007 19:06:22 -0000
@@ -993,7 +993,7 @@ arm_stub_unwind_sniffer (struct frame_in
 {
   char dummy[4];
 
-  if (in_plt_section (frame_unwind_address_in_block (next_frame), NULL)
+  if (in_plt_section (normal_frame_unwind_address_in_block (next_frame), NULL)
       || target_read_memory (frame_pc_unwind (next_frame), dummy, 4) != 0)
     return &arm_stub_unwind;
 
Index: dwarf2-frame.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2-frame.c,v
retrieving revision 1.64
diff -u -p -r1.64 dwarf2-frame.c
--- dwarf2-frame.c	28 Nov 2006 17:28:29 -0000	1.64
+++ dwarf2-frame.c	1 Jan 2007 19:06:22 -0000
@@ -1104,8 +1104,11 @@ dwarf2_frame_sniffer (struct frame_info 
 {
   /* Grab an address that is guarenteed to reside somewhere within the
      function.  frame_pc_unwind(), for a no-return next function, can
-     end up returning something past the end of this function's body.  */
-  CORE_ADDR block_addr = frame_unwind_address_in_block (next_frame);
+     end up returning something past the end of this function's body.
+     If the frame we're sniffing for is a signal frame whose start
+     address is placed on the stack by the OS, its FDE must
+     extend one byte before its start address or we will miss it.  */
+  CORE_ADDR block_addr = normal_frame_unwind_address_in_block (next_frame);
   struct dwarf2_fde *fde = dwarf2_frame_find_fde (&block_addr);
   if (!fde)
     return NULL;
@@ -1149,8 +1152,8 @@ static const struct frame_base dwarf2_fr
 const struct frame_base *
 dwarf2_frame_base_sniffer (struct frame_info *next_frame)
 {
-  CORE_ADDR pc = frame_pc_unwind (next_frame);
-  if (dwarf2_frame_find_fde (&pc))
+  CORE_ADDR block_addr = normal_frame_unwind_address_in_block (next_frame);
+  if (dwarf2_frame_find_fde (&block_addr))
     return &dwarf2_frame_base;
 
   return NULL;
Index: frame-unwind.c
===================================================================
RCS file: /cvs/src/src/gdb/frame-unwind.c,v
retrieving revision 1.17
diff -u -p -r1.17 frame-unwind.c
--- frame-unwind.c	17 Dec 2005 22:33:59 -0000	1.17
+++ frame-unwind.c	1 Jan 2007 19:06:22 -0000
@@ -28,6 +28,8 @@
 
 static struct gdbarch_data *frame_unwind_data;
 
+static int searching_for_unwind;
+
 struct frame_unwind_table_entry
 {
   frame_unwind_sniffer_ftype *sniffer;
@@ -83,6 +85,12 @@ frame_unwind_prepend_unwinder (struct gd
   (*table->osabi_head) = entry;
 }
 
+static void
+clear_searching_flag (void *arg)
+{
+  searching_for_unwind = 0;
+}
+
 const struct frame_unwind *
 frame_unwind_find_by_frame (struct frame_info *next_frame, void **this_cache)
 {
@@ -90,6 +98,16 @@ frame_unwind_find_by_frame (struct frame
   struct gdbarch *gdbarch = get_frame_arch (next_frame);
   struct frame_unwind_table *table = gdbarch_data (gdbarch, frame_unwind_data);
   struct frame_unwind_table_entry *entry;
+  struct cleanup *back_to;
+
+  /* Ensure we were not re-entered.  This happens if the sniffer calls
+     anything which requires NEXT_FRAME's unwinder, for instance by
+     calling frame_unwind_address_in_block instead of frame_pc_unwind or
+     normal_frame_unwind_address_in_block.  */
+  gdb_assert (!searching_for_unwind);
+  searching_for_unwind = 1;
+  back_to = make_cleanup (clear_searching_flag, NULL);
+
   for (entry = table->list; entry != NULL; entry = entry->next)
     {
       if (entry->sniffer != NULL)
@@ -97,15 +115,22 @@ frame_unwind_find_by_frame (struct frame
 	  const struct frame_unwind *desc = NULL;
 	  desc = entry->sniffer (next_frame);
 	  if (desc != NULL)
-	    return desc;
+	    {
+	      do_cleanups (back_to);
+	      return desc;
+	    }
 	}
       if (entry->unwinder != NULL)
 	{
 	  if (entry->unwinder->sniffer (entry->unwinder, next_frame,
 					this_cache))
-	    return entry->unwinder;
+	    {
+	      do_cleanups (back_to);
+	      return entry->unwinder;
+	    }
 	}
     }
+  do_cleanups (back_to);
   internal_error (__FILE__, __LINE__, _("frame_unwind_find_by_frame failed"));
 }
 
Index: frame.c
===================================================================
RCS file: /cvs/src/src/gdb/frame.c,v
retrieving revision 1.215
diff -u -p -r1.215 frame.c
--- frame.c	10 Nov 2006 20:11:35 -0000	1.215
+++ frame.c	1 Jan 2007 19:06:22 -0000
@@ -1412,10 +1412,13 @@ get_frame_pc (struct frame_info *frame)
   return frame_pc_unwind (frame->next);
 }
 
-/* Return an address of that falls within the frame's code block.  */
+/* Return an address that falls within NEXT_FRAME's caller's code
+   block, assuming that the caller is a normal function (i.e. not
+   a signal trampoline).  This function should only be called from
+   frame unwind sniffers.  */
 
 CORE_ADDR
-frame_unwind_address_in_block (struct frame_info *next_frame)
+normal_frame_unwind_address_in_block (struct frame_info *next_frame)
 {
   /* A draft address.  */
   CORE_ADDR pc = frame_pc_unwind (next_frame);
@@ -1425,7 +1428,7 @@ frame_unwind_address_in_block (struct fr
      frame's PC ends up pointing at the instruction fallowing the
      "call".  Adjust that PC value so that it falls on the call
      instruction (which, hopefully, falls within THIS frame's code
-     block.  So far it's proved to be a very good approximation.  See
+     block).  So far it's proved to be a very good approximation.  See
      get_frame_type() for why ->type can't be used.  */
   if (next_frame->level >= 0
       && get_frame_type (next_frame) == NORMAL_FRAME)
@@ -1433,6 +1436,28 @@ frame_unwind_address_in_block (struct fr
   return pc;
 }
 
+/* Return an address that falls within NEXT_FRAME's caller's code
+   block.  This function must not be called from frame unwind
+   sniffers, because it relies on both NEXT_FRAME and its
+   caller.  */
+
+CORE_ADDR
+frame_unwind_address_in_block (struct frame_info *next_frame)
+{
+  struct frame_info *this_frame;
+
+  /* If this frame was called by a signal frame or dummy frame, then
+     we shold not adjust the unwound PC.  These frames may not call
+     their next frame in the normal way; the operating system or GDB
+     may have pushed their resume address manually onto the stack,
+     so it may be the very first instruction.  */
+  this_frame = get_prev_frame_1 (next_frame);
+  if (this_frame != NULL && get_frame_type (this_frame) != NORMAL_FRAME)
+    return frame_pc_unwind (next_frame);
+
+  return normal_frame_unwind_address_in_block (next_frame);
+}
+
 CORE_ADDR
 get_frame_address_in_block (struct frame_info *this_frame)
 {
Index: frame.h
===================================================================
RCS file: /cvs/src/src/gdb/frame.h,v
retrieving revision 1.150
diff -u -p -r1.150 frame.h
--- frame.h	10 Nov 2006 20:11:35 -0000	1.150
+++ frame.h	1 Jan 2007 19:06:22 -0000
@@ -266,6 +266,14 @@ extern CORE_ADDR get_frame_pc (struct fr
 extern CORE_ADDR get_frame_address_in_block (struct frame_info *this_frame);
 extern CORE_ADDR frame_unwind_address_in_block (struct frame_info *next_frame);
 
+/* Similar to frame_unwind_address_in_block, find an address in the
+   block which logically called NEXT_FRAME.  This function is a
+   support routine for frame unwind sniffers, and should not be called
+   from anywhere else.  */
+
+extern CORE_ADDR
+  normal_frame_unwind_address_in_block (struct frame_info *next_frame);
+
 /* The frame's inner-most bound.  AKA the stack-pointer.  Confusingly
    known as top-of-stack.  */
 
Index: hppa-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/hppa-tdep.c,v
retrieving revision 1.223
diff -u -p -r1.223 hppa-tdep.c
--- hppa-tdep.c	8 Aug 2006 21:32:48 -0000	1.223
+++ hppa-tdep.c	1 Jan 2007 19:06:23 -0000
@@ -1786,9 +1786,14 @@ hppa_skip_prologue (CORE_ADDR pc)
 static struct unwind_table_entry *
 hppa_find_unwind_entry_in_block (struct frame_info *f)
 {
-  CORE_ADDR pc;
+  CORE_ADDR pc = normal_frame_unwind_address_in_block (f);
 
-  pc = frame_unwind_address_in_block (f);
+  /* FIXME drow/20070101: Calling gdbarch_addr_bits_remove on the
+     result of normal_frame_unwind_address_in_block implies a problem.
+     The bits should have been removed earlier, before the return
+     value of frame_pc_unwind.  That might be happening already;
+     if it isn't, it should be fixed.  Then this call can be
+     removed.  */
   pc = gdbarch_addr_bits_remove (get_frame_arch (f), pc);
   return find_unwind_entry (pc);
 }
@@ -2440,7 +2445,7 @@ static const struct frame_unwind hppa_st
 static const struct frame_unwind *
 hppa_stub_unwind_sniffer (struct frame_info *next_frame)
 {
-  CORE_ADDR pc = frame_pc_unwind (next_frame);
+  CORE_ADDR pc = normal_frame_unwind_address_in_block (next_frame);
   struct gdbarch *gdbarch = get_frame_arch (next_frame);
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
 
Index: mips-mdebug-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-mdebug-tdep.c,v
retrieving revision 1.7
diff -u -p -r1.7 mips-mdebug-tdep.c
--- mips-mdebug-tdep.c	25 Apr 2006 18:58:09 -0000	1.7
+++ mips-mdebug-tdep.c	1 Jan 2007 19:06:23 -0000
@@ -412,7 +412,7 @@ static const struct frame_unwind mips_md
 static const struct frame_unwind *
 mips_mdebug_frame_sniffer (struct frame_info *next_frame)
 {
-  CORE_ADDR pc = frame_pc_unwind (next_frame);
+  CORE_ADDR pc = normal_frame_unwind_address_in_block (next_frame);
   CORE_ADDR startaddr = 0;
   struct mdebug_extra_func_info *proc_desc;
   int kernel_trap;
Index: mips-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-tdep.c,v
retrieving revision 1.399
diff -u -p -r1.399 mips-tdep.c
--- mips-tdep.c	28 Nov 2006 22:14:31 -0000	1.399
+++ mips-tdep.c	1 Jan 2007 19:06:23 -0000
@@ -1698,7 +1698,7 @@ static const struct frame_unwind mips_in
 static const struct frame_unwind *
 mips_insn16_frame_sniffer (struct frame_info *next_frame)
 {
-  CORE_ADDR pc = frame_pc_unwind (next_frame);
+  CORE_ADDR pc = normal_frame_unwind_address_in_block (next_frame);
   if (mips_pc_is_mips16 (pc))
     return &mips_insn16_frame_unwind;
   return NULL;
@@ -2018,7 +2018,7 @@ static const struct frame_unwind mips_in
 static const struct frame_unwind *
 mips_insn32_frame_sniffer (struct frame_info *next_frame)
 {
-  CORE_ADDR pc = frame_pc_unwind (next_frame);
+  CORE_ADDR pc = normal_frame_unwind_address_in_block (next_frame);
   if (! mips_pc_is_mips16 (pc))
     return &mips_insn32_frame_unwind;
   return NULL;
@@ -2113,7 +2113,7 @@ static const struct frame_unwind *
 mips_stub_frame_sniffer (struct frame_info *next_frame)
 {
   struct obj_section *s;
-  CORE_ADDR pc = frame_pc_unwind (next_frame);
+  CORE_ADDR pc = normal_frame_unwind_address_in_block (next_frame);
 
   if (in_plt_section (pc, NULL))
     return &mips_stub_frame_unwind;
Index: s390-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/s390-tdep.c,v
retrieving revision 1.156
diff -u -p -r1.156 s390-tdep.c
--- s390-tdep.c	28 Nov 2006 21:41:02 -0000	1.156
+++ s390-tdep.c	1 Jan 2007 19:06:24 -0000
@@ -1575,14 +1575,13 @@ static const struct frame_unwind s390_st
 static const struct frame_unwind *
 s390_stub_frame_sniffer (struct frame_info *next_frame)
 {
-  CORE_ADDR pc = frame_pc_unwind (next_frame);
   bfd_byte insn[S390_MAX_INSTR_SIZE];
 
   /* If the current PC points to non-readable memory, we assume we
      have trapped due to an invalid function pointer call.  We handle
      the non-existing current function like a PLT stub.  */
-  if (in_plt_section (pc, NULL)
-      || s390_readinstruction (insn, pc) < 0)
+  if (in_plt_section (normal_frame_unwind_address_in_block (next_frame), NULL)
+      || s390_readinstruction (insn, frame_pc_unwind (next_frame)) < 0)
     return &s390_stub_frame_unwind;
   return NULL;
 }
Index: sparc64obsd-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/sparc64obsd-tdep.c,v
retrieving revision 1.20
diff -u -p -r1.20 sparc64obsd-tdep.c
--- sparc64obsd-tdep.c	31 Dec 2006 01:28:07 -0000	1.20
+++ sparc64obsd-tdep.c	1 Jan 2007 19:06:24 -0000
@@ -205,7 +205,7 @@ sparc64obsd_sigtramp_frame_sniffer (stru
 /* Kernel debugging support.  */
 
 static struct sparc_frame_cache *
-sparc64obsd_trapframe_cache(struct frame_info *next_frame, void **this_cache)
+sparc64obsd_trapframe_cache (struct frame_info *next_frame, void **this_cache)
 {
   struct sparc_frame_cache *cache;
   CORE_ADDR sp, trapframe_addr;
@@ -267,15 +267,17 @@ static const struct frame_unwind sparc64
 static const struct frame_unwind *
 sparc64obsd_trapframe_sniffer (struct frame_info *next_frame)
 {
+  CORE_ADDR pc;
   ULONGEST pstate;
   char *name;
 
   /* Check whether we are in privileged mode, and bail out if we're not.  */
-  pstate = frame_unwind_register_unsigned(next_frame, SPARC64_PSTATE_REGNUM);
+  pstate = frame_unwind_register_unsigned (next_frame, SPARC64_PSTATE_REGNUM);
   if ((pstate & SPARC64_PSTATE_PRIV) == 0)
     return NULL;
 
-  find_pc_partial_function (frame_pc_unwind (next_frame), &name, NULL, NULL);
+  pc = normal_frame_unwind_address_in_block (next_frame);
+  find_pc_partial_function (pc, &name, NULL, NULL);
   if (name && strcmp (name, "Lslowtrap_reenter") == 0)
     return &sparc64obsd_trapframe_unwind;