Re: [PATCH 1/2] mi: Restore original thread/frame when specifying --thread or --thread-group

[Simon Marchi <simon dot marchi at ericsson dot com>]

On 16-08-02 10:49 AM, Pedro Alves wrote:
> On 08/01/2016 10:14 PM, Simon Marchi wrote:
>> I can get around #1 by having an ugly global variable
>> "meant_to_change_current_thread" that can be set to mean that the thread
>> should not be restored to its original value, because the command
>> actually wants to change the user selected thread.  gdb_thread_select,
>> for example, would do that.  It really feels hackish though.
> 
> Yeah.  Ugly, though might be the simplest.
> 
>>
>> Perhaps #2 could be solved the same way, but I don't really know where
>> we would set meant_to_change_current_thread.  IOW, where does GDB take
>> the conscious decision to change/leave the user selected thread to the
>> thread that hit the breakpoint?
> 
> It's done in normal_stop, here:
> 
> ...
>      Also skip saying anything in non-stop mode.  In that mode, as we
>      don't want GDB to switch threads behind the user's back, to avoid
>      races where the user is typing a command to apply to thread x,
>      but GDB switches to thread y before the user finishes entering
>      the command, fetch_inferior_event installs a cleanup to restore
>      the current thread back to the thread the user had selected right
>      after this event is handled, so we're not really switching, only
>      informing of a stop.  */
>   if (!non_stop
>       && !ptid_equal (previous_inferior_ptid, inferior_ptid)
>       && target_has_execution
>       && last.kind != TARGET_WAITKIND_SIGNALLED
>       && last.kind != TARGET_WAITKIND_EXITED
>       && last.kind != TARGET_WAITKIND_NO_RESUMED)
>     {
>       SWITCH_THRU_ALL_UIS (state)
> 	{
> 	  target_terminal_ours_for_output ();
> 	  printf_filtered (_("[Switching to %s]\n"),
> 			   target_pid_to_str (inferior_ptid));
> 	  annotate_thread_changed ();
> 	}
>       previous_inferior_ptid = inferior_ptid;
>     }
> 
> 
> An alternative may be to decouple the "user-selected thread" from
> "inferior_ptid" further.  previous_inferior_ptid is already
> something like that, but not explicitly.
> 
> So we'd make previous_inferior_ptid be the "user-selected thread", and make
> the places that want to explicitly change the user-selected thread change
> that global.  That'd be gdb_thread_select, etc. and likely "kill", "detach",
> "attach", "run" and "target remote" too.
> 
> The input/command entry points would then be responsible for making
> inferior_ptid (the internal selected thread) point to the user-selected
> thread.  We'd no longer need to use make_cleanup_restore_current_thread
> to restore back the internal gdb selected thread, as it's simply
> no longer necessary.  Reducing all the temporary thread switching
> may be a good thing.  E.g., it likely reduces register cache refetching.
> 
> This would be similar to how remote.c uses a lazy scheme that reduces
> Hg traffic, where gdb keeps a local cache of the remote's selected thread,
> and delays updating it on the remote target end until memory, registers
> etc. are accessed.  That is, even if core gdb switches inferior_ptid around,
> that doesn't trigger immediate Hg packets.  If the user has thread 3
> selected, and gdb happens to need to read memory/registers off of
> thread 1, spread over several packets, then we only switch the remote
> side to thread 1 once, and don't switch it back to thread 3, even if
> inferior_ptid is switched back.
> 
> So, assuming a simply implementation for clarity, here's what
> would happen inside gdb's little brain.
> 
> Assume the user-selected thread starts out as thread 1:
> 
>>    -thread-select --thread 3 4
> 
> . MI starts processing input.  The user-selected thread is thread 1,
>   so MI switches inferior_ptid to match.  Whatever was inferior_ptid
>   before is irrelevant.
> 
> . MI processes the "--thread 3" switch, which is handled by MI common
>   code, and this causes inferior_ptid to be set to thread 3
>   (but not the user-selected thread global).
> 
> . The "-thread-select" implementation switches both
>   inferior_ptid and the current user-selected thread to
>   thread 4.
> 
>>    -interpreter-exec --thread 3 console "thread 5"
> 
>   Similar.  The last point is replaced by:
> 
> . The "thread 5" implementation switches the user-visible
>   thread to thread 5.
> 
>>    -interpreter-exec --thread 1 console "c"
> 
>   and then thread 3 hits a breakpoint.
> 
>   This is similar too.  The last point is replaced by:
> 
> . normal_stop switches the user-visible thread to thread 3.

Ok, I kinda had the same design idea, but it was blurry.  Now that you explain it
(I didn't know what previous_inferior_ptid was), it's clear.  I'll try to prototype it
quickly to see if it's viable.

> I think this might also pave the way to optionally make each UI have
> its own independently selected thread.  (That would also solve these
> problems, I guess, though it bring in a set of its own problems.
> I think pulling that off would be a too large a change to make at
> this point.  A frontend that would want to keep CLI and MI in sync
> would do that explicitly, by reacting to  =thread-selected etc. events, which
> would be augmented to indicate the originating UI, and switching MI's
> selected thread accordingly.  But certainly we'd need to make selected
> frame at least be per-UI as well, and who knows what else.)

We thought about that too for the future.  Not only =thread-selected events need
to include the originating UI, but the -thread-select command needs to be able
to change the thread for another UI than the current one, so that when you click
on a thread in the UI, the front-end (MI) can change the thread for the CLI.

Thanks!

Simon