Re: [PATCH 1/3] Add a select_frame_reason enum parameter to select_frame

Pedro Alves
2013-05-06T16:54:07Z

On 04/29/2013 09:27 PM, Tom Tromey wrote:
>>>>>> "Maxime" == Maxime Coste <frrrwww@gmail.com> writes:
>
> Maxime> select_frame calls specify if the frame is selected due to
> Maxime> a breakpoint/signal (REASON_STOP), a user command (REASON_USER)
> Maxime> or just as an implementation detail (REASON_IMPL_DETAIL) which
> Maxime> should restore the previous frame once finished.
>
> I don't mind this approach but I would like to hear from other
> maintainers.

I'm not sure I like the approach.

I couldn't find any use for the REASON_STOP/REASON_USER
distinction in the series. If there's a rationale for it,
it'd be good to hear it, otherwise it just looks like over engineering.

The (fi != selected_frame) check in the REASON_STOP
looks suspicious. Was the intent to prevent a notification
if the program stopped in the same function it was stopped
before it was resumed?

IMO, it'd be better to leave select_frame unaware of why it's being
called, and just add a wrapper for user visible frame selection.
It feels like different conceptual levels to me. IOW, something like:

void
select_user_frame (struct frame_info *fi)
{
int changed = (fi != selected_frame);

select_frame (fi);

if (changed)
observer_notify_frame_changed (fi);
}

But,
note that in non-stop mode, e.g., thread #2 may be running while
you inspect thread #1. If thread #2 stops for any reason, say, a breakpoint,
GDB switches temporarily to thread #2 to handle the event, and then that ends
up calling normal_stop and select_frame. The patch makes that a
user visible frame change. But, what happens is that GDB immediately
afterwards changes back to thread #1 (and its frame), using a cleanup, giving
the illusion that thread #1 was selected the whole time. This frame restore
is an IMPL_DETAIL select_frame in this approach, so I think the frame
change observer (python, in this case) ends up confused.

Another problematic case I can think of off hand, is around
errors. Say, the target is running, and while handling an
event, we error out, and normal_stop isn't reached.

I'm wondering whether we really need to call the frame change
observers for stops. We have stop notifications/observers in
both Python and MI, so it seems to be a script/frontend can
already get the info it needs from those.

If we limit frame change notifications to user frame changes
(CLI up/down/frame and MI equivalents), then these issues
with stops would be non-issues by design...

--
Pedro Alves