This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/3] Add a select_frame_reason enum parameter to select_frame
- From: Pedro Alves <palves at redhat dot com>
- To: Tom Tromey <tromey at redhat dot com>
- Cc: Maxime Coste <frrrwww at gmail dot com>, gdb-patches at sourceware dot org
- Date: Mon, 06 May 2013 17:54:01 +0100
- Subject: Re: [PATCH 1/3] Add a select_frame_reason enum parameter to select_frame
- References: <1367002961-12311-1-git-send-email-frrrwww at gmail dot com> <1367002961-12311-2-git-send-email-frrrwww at gmail dot com> <87r4htxeth dot fsf at fleche dot redhat dot com>
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