RFC: Mark outer frames

[Daniel Jacobowitz <drow at false dot org>]

This patch is a very lightly tested HEAD port of a patch I've been using
for several months.  I'm posting this because, as Joel reminded me, it
might help out with PR/9711 (Quadratic slowdown for "where" command).
It might not, too :-)  I haven't even tried.

This patch was originally for an entirely different problem, namely,
the dreaded "Value is not active" error.  What happens with that is
that we connect to some target, and we're in the middle of its startup
code before there's a stack frame.  No unwinder thinks we have a way
out of this frame, so nothing returns a valid this_id.  So the frame
id is null_frame_id - which is overloaded to mean 'unknown id' and
'no frame at all'.  The value checks are using it one way and the
unwinder is using it the other.

So this patch breaks it in half.  There's null_frame_id, and a new
outer_frame_id which is assigned to any frame that no unwinder has an
ID for.

The obvious pitfall is that the outer frame isn't a single consistent
frame.  So there's an ugly bit in infrun that says if we set the stack
pointer while inside an outer frame, and suddenly we're in a frame we
think we can unwind from (mostly incorrectly at this point), then
we've not changed functions.  Otherwise stepping through _start will
blow up on some platforms I tried.

The reason this might help with 9711 is that we'll have a valid frame
ID for the register values of the outermost frame.  Previously we had
nothing.  I'm not entirely sure, now, why I was convinced that would
help.

Thoughts?  Does this actually help with 9711?  Is it too ugly to live?

-- 
Daniel Jacobowitz
CodeSourcery

2009-08-28  Daniel Jacobowitz  <dan@codesourcery.com>

	* frame.c (get_frame_id): Default to outer_frame_id if the this_id
	method does not supply an ID.  Assert that the result is not
	null_frame_id.
	(outer_frame_id): New.
	(frame_id_p): Accept outer_frame_id.
	(frame_id_eq): Allow outer_frame_id to be equal to itself.
	(frame_find_by_id): Revert previous local workarounds.
	(get_prev_frame_1): Adjust end-of-stack check to test outer_frame_id.
	* frame.h (null_frame_id, frame_id_p): Update comments.
	(outer_frame_id): Declare.
	* infrun.c (handle_inferior_event): Do not treat all steps from the
	outermost frame as subroutine calls.

	* libunwind-frame.c (libunwind_frame_this_id): Do not clear THIS_ID.
	* hppa-tdep.c (hppa_stub_frame_this_id): Likewise.
	* ia64-tdep.c (ia64_frame_this_id): Likewise.
	(ia64_libunwind_frame_this_id, ia64_libunwind_sigtramp_frame_this_id):
	Use outer_frame_id instead of null_frame_id.
	* amd64obsd-tdep.c (amd64obsd_trapframe_cache): Use outer_frame_id.
	* i386obsd-tdep.c (i386obsd_trapframe_cache): Likewise.
	* inline-frame.c (inline_frame_this_id): Refuse outer_frame_id.
	* thread.c (restore_selected_frame): Update comment and remove
	frame_id_p check.

	gdb/doc/
	* gdbint.texinfo (Unwinding the Frame ID): Reference outer_frame_id.

Index: amd64obsd-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/amd64obsd-tdep.c,v
retrieving revision 1.29
diff -u -p -r1.29 amd64obsd-tdep.c
--- amd64obsd-tdep.c	2 Jul 2009 17:25:52 -0000	1.29
+++ amd64obsd-tdep.c	28 Aug 2009 21:14:03 -0000
@@ -380,7 +380,7 @@ amd64obsd_trapframe_cache (struct frame_
   if ((cs & I386_SEL_RPL) == I386_SEL_UPL)
     {
       /* Trap from user space; terminate backtrace.  */
-      trad_frame_set_id (cache, null_frame_id);
+      trad_frame_set_id (cache, outer_frame_id);
     }
   else
     {
Index: frame.c
===================================================================
RCS file: /cvs/src/src/gdb/frame.c,v
retrieving revision 1.271
diff -u -p -r1.271 frame.c
--- frame.c	2 Jul 2009 17:25:53 -0000	1.271
+++ frame.c	28 Aug 2009 21:14:03 -0000
@@ -291,7 +291,10 @@ get_frame_id (struct frame_info *fi)
       if (fi->unwind == NULL)
 	fi->unwind = frame_unwind_find_by_frame (fi, &fi->prologue_cache);
       /* Find THIS frame's ID.  */
+      /* Default to outermost if no ID is found.  */
+      fi->this_id.value = outer_frame_id;
       fi->unwind->this_id (fi, &fi->prologue_cache, &fi->this_id.value);
+      gdb_assert (frame_id_p (fi->this_id.value));
       fi->this_id.p = 1;
       if (frame_debug)
 	{
@@ -328,6 +331,7 @@ frame_unwind_caller_id (struct frame_inf
 }
 
 const struct frame_id null_frame_id; /* All zeros.  */
+const struct frame_id outer_frame_id = { 0, 0, 0, 0, 0, 1, 0 };
 
 struct frame_id
 frame_id_build_special (CORE_ADDR stack_addr, CORE_ADDR code_addr,
@@ -369,6 +373,9 @@ frame_id_p (struct frame_id l)
   int p;
   /* The frame is valid iff it has a valid stack address.  */
   p = l.stack_addr_p;
+  /* outer_frame_id is also valid.  */
+  if (!p && memcmp (&l, &outer_frame_id, sizeof (l)) == 0)
+    p = 1;
   if (frame_debug)
     {
       fprintf_unfiltered (gdb_stdlog, "{ frame_id_p (l=");
@@ -391,7 +398,14 @@ int
 frame_id_eq (struct frame_id l, struct frame_id r)
 {
   int eq;
-  if (!l.stack_addr_p || !r.stack_addr_p)
+  if (!l.stack_addr_p && l.special_addr_p && !r.stack_addr_p && r.special_addr_p)
+    /* The outermost frame marker is equal to itself.  This is the
+       dodgy think about outer_frame_id, since between execution steps
+       we might step into another function - from which we can't
+       unwind either.  More thought required to get rid of
+       outer_frame_id.  */
+    eq = 1;
+  else if (!l.stack_addr_p || !r.stack_addr_p)
     /* Like a NaN, if either ID is invalid, the result is false.
        Note that a frame ID is invalid iff it is the null frame ID.  */
     eq = 0;
@@ -1376,7 +1390,7 @@ get_prev_frame_1 (struct frame_info *thi
      unwind to the prev frame.  Be careful to not apply this test to
      the sentinel frame.  */
   this_id = get_frame_id (this_frame);
-  if (this_frame->level >= 0 && !frame_id_p (this_id))
+  if (this_frame->level >= 0 && frame_id_eq (this_id, outer_frame_id))
     {
       if (frame_debug)
 	{
Index: frame.h
===================================================================
RCS file: /cvs/src/src/gdb/frame.h,v
retrieving revision 1.178
diff -u -p -r1.178 frame.h
--- frame.h	2 Jul 2009 17:09:28 -0000	1.178
+++ frame.h	28 Aug 2009 21:14:04 -0000
@@ -142,9 +142,14 @@ struct frame_id
 
 /* Methods for constructing and comparing Frame IDs.  */
 
-/* For convenience.  All fields are zero.  */
+/* For convenience.  All fields are zero.  This means "there is no frame".  */
 extern const struct frame_id null_frame_id;
 
+/* This means "there is no frame ID, but there is a frame".  It should be
+   replaced by best-effort frame IDs for the outermost frame, somehow.
+   The implementation is only special_addr_p set.  */
+extern const struct frame_id outer_frame_id;
+
 /* Flag to control debugging.  */
 
 extern int frame_debug;
@@ -170,7 +175,8 @@ extern struct frame_id frame_id_build_sp
 extern struct frame_id frame_id_build_wild (CORE_ADDR stack_addr);
 
 /* Returns non-zero when L is a valid frame (a valid frame has a
-   non-zero .base).  */
+   non-zero .base).  The outermost frame is valid even without an
+   ID.  */
 extern int frame_id_p (struct frame_id l);
 
 /* Returns non-zero when L is a valid frame representing an inlined
Index: hppa-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/hppa-tdep.c,v
retrieving revision 1.270
diff -u -p -r1.270 hppa-tdep.c
--- hppa-tdep.c	2 Jul 2009 17:25:54 -0000	1.270
+++ hppa-tdep.c	28 Aug 2009 21:14:04 -0000
@@ -2427,8 +2427,6 @@ hppa_stub_frame_this_id (struct frame_in
 
   if (info)
     *this_id = frame_id_build (info->base, get_frame_func (this_frame));
-  else
-    *this_id = null_frame_id;
 }
 
 static struct value *
Index: i386obsd-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/i386obsd-tdep.c,v
retrieving revision 1.37
diff -u -p -r1.37 i386obsd-tdep.c
--- i386obsd-tdep.c	2 Jul 2009 17:25:54 -0000	1.37
+++ i386obsd-tdep.c	28 Aug 2009 21:14:04 -0000
@@ -376,7 +376,7 @@ i386obsd_trapframe_cache (struct frame_i
   if ((cs & I386_SEL_RPL) == I386_SEL_UPL)
     {
       /* Trap from user space; terminate backtrace.  */
-      trad_frame_set_id (cache, null_frame_id);
+      trad_frame_set_id (cache, outer_frame_id);
     }
   else
     {
Index: ia64-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/ia64-tdep.c,v
retrieving revision 1.196
diff -u -p -r1.196 ia64-tdep.c
--- ia64-tdep.c	25 Aug 2009 14:06:47 -0000	1.196
+++ ia64-tdep.c	28 Aug 2009 21:14:05 -0000
@@ -1748,9 +1748,7 @@ ia64_frame_this_id (struct frame_info *t
     ia64_frame_cache (this_frame, this_cache);
 
   /* If outermost frame, mark with null frame id.  */
-  if (cache->base == 0)
-    (*this_id) = null_frame_id;
-  else
+  if (cache->base != 0)
     (*this_id) = frame_id_build_special (cache->base, cache->pc, cache->bsp);
   if (gdbarch_debug >= 1)
     fprintf_unfiltered (gdb_stdlog,
@@ -2764,15 +2762,14 @@ ia64_libunwind_frame_this_id (struct fra
 {
   struct gdbarch *gdbarch = get_frame_arch (this_frame);
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-  struct frame_id id;
+  struct frame_id id = outer_frame_id;
   char buf[8];
   CORE_ADDR bsp;
 
-
   libunwind_frame_this_id (this_frame, this_cache, &id);
-  if (frame_id_eq (id, null_frame_id))
+  if (frame_id_eq (id, outer_frame_id))
     {
-      (*this_id) = null_frame_id;
+      (*this_id) = outer_frame_id;
       return;
     }
 
@@ -2897,13 +2894,13 @@ ia64_libunwind_sigtramp_frame_this_id (s
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   char buf[8];
   CORE_ADDR bsp;
-  struct frame_id id;
+  struct frame_id id = outer_frame_id;
   CORE_ADDR prev_ip;
 
   libunwind_frame_this_id (this_frame, this_cache, &id);
-  if (frame_id_eq (id, null_frame_id))
+  if (frame_id_eq (id, outer_frame_id))
     {
-      (*this_id) = null_frame_id;
+      (*this_id) = outer_frame_id;
       return;
     }
 
Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.407
diff -u -p -r1.407 infrun.c
--- infrun.c	21 Aug 2009 18:54:44 -0000	1.407
+++ infrun.c	28 Aug 2009 21:14:06 -0000
@@ -3796,10 +3796,22 @@ infrun: not switching back to stepped th
      NOTE: frame_id_eq will never report two invalid frame IDs as
      being equal, so to get into this block, both the current and
      previous frame must have valid frame IDs.  */
+  /* The outer_frame_id check is a heuristic to detect stepping
+     through startup code.  If we step over an instruction which
+     sets the stack pointer from an invalid value to a valid value,
+     we may detect that as a subroutine call from the mythical
+     "outermost" function.  This could be fixed by marking
+     outermost frames as !stack_p,code_p,special_p.  Then the
+     initial outermost frame, before sp was valid, would
+     have code_addr == &_start.  See the commend in frame_id_eq
+     for more.  */
   if (!frame_id_eq (get_stack_frame_id (frame),
 		    ecs->event_thread->step_stack_frame_id)
-      && frame_id_eq (frame_unwind_caller_id (frame),
-		      ecs->event_thread->step_stack_frame_id))
+      && (frame_id_eq (frame_unwind_caller_id (get_current_frame ()),
+		       ecs->event_thread->step_stack_frame_id)
+	  && (!frame_id_eq (ecs->event_thread->step_stack_frame_id,
+			    outer_frame_id)
+	      || step_start_function != find_pc_function (stop_pc))))
     {
       CORE_ADDR real_stop_pc;
 
Index: inline-frame.c
===================================================================
RCS file: /cvs/src/src/gdb/inline-frame.c,v
retrieving revision 1.1
diff -u -p -r1.1 inline-frame.c
--- inline-frame.c	28 Jun 2009 00:20:22 -0000	1.1
+++ inline-frame.c	28 Aug 2009 21:14:06 -0000
@@ -148,6 +148,10 @@ inline_frame_this_id (struct frame_info 
      frame").  This will take work.  */
   gdb_assert (frame_id_p (*this_id));
 
+  /* For now, require we don't match outer_frame_id either (see
+     comment above).  */
+  gdb_assert (!frame_id_eq (*this_id, outer_frame_id));
+
   /* Future work NOTE: Alexandre Oliva applied a patch to GCC 4.3
      which generates DW_AT_entry_pc for inlined functions when
      possible.  If this attribute is available, we should use it
Index: libunwind-frame.c
===================================================================
RCS file: /cvs/src/src/gdb/libunwind-frame.c,v
retrieving revision 1.23
diff -u -p -r1.23 libunwind-frame.c
--- libunwind-frame.c	3 Jan 2009 05:57:52 -0000	1.23
+++ libunwind-frame.c	28 Aug 2009 21:14:06 -0000
@@ -278,8 +278,6 @@ libunwind_frame_this_id (struct frame_in
 
   if (cache != NULL)
     (*this_id) = frame_id_build (cache->base, cache->func_addr);
-  else
-    (*this_id) = null_frame_id;
 }
 
 struct value *
Index: thread.c
===================================================================
RCS file: /cvs/src/src/gdb/thread.c,v
retrieving revision 1.115
diff -u -p -r1.115 thread.c
--- thread.c	2 Jul 2009 21:57:27 -0000	1.115
+++ thread.c	28 Aug 2009 21:14:06 -0000
@@ -885,15 +885,11 @@ restore_selected_frame (struct frame_id 
   frame = find_relative_frame (get_current_frame (), &count);
   if (count == 0
       && frame != NULL
-      /* Either the frame ids match, of they're both invalid.  The
-	 latter case is not failsafe, but since it's highly unlikely
+      /* The frame ids must match - either both valid or both outer_frame_id.
+	 The latter case is not failsafe, but since it's highly unlikely
 	 the search by level finds the wrong frame, it's 99.9(9)% of
 	 the time (for all practical purposes) safe.  */
-      && (frame_id_eq (get_frame_id (frame), a_frame_id)
-	  /* Note: could be better to check every frame_id
-	     member for equality here.  */
-	  || (!frame_id_p (get_frame_id (frame))
-	      && !frame_id_p (a_frame_id))))
+      && frame_id_eq (get_frame_id (frame), a_frame_id))
     {
       /* Cool, all is fine.  */
       select_frame (frame);
Index: doc/gdbint.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdbint.texinfo,v
retrieving revision 1.311
diff -u -p -r1.311 gdbint.texinfo
--- doc/gdbint.texinfo	26 Aug 2009 04:16:38 -0000	1.311
+++ doc/gdbint.texinfo	28 Aug 2009 21:14:07 -0000
@@ -2042,7 +2042,7 @@ address as its caller.  On some platform
 the ID to further disambiguate frames---for instance, on IA-64
 the separate register stack address is included in the ID.
 
-An invalid frame ID (@code{null_frame_id}) returned from the
+An invalid frame ID (@code{outer_frame_id}) returned from the
 @code{this_id} method means to stop unwinding after this frame.
 
 @section Unwinding Registers