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

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

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?

[0] https://sourceware.org/ml/gdb-patches/2016-05/msg00098.html
[1] https://www.sourceware.org/ml/gdb/2015-10/msg00073.html

gdb/ChangeLog:

	* mi/mi-main.c (mi_execute_command): Simplify computation of
	report_change.
	(mi_cmd_execute): Create restore current thread cleanup if
	one of --thread or --thread-group is used.
---
 gdb/mi/mi-main.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)
  

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
  

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