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

[Pedro Alves <palves at redhat dot com>]

On 08/01/2016 10:14 PM, Simon Marchi wrote:
> I am sending a first version of this patch, even though I am not
> completely satisfied with it.  Hopefully I can get some input from the
> community.
> 
> We are in the process of integrating Pedro's great separate ui work [0]
> in Eclipse CDT.  With this, the hope is that GDB users will feel right
> at home with a console that behaves just like plain GDB in a terminal,
> and appreciate the graphical support of CDT.
> 
> Earlier [1], we found that if the current thread is X, passing
> "--thread Y" to an MI command would leave Y as the current thread.
> This is not a problem for CDT itself, since it always uses --thread for
> MI commands.  However, it also affects the user selected thread.  So the
> internal front-end logic can unexpectedly change the currently selected
> thread from the point of view of the user using the console.
> 
> So, to avoid changing the current selection under the user's feet while
> they are typing a command,  --thread and --thread-group should leave the
> inferior/thread/frame selection in their original state.
> 
> This naive patch simply adds a restore_current_thread cleanup whenever
> --thread or --thread-group is used (using --frame implies using
> --thread).  As a side effect, the code that checks whether a
> =thread-selected event should be emitted was simplified.
> 
> It works okay for most commands, but I am worried about these corner
> cases:
> 
> 1. If the command is actually meant to change the current thread and
>    --thread/--thread-group is specified, the command will have no
>    effect.  Some front-ends might add --thread X for everything, so I
>    think this is a plausible scenario:
> 
>    -thread-select --thread 3 4
>    -interpreter-exec --thread 3 console "thread 4"
> 
>    Eclipse CDT is not affected by this, but I am mentionning it anyway.
> 
> 2. When a breakpoint is hit in _all-stop_, GDB's behavior is to switch to
>    the thread that hit the breakpoint (which makes sense).  With a
>    _synchronous_ target, I have the feeling that it could inhibit this
>    behavior, even though I couldn't reproduce it with "maint set
>    target-async off".  My idea is that the events could go like this:
> 
>     1. The front-end issues "-exec-continue --thread-group i1".  Or with
>        --thread 1, it doesn't matter because it's all-stop.
>     2. We create a restore_current_thread cleanup with the current thread
>        (let's say #1)
>     3. We enter the implementation of -exec-continue and we block somewhere
>        deep in there because the target is sync

This doesn't happen, because even sync targets go through the
event loop -> fetch_inferior_event nowadays.  You may be able to
trigger something with infcalls, as those are always blocking and
go through different paths.  Or maybe with execution commands
inside a canned sequence of commands, and/or playing
with "interpreter-exec mi" in the CLI.

>     4. A breakpoint is hit by thread #2, so the implementation of
>        -exec-continue eventually returns.  It leaves thread #2 as the
>        current thread, because it's the one that hit the breakpoint.
>     5. The cleanup (wrongfully) restores thread #1 as being the current thread.
> 
> 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.


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.)

Thanks,
Pedro Alves