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

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

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

diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index b1cbd8b..3966b91 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -2163,18 +2163,9 @@ mi_execute_command (const char *cmd, int from_tty)
 	    = (struct mi_interp *) top_level_interpreter_data ();
 	  int report_change = 0;
 
-	  if (command->thread == -1)
-	    {
-	      report_change = (!ptid_equal (previous_ptid, null_ptid)
-			       && !ptid_equal (inferior_ptid, previous_ptid)
-			       && !ptid_equal (inferior_ptid, null_ptid));
-	    }
-	  else if (!ptid_equal (inferior_ptid, null_ptid))
-	    {
-	      struct thread_info *ti = inferior_thread ();
-
-	      report_change = (ti->global_num != command->thread);
-	    }
+	  report_change = (!ptid_equal (previous_ptid, null_ptid)
+			   && !ptid_equal (inferior_ptid, previous_ptid)
+			   && !ptid_equal (inferior_ptid, null_ptid));
 
 	  if (report_change)
 	    {
@@ -2224,6 +2215,8 @@ mi_cmd_execute (struct mi_parse *parse)
       if (!inf)
 	error (_("Invalid thread group for the --thread-group option"));
 
+      make_cleanup_restore_current_thread ();
+
       set_current_inferior (inf);
       /* This behaviour means that if --thread-group option identifies
 	 an inferior with multiple threads, then a random one will be
@@ -2246,6 +2239,8 @@ mi_cmd_execute (struct mi_parse *parse)
       if (is_exited (tp->ptid))
 	error (_("Thread id: %d has terminated"), parse->thread);
 
+      make_cleanup_restore_current_thread ();
+
       switch_to_thread (tp->ptid);
     }
 
@@ -2302,6 +2297,7 @@ mi_cmd_execute (struct mi_parse *parse)
       make_cleanup_ui_file_delete (stb);
       error_stream (stb);
     }
+
   do_cleanups (cleanup);
 }
 
-- 
2.9.2