RFC: Mark outer frames

RFC: Mark outer frames

[Daniel Jacobowitz]

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

Re: RFC: Mark outer frames

[Eli Zaretskii]

> Date: Fri, 28 Aug 2009 17:32:37 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> 	gdb/doc/
> 	* gdbint.texinfo (Unwinding the Frame ID): Reference outer_frame_id.

This part is OK, but the code still has null_frame_id, so perhaps
deleting it entirely here is not really right.  How about adding a
sentence explaining how null_frame_id differs from outer_frame_id?

Re: RFC: Mark outer frames

[Joel Brobecker]

Hi Daniel,

Thanks for posting your patch.

> Does this actually help with 9711?

As far as I can tell, it doesn't. I am not seeing any noticeable
change in the timing. Oh well.

> Is it too ugly to live?

Not sure. I haven't taken the time to look at the patch itself
(trying to focus on the blocking items for the next release),
but if no one else has a better idea... We could try to brainstorm
on that after the branch has been cut...


-- 
Joel

Re: RFC: Mark outer frames

[Doug Evans]

On Mon, Aug 31, 2009 at 3:51 PM, Joel Brobecker<brobecker@adacore.com> wrote:
>>[...]
>> 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.
>>[...]
>> Is it too ugly to live?
>
> Not sure. I haven't taken the time to look at the patch itself
> (trying to focus on the blocking items for the next release),
> but if no one else has a better idea... We could try to brainstorm
> on that after the branch has been cut...

fwiw,
I've tripped over this myself a couple of times ("this" being the overloading).
I think this patch, or something akin to it, is needed.

Re: RFC: Mark outer frames

[Joel Brobecker]

FWIW:

> 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.

Didn't we have the same problem with null_frame_id before? I guess
not because equality to the null_frame_id is always false... The bit
in infrun does not seem all that horrible to me, but your comment does
suggest another way that you might think is better?

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

This does not strike me as something that would set us back in terms
of maintenance. I like the idea of splitting null_frame_id in two
different entities with a narrower meaning. The heuristic in infrun
is a little unfortunate, but not the worse I've seen by a large measure.
I would personally be OK with that patch, especially if it's useful to
others (Doug, for instance).

+    /* The outermost frame marker is equal to itself.  This is the
+       dodgy think about outer_frame_id, since between execution steps
              ^^^^^ thing

I wouldn't mind a more detailed comment about when outer_frame_id
should be used (we're in startup code and the associated frame has
not been setup yet). I got confused by "there is no frame ID, but
there is a frame".

-- 
Joel

Re: RFC: Mark outer frames

[Daniel Jacobowitz]

On Wed, Sep 02, 2009 at 10:03:24AM -0700, Joel Brobecker wrote:
> > 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.
> 
> Didn't we have the same problem with null_frame_id before? I guess
> not because equality to the null_frame_id is always false... The bit
> in infrun does not seem all that horrible to me, but your comment does
> suggest another way that you might think is better?

Sorry, I meant to go into that.

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.

I haven't spec'd out a full approach to this problem, just speculated
about it.

> +    /* The outermost frame marker is equal to itself.  This is the
> +       dodgy think about outer_frame_id, since between execution steps
>               ^^^^^ thing
> 

Thanks!

> I wouldn't mind a more detailed comment about when outer_frame_id
> should be used (we're in startup code and the associated frame has
> not been setup yet). I got confused by "there is no frame ID, but
> there is a frame".

This is, indeed, the central problem.

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: Mark outer frames

[Joel Brobecker]

> 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.

I just started looking at PR/9786:
    info frame: generates an error when remote debugging

Basically, on x86:

    (gdb) target rem :4444
    Remote debugging using :4444
    0xb7f6d840 in _start () from /lib/ld-linux.so.2
    (gdb) info frame
    Stack level 0, frame at 0x0:
     eip = 0xb7f6d840 in _start; saved eip 
    /[...]/findvar.c:304: internal-error: value_of_register_lazy: Assertion `frame_id_p (get_frame_id (frame))' failed.

This is the same issue as above, I believe, and your patch fixes
the issue too. After applying it, I get:

    (gdb) info frame
    Stack level 0, frame at 0x0:
     eip = 0xb7f6d840 in _start; saved eip 0xb7f6d840
     Outermost frame: unwinder did not report frame ID
     Arglist at unknown address.
     Locals at unknown address, Previous frame's sp in esp

So, should we apply you patch? (we don't have time for much else
except trying to see if we could plug that particular hole with
a temporary, localized, patch...

-- 
Joel

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

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 3204 bytes --]

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

[-- Attachment #2: outer-frame-id.diff --]
[-- Type: text/x-diff, Size: 13088 bytes --]

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

[-- Attachment #3: info-frame.diff --]
[-- Type: text/x-diff, Size: 3000 bytes --]

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

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

[Daniel Jacobowitz]

On Wed, Sep 09, 2009 at 07:28:34PM -0700, Joel Brobecker wrote:
> 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.

I have no objection.  I think the patch is progress.

There are two pieces of feedback: s/think/thing/ you found, and Eli
requested a slightly expanded explanation in the internals
documentation.  Do you feel up to the latter?

I apologize for being such a bum about this, but I haven't got the
time to spare right now; I'm under deadline pressure at work.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Joel Brobecker]

> There are two pieces of feedback: s/think/thing/ you found, and Eli
> requested a slightly expanded explanation in the internals
> documentation.  Do you feel up to the latter?

Sure, I'll give it a try.

> I apologize for being such a bum about this, but I haven't got the
> time to spare right now; I'm under deadline pressure at work.

No problem - as far as I am concerned, it's already been helpful to have
your patch to start with.

-- 
Joel

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

[Joel Brobecker]

Eli,

This patch adds some documentation back about null_frame_id, as you
asked a while back. Does this looks good to you?

-- 
Joel

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

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 165 bytes --]

[with the patch, this time...]

Eli,

This patch adds some documentation back about null_frame_id, as you
asked a while back. Does this looks good to you?

-- 
Joel

[-- Attachment #2: outer-frame.diff --]
[-- Type: text/x-diff, Size: 12537 bytes --]

commit dbcfda80ee04f0aba772bc7b58f10969243fddf7
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Sat Sep 12 14:10:27 2009 -0700

        * 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.

diff --git a/gdb/doc/gdbint.texinfo b/gdb/doc/gdbint.texinfo
index e706caa..52155c1 100644
--- a/gdb/doc/gdbint.texinfo
+++ b/gdb/doc/gdbint.texinfo
@@ -2042,9 +2042,17 @@ address as its caller.  On some platforms, a third address is part of
 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.
 
+@code{null_frame_id} is another invalid frame ID which should be used
+when there is no frame. For instance, certain breakpoints are attached
+to a specific frame, and that frame is identified through its frame ID
+(we use this to implement the "finish" command).  Using
+@code{null_frame_id} as the frame ID for a given breakpoint means
+that the breakpoint is not specific to any frame. The @code{this_id}
+method should never return @code{null_frame_id}.
+
 @section Unwinding Registers
 
 Each unwinder includes a @code{prev_register} method.  This method
diff --git a/gdb/amd64obsd-tdep.c b/gdb/amd64obsd-tdep.c
index 1938bdd..ed57cb8 100644
--- a/gdb/amd64obsd-tdep.c
+++ b/gdb/amd64obsd-tdep.c
@@ -380,7 +380,7 @@ amd64obsd_trapframe_cache (struct frame_info *this_frame, void **this_cache)
   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
     {
diff --git a/gdb/frame.c b/gdb/frame.c
index 9ca69bf..7932b48 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -324,7 +324,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)
 	{
@@ -364,6 +367,7 @@ frame_unwind_caller_id (struct frame_info *next_frame)
 }
 
 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,
@@ -405,6 +409,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=");
@@ -427,7 +434,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 thing 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;
@@ -1425,7 +1439,7 @@ get_prev_frame_1 (struct frame_info *this_frame)
      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)
 	{
diff --git a/gdb/frame.h b/gdb/frame.h
index 611c6d3..994b529 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -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_special (CORE_ADDR stack_addr,
 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
diff --git a/gdb/hppa-tdep.c b/gdb/hppa-tdep.c
index 072e687..03aa6c6 100644
--- a/gdb/hppa-tdep.c
+++ b/gdb/hppa-tdep.c
@@ -2427,8 +2427,6 @@ hppa_stub_frame_this_id (struct frame_info *this_frame,
 
   if (info)
     *this_id = frame_id_build (info->base, get_frame_func (this_frame));
-  else
-    *this_id = null_frame_id;
 }
 
 static struct value *
diff --git a/gdb/i386obsd-tdep.c b/gdb/i386obsd-tdep.c
index 8621838..c33a24e 100644
--- a/gdb/i386obsd-tdep.c
+++ b/gdb/i386obsd-tdep.c
@@ -376,7 +376,7 @@ i386obsd_trapframe_cache (struct frame_info *this_frame, void **this_cache)
   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
     {
diff --git a/gdb/ia64-tdep.c b/gdb/ia64-tdep.c
index bc72e40..674204a 100644
--- a/gdb/ia64-tdep.c
+++ b/gdb/ia64-tdep.c
@@ -1774,9 +1774,7 @@ ia64_frame_this_id (struct frame_info *this_frame, void **this_cache,
     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,
@@ -2790,15 +2788,14 @@ ia64_libunwind_frame_this_id (struct frame_info *this_frame, void **this_cache,
 {
   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;
     }
 
@@ -2923,13 +2920,13 @@ ia64_libunwind_sigtramp_frame_this_id (struct frame_info *this_frame,
   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;
     }
 
diff --git a/gdb/infrun.c b/gdb/infrun.c
index c907635..a6ca2e3 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3796,10 +3796,22 @@ infrun: not switching back to stepped thread, it has vanished\n");
      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;
 
diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c
index 731d3ad..0e6dae3 100644
--- a/gdb/inline-frame.c
+++ b/gdb/inline-frame.c
@@ -148,6 +148,10 @@ inline_frame_this_id (struct frame_info *this_frame,
      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
diff --git a/gdb/libunwind-frame.c b/gdb/libunwind-frame.c
index b1896dc..921d00e 100644
--- a/gdb/libunwind-frame.c
+++ b/gdb/libunwind-frame.c
@@ -278,8 +278,6 @@ libunwind_frame_this_id (struct frame_info *this_frame, void **this_cache,
 
   if (cache != NULL)
     (*this_id) = frame_id_build (cache->base, cache->func_addr);
-  else
-    (*this_id) = null_frame_id;
 }
 
 struct value *
diff --git a/gdb/thread.c b/gdb/thread.c
index a7ac3c8..55b4b96 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -885,15 +885,11 @@ restore_selected_frame (struct frame_id a_frame_id, int frame_level)
   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);

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

[Eli Zaretskii]

> Date: Sat, 12 Sep 2009 15:14:32 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> 
> This patch adds some documentation back about null_frame_id, as you
> asked a while back. Does this looks good to you?

Yes, but please be sure to have two spaces between sentences, not one.

Thanks.

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

[Joel Brobecker]

> > This patch adds some documentation back about null_frame_id, as you
> > asked a while back. Does this looks good to you?
> 
> Yes, but please be sure to have two spaces between sentences, not one.

Outch, rookie mistake :-(, sorry about that. I'm glad there is a review
process. Fixed and checked in.

Thanks,
-- 
Joel