Stash frame id of current frame before stashing frame id for previous frame

This patch ensures that the frame id for the current frame is stashed
before that of the previous frame (to the current frame).

First, it should be noted that the frame id for the current frame is
not stashed by get_current_frame().  The current frame's frame id is
lazily computed and stashed via calls to get_frame_id().  However,
it's possible for get_prev_frame() to be called without first stashing
the current frame.

The frame stash is used not only to speed up frame lookups, but
also to detect cycles.  When attempting to compute the frame id
for a "previous" frame (in get_prev_frame_if_no_cycle), a cycle
is detected if the computed frame id is already in the stash.

If it should happen that a previous frame id is stashed which should
represent a cycle for the current frame, then an assertion failure
will trigger should get_frame_id() be later called to determine
the frame id for the current frame.

As of late 2016, with the "Tweak meaning of VALUE_FRAME_ID" patch in
place, this actually occurs when running the
gdb.dwarf2/dw2-dup-frame.exp test.  While attempting to generate a
backtrace, the python frame filter code is invoked, leading to
frame_info_to_frame_object() (in python/py-frame.c) being called.
That function will potentially call get_prev_frame() before
get_frame_id() is called.  The call to get_prev_frame() can eventually
end up in get_prev_frame_if_no_cycle() which, in turn, calls
compute_frame_id(), after which the frame id is stashed for the
previous frame.

If the frame id for the current frame is stashed, the cycle detection
code (which relies on the frame stash) in get_prev_frame_if_no_cycle()
will be triggered for a cycle starting with the current frame.  If the
current frame's id is not stashed, the cycle detecting code can't
operate as designed.  Instead, when get_frame_id() is called on the
current frame at some later point, the current frame's id will found
to be already in the stash, triggering an assertion failure.

Below is an in depth examination of the failure which lead to this change.
I've shortened pathnames for brevity and readability.

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: testsuite/outputs/gdb.dwarf2/dw2-dup-frame/dw2-dup-frame

Breakpoint 1, stop_frame () at dw2-dup-frame.c:22
22	}
(gdb) bt
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 "gdb/frame.c", line=544,
	fmt=0x932b20 "%s: Assertion `%s' failed.")
	at gdb/common/errors.c:54
    #1  0x000000000072207e in get_frame_id (fi=0xe5a760)
	at gdb/frame.c:544
    #2  0x00000000004eb50d in frame_info_to_frame_object (frame=0xe5a760)
	at gdb/python/py-frame.c:390
    #3  0x00000000004ef5be in bootstrap_python_frame_filters (frame=0xe5a760,
	frame_low=0, frame_high=-1)
	at 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 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 gdb/extension.c:572
    #6  0x00000000005ea896 in backtrace_command_1 (count_exp=0x0, show_locals=0,
	no_filters=0, from_tty=1)
	at 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 gdb/frame.c:496
    #1  0x0000000000724a67 in get_prev_frame_if_no_cycle (this_frame=0xe5a760)
	at gdb/frame.c:1871
    #2  0x0000000000725136 in get_prev_frame_always_1 (this_frame=0xe5a760)
	at gdb/frame.c:2045
    #3  0x000000000072516b in get_prev_frame_always (this_frame=0xe5a760)
	at gdb/frame.c:2061
    #4  0x000000000072570f in get_prev_frame (this_frame=0xe5a760)
	at gdb/frame.c:2303
    #5  0x00000000004eb471 in frame_info_to_frame_object (frame=0xe5a760)
	at 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 gdb/frame.c:496
    #1  0x000000000072203d in get_frame_id (fi=0xe5a760)
	at gdb/frame.c:539
    #2  0x00000000004eb50d in frame_info_to_frame_object (frame=0xe5a760)
	at 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.

I considered two other solutions to solving this problem:

We could prevent get_prev_frame() from being called before
get_frame_id() in frame_info_to_frame_object().  (See above for the
snippet of code where this happens.) A call to get_frame_id (frame)
could be placed ahead of that code snippet above.  I have tested this
approach and, while it does work, I can't be certain that
get_prev_frame() isn't called ahead of stashing the current frame
somewhere else in GDB, but in a less obvious way.

Another approach is to stash the current frame's id by calling
get_frame_id() in get_current_frame().  This approach is conceptually
simpler, but when importing a python unwinder, has the unwelcome side
effect of causing the unwinder to be called during import.

A cleaner looking fix would be to place this code after code
corresponding to the "Don't compute the frame id of the current frame
yet..." comment in get_prev_frame_if_no_cycle().  Sadly, this does not
work though; by the time we get to this point, the frame state for the
prev frame has been modified just enough to cause an internal error to
occur when attempting to compute the (current) frame id for inline
frames.  (The unexpected failure count increases by roughly 130
failures.)  Therefore, I decided to place it as early as possible
in get_prev_frame().

gdb/ChangeLog:

	* frame.c (get_prev_frame): Stash frame id for current frame
	prior to computing frame id for previous frame.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 5b53dad..77180ef 100644 (file)
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2016-11-16  Kevin Buettner  <kevinb@redhat.com>
 
+       * frame.c (get_prev_frame): Stash frame id for current frame
+       prior to computing frame id for previous frame.
+
+2016-11-16  Kevin Buettner  <kevinb@redhat.com>
+
        * python/py-unwind.c (pending_framepy_read_register): Use
        value_of_register() instead of get_frame_register_value().
 

diff --git a/gdb/frame.c b/gdb/frame.c
index 02635e6..5414cb3 100644 (file)
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -2220,6 +2220,17 @@ get_prev_frame (struct frame_info *this_frame)
      something should be calling get_selected_frame() or
      get_current_frame().  */
   gdb_assert (this_frame != NULL);
+  
+  /* If this_frame is the current frame, then compute and stash
+     its frame id prior to fetching and computing the frame id of the
+     previous frame.  Otherwise, the cycle detection code in
+     get_prev_frame_if_no_cycle() will not work correctly.  When
+     get_frame_id() is called later on, an assertion error will
+     be triggered in the event of a cycle between the current
+     frame and its previous frame.  */
+  if (this_frame->level == 0)
+    get_frame_id (this_frame);
+
   frame_pc_p = get_frame_pc_if_available (this_frame, &frame_pc);
 
   /* tausq/2004-12-07: Dummy frames are skipped because it doesn't make much