Do not unwind frames past NULL PC

Do not unwind frames past NULL PC

[Maciej W. Rozycki]

Hello,

 Some architectures, like MIPS, specify in the ABI that the value of the 
return address in a frame (or in other words the value of the PC the frame 
would have been called from) being zero denotes the outermost frame.  At 
the moment GDB does not seem to have a way to terminate frame unwinding in 
an architecture-specific way (or to that matter any that would not imply 
an error condition) in get_prev_frame_1(), which is where such a check 
would be needed.

 However even for these architectures which may not necessarily specify in 
the relevant ABI that a NULL PC is the terminating value it seems rather 
unlikely for a function to have been called in a way which would make its 
return address to be zero and yet it having a genuine caller with an 
associated frame.  Therefore I propose the following check to be 
introduced to get_prev_frame_1().  It removes the confusing bogus frame at 
the bottom of a backtrace like below:

(gdb) bt
#0  main (argc=1, argv=0x8114fdd0, envp=0x801065a8)
    at gdb/testsuite/gdb.base/run.c:59
#1  0x801003a3 in __wrap_main ()
#2  0x801000a4 in _start ()
    at sdemdi/crt0.S:93
#3  0x00000000 in ?? ()
(gdb)

 If my assumption is in fact wrong for some other architecture, then 
please let me know.  Otherwise this change has been tested using the 
mipsisa32-sde-elf target, with the mips-sim-sde32/-EB/-mips32r2 and 
mips-sim-sde32/-EL/-mips32r2 boards with no regressions.

2008-02-22  Maciej W. Rozycki  <macro@mips.com>

	* frame.c (get_prev_frame_1): Stop unwinding if the PC of zero
	has been reached.

 OK to apply?

  Maciej

gdb-get_prev_frame.diff
Index: binutils-quilt/ChangeLog-gdb-get_prev_frame
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils-quilt/ChangeLog-gdb-get_prev_frame	2008-02-22 16:38:40.000000000 +0000
@@ -0,0 +1,4 @@
+2008-02-22  Maciej W. Rozycki  <macro@mips.com>
+
+	* frame.c (get_prev_frame_1): Stop unwinding if the PC of zero
+	has been reached.
Index: binutils-quilt/src/gdb/frame.c
===================================================================
--- binutils-quilt.orig/src/gdb/frame.c	2008-02-22 14:52:45.000000000 +0000
+++ binutils-quilt/src/gdb/frame.c	2008-02-22 16:38:40.000000000 +0000
@@ -1122,13 +1122,18 @@
 static struct frame_info *
 get_prev_frame_1 (struct frame_info *this_frame)
 {
+  enum frame_type this_frame_type;
   struct frame_info *prev_frame;
   struct frame_id this_id;
   struct gdbarch *gdbarch;
+  int pc_regnum;
 
   gdb_assert (this_frame != NULL);
   gdbarch = get_frame_arch (this_frame);
 
+  pc_regnum = gdbarch_pc_regnum (gdbarch);
+  this_frame_type = get_frame_type (this_frame);
+
   if (frame_debug)
     {
       fprintf_unfiltered (gdb_stdlog, "{ get_prev_frame_1 (this_frame=");
@@ -1219,19 +1224,17 @@
      method set the same lval and location information as
      frame_register_unwind.  */
   if (this_frame->level > 0
-      && gdbarch_pc_regnum (gdbarch) >= 0
-      && get_frame_type (this_frame) == NORMAL_FRAME
+      && pc_regnum >= 0
+      && this_frame_type == NORMAL_FRAME
       && get_frame_type (this_frame->next) == NORMAL_FRAME)
     {
       int optimized, realnum, nrealnum;
       enum lval_type lval, nlval;
       CORE_ADDR addr, naddr;
 
-      frame_register_unwind_location (this_frame,
-				      gdbarch_pc_regnum (gdbarch),
+      frame_register_unwind_location (this_frame, pc_regnum,
 				      &optimized, &lval, &addr, &realnum);
-      frame_register_unwind_location (get_next_frame (this_frame),
-				      gdbarch_pc_regnum (gdbarch),
+      frame_register_unwind_location (get_next_frame (this_frame), pc_regnum,
 				      &optimized, &nlval, &naddr, &nrealnum);
 
       if ((lval == lval_memory && lval == nlval && addr == naddr)
@@ -1250,6 +1253,23 @@
 	}
     }
 
+  /* Check for the unwound PC being zero, which means this is
+     the outermost frame.  */
+  if (pc_regnum >= 0
+      && this_frame_type == NORMAL_FRAME
+      && frame_pc_unwind (this_frame) == 0)
+    {
+      if (frame_debug)
+	{
+	  fprintf_unfiltered (gdb_stdlog, "-> ");
+	  fprint_frame (gdb_stdlog, NULL);
+	  fprintf_unfiltered (gdb_stdlog, " // NULL saved PC }\n");
+	}
+
+      this_frame->prev = NULL;
+      return NULL;
+    }
+
   /* Allocate the new frame but do not wire it in to the frame chain.
      Some (bad) code in INIT_FRAME_EXTRA_INFO tries to look along
      frame->next to pull some fancy tricks (of course such code is, by

Re: Do not unwind frames past NULL PC

[Daniel Jacobowitz]

On Fri, Feb 22, 2008 at 05:12:18PM +0000, Maciej W. Rozycki wrote:
> Hello,
> 
>  Some architectures, like MIPS, specify in the ABI that the value of the 
> return address in a frame (or in other words the value of the PC the frame 
> would have been called from) being zero denotes the outermost frame.  At 
> the moment GDB does not seem to have a way to terminate frame unwinding in 
> an architecture-specific way (or to that matter any that would not imply 
> an error condition) in get_prev_frame_1(), which is where such a check 
> would be needed.
> 
>  However even for these architectures which may not necessarily specify in 
> the relevant ABI that a NULL PC is the terminating value it seems rather 
> unlikely for a function to have been called in a way which would make its 
> return address to be zero and yet it having a genuine caller with an 
> associated frame.  Therefore I propose the following check to be 
> introduced to get_prev_frame_1().  It removes the confusing bogus frame at 
> the bottom of a backtrace like below:

Similar changes have been proposed several times, but were controversial.

  http://sourceware.org/ml/gdb-patches/2006-05/msg00196.html
  http://sourceware.org/ml/gdb-patches/2006-07/msg00296.html

and more recently:

  http://sourceware.org/ml/gdb-patches/2007-12/msg00004.html

-- 
Daniel Jacobowitz
CodeSourcery

Re: Do not unwind frames past NULL PC

[Maciej W. Rozycki]

On Fri, 22 Feb 2008, Daniel Jacobowitz wrote:

> Similar changes have been proposed several times, but were controversial.
> 
>   http://sourceware.org/ml/gdb-patches/2006-05/msg00196.html
>   http://sourceware.org/ml/gdb-patches/2006-07/msg00296.html
> 
> and more recently:
> 
>   http://sourceware.org/ml/gdb-patches/2007-12/msg00004.html

 Hmm, it looks it has been discussed deeply and fiercely enough for me not 
to dare question the outcome (or the lack of), but given the situation 
wouldn't it be reasonable to place a comment within the code of this 
function stating that outermost frame determination has been deliberately 
omitted so that cases like stack corruption are easier for people to 
debug?

  Maciej

Re: Do not unwind frames past NULL PC

[Maciej W. Rozycki]

On Fri, 22 Feb 2008, Maciej W. Rozycki wrote:

>  Hmm, it looks it has been discussed deeply and fiercely enough for me not 
> to dare question the outcome (or the lack of), but given the situation 
> wouldn't it be reasonable to place a comment within the code of this 
> function stating that outermost frame determination has been deliberately 
> omitted so that cases like stack corruption are easier for people to 
> debug?

 OK, here is the patch.  I hope I have gathered the intent from the 
discussions correctly and expressed it clearly enough.

 Personally I think for MIPS there is no gain from printing a frame with a 
zero PC.  As for MIPS the frame is associated with the PC, there will 
never be further backtrace past a zero PC, because there will never be a 
frame described for the code address of zero.  So whether the dangling 
"frame" is displayed or not makes no difference -- unless there is an 
error reported with a backtrace, the null PC will always be there.  I 
recognise that other architectures may have a different view on the frames 
though.

2008-02-25  Maciej W. Rozycki  <macro@mips.com>

	* frame.c (get_prev_frame_1): Add a note about unwillingness to
	check for the outermost frame.

 OK to apply?

  Maciej

gdb-get_prev_frame.diff
Index: binutils-quilt/src/gdb/frame.c
===================================================================
--- binutils-quilt.orig/src/gdb/frame.c	2008-02-25 10:42:37.000000000 +0000
+++ binutils-quilt/src/gdb/frame.c	2008-02-25 11:48:56.000000000 +0000
@@ -1250,6 +1250,14 @@
 	}
     }
 
+  /* This is the place where a check for the ABI-specific condition
+     denoting the outermost frame could be done.  We do not do this
+     though, quite deliberately, because we have no means to verify
+     whether this condition would be intentional or a result of a
+     possible stack corruption.  If the latter was the case we would
+     remove information from output which for some ABIs could
+     provide a hint that a stack corruption actually happened.  */
+
   /* Allocate the new frame but do not wire it in to the frame chain.
      Some (bad) code in INIT_FRAME_EXTRA_INFO tries to look along
      frame->next to pull some fancy tricks (of course such code is, by