Re: [PATCH 1/4] Distinguish sentinel frame from null frame

[Kevin Buettner <kevin at buettner dot to>]

On Wed, 12 Oct 2016 14:07:13 +0100
Pedro Alves <palves@redhat.com> wrote:

> > @@ -323,6 +334,8 @@ fprint_frame_id (struct ui_file *file, struct frame_id id)
> >      fprintf_unfiltered (file, "!stack");
> >    else if (id.stack_status == FID_STACK_UNAVAILABLE)
> >      fprintf_unfiltered (file, "stack=<unavailable>");
> > +  else if (id.stack_status == FID_STACK_SENTINEL)
> > +    fprintf_unfiltered (file, "stack=<sentinel>");
> >    else
> >      fprintf_unfiltered (file, "stack=%s", hex_string (id.stack_addr));
> >    fprintf_unfiltered (file, ",");
> > @@ -511,6 +524,9 @@ get_frame_id (struct frame_info *fi)
> >    if (fi == NULL)
> >      return null_frame_id;
> >  
> > +  if (fi == sentinel_frame)
> > +    return sentinel_frame_id;
> > +  
> 
> Why do you need this?  When the sentinel frame is created, it's
> given a frame id already:

You're right - this isn't needed.  I've removed it from a new
version of the patch which I'm preparing.

> >  /* Cache for frame addresses already read by gdb.  Valid only while
> >     inferior is stopped.  Control variables for the frame cache should
> >     be local to this module.  */
> > @@ -1511,6 +1527,9 @@ static struct frame_info *get_prev_frame_always_1 (struct frame_info *this_frame
> >  struct frame_info *
> >  get_current_frame (void)
> >  {
> > +  struct frame_info *current_frame;
> > +  int new_sentinel_p = 0;
> > +  /* Set the current frame before computing the frame id, to avoid
> > +     recursion inside compute_frame_id, in case the frame's
> > +     unwinder decides to do a symbol lookup (which depends on the
> > +     selected frame's block).
> > +
> > +     This call must always succeed.  In particular, nothing inside
> > +     get_prev_frame_always_1 should try to unwind from the
> > +     sentinel frame, because that could fail/throw, and we always
> > +     want to leave with the current frame created and linked in --
> > +     we should never end up with the sentinel frame as outermost
> > +     frame.  */
> > +  current_frame = get_prev_frame_always_1 (sentinel_frame);
> > +  gdb_assert (current_frame != NULL);
> > +
> > +  /* The call to get_frame_id, below, is not necessary when the stack is
> > +     well behaved.  But when it's not well behaved, we want to ensure
> > +     that the frame id for the current frame is known (stashed) prior to
> > +     finding frame id values for any outer frames.
> > +
> > +     The test gdb.dwarf2/dw2-dup-frame.exp shows an internal error
> > +     in the "backtrace from stop_frame" test when we don't do this here.  */
> > +  if (new_sentinel_p)
> > +    get_frame_id (current_frame);  
> 
> I don't understand this.  I applied patch #1 locally with this bit
> removed, and I don't see said internal error.
> 
> Oh, this only happens when the following patches are applied.
> 
> IMO, it's better to move this hunk to the patch that actually
> creates the requirement for it, so that we have the whole
> logical change in one patch.  Can you elaborate more on
> why the test causes the internal "backtrace from stop_frame"
> error without this here?

Here's the portion of the log file showing the failure/internal error:

(gdb) break stop_frame
Breakpoint 1 at 0x40059a: file dw2-dup-frame.c, line 22.
(gdb) run 
Starting program: /mesquite2/sourceware-git/mesquite-native-python-unwind-2-split/bld/gdb/testsuite/outputs/gdb.dwarf2/dw2-dup-frame/dw2-dup-frame 

Breakpoint 1, stop_frame () at dw2-dup-frame.c:22
22	}
(gdb) bt
/ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/frame.c:544: internal-error: frame_id get_frame_id(frame_info*): Assertion `stashed' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n) FAIL: gdb.dwarf2/dw2-dup-frame.exp: backtrace from stop_frame (GDB internal error)

Here's a partial backtrace from the internal error, showing the frames
which I think are relevant, plus several extra to provide context:

#0  internal_error (
    file=0x932b98 "/ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/frame.c", line=544, 
    fmt=0x932b20 "%s: Assertion `%s' failed.")
    at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/common/errors.c:54
#1  0x000000000072207e in get_frame_id (fi=0xe5a760)
    at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/frame.c:544
#2  0x00000000004eb50d in frame_info_to_frame_object (frame=0xe5a760)
    at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/python/py-frame.c:390
#3  0x00000000004ef5be in bootstrap_python_frame_filters (frame=0xe5a760, 
    frame_low=0, frame_high=-1)
    at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/python/py-framefilter.c:1453
#4  0x00000000004ef7a9 in gdbpy_apply_frame_filter (
    extlang=0x8857e0 <extension_language_python>, frame=0xe5a760, flags=7, 
    args_type=CLI_SCALAR_VALUES, out=0xf6def0, frame_low=0, frame_high=-1)
    at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/python/py-framefilter.c:1548
#5  0x00000000005f2c5a in apply_ext_lang_frame_filter (frame=0xe5a760, 
    flags=7, args_type=CLI_SCALAR_VALUES, out=0xf6def0, frame_low=0, 
    frame_high=-1)
    at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/extension.c:572
#6  0x00000000005ea896 in backtrace_command_1 (count_exp=0x0, show_locals=0, 
    no_filters=0, from_tty=1)
    at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/stack.c:1834

Examination of the code in frame_info_to_frame_object(), which is in
python/py-frame.c, is key to understanding this problem:

      if (get_prev_frame (frame) == NULL
	  && get_frame_unwind_stop_reason (frame) != UNWIND_NO_REASON
	  && get_next_frame (frame) != NULL)
	{
	  frame_obj->frame_id = get_frame_id (get_next_frame (frame));
	  frame_obj->frame_id_is_next = 1;
	}
      else
	{
	  frame_obj->frame_id = get_frame_id (frame);
	  frame_obj->frame_id_is_next = 0;
	}

I will first note that the frame id for frame has not been computed yet.  (This
was verified by placing a breakpoint on compute_frame_id().)

The call to get_prev_frame() causes the the frame id to (eventually) be
computed for the previous frame.  Here's a backtrace showing how we
get there:

#0  compute_frame_id (fi=0x10e2810)
    at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/frame.c:496
#1  0x0000000000724a67 in get_prev_frame_if_no_cycle (this_frame=0xe5a760)
    at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/frame.c:1871
#2  0x0000000000725136 in get_prev_frame_always_1 (this_frame=0xe5a760)
    at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/frame.c:2045
#3  0x000000000072516b in get_prev_frame_always (this_frame=0xe5a760)
    at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/frame.c:2061
#4  0x000000000072570f in get_prev_frame (this_frame=0xe5a760)
    at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/frame.c:2303
#5  0x00000000004eb471 in frame_info_to_frame_object (frame=0xe5a760)
    at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/python/py-frame.c:381

For this particular case, we end up in the else clause of the code above
which calls get_frame_id (frame).  It's at this point that the frame id
for frame is computed.  Again, here's a backtrace:

#0  compute_frame_id (fi=0xe5a760)
    at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/frame.c:496
#1  0x000000000072203d in get_frame_id (fi=0xe5a760)
    at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/frame.c:539
#2  0x00000000004eb50d in frame_info_to_frame_object (frame=0xe5a760)
    at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/python/py-frame.c:390

The test in question, dw2-dup-frame.exp, deliberately creates a broken
(cyclic) stack.  So, in this instance, the frame id for the prev
`frame' will be the same as that for `frame'.  But that particular
frame id ended up in the stash during the previous frame operation. 
When, just a few lines later, we compute the frame id for `frame', the
id in question is already in the stash, thus triggering the assertion
failure.

An alternate "fix" (or perhaps band-aid) for this problem is as
follows:

diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c
index 6bdac08..a1d305e 100644
--- a/gdb/python/py-frame.c
+++ b/gdb/python/py-frame.c
@@ -375,6 +375,10 @@ frame_info_to_frame_object (struct frame_info *frame)
   TRY
     {
 
+      /* Avoid case where the frame id for previous frame is computed
+        before that for the frame under consideration.  */
+      (void) get_frame_id (frame);
+
       /* Try to get the previous frame, to determine if this is the last frame
 	 in a corrupt stack.  If so, we need to store the frame_id of the next
 	 frame and not of this one (which is possibly invalid).  */

I'm not especially fond of this solution though.  If it's necessary to
create frame ids in a certain sequence, this ordering should be
imposed elsewhere.  Also, I've caught just one case here.  There may
also be similar problems lurking which we haven't caught yet.

I hope you that you (or someone else) can suggest a better solution...

Kevin