[rfc, frame] Add backtrace stop reasons

[rfc, frame] Add backtrace stop reasons

[Daniel Jacobowitz]

I've been thinking some more about the conversation we had, on treating
PC==0 as a frame terminator.  This patch and the next patch don't
directly affect that, but they address the general issue Mark raised:
letting the user know why a backtrace stopped.

This patch adds a list of "enum unwind_stop_reason".  When we try to
unwind a frame and fail, we record the reason why we failed, rather
than calling error.  Backtrace can then choose to stop silently or
display the reason.  I preserved which reasons get displayed and which
don't.  The zero frame PC check doesn't happen here, so an independent
patch would be to move it here so that a reason is included, and
another independent patch would be whether to treat it as an error
condition or not.

A nice bonus, in my opinion: if the backtrace stops because unwinding
detects an error or the end of the stack, "info frame" will explain
why.  For instance:

  Stops backtrace: previous frame inner to this frame (corrupt stack?)

Any comments on this patch?  It doesn't have any major change on GDB's
visible behavior; it changes one error to a printf, but that error was
always wrapped in a catch_exceptions anyway, so this is unimportant.
The text of the message on a corrupt stack is changed mildly; it says
"Backtrace stopped: previous..." rather than just "Previous...".

-- 
Daniel Jacobowitz
CodeSourcery

2006-08-19  Daniel Jacobowitz  <dan@codesourcery.com>

	* frame.c (struct frame_info): Add stop_reason.
	(get_prev_frame_1): Set stop_reason.  Don't call error for
	stop reasons.
	(get_frame_unwind_stop_reason, frame_stop_reason_string): New.
	* frame.h (enum unwind_stop_reason): New.
	(get_frame_unwind_stop_reason, frame_stop_reason_string): New
	prototypes.
	* stack.c (frame_info): Print the stop reason.
	(backtrace_command_1): Print the stop reason for errors.

---
 gdb/frame.c |   67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 gdb/frame.h |   44 +++++++++++++++++++++++++++++++++++++++
 gdb/stack.c |   25 ++++++++++++++++++++++
 3 files changed, 134 insertions(+), 2 deletions(-)

Index: src/gdb/frame.c
===================================================================
--- src.orig/gdb/frame.c	2006-08-19 11:28:50.000000000 -0400
+++ src/gdb/frame.c	2006-08-19 11:32:17.000000000 -0400
@@ -107,6 +107,10 @@ struct frame_info
   struct frame_info *next; /* down, inner, younger */
   int prev_p;
   struct frame_info *prev; /* up, outer, older */
+
+  /* The reason why we could not set PREV, or UNWIND_NO_REASON if we
+     could.  Only valid when PREV_P is set.  */
+  enum unwind_stop_reason stop_reason;
 };
 
 /* Flag to control debugging.  */
@@ -1055,6 +1059,7 @@ get_prev_frame_1 (struct frame_info *thi
       return this_frame->prev;
     }
   this_frame->prev_p = 1;
+  this_frame->stop_reason = UNWIND_NO_REASON;
 
   /* Check that this frame's ID was valid.  If it wasn't, don't try to
      unwind to the prev frame.  Be careful to not apply this test to
@@ -1068,6 +1073,7 @@ get_prev_frame_1 (struct frame_info *thi
 	  fprint_frame (gdb_stdlog, NULL);
 	  fprintf_unfiltered (gdb_stdlog, " // this ID is NULL }\n");
 	}
+      this_frame->stop_reason = UNWIND_NULL_ID;
       return NULL;
     }
 
@@ -1078,14 +1084,32 @@ get_prev_frame_1 (struct frame_info *thi
   if (this_frame->next->level >= 0
       && this_frame->next->unwind->type != SIGTRAMP_FRAME
       && frame_id_inner (this_id, get_frame_id (this_frame->next)))
-    error (_("Previous frame inner to this frame (corrupt stack?)"));
+    {
+      if (frame_debug)
+	{
+	  fprintf_unfiltered (gdb_stdlog, "-> ");
+	  fprint_frame (gdb_stdlog, NULL);
+	  fprintf_unfiltered (gdb_stdlog, " // this frame ID is inner }\n");
+	}
+      this_frame->stop_reason = UNWIND_INNER_ID;
+      return NULL;
+    }
 
   /* Check that this and the next frame are not identical.  If they
      are, there is most likely a stack cycle.  As with the inner-than
      test above, avoid comparing the inner-most and sentinel frames.  */
   if (this_frame->level > 0
       && frame_id_eq (this_id, get_frame_id (this_frame->next)))
-    error (_("Previous frame identical to this frame (corrupt stack?)"));
+    {
+      if (frame_debug)
+	{
+	  fprintf_unfiltered (gdb_stdlog, "-> ");
+	  fprint_frame (gdb_stdlog, NULL);
+	  fprintf_unfiltered (gdb_stdlog, " // this frame has same ID }\n");
+	}
+      this_frame->stop_reason = UNWIND_SAME_ID;
+      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
@@ -1556,6 +1580,45 @@ frame_sp_unwind (struct frame_info *next
   internal_error (__FILE__, __LINE__, _("Missing unwind SP method"));
 }
 
+/* Return the reason why we can't unwind past FRAME.  */
+
+enum unwind_stop_reason
+get_frame_unwind_stop_reason (struct frame_info *frame)
+{
+  /* If we haven't tried to unwind past this point yet, then assume
+     that unwinding would succeed.  */
+  if (frame->prev_p == 0)
+    return UNWIND_NO_REASON;
+
+  /* Otherwise, we set a reason when we succeeded (or failed) to
+     unwind.  */
+  return frame->stop_reason;
+}
+
+/* Return a string explaining REASON.  */
+
+const char *
+frame_stop_reason_string (enum unwind_stop_reason reason)
+{
+  switch (reason)
+    {
+    case UNWIND_NULL_ID:
+      return _("unwinder did not report frame ID");
+
+    case UNWIND_INNER_ID:
+      return _("previous frame inner to this frame (corrupt stack?)");
+
+    case UNWIND_SAME_ID:
+      return _("previous frame identical to this frame (corrupt stack?)");
+
+    case UNWIND_NO_REASON:
+    case UNWIND_FIRST_ERROR:
+    default:
+      internal_error (__FILE__, __LINE__,
+		      "Invalid frame stop reason");
+    }
+}
+
 extern initialize_file_ftype _initialize_frame; /* -Wmissing-prototypes */
 
 static struct cmd_list_element *set_backtrace_cmdlist;
Index: src/gdb/frame.h
===================================================================
--- src.orig/gdb/frame.h	2006-08-19 11:28:50.000000000 -0400
+++ src/gdb/frame.h	2006-08-19 11:29:33.000000000 -0400
@@ -394,6 +394,50 @@ enum frame_type
 };
 extern enum frame_type get_frame_type (struct frame_info *);
 
+/* For frames where we can not unwind further, describe why.  */
+
+enum unwind_stop_reason
+  {
+    /* No particular reason; either we haven't tried unwinding yet,
+       or we didn't fail.  */
+    UNWIND_NO_REASON,
+
+    /* The previous frame's analyzer returns an invalid result
+       from this_id.
+
+       FIXME drow/2006-08-16: This is how GDB used to indicate end of
+       stack.  We should migrate to a model where frames always have a
+       valid ID, and this becomes not just an error but an internal
+       error.  But that's a project for another day.  */
+    UNWIND_NULL_ID,
+
+    /* All the conditions after this point are considered errors;
+       abnormal stack termination.  If a backtrace stops for one
+       of these reasons, we'll let the user know.  This marker
+       is not a valid stop reason.  */
+    UNWIND_FIRST_ERROR,
+
+    /* This frame ID looks like it ought to belong to a NEXT frame,
+       but we got it for a PREV frame.  Normally, this is a sign of
+       unwinder failure.  It could also indicate stack corruption.  */
+    UNWIND_INNER_ID,
+
+    /* This frame has the same ID as the previous one.  That means
+       that unwinding further would almost certainly give us another
+       frame with exactly the same ID, so break the chain.  Normally,
+       this is a sign of unwinder failure.  It could also indicate
+       stack corruption.  */
+    UNWIND_SAME_ID,
+  };
+
+/* Return the reason why we can't unwind past this frame.  */
+
+enum unwind_stop_reason get_frame_unwind_stop_reason (struct frame_info *);
+
+/* Translate a reason code to an informative string.  */
+
+const char *frame_stop_reason_string (enum unwind_stop_reason);
+
 /* Unwind the stack frame so that the value of REGNUM, in the previous
    (up, older) frame is returned.  If VALUEP is NULL, don't
    fetch/compute the value.  Instead just return the location of the
Index: src/gdb/stack.c
===================================================================
--- src.orig/gdb/stack.c	2006-08-19 11:28:50.000000000 -0400
+++ src/gdb/stack.c	2006-08-19 11:28:59.000000000 -0400
@@ -923,6 +923,15 @@ frame_info (char *addr_exp, int from_tty
   deprecated_print_address_numeric (frame_pc_unwind (fi), 1, gdb_stdout);
   printf_filtered ("\n");
 
+  if (calling_frame_info == NULL)
+    {
+      enum unwind_stop_reason reason;
+
+      reason = get_frame_unwind_stop_reason (fi);
+      printf_filtered (_(" Stops backtrace: %s\n"),
+			 frame_stop_reason_string (reason));
+    }
+
   if (calling_frame_info)
     {
       printf_filtered (" called by frame at ");
@@ -940,6 +949,7 @@ frame_info (char *addr_exp, int from_tty
     }
   if (get_next_frame (fi) || calling_frame_info)
     puts_filtered ("\n");
+
   if (s)
     printf_filtered (" source language %s.\n",
 		     language_str (s->language));
@@ -1163,11 +1173,26 @@ backtrace_command_1 (char *count_exp, in
       print_frame_info (fi, 1, LOCATION, 1);
       if (show_locals)
 	print_frame_local_vars (fi, 1, gdb_stdout);
+
+      /* Save the last frame to check for error conditions.  */
+      trailing = fi;
     }
 
   /* If we've stopped before the end, mention that.  */
   if (fi && from_tty)
     printf_filtered (_("(More stack frames follow...)\n"));
+
+  /* If we've run out of frames, and the reason appears to be an error
+     condition, print it.  */
+  if (fi == NULL && trailing != NULL)
+    {
+      enum unwind_stop_reason reason;
+
+      reason = get_frame_unwind_stop_reason (trailing);
+      if (reason > UNWIND_FIRST_ERROR)
+	printf_filtered (_("Backtrace stopped: %s\n"),
+			 frame_stop_reason_string (reason));
+    }
 }
 
 struct backtrace_command_args

Re: [rfc, frame] Add backtrace stop reasons

[Mark Kettenis]

> Date: Sat, 19 Aug 2006 11:46:46 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> I've been thinking some more about the conversation we had, on treating
> PC==0 as a frame terminator.  This patch and the next patch don't
> directly affect that, but they address the general issue Mark raised:
> letting the user know why a backtrace stopped.
> 
> This patch adds a list of "enum unwind_stop_reason".  When we try to
> unwind a frame and fail, we record the reason why we failed, rather
> than calling error.  Backtrace can then choose to stop silently or
> display the reason.  I preserved which reasons get displayed and which
> don't.  The zero frame PC check doesn't happen here, so an independent
> patch would be to move it here so that a reason is included, and
> another independent patch would be whether to treat it as an error
> condition or not.
> 
> A nice bonus, in my opinion: if the backtrace stops because unwinding
> detects an error or the end of the stack, "info frame" will explain
> why.  For instance:
> 
>   Stops backtrace: previous frame inner to this frame (corrupt stack?)

Is this really useful?  It adds an extra line to the "info frame"
output, which might mean that more useful information about the frame
scrolls of my screen :(.

> Any comments on this patch?  It doesn't have any major change on GDB's
> visible behavior; it changes one error to a printf, but that error was
> always wrapped in a catch_exceptions anyway, so this is unimportant.
> The text of the message on a corrupt stack is changed mildly; it says
> "Backtrace stopped: previous..." rather than just "Previous...".

That's a useful clarification I think.

The implementation looks good to me, but I don't see why we need this
functionality yet.  Can you hold on to this until we really need it?

> 2006-08-19  Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	* frame.c (struct frame_info): Add stop_reason.
> 	(get_prev_frame_1): Set stop_reason.  Don't call error for
> 	stop reasons.
> 	(get_frame_unwind_stop_reason, frame_stop_reason_string): New.
> 	* frame.h (enum unwind_stop_reason): New.
> 	(get_frame_unwind_stop_reason, frame_stop_reason_string): New
> 	prototypes.
> 	* stack.c (frame_info): Print the stop reason.
> 	(backtrace_command_1): Print the stop reason for errors.

Re: [rfc, frame] Add backtrace stop reasons

[Daniel Jacobowitz]

Hi Mark, thanks for looking at these.

On Sun, Aug 20, 2006 at 04:38:19PM +0200, Mark Kettenis wrote:
> > A nice bonus, in my opinion: if the backtrace stops because unwinding
> > detects an error or the end of the stack, "info frame" will explain
> > why.  For instance:
> > 
> >   Stops backtrace: previous frame inner to this frame (corrupt stack?)
> 
> Is this really useful?  It adds an extra line to the "info frame"
> output, which might mean that more useful information about the frame
> scrolls of my screen :(.

(gdb) info frame
Stack level 0, frame at 0xbf8e5838:
 eip = 0xffffe410 in __kernel_vsyscall; saved eip 0xb7ea61cb
 called by frame at 0xbf8e5850
 Arglist at 0xbf8e5830, args:
 Locals at 0xbf8e5830, Previous frame's sp is 0xbf8e5838
 Saved registers:
  ebp at 0xbf8e5828, eip at 0xbf8e5834

Where are we really in danger of this scrolling off the screen?  Frames
with dozens of arguments (which generally wouldn't stop the backtrace
anyway, so wouldn't have the line)?  I can imagine reasons to dislike
adding more information, but I confess this one confuses me a bit :-)

I think it will be useful, especially as the set of reasons grows.
As you correctly noticed, the other patches don't fundamentally
depend on this one, but I did them in this order deliberately;
two later followups could be converting the zero PC check to use a stop
reason, and allowing the architecture unwinders to return stop reasons.
Backtrace would print out the "error" reasons, but not the "normal"
reasons; you could look in info frame for the "normal" reasons, like
"Unwinding indicated no return address" for the undefined retaddr
column markers we use in DWARF-2.

> The implementation looks good to me, but I don't see why we need this
> functionality yet.  Can you hold on to this until we really need it?

If you want; there's no point in this unless at least one of the other
patches goes in.

The problem with holding things until we really need them is that we
build up these huge queues of patches related to a single project.
I'm sure this code wouldn't turn into abandonware.  But it's a small
patch, so I can easily carry it along until we're ready for it if you
prefer.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc, frame] Add backtrace stop reasons

[Daniel Jacobowitz]

On Sun, Aug 20, 2006 at 04:38:19PM +0200, Mark Kettenis wrote:
> > Any comments on this patch?  It doesn't have any major change on GDB's
> > visible behavior; it changes one error to a printf, but that error was
> > always wrapped in a catch_exceptions anyway, so this is unimportant.
> > The text of the message on a corrupt stack is changed mildly; it says
> > "Backtrace stopped: previous..." rather than just "Previous...".
> 
> That's a useful clarification I think.

Oh, here's another thing which I think is useful: you'll get the
"Backtrace stopped" message every time you say backtrace, but you used
to only get the Previous... error the first time, which I always found
somewhat confusing.  That's because we cached the failure, but not the
failure reason.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc, frame] Add backtrace stop reasons

[Mark Kettenis]

> Date: Sun, 20 Aug 2006 12:28:21 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> Hi Mark, thanks for looking at these.
> 
> On Sun, Aug 20, 2006 at 04:38:19PM +0200, Mark Kettenis wrote:
> > > A nice bonus, in my opinion: if the backtrace stops because unwinding
> > > detects an error or the end of the stack, "info frame" will explain
> > > why.  For instance:
> > > 
> > >   Stops backtrace: previous frame inner to this frame (corrupt stack?)
> > 
> > Is this really useful?  It adds an extra line to the "info frame"
> > output, which might mean that more useful information about the frame
> > scrolls of my screen :(.
> 
> (gdb) info frame
> Stack level 0, frame at 0xbf8e5838:
>  eip = 0xffffe410 in __kernel_vsyscall; saved eip 0xb7ea61cb
>  called by frame at 0xbf8e5850
>  Arglist at 0xbf8e5830, args:
>  Locals at 0xbf8e5830, Previous frame's sp is 0xbf8e5838
>  Saved registers:
>   ebp at 0xbf8e5828, eip at 0xbf8e5834
> 
> Where are we really in danger of this scrolling off the screen?  Frames
> with dozens of arguments (which generally wouldn't stop the backtrace
> anyway, so wouldn't have the line)?  I can imagine reasons to dislike
> adding more information, but I confess this one confuses me a bit :-)

But you're cheating by choosing an example from a register-starved
architecture ;-)  Here's the output on 64-bit SPARC:

Stack level 2, frame at 0xfffffffffffc8cc0:
 pc = 0x100a54 in main (../../../../src/gdb/gdb/testsuite/gdb.base/run.c:63); 
    saved pc 0x1007b8
 caller of frame at 0xfffffffffffc8bf0
 source language c.
 Arglist at 0xfffffffffffc8cc0, args: argc=2, argv=0xfffffffffffc8e08, 
    envp=0xfffffffffffc8e20
 Locals at 0xfffffffffffc8cc0, Previous frame's sp in fp
 Saved registers:
  l0 at 0xfffffffffffc8cc0, l1 at 0xfffffffffffc8cc8, l2 at 0xfffffffffffc8cd0,
  l3 at 0xfffffffffffc8cd8, l4 at 0xfffffffffffc8ce0, l5 at 0xfffffffffffc8ce8,
  l6 at 0xfffffffffffc8cf0, l7 at 0xfffffffffffc8cf8, i0 at 0xfffffffffffc8d00,
  i1 at 0xfffffffffffc8d08, i2 at 0xfffffffffffc8d10, i3 at 0xfffffffffffc8d18,
  i4 at 0xfffffffffffc8d20, i5 at 0xfffffffffffc8d28, fp at 0xfffffffffffc8d30

It's not yet a screenful, but already getting close.  I think I've
seen even worse on 64-bit MIPS, but indeed it is not too bad yet.

But I guess I'd really wanted to point out that we should be careful
about printing out too much information.  On the other hand we would
only print the additionol info for the last frame on the chain.  It's
my feeling though that "Stops backtrace" does not indicate a property
of the frame like the other things we print.  But printing something
like "Outermost frame: unwinding indicated no return address".  sounds
better to me.

What do other people think?

> If you want; there's no point in this unless at least one of the other
> patches goes in.
> 
> The problem with holding things until we really need them is that we
> build up these huge queues of patches related to a single project.
> I'm sure this code wouldn't turn into abandonware.  But it's a small
> patch, so I can easily carry it along until we're ready for it if you
> prefer.

I think we can work something out that we both agree on pretty soon,
at least for this part

Mark

Re: [rfc, frame] Add backtrace stop reasons

[Daniel Jacobowitz]

On Tue, Aug 22, 2006 at 10:09:31PM +0200, Mark Kettenis wrote:
> But you're cheating by choosing an example from a register-starved
> architecture ;-)  Here's the output on 64-bit SPARC:

Well yeah :-)

> It's not yet a screenful, but already getting close.  I think I've
> seen even worse on 64-bit MIPS, but indeed it is not too bad yet.

I doubt it; SPARC64 tends to have more saved registers, because of the
large windows, than MIPS.  I wonder how bad IA64 is though!

> But I guess I'd really wanted to point out that we should be careful
> about printing out too much information.  On the other hand we would
> only print the additionol info for the last frame on the chain.  It's
> my feeling though that "Stops backtrace" does not indicate a property
> of the frame like the other things we print.  But printing something
> like "Outermost frame: unwinding indicated no return address".  sounds
> better to me.

Ooh, that's a good point.  I've changed the message in my copies of the
patch; I like yours much better!

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc, frame] Add backtrace stop reasons

[Mark Kettenis]

> Date: Sun, 20 Aug 2006 12:50:07 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> On Sun, Aug 20, 2006 at 04:38:19PM +0200, Mark Kettenis wrote:
> > > Any comments on this patch?  It doesn't have any major change on GDB's
> > > visible behavior; it changes one error to a printf, but that error was
> > > always wrapped in a catch_exceptions anyway, so this is unimportant.
> > > The text of the message on a corrupt stack is changed mildly; it says
> > > "Backtrace stopped: previous..." rather than just "Previous...".
> > 
> > That's a useful clarification I think.
> 
> Oh, here's another thing which I think is useful: you'll get the
> "Backtrace stopped" message every time you say backtrace, but you used
> to only get the Previous... error the first time, which I always found
> somewhat confusing.  That's because we cached the failure, but not the
> failure reason.

Ouch yes, this is a very good reason to add this functionality.

Re: [rfc, frame] Add backtrace stop reasons

[Mark Kettenis]

> Date: Tue, 22 Aug 2006 16:24:25 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> On Tue, Aug 22, 2006 at 10:09:31PM +0200, Mark Kettenis wrote:
> > But you're cheating by choosing an example from a register-starved
> > architecture ;-)  Here's the output on 64-bit SPARC:
> 
> Well yeah :-)
> 
> > It's not yet a screenful, but already getting close.  I think I've
> > seen even worse on 64-bit MIPS, but indeed it is not too bad yet.
> 
> I doubt it; SPARC64 tends to have more saved registers, because of the
> large windows, than MIPS.  I wonder how bad IA64 is though!

Heh, I wanted to give a *reasonable* example ;-).

> > But I guess I'd really wanted to point out that we should be careful
> > about printing out too much information.  On the other hand we would
> > only print the additionol info for the last frame on the chain.  It's
> > my feeling though that "Stops backtrace" does not indicate a property
> > of the frame like the other things we print.  But printing something
> > like "Outermost frame: unwinding indicated no return address".  sounds
> > better to me.
> 
> Ooh, that's a good point.  I've changed the message in my copies of the
> patch; I like yours much better!

Perhaps you should post that updated patch!

Mark

Re: [rfc, frame] Add backtrace stop reasons

[Daniel Jacobowitz]

On Tue, Aug 22, 2006 at 10:37:53PM +0200, Mark Kettenis wrote:
> Perhaps you should post that updated patch!

Sure; here it is.

-- 
Daniel Jacobowitz
CodeSourcery

2006-08-19  Daniel Jacobowitz  <dan@codesourcery.com>

	* frame.c (struct frame_info): Add stop_reason.
	(get_prev_frame_1): Set stop_reason.  Don't call error for
	stop reasons.
	(get_frame_unwind_stop_reason, frame_stop_reason_string): New.
	* frame.h (enum unwind_stop_reason): New.
	(get_frame_unwind_stop_reason, frame_stop_reason_string): New
	prototypes.
	* stack.c (frame_info): Print the stop reason.
	(backtrace_command_1): Print the stop reason for errors.

---
 gdb/frame.c |   67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 gdb/frame.h |   44 +++++++++++++++++++++++++++++++++++++++
 gdb/stack.c |   25 ++++++++++++++++++++++
 3 files changed, 134 insertions(+), 2 deletions(-)

Index: src/gdb/frame.c
===================================================================
--- src.orig/gdb/frame.c	2006-08-19 11:28:50.000000000 -0400
+++ src/gdb/frame.c	2006-08-19 11:32:17.000000000 -0400
@@ -107,6 +107,10 @@ struct frame_info
   struct frame_info *next; /* down, inner, younger */
   int prev_p;
   struct frame_info *prev; /* up, outer, older */
+
+  /* The reason why we could not set PREV, or UNWIND_NO_REASON if we
+     could.  Only valid when PREV_P is set.  */
+  enum unwind_stop_reason stop_reason;
 };
 
 /* Flag to control debugging.  */
@@ -1055,6 +1059,7 @@ get_prev_frame_1 (struct frame_info *thi
       return this_frame->prev;
     }
   this_frame->prev_p = 1;
+  this_frame->stop_reason = UNWIND_NO_REASON;
 
   /* Check that this frame's ID was valid.  If it wasn't, don't try to
      unwind to the prev frame.  Be careful to not apply this test to
@@ -1068,6 +1073,7 @@ get_prev_frame_1 (struct frame_info *thi
 	  fprint_frame (gdb_stdlog, NULL);
 	  fprintf_unfiltered (gdb_stdlog, " // this ID is NULL }\n");
 	}
+      this_frame->stop_reason = UNWIND_NULL_ID;
       return NULL;
     }
 
@@ -1078,14 +1084,32 @@ get_prev_frame_1 (struct frame_info *thi
   if (this_frame->next->level >= 0
       && this_frame->next->unwind->type != SIGTRAMP_FRAME
       && frame_id_inner (this_id, get_frame_id (this_frame->next)))
-    error (_("Previous frame inner to this frame (corrupt stack?)"));
+    {
+      if (frame_debug)
+	{
+	  fprintf_unfiltered (gdb_stdlog, "-> ");
+	  fprint_frame (gdb_stdlog, NULL);
+	  fprintf_unfiltered (gdb_stdlog, " // this frame ID is inner }\n");
+	}
+      this_frame->stop_reason = UNWIND_INNER_ID;
+      return NULL;
+    }
 
   /* Check that this and the next frame are not identical.  If they
      are, there is most likely a stack cycle.  As with the inner-than
      test above, avoid comparing the inner-most and sentinel frames.  */
   if (this_frame->level > 0
       && frame_id_eq (this_id, get_frame_id (this_frame->next)))
-    error (_("Previous frame identical to this frame (corrupt stack?)"));
+    {
+      if (frame_debug)
+	{
+	  fprintf_unfiltered (gdb_stdlog, "-> ");
+	  fprint_frame (gdb_stdlog, NULL);
+	  fprintf_unfiltered (gdb_stdlog, " // this frame has same ID }\n");
+	}
+      this_frame->stop_reason = UNWIND_SAME_ID;
+      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
@@ -1556,6 +1580,45 @@ frame_sp_unwind (struct frame_info *next
   internal_error (__FILE__, __LINE__, _("Missing unwind SP method"));
 }
 
+/* Return the reason why we can't unwind past FRAME.  */
+
+enum unwind_stop_reason
+get_frame_unwind_stop_reason (struct frame_info *frame)
+{
+  /* If we haven't tried to unwind past this point yet, then assume
+     that unwinding would succeed.  */
+  if (frame->prev_p == 0)
+    return UNWIND_NO_REASON;
+
+  /* Otherwise, we set a reason when we succeeded (or failed) to
+     unwind.  */
+  return frame->stop_reason;
+}
+
+/* Return a string explaining REASON.  */
+
+const char *
+frame_stop_reason_string (enum unwind_stop_reason reason)
+{
+  switch (reason)
+    {
+    case UNWIND_NULL_ID:
+      return _("unwinder did not report frame ID");
+
+    case UNWIND_INNER_ID:
+      return _("previous frame inner to this frame (corrupt stack?)");
+
+    case UNWIND_SAME_ID:
+      return _("previous frame identical to this frame (corrupt stack?)");
+
+    case UNWIND_NO_REASON:
+    case UNWIND_FIRST_ERROR:
+    default:
+      internal_error (__FILE__, __LINE__,
+		      "Invalid frame stop reason");
+    }
+}
+
 extern initialize_file_ftype _initialize_frame; /* -Wmissing-prototypes */
 
 static struct cmd_list_element *set_backtrace_cmdlist;
Index: src/gdb/frame.h
===================================================================
--- src.orig/gdb/frame.h	2006-08-19 11:28:50.000000000 -0400
+++ src/gdb/frame.h	2006-08-19 11:29:33.000000000 -0400
@@ -394,6 +394,50 @@ enum frame_type
 };
 extern enum frame_type get_frame_type (struct frame_info *);
 
+/* For frames where we can not unwind further, describe why.  */
+
+enum unwind_stop_reason
+  {
+    /* No particular reason; either we haven't tried unwinding yet,
+       or we didn't fail.  */
+    UNWIND_NO_REASON,
+
+    /* The previous frame's analyzer returns an invalid result
+       from this_id.
+
+       FIXME drow/2006-08-16: This is how GDB used to indicate end of
+       stack.  We should migrate to a model where frames always have a
+       valid ID, and this becomes not just an error but an internal
+       error.  But that's a project for another day.  */
+    UNWIND_NULL_ID,
+
+    /* All the conditions after this point are considered errors;
+       abnormal stack termination.  If a backtrace stops for one
+       of these reasons, we'll let the user know.  This marker
+       is not a valid stop reason.  */
+    UNWIND_FIRST_ERROR,
+
+    /* This frame ID looks like it ought to belong to a NEXT frame,
+       but we got it for a PREV frame.  Normally, this is a sign of
+       unwinder failure.  It could also indicate stack corruption.  */
+    UNWIND_INNER_ID,
+
+    /* This frame has the same ID as the previous one.  That means
+       that unwinding further would almost certainly give us another
+       frame with exactly the same ID, so break the chain.  Normally,
+       this is a sign of unwinder failure.  It could also indicate
+       stack corruption.  */
+    UNWIND_SAME_ID,
+  };
+
+/* Return the reason why we can't unwind past this frame.  */
+
+enum unwind_stop_reason get_frame_unwind_stop_reason (struct frame_info *);
+
+/* Translate a reason code to an informative string.  */
+
+const char *frame_stop_reason_string (enum unwind_stop_reason);
+
 /* Unwind the stack frame so that the value of REGNUM, in the previous
    (up, older) frame is returned.  If VALUEP is NULL, don't
    fetch/compute the value.  Instead just return the location of the
Index: src/gdb/stack.c
===================================================================
--- src.orig/gdb/stack.c	2006-08-19 11:28:50.000000000 -0400
+++ src/gdb/stack.c	2006-08-19 11:28:59.000000000 -0400
@@ -923,6 +923,15 @@ frame_info (char *addr_exp, int from_tty
   deprecated_print_address_numeric (frame_pc_unwind (fi), 1, gdb_stdout);
   printf_filtered ("\n");
 
+  if (calling_frame_info == NULL)
+    {
+      enum unwind_stop_reason reason;
+
+      reason = get_frame_unwind_stop_reason (fi);
+      printf_filtered (_(" Outermost frame: %s\n"),
+			 frame_stop_reason_string (reason));
+    }
+
   if (calling_frame_info)
     {
       printf_filtered (" called by frame at ");
@@ -940,6 +949,7 @@ frame_info (char *addr_exp, int from_tty
     }
   if (get_next_frame (fi) || calling_frame_info)
     puts_filtered ("\n");
+
   if (s)
     printf_filtered (" source language %s.\n",
 		     language_str (s->language));
@@ -1163,11 +1173,26 @@ backtrace_command_1 (char *count_exp, in
       print_frame_info (fi, 1, LOCATION, 1);
       if (show_locals)
 	print_frame_local_vars (fi, 1, gdb_stdout);
+
+      /* Save the last frame to check for error conditions.  */
+      trailing = fi;
     }
 
   /* If we've stopped before the end, mention that.  */
   if (fi && from_tty)
     printf_filtered (_("(More stack frames follow...)\n"));
+
+  /* If we've run out of frames, and the reason appears to be an error
+     condition, print it.  */
+  if (fi == NULL && trailing != NULL)
+    {
+      enum unwind_stop_reason reason;
+
+      reason = get_frame_unwind_stop_reason (trailing);
+      if (reason > UNWIND_FIRST_ERROR)
+	printf_filtered (_("Backtrace stopped: %s\n"),
+			 frame_stop_reason_string (reason));
+    }
 }
 
 struct backtrace_command_args

Re: [rfc, frame] Add backtrace stop reasons

[Daniel Jacobowitz]

On Tue, Aug 22, 2006 at 04:41:31PM -0400, Daniel Jacobowitz wrote:
> On Tue, Aug 22, 2006 at 10:37:53PM +0200, Mark Kettenis wrote:
> > Perhaps you should post that updated patch!
> 
> Sure; here it is.

> 2006-08-19  Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	* frame.c (struct frame_info): Add stop_reason.
> 	(get_prev_frame_1): Set stop_reason.  Don't call error for
> 	stop reasons.
> 	(get_frame_unwind_stop_reason, frame_stop_reason_string): New.
> 	* frame.h (enum unwind_stop_reason): New.
> 	(get_frame_unwind_stop_reason, frame_stop_reason_string): New
> 	prototypes.
> 	* stack.c (frame_info): Print the stop reason.
> 	(backtrace_command_1): Print the stop reason for errors.

Mark, I believe you were happy with this version of the patch; is that
right?  No one else commented, so if you think it's OK, I will check it
in.

The second patch from this group of three definitely requires more
discussion, but the third was pretty clear-cut, and Michael reminded
me that I hadn't gotten back to it.  It could be untangled from this
one, but if this is OK, I'd rather put it in first.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc, frame] Add backtrace stop reasons

[Daniel Jacobowitz]

On Tue, Aug 22, 2006 at 04:41:31PM -0400, Daniel Jacobowitz wrote:
> 2006-08-19  Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	* frame.c (struct frame_info): Add stop_reason.
> 	(get_prev_frame_1): Set stop_reason.  Don't call error for
> 	stop reasons.
> 	(get_frame_unwind_stop_reason, frame_stop_reason_string): New.
> 	* frame.h (enum unwind_stop_reason): New.
> 	(get_frame_unwind_stop_reason, frame_stop_reason_string): New
> 	prototypes.
> 	* stack.c (frame_info): Print the stop reason.
> 	(backtrace_command_1): Print the stop reason for errors.

There was general agreement on this patch, and no recent comments.
I retested it (fixing a silly bug in the process), and committed it.

-- 
Daniel Jacobowitz
CodeSourcery

2006-10-18  Daniel Jacobowitz  <dan@codesourcery.com>

	* frame.c (struct frame_info): Add stop_reason.
	(get_prev_frame_1): Set stop_reason.  Don't call error for
	stop reasons.
	(get_frame_unwind_stop_reason, frame_stop_reason_string): New.
	* frame.h (enum unwind_stop_reason): New.
	(get_frame_unwind_stop_reason, frame_stop_reason_string): New
	prototypes.
	* stack.c (frame_info): Print the stop reason.
	(backtrace_command_1): Print the stop reason for errors.

Index: frame.c
===================================================================
RCS file: /cvs/src/src/gdb/frame.c,v
retrieving revision 1.213
diff -u -p -r1.213 frame.c
--- frame.c	8 Aug 2006 21:32:37 -0000	1.213
+++ frame.c	18 Oct 2006 19:39:02 -0000
@@ -107,6 +107,10 @@ struct frame_info
   struct frame_info *next; /* down, inner, younger */
   int prev_p;
   struct frame_info *prev; /* up, outer, older */
+
+  /* The reason why we could not set PREV, or UNWIND_NO_REASON if we
+     could.  Only valid when PREV_P is set.  */
+  enum unwind_stop_reason stop_reason;
 };
 
 /* Flag to control debugging.  */
@@ -1055,6 +1059,7 @@ get_prev_frame_1 (struct frame_info *thi
       return this_frame->prev;
     }
   this_frame->prev_p = 1;
+  this_frame->stop_reason = UNWIND_NO_REASON;
 
   /* Check that this frame's ID was valid.  If it wasn't, don't try to
      unwind to the prev frame.  Be careful to not apply this test to
@@ -1068,6 +1073,7 @@ get_prev_frame_1 (struct frame_info *thi
 	  fprint_frame (gdb_stdlog, NULL);
 	  fprintf_unfiltered (gdb_stdlog, " // this ID is NULL }\n");
 	}
+      this_frame->stop_reason = UNWIND_NULL_ID;
       return NULL;
     }
 
@@ -1078,14 +1084,32 @@ get_prev_frame_1 (struct frame_info *thi
   if (this_frame->next->level >= 0
       && this_frame->next->unwind->type != SIGTRAMP_FRAME
       && frame_id_inner (this_id, get_frame_id (this_frame->next)))
-    error (_("Previous frame inner to this frame (corrupt stack?)"));
+    {
+      if (frame_debug)
+	{
+	  fprintf_unfiltered (gdb_stdlog, "-> ");
+	  fprint_frame (gdb_stdlog, NULL);
+	  fprintf_unfiltered (gdb_stdlog, " // this frame ID is inner }\n");
+	}
+      this_frame->stop_reason = UNWIND_INNER_ID;
+      return NULL;
+    }
 
   /* Check that this and the next frame are not identical.  If they
      are, there is most likely a stack cycle.  As with the inner-than
      test above, avoid comparing the inner-most and sentinel frames.  */
   if (this_frame->level > 0
       && frame_id_eq (this_id, get_frame_id (this_frame->next)))
-    error (_("Previous frame identical to this frame (corrupt stack?)"));
+    {
+      if (frame_debug)
+	{
+	  fprintf_unfiltered (gdb_stdlog, "-> ");
+	  fprint_frame (gdb_stdlog, NULL);
+	  fprintf_unfiltered (gdb_stdlog, " // this frame has same ID }\n");
+	}
+      this_frame->stop_reason = UNWIND_SAME_ID;
+      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
@@ -1556,6 +1580,45 @@ frame_sp_unwind (struct frame_info *next
   internal_error (__FILE__, __LINE__, _("Missing unwind SP method"));
 }
 
+/* Return the reason why we can't unwind past FRAME.  */
+
+enum unwind_stop_reason
+get_frame_unwind_stop_reason (struct frame_info *frame)
+{
+  /* If we haven't tried to unwind past this point yet, then assume
+     that unwinding would succeed.  */
+  if (frame->prev_p == 0)
+    return UNWIND_NO_REASON;
+
+  /* Otherwise, we set a reason when we succeeded (or failed) to
+     unwind.  */
+  return frame->stop_reason;
+}
+
+/* Return a string explaining REASON.  */
+
+const char *
+frame_stop_reason_string (enum unwind_stop_reason reason)
+{
+  switch (reason)
+    {
+    case UNWIND_NULL_ID:
+      return _("unwinder did not report frame ID");
+
+    case UNWIND_INNER_ID:
+      return _("previous frame inner to this frame (corrupt stack?)");
+
+    case UNWIND_SAME_ID:
+      return _("previous frame identical to this frame (corrupt stack?)");
+
+    case UNWIND_NO_REASON:
+    case UNWIND_FIRST_ERROR:
+    default:
+      internal_error (__FILE__, __LINE__,
+		      "Invalid frame stop reason");
+    }
+}
+
 extern initialize_file_ftype _initialize_frame; /* -Wmissing-prototypes */
 
 static struct cmd_list_element *set_backtrace_cmdlist;
Index: frame.h
===================================================================
RCS file: /cvs/src/src/gdb/frame.h,v
retrieving revision 1.148
diff -u -p -r1.148 frame.h
--- frame.h	30 Mar 2006 16:37:12 -0000	1.148
+++ frame.h	18 Oct 2006 19:39:02 -0000
@@ -394,6 +394,50 @@ enum frame_type
 };
 extern enum frame_type get_frame_type (struct frame_info *);
 
+/* For frames where we can not unwind further, describe why.  */
+
+enum unwind_stop_reason
+  {
+    /* No particular reason; either we haven't tried unwinding yet,
+       or we didn't fail.  */
+    UNWIND_NO_REASON,
+
+    /* The previous frame's analyzer returns an invalid result
+       from this_id.
+
+       FIXME drow/2006-08-16: This is how GDB used to indicate end of
+       stack.  We should migrate to a model where frames always have a
+       valid ID, and this becomes not just an error but an internal
+       error.  But that's a project for another day.  */
+    UNWIND_NULL_ID,
+
+    /* All the conditions after this point are considered errors;
+       abnormal stack termination.  If a backtrace stops for one
+       of these reasons, we'll let the user know.  This marker
+       is not a valid stop reason.  */
+    UNWIND_FIRST_ERROR,
+
+    /* This frame ID looks like it ought to belong to a NEXT frame,
+       but we got it for a PREV frame.  Normally, this is a sign of
+       unwinder failure.  It could also indicate stack corruption.  */
+    UNWIND_INNER_ID,
+
+    /* This frame has the same ID as the previous one.  That means
+       that unwinding further would almost certainly give us another
+       frame with exactly the same ID, so break the chain.  Normally,
+       this is a sign of unwinder failure.  It could also indicate
+       stack corruption.  */
+    UNWIND_SAME_ID,
+  };
+
+/* Return the reason why we can't unwind past this frame.  */
+
+enum unwind_stop_reason get_frame_unwind_stop_reason (struct frame_info *);
+
+/* Translate a reason code to an informative string.  */
+
+const char *frame_stop_reason_string (enum unwind_stop_reason);
+
 /* Unwind the stack frame so that the value of REGNUM, in the previous
    (up, older) frame is returned.  If VALUEP is NULL, don't
    fetch/compute the value.  Instead just return the location of the
Index: stack.c
===================================================================
RCS file: /cvs/src/src/gdb/stack.c,v
retrieving revision 1.139
diff -u -p -r1.139 stack.c
--- stack.c	19 May 2006 20:42:47 -0000	1.139
+++ stack.c	18 Oct 2006 19:39:03 -0000
@@ -923,6 +923,16 @@ frame_info (char *addr_exp, int from_tty
   deprecated_print_address_numeric (frame_pc_unwind (fi), 1, gdb_stdout);
   printf_filtered ("\n");
 
+  if (calling_frame_info == NULL)
+    {
+      enum unwind_stop_reason reason;
+
+      reason = get_frame_unwind_stop_reason (fi);
+      if (reason != UNWIND_NO_REASON)
+	printf_filtered (_(" Outermost frame: %s\n"),
+			 frame_stop_reason_string (reason));
+    }
+
   if (calling_frame_info)
     {
       printf_filtered (" called by frame at ");
@@ -940,6 +950,7 @@ frame_info (char *addr_exp, int from_tty
     }
   if (get_next_frame (fi) || calling_frame_info)
     puts_filtered ("\n");
+
   if (s)
     printf_filtered (" source language %s.\n",
 		     language_str (s->language));
@@ -1163,11 +1174,26 @@ backtrace_command_1 (char *count_exp, in
       print_frame_info (fi, 1, LOCATION, 1);
       if (show_locals)
 	print_frame_local_vars (fi, 1, gdb_stdout);
+
+      /* Save the last frame to check for error conditions.  */
+      trailing = fi;
     }
 
   /* If we've stopped before the end, mention that.  */
   if (fi && from_tty)
     printf_filtered (_("(More stack frames follow...)\n"));
+
+  /* If we've run out of frames, and the reason appears to be an error
+     condition, print it.  */
+  if (fi == NULL && trailing != NULL)
+    {
+      enum unwind_stop_reason reason;
+
+      reason = get_frame_unwind_stop_reason (trailing);
+      if (reason > UNWIND_FIRST_ERROR)
+	printf_filtered (_("Backtrace stopped: %s\n"),
+			 frame_stop_reason_string (reason));
+    }
 }
 
 struct backtrace_command_args