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

[Pedro Alves <palves@redhat.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