[rfc, frame] Move the corrupt frame checks earlier

[Daniel Jacobowitz <drow at false dot org>]

This patch is a follow-on to the unwind_stop_reason patch, although not
actually dependent.  It moves the frame-went-inner and frame-same-ID
checks earlier in the backtrace process.  Although they move down
in the function, they also check PREV rather than THIS.

The benefit of doing this is that we display the stop reason before,
rather than after, the frame we've decided is corrupt.  For instance,
suppose we have two frames with the same ID.  That means they have the
same CFA and the same function.  In other words, the same stack frame.
So the second backtrace entry will be a duplicate of the first. There's
no added value in displaying it, and logically it doesn't need to exist
on the frame chain.

The risk of doing this is performance; we call get_frame_id in
get_prev_frame_1 now.  That defeats some of the advantages of the lazy
frame structure.  But, it turns out, that's OK.  The lazy frame
structure is still good and useful for the first frame, where it allows
us to maintain our principle of "there is always a frame" without
having to figure out much about that frame.  But any time that someone
wants the second frame on the stack, they usually want its ID too. So
we fetch it slightly earlier and there's no net change in work done.

I verified this by staring at "set debug frame 1" output for a couple
of different test cases.  Step within a function and step into a
function cause the same number of ID fetches (zero and two
respectively).  Backtrace also fetches roughly the same number of IDs;
it may do one extra at the bottom of the stack, which is fine.  I
also did some benchmarking; it was in the noise.

Any thoughts on this patch?  I think it's an improvement, and a
template for where more similar checks should go (like the zero PC
check, whether it's an error or not).

-- 
Daniel Jacobowitz
CodeSourcery

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

	* frame.c (get_prev_frame_1): Move some unwinding failure
	checks to after the frame is created, and one frame earlier.

---
 gdb/frame.c |   80 ++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 46 insertions(+), 34 deletions(-)

Index: src/gdb/frame.c
===================================================================
--- src.orig/gdb/frame.c	2006-08-19 00:29:06.000000000 -0400
+++ src/gdb/frame.c	2006-08-19 00:30:13.000000000 -0400
@@ -1077,40 +1077,6 @@ get_prev_frame_1 (struct frame_info *thi
       return NULL;
     }
 
-  /* Check that this frame's ID isn't inner to (younger, below, next)
-     the next frame.  This happens when a frame unwind goes backwards.
-     Exclude signal trampolines (due to sigaltstack the frame ID can
-     go backwards) and sentinel frames (the test is meaningless).  */
-  if (this_frame->next->level >= 0
-      && this_frame->next->unwind->type != SIGTRAMP_FRAME
-      && frame_id_inner (this_id, get_frame_id (this_frame->next)))
-    {
-      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)))
-    {
-      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
      frame->next to pull some fancy tricks (of course such code is, by
@@ -1147,6 +1113,52 @@ get_prev_frame_1 (struct frame_info *thi
   this_frame->prev = prev_frame;
   prev_frame->next = this_frame;
 
+  /* Now that the frame chain is in a consistant state, check whether
+     this frame is useful.  If it is not, unlink it.  Its storage will
+     be reclaimed the next time the frame cache is flushed, and we
+     will not try to unwind THIS_FRAME again.  We can't do these tests
+     earlier; we call things like get_frame_type, which need the frame
+     to be hooked up.  None of these checks apply to the current
+     frame; we deliberately avoid doing them for the current frame, to
+     improve single step performance.  */
+
+  /* Check that the new frame's ID isn't inner to (younger, below,
+     next) the current frame.  This happens when a frame unwind goes
+     backwards.  Exclude signal trampolines (due to sigaltstack the
+     frame ID can go backwards) and sentinel frames (the test is
+     meaningless).  */
+  if (prev_frame->level > 0
+      && prev_frame->next->unwind->type != SIGTRAMP_FRAME
+      && frame_id_inner (get_frame_id (prev_frame), this_id))
+    {
+      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;
+      this_frame->prev = NULL;
+      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 (prev_frame->level > 0
+      && frame_id_eq (get_frame_id (prev_frame), this_id))
+    {
+      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;
+      this_frame->prev = NULL;
+      return NULL;
+    }
+
   if (frame_debug)
     {
       fprintf_unfiltered (gdb_stdlog, "-> ");