[gdb-7.0] Re: RFC: Mark outer frames

[Joel Brobecker <brobecker at adacore dot com>]

Hello,

We need to make a decision. We have a failed-assertion on x86-linux,
and it is a regression compared to gdb-6.8.  We have decided that
we had to fix this issue before starting the gdb-7.0 release.

This failed assertion is one symptom of the same issue that manifests
itself in several ways (I think we have at least a couple of PRs for
that).  Daniel sent a patch as an RFC, because it implements a temporary
solution to a larger design shortcoming:

> The magic outer frame ID is, IMO, a workaround.  What we really want
> is to have frame IDs wherever we can.  Either "stack address
> uncertain, but function known" or even "stack address and function
> known, but outerness detected".  This requires changes to the
> unwinding interface, and additional changes to each affected unwinder
> to build the best IDs we can and to mark the outer frame some other
> way.

The issue with the current patch is summarized:

> 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 "ugly" piece in infrun.c is the following hunk:

  +  /* 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))))

Maybe the patch is not perfect, but we're a little under pressure
if we want to release soon, and the patch itself is fairly localized.

If we want to only fix the PR that blocks us, we can try the patch that
I am attaching, which guards "info frame" against null frame IDs. It's
not very invasive, but it is at least as ugly as Daniel finds his patch
ugly, and only fixes the "info frame" command. However, if there are
objections to Daniel's patch, then my hack allows us to get going for
the relase without a major delay.

Any objection to Daniel's patch? I'm going to be aggressive here,
and apply on Friday unless someone objects. I am attaching Daniel's
patch for easier reference. Mine is attached next.

-- 
Joel

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

commit ddc49fb123fbca081091847a8f0f7a66475518b8
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Wed Sep 9 22:09:53 2009 -0400

        * stack.c (frame_info): Do not try to print the location of the saved
        registers for frames that have a NULL frame ID.

diff --git a/gdb/stack.c b/gdb/stack.c
index 1c37801..99459fe 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -986,6 +986,7 @@ static void
 frame_info (char *addr_exp, int from_tty)
 {
   struct frame_info *fi;
+  int outer_frame_p;
   struct symtab_and_line sal;
   struct symbol *func;
   struct symtab *s;
@@ -1001,6 +1002,16 @@ frame_info (char *addr_exp, int from_tty)
   fi = parse_frame_specification_1 (addr_exp, "No stack.", &selected_frame_p);
   gdbarch = get_frame_arch (fi);
 
+  /* The outer frame may have a null frame ID.  This can happen when
+     to a program that is stopped at the entry point, for instance.
+     We have to be careful of this case, because we cannot unwind registers
+     unless we have a valid frame ID.
+
+     FIXME: brobecker/2009-09-09:  This distinction is necessary only
+     because the unwinding machinery does not have a way to distinguish
+     null frames from outer frames.  If we fix that, we should be able
+     to get rid of this check and all the associated code that uses it.  */
+  outer_frame_p = !frame_id_p (get_frame_id (fi));
   /* Name of the value returned by get_frame_pc().  Per comments, "pc"
      is not a good name.  */
   if (gdbarch_pc_regnum (gdbarch) >= 0)
@@ -1077,9 +1088,15 @@ frame_info (char *addr_exp, int from_tty)
   if (sal.symtab)
     printf_filtered (" (%s:%d)", sal.symtab->filename, sal.line);
   puts_filtered ("; ");
-  wrap_here ("    ");
-  printf_filtered ("saved %s ", pc_regname);
-  fputs_filtered (paddress (gdbarch, frame_unwind_caller_pc (fi)), gdb_stdout);
+  if (!outer_frame_p)
+    {
+      /* This is the outer frame: None of the registers have been saved.
+         So do not try to print the address were the PC has been saved.  */
+      wrap_here ("    ");
+      printf_filtered ("saved %s ", pc_regname);
+      fputs_filtered (paddress (gdbarch, frame_unwind_caller_pc (fi)),
+		      gdb_stdout);
+    }
   printf_filtered ("\n");
 
   if (calling_frame_info == NULL)
@@ -1175,6 +1192,18 @@ frame_info (char *addr_exp, int from_tty)
     int i;
     int need_nl = 1;
 
+    /* We need to be careful with the outer frame which has a null
+       frame ID, as most of the register unwinding routines assume
+       a non-null frame ID.  We know that there are no saved register
+       at this point of the execution anyway, so just report this to
+       the user and return.  */
+    if (outer_frame_p)
+      {
+	printf_filtered (" no saved registers.\n");
+	do_cleanups (back_to);
+	return;
+      }
+
     /* The sp is special; what's displayed isn't the save address, but
        the value of the previous frame's sp.  This is a legacy thing,
        at one stage the frame cached the previous frame's SP instead