Re: [PATCH 1/9] gdb: Check the selected-frame in frame_find_by_id.

[Pedro Alves <palves at redhat dot com>]

Hi Andrew,

On 09/11/2015 07:49 PM, Andrew Burgess wrote:
> In gdb we track two different things, the current-frame, and the
> selected-frame.  The current-frame represents the frame in which we are
> currently stopped.  If gdb stops at a breakpoint and the user asks for a
> backtrace then all the frames in the backtrace will be the ancestors of
> the the current-frame.
> 
> Also the current-frame does not change until the inferior is set running
> again (though it is possible for the user to alter the contents of the
> current-frame this is not the same as changing the current-frame).
> 
> There is also the selected-frame.  This is the frame that the user has
> "selected" using the frame selection commands 'frame', 'up', or 'down'
> being the most common.
> 
> Usually the selected-frame is always an ancestor of the current-frame,
> however, there is a feature of the 'frame' command that allows arbitrary
> frames to be created, in this case the selected-frame might not be an
> ancestor of the current-frame.
> 
> The problem this causes is that lazy register values hold the frame-id
> for the frame in which the real register value can be obtained.  When
> gdb needs to fetch the actual register value we try to find the frame
> using frame_find_by_id, which currently looks at the current-frame, and
> all the ancestors of the current-frame.
> 
> When the selected-frame is not an ancestor of the current-frame then
> frame_find_by_id will fail to find the required frame, and gdb will fail
> with an assertion while trying to fetch the lazy value (the assertion is
> that the frame will always be found).
> 
> This patch extends frame_find_by_id to also check the selected-frame as
> a separate comparison as well as the current-frame and its ancestors.
> This resolves the issue.
> 
> There are also two new tests added that show this issue.  The first is
> an mi test using var objects, which is where I first saw this issue.
> The second is a slightly simpler test, just creating a new frame then
> calling 'info frame' is enough to trigger the assertion.

Seems to me that this issue runs deeper.

Two points:

#1 - Consider what happens if something flushes the frame cache while
the user is looking at a manually-created frame.  Anything that
calls registers_changed() and relies on the old selected frame _id_
to re-unwind, re-populate the frame cache, and re-find the old selected
frame object (e.g., see make_cleanup_restore_current_thread).
As is, the selected frame's frame_info object is gone along with the frame cache
flush, so gdb will just unexpectedly lose the selected frame when it tries
to re-find it.  E.g., something around:

thread2 ()
{
  while (1)
    sleep (1);
}

main ()
{
  create thread (thread2);
  join thread2;
}


(gdb) set non-stop on
(gdb) break sleep if 0
(gdb) c -a&
(gdb) thread 1
(gdb) interrupt
(gdb) frame create 0xaddr
// now the breakpoint at sleep triggers
// while we're looking at the created frame.
// gdb switches to thread 2 internally to handle
// the event, re-resumes the thread (bp condition evals false),
// and then tries to restore the previously selected frame, which
// is gone.  so gdb falls back to re-selecting the current frame...


Or simpler, from reading the code, it seems like just "info threads"
should trigger that too.

#2 - As mentioned in my comment here:

 https://sourceware.org/bugzilla/show_bug.cgi?id=18074#c1

I think the fix should consider what should happen to
"backtrace" after "frame create".  Currently, we always start
the backtrace at the current frame, so the created frame does
not appear in the backtrace.  But if the user does "frame create",
and can follow that with "up", shouldn't "bt" after "frame ADDR" to
attempt to backtrace starting at the created frame too?

That would suggest that the fix for this instead would be for
"frame create" to override the source of the current frame.
That then raises the question of how to get out of that
frame.

I'm not saying that that's the best model, but I do think this
should be considered, and the way forward depends on
how frame-related / backtrace commands _should_ work around
"frame create".

Thanks,
Pedro Alves