[PATCH 0/2] Two MI changes related to separate UIs

[PATCH 0/2] Two MI changes related to separate UIs

[Simon Marchi]

Hi all,

We are pretty much done integrating a native GDB console in Eclipse CDT, using
the new separate UI feature.  Doing so, we found a few issues that would need
to be fixed in order to provide a good enough user experience.  The two most
important ones, addressed by these patches, are:

  1. When it receives a =thread-group-started, the front-end doesn't know whether
     the process was started or attached.  This makes it difficult to correctly
     handle a user typing run/start/attach from the console.
  2. When the front-end issues some MI commands with --thread or --thread-group,
     the GDB selected thread is changed but is not restored.  The selected thread
     in the console can then appear to change unexpectedly from the point of
     view of the user.

We were hoping to get those two changes in GDB 7.12.  The idea is that the
separate UI feature was merged in GDB during this development cycle, and these
issues (especially the second one) would severely impact its usefulness.

There is another patch in the pipeline, although not as critical.  We would
like to implement two-way synchronisation of the GUI threads view and the
command line, which means that clicking on a thread in the GUI will select that
thread in the GDB console, and switching thread in the GDB console will select
that thread in the GUI.  The =thread-selected event has some issues which
prevent us from doing it right now.  For example, the event is not sent on all
UIs and it's not sent at all when the thread is changed from CLI.  It would be
nice if we could fix this in 7.12, but if that is too much of a stretch, we
could integrate the console without synchronisation first, and add
synchronisation later when those issues are fixed in GDB.

Simon

Simon Marchi (2):
  mi: Restore original thread/frame when specifying --thread or
    --thread-group
  mi: Add launch-type={run,attach} in =thread-group-started

 gdb/NEWS                           |  3 +++
 gdb/darwin-nat.c                   |  2 +-
 gdb/doc/gdb.texinfo                |  6 +++--
 gdb/gnu-nat.c                      |  2 +-
 gdb/inf-ptrace.c                   |  2 +-
 gdb/mi/mi-interp.c                 |  6 +++--
 gdb/mi/mi-main.c                   | 20 +++++++----------
 gdb/nto-procfs.c                   |  4 ++--
 gdb/procfs.c                       |  2 +-
 gdb/remote.c                       |  3 ++-
 gdb/testsuite/gdb.mi/mi-attach.c   | 31 +++++++++++++++++++++++++
 gdb/testsuite/gdb.mi/mi-attach.exp | 46 ++++++++++++++++++++++++++++++++++++++
 gdb/windows-nat.c                  |  2 +-
 13 files changed, 105 insertions(+), 24 deletions(-)
 create mode 100644 gdb/testsuite/gdb.mi/mi-attach.c
 create mode 100644 gdb/testsuite/gdb.mi/mi-attach.exp

-- 
2.9.2

[PATCH 2/2] mi: Add launch-type={run,attach} in =thread-group-started

[Simon Marchi]

For Eclipse CDT to properly handle processes attached or started from
the console, it would be useful to have a hint of whether the
thread-group was started by gdb or attached.  In particular, it helps
for inferior pty handling, since a process that is "ran" requires some
special handling for its output to appear in a console in CDT, whereas
an "attached" process doesn't require any intervention.

This patch adds launch-type="run" or launch-type="attach" to the
=thread-group-started event.  "launch" comes from the Eclipse
terminology, so it's perhaps not the best term.  It would be nice to
have something that fits better in the gdb terminology.  Suggestions are
welcome.  I also thought of using attached="{1,0}", but I think the
other approach is more easily extensible if we ever need to.

Note that launch-type="run" will be present even "starting" a core file.
That's because we display "run" if inferior->attach_flag is 0 and
"attach" otherwise.  If that's a problem, we could find a way to omit
the field completely when it's irrelevant.  For example, when none of
the target present on the target stack know how to run or attach (which
is the case when inspecting a core file I believe), then it's probably
irrelevant.

I needed to do some little changes in various targets, so that struct
inferior's attach_flag field is set before inferior_appeared is called.
I can't test all of them, but the changes seem very low risk to me.

I have added a test for an attach in MI (it seems like there was none),
in which I test =thread-group-started with launch-type="attach".
Testing launch-type="run" could probably be done as well, but would be a
bit more involved (we could perhaps test it as part of
mi_run_cmd_full...).

Regtested on x86, unix/native-gdbserver/native-extended-gdbserver.

gdb/ChangeLog:

	* NEWS: Announce new "launch-type" field in =thread-group-started.
	* mi/mi-interp.c (mi_inferior_appeared): Output "launch-type"
	attribute in =thread-group-started event.
	* darwin-nat.c (darwin_attach): Move setting of attach_flag
	before call to inferior_appeared.
	* gnu-nat.c (gnu_attach): Likewise.
	* inf-ptrace.c (inf_ptrace_attach): Likewise.
	* nto-procfs.c (procfs_attach): Likewise.
	* procfs.c (do_attach): Likewise.
	* remote.c (remote_add_inferior): Likewise.
	* windows-nat.c (do_initial_windows_stuff): Likewise.

gdb/doc/ChangeLog:

	* gdb.texinfo (GDB/MI Async Records): Document new "launch-type" field
	in =thread-group-started.

gdb/testsuite/ChangeLog:

	* gdb.mi/mi-attach.exp: New file.
	* gdb.mi/mi-attach.c: New file.
---
 gdb/NEWS                           |  3 +++
 gdb/darwin-nat.c                   |  2 +-
 gdb/doc/gdb.texinfo                |  6 +++--
 gdb/gnu-nat.c                      |  2 +-
 gdb/inf-ptrace.c                   |  2 +-
 gdb/mi/mi-interp.c                 |  6 +++--
 gdb/nto-procfs.c                   |  4 ++--
 gdb/procfs.c                       |  2 +-
 gdb/remote.c                       |  3 ++-
 gdb/testsuite/gdb.mi/mi-attach.c   | 31 +++++++++++++++++++++++++
 gdb/testsuite/gdb.mi/mi-attach.exp | 46 ++++++++++++++++++++++++++++++++++++++
 gdb/windows-nat.c                  |  2 +-
 12 files changed, 97 insertions(+), 12 deletions(-)
 create mode 100644 gdb/testsuite/gdb.mi/mi-attach.c
 create mode 100644 gdb/testsuite/gdb.mi/mi-attach.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index d2186fd..d77c8e4 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,9 @@
 
 *** Changes since GDB 7.12
 
+* MI async record =thread-group-started now includes a
+  launch-type={run,attach} attribute.
+
 *** Changes in GDB 7.12
 
 * GDBserver now supports recording btrace without maintaining an active
diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
index 590c2ad..f2a6ddd 100644
--- a/gdb/darwin-nat.c
+++ b/gdb/darwin-nat.c
@@ -1704,8 +1704,8 @@ darwin_attach (struct target_ops *ops, const char *args, int from_tty)
 
   inferior_ptid = pid_to_ptid (pid);
   inf = current_inferior ();
-  inferior_appeared (inf, pid);
   inf->attach_flag = 1;
+  inferior_appeared (inf, pid);
 
   /* Always add a main thread.  */
   add_thread_silent (inferior_ptid);
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index f5dde61..2d4c154 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -26437,12 +26437,14 @@ group is added, it generally might not be associated with a running
 process.  When a thread group is removed, its id becomes invalid and
 cannot be used in any way.
 
-@item =thread-group-started,id="@var{id}",pid="@var{pid}"
+@item =thread-group-started,id="@var{id}",pid="@var{pid}",launch-type="@var{launch-type}"
 A thread group became associated with a running program,
 either because the program was just started or the thread group
 was attached to a program.  The @var{id} field contains the
 @value{GDBN} identifier of the thread group.  The @var{pid} field
-contains process identifier, specific to the operating system.
+contains process identifier, specific to the operating system.  The
+@var{launch-type} field has the value @code{"run"} if the program was started
+by gdb and @code{"attach"} if it was attached.
 
 @item =thread-group-exited,id="@var{id}"[,exit-code="@var{code}"]
 A thread group is no longer associated with a running program,
diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
index c268732..2e58545 100644
--- a/gdb/gnu-nat.c
+++ b/gdb/gnu-nat.c
@@ -2200,8 +2200,8 @@ gnu_attach (struct target_ops *ops, const char *args, int from_tty)
   push_target (ops);
 
   inferior = current_inferior ();
-  inferior_appeared (inferior, pid);
   inferior->attach_flag = 1;
+  inferior_appeared (inferior, pid);
 
   inf_update_procs (inf);
 
diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index 0896cff..652d62f 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -203,8 +203,8 @@ inf_ptrace_attach (struct target_ops *ops, const char *args, int from_tty)
 #endif
 
   inf = current_inferior ();
-  inferior_appeared (inf, pid);
   inf->attach_flag = 1;
+  inferior_appeared (inf, pid);
   inferior_ptid = pid_to_ptid (pid);
 
   /* Always add a main thread.  If some target extends the ptrace
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index e3c7dbd..1154c3b 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -481,6 +481,7 @@ static void
 mi_inferior_appeared (struct inferior *inf)
 {
   struct switch_thru_all_uis state;
+  const char *launch_type = inf->attach_flag ? "attach" : "run";
 
   SWITCH_THRU_ALL_UIS (state)
     {
@@ -494,8 +495,9 @@ mi_inferior_appeared (struct inferior *inf)
       target_terminal_ours_for_output ();
 
       fprintf_unfiltered (mi->event_channel,
-			  "thread-group-started,id=\"i%d\",pid=\"%d\"",
-			  inf->num, inf->pid);
+			  "thread-group-started,id=\"i%d\",pid=\"%d\","
+			  "launch-type=\"%s\"",
+			  inf->num, inf->pid, launch_type);
       gdb_flush (mi->event_channel);
       do_cleanups (old_chain);
     }
diff --git a/gdb/nto-procfs.c b/gdb/nto-procfs.c
index f49453d..e5f90ab 100644
--- a/gdb/nto-procfs.c
+++ b/gdb/nto-procfs.c
@@ -672,8 +672,8 @@ procfs_attach (struct target_ops *ops, const char *args, int from_tty)
     }
   inferior_ptid = do_attach (pid_to_ptid (pid));
   inf = current_inferior ();
-  inferior_appeared (inf, pid);
   inf->attach_flag = 1;
+  inferior_appeared (inf, pid);
 
   if (!target_is_pushed (ops))
     push_target (ops);
@@ -1270,8 +1270,8 @@ procfs_create_inferior (struct target_ops *ops, char *exec_file,
   procfs_update_thread_list (ops);
 
   inf = current_inferior ();
-  inferior_appeared (inf, pid);
   inf->attach_flag = 0;
+  inferior_appeared (inf, pid);
 
   flags = _DEBUG_FLAG_KLC;	/* Kill-on-Last-Close flag.  */
   errn = devctl (ctl_fd, DCMD_PROC_SET_FLAG, &flags, sizeof (flags), 0);
diff --git a/gdb/procfs.c b/gdb/procfs.c
index 67b424f..5db0711 100644
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -3135,9 +3135,9 @@ do_attach (ptid_t ptid)
     dead_procinfo (pi, "do_attach: failed in procfs_debug_inferior", NOKILL);
 
   inf = current_inferior ();
-  inferior_appeared (inf, pi->pid);
   /* Let GDB know that the inferior was attached.  */
   inf->attach_flag = 1;
+  inferior_appeared (inf, pi->pid);
 
   /* Create a procinfo for the current lwp.  */
   lwpid = proc_get_current_thread (pi);
diff --git a/gdb/remote.c b/gdb/remote.c
index 7944983..2a4ed35 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1783,6 +1783,7 @@ remote_add_inferior (int fake_pid_p, int pid, int attached,
 	 space.  */
       inf->aspace = maybe_new_address_space ();
       inf->pspace = current_program_space;
+      inf->attach_flag = attached;
     }
   else
     {
@@ -1790,10 +1791,10 @@ remote_add_inferior (int fake_pid_p, int pid, int attached,
 	 between program/address spaces.  We simply bind the inferior
 	 to the program space's address space.  */
       inf = current_inferior ();
+      inf->attach_flag = attached;
       inferior_appeared (inf, pid);
     }
 
-  inf->attach_flag = attached;
   inf->fake_pid_p = fake_pid_p;
 
   /* If no main executable is currently open then attempt to
diff --git a/gdb/testsuite/gdb.mi/mi-attach.c b/gdb/testsuite/gdb.mi/mi-attach.c
new file mode 100644
index 0000000..00ed35c
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-attach.c
@@ -0,0 +1,31 @@
+/* Copyright 2016 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <unistd.h>
+
+int
+main ()
+{
+  int i;
+
+  for (i = 0; i < 30; i++)
+    {
+      sleep (1);
+    }
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.mi/mi-attach.exp b/gdb/testsuite/gdb.mi/mi-attach.exp
new file mode 100644
index 0000000..000a91a
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-attach.exp
@@ -0,0 +1,46 @@
+# Copyright 2016 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+load_lib mi-support.exp
+
+standard_testfile
+
+if {![can_spawn_for_attach]} {
+    untested ${testfile}.exp
+    return
+}
+
+if  {[gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != ""} {
+     untested ${testfile}.exp
+     return -1
+}
+
+# Test an attach in MI.
+
+proc test_mi_attach {} {
+    global binfile
+
+    set exec_spawn_id [spawn_wait_for_attach $binfile]
+    set exec_pid [spawn_id_get_pid $exec_spawn_id]
+
+    mi_gdb_start
+
+    mi_gdb_test "123-target-attach $exec_pid" \
+	[multi_line "=thread-group-started,id=\"i1\",pid=\"$exec_pid\",launch-type=\"attach\"" \
+		    "=thread-created,id=\"1\",group-id=\"i1\".*" ] \
+	"attach to process"
+}
+
+test_mi_attach
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 3f67486..19f13e4 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -1697,8 +1697,8 @@ do_initial_windows_stuff (struct target_ops *ops, DWORD pid, int attaching)
   init_wait_for_inferior ();
 
   inf = current_inferior ();
-  inferior_appeared (inf, pid);
   inf->attach_flag = attaching;
+  inferior_appeared (inf, pid);
 
   /* Make the new process the current inferior, so terminal handling
      can rely on it.  When attaching, we don't know about any thread
-- 
2.9.2

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

[Simon Marchi]

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

Re: [PATCH 2/2] mi: Add launch-type={run,attach} in =thread-group-started

[Eli Zaretskii]

> From: Simon Marchi <simon.marchi@ericsson.com>
> CC: Simon Marchi <simon.marchi@ericsson.com>
> Date: Mon, 1 Aug 2016 17:14:01 -0400
> 
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -26437,12 +26437,14 @@ group is added, it generally might not be associated with a running
>  process.  When a thread group is removed, its id becomes invalid and
>  cannot be used in any way.
>  
> -@item =thread-group-started,id="@var{id}",pid="@var{pid}"
> +@item =thread-group-started,id="@var{id}",pid="@var{pid}",launch-type="@var{launch-type}"
>  A thread group became associated with a running program,
>  either because the program was just started or the thread group
>  was attached to a program.  The @var{id} field contains the
>  @value{GDBN} identifier of the thread group.  The @var{pid} field
> -contains process identifier, specific to the operating system.
> +contains process identifier, specific to the operating system.  The
> +@var{launch-type} field has the value @code{"run"} if the program was started
> +by gdb and @code{"attach"} if it was attached.
      ^^^
@value{GDBN}

Also, @code{"foo"} looks bad in Info due to too many quotes.  Suggest
to use just @code{foo} instead.

The documentation parts are okay with these fixed.

Thanks.

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

[Pedro Alves]

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

Re: [PATCH 2/2] mi: Add launch-type={run,attach} in =thread-group-started

[Simon Marchi]

On 16-08-02 10:49 AM, Eli Zaretskii wrote:
>> From: Simon Marchi <simon.marchi@ericsson.com>
>> CC: Simon Marchi <simon.marchi@ericsson.com>
>> Date: Mon, 1 Aug 2016 17:14:01 -0400
>>
>> --- a/gdb/doc/gdb.texinfo
>> +++ b/gdb/doc/gdb.texinfo
>> @@ -26437,12 +26437,14 @@ group is added, it generally might not be associated with a running
>>  process.  When a thread group is removed, its id becomes invalid and
>>  cannot be used in any way.
>>  
>> -@item =thread-group-started,id="@var{id}",pid="@var{pid}"
>> +@item =thread-group-started,id="@var{id}",pid="@var{pid}",launch-type="@var{launch-type}"
>>  A thread group became associated with a running program,
>>  either because the program was just started or the thread group
>>  was attached to a program.  The @var{id} field contains the
>>  @value{GDBN} identifier of the thread group.  The @var{pid} field
>> -contains process identifier, specific to the operating system.
>> +contains process identifier, specific to the operating system.  The
>> +@var{launch-type} field has the value @code{"run"} if the program was started
>> +by gdb and @code{"attach"} if it was attached.
>       ^^^
> @value{GDBN}
> 
> Also, @code{"foo"} looks bad in Info due to too many quotes.  Suggest
> to use just @code{foo} instead.
> 
> The documentation parts are okay with these fixed.
> 
> Thanks.
> 

Thanks!  Updated locally.

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

[Simon Marchi]

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

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

[Simon Marchi]

On 16-08-02 01:38 PM, Simon Marchi wrote:
> 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 started to prototype what you described, but it's definitely not a small job.  It would
also be too big of a change to put in 7.12.  I'd be really interested to work on that for
the next development cycle though.

In the mean time, here is a new version of the patch that implements what I had described
(with the global flag), which I think is less risky.


From d2b49f3b291ce9fa603a65324b8760bab3831ad3 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Mon, 1 Aug 2016 17:12:51 -0400
Subject: [PATCH] mi: Restore original thread/frame when specifying --thread or --thread-group

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 patch adds a restore_current_thread cleanup whenever --thread or
--thread-group is used (using --frame implies using --thread).  To
support the case where the --thread flag would be used along with a
command that is meant to change the thread, it also adds a global
variable "command_changes_user_selected_ptid".  The GDB core can set
this variable to indicate that it wants to change the user-selected
thread.  When this variable is set, the cleanup is discarded so that the
newly selected thread is kept.  Because there is a single chain of
cleanups, I had to reorder things in mi_cmd_execute so that it was
possible to only discard this cleanup.

As a side effect, the code that checks whether a =thread-selected event
should be emitted was simplified.

This is intended to be a temporary solution, until we implement
something like what Pedro suggested in [2].

Regtested with --target_board=unix on Linux x86.

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

gdb/ChangeLog:

	* infcmd.c (command_changes_user_selected_ptid): New variable.
	* inferior.h (command_changes_user_selected_ptid): New
	declaration.
	* 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.  Move language and
	suppress_notification code blocks up.  Discard current thread
	cleanup if command_changes_user_selected_ptid is set.
	* thread.c (do_captured_thread_select): Set
	command_changes_user_selected_ptid.
	* top.c (prepare_execute_command): Unset
	command_changes_user_selected_ptid.
---
 gdb/infcmd.c     |  4 ++++
 gdb/inferior.h   |  5 +++++
 gdb/mi/mi-main.c | 48 ++++++++++++++++++++++++------------------------
 gdb/thread.c     |  1 +
 gdb/top.c        |  1 +
 5 files changed, 35 insertions(+), 24 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 58ba1cb..65adc63 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -124,6 +124,10 @@ static char *inferior_io_terminal_scratch;

 ptid_t inferior_ptid;

+/* See inferior.h.  */
+
+int command_changes_user_selected_ptid;
+
 /* Address at which inferior stopped.  */

 CORE_ADDR stop_pc;
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 571d26a..edf829e 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -82,6 +82,11 @@ extern const char *get_inferior_io_terminal (void);

 extern ptid_t inferior_ptid;

+/* Flag indicating that we explicitly want to change the user selected
+   thread/ptid.  */
+
+extern int command_changes_user_selected_ptid;
+
 extern void generic_mourn_inferior (void);

 extern CORE_ADDR unsigned_pointer_to_address (struct gdbarch *gdbarch,
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index b1cbd8b..0933615 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)
 	    {
@@ -2201,6 +2192,7 @@ static void
 mi_cmd_execute (struct mi_parse *parse)
 {
   struct cleanup *cleanup;
+  struct cleanup *thread_restore_cleanup = NULL;

   cleanup = prepare_execute_command ();

@@ -2216,6 +2208,18 @@ mi_cmd_execute (struct mi_parse *parse)
   if (parse->frame != -1 && parse->thread == -1)
     error (_("Cannot specify --frame without --thread"));

+  if (parse->language != language_unknown)
+    {
+      make_cleanup_restore_current_language ();
+      set_language (parse->language);
+    }
+
+  if (parse->cmd->suppress_notification != NULL)
+    {
+      make_cleanup_restore_integer (parse->cmd->suppress_notification);
+      *parse->cmd->suppress_notification = 1;
+    }
+
   if (parse->thread_group != -1)
     {
       struct inferior *inf = find_inferior_id (parse->thread_group);
@@ -2224,6 +2228,8 @@ mi_cmd_execute (struct mi_parse *parse)
       if (!inf)
 	error (_("Invalid thread group for the --thread-group option"));

+      thread_restore_cleanup = 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 +2252,8 @@ mi_cmd_execute (struct mi_parse *parse)
       if (is_exited (tp->ptid))
 	error (_("Thread id: %d has terminated"), parse->thread);

+      thread_restore_cleanup = make_cleanup_restore_current_thread ();
+
       switch_to_thread (tp->ptid);
     }

@@ -2262,20 +2270,8 @@ mi_cmd_execute (struct mi_parse *parse)
 	error (_("Invalid frame id: %d"), frame);
     }

-  if (parse->language != language_unknown)
-    {
-      make_cleanup_restore_current_language ();
-      set_language (parse->language);
-    }
-
   current_context = parse;

-  if (parse->cmd->suppress_notification != NULL)
-    {
-      make_cleanup_restore_integer (parse->cmd->suppress_notification);
-      *parse->cmd->suppress_notification = 1;
-    }
-
   if (parse->cmd->argv_func != NULL)
     {
       parse->cmd->argv_func (parse->command, parse->argv, parse->argc);
@@ -2302,6 +2298,10 @@ mi_cmd_execute (struct mi_parse *parse)
       make_cleanup_ui_file_delete (stb);
       error_stream (stb);
     }
+
+  if (command_changes_user_selected_ptid && thread_restore_cleanup != NULL)
+    discard_cleanups (thread_restore_cleanup);
+
   do_cleanups (cleanup);
 }

diff --git a/gdb/thread.c b/gdb/thread.c
index ab98777..7aa1ce7 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -2055,6 +2055,7 @@ do_captured_thread_select (struct ui_out *uiout, void *tidstr_v)
     error (_("Thread ID %s has terminated."), tidstr);

   switch_to_thread (tp->ptid);
+  command_changes_user_selected_ptid = 1;

   annotate_thread_changed ();

diff --git a/gdb/top.c b/gdb/top.c
index 36c300e..79b1ad6 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -503,6 +503,7 @@ prepare_execute_command (void)

   mark = value_mark ();
   cleanup = make_cleanup_value_free_to_mark (mark);
+  command_changes_user_selected_ptid = 0;

   /* With multiple threads running while the one we're examining is
      stopped, the dcache can get stale without us being able to detect
-- 
2.9.2

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

[Pedro Alves]

On 08/02/2016 11:14 PM, Simon Marchi wrote:
> On 16-08-02 01:38 PM, Simon Marchi wrote:

>> 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 started to prototype what you described, but it's definitely not a small job.  It would
> also be too big of a change to put in 7.12.  I'd be really interested to work on that for
> the next development cycle though.
> 
> In the mean time, here is a new version of the patch that implements what I had described
> (with the global flag), which I think is less risky.

I think you need to consider infcalls, and canned sequence of commands
Python/Scheme, etc., basically any place that sets ui->async = 0 ...

guile/guile.c:169:  current_ui->async = 0;
guile/guile.c:202:  current_ui->async = 0;
guile/guile.c:332:      current_ui->async = 0;
guile/scm-ports.c:521:  current_ui->async = 0;
top.c:700:  current_ui->async = 0;
compile/compile.c:95:  current_ui->async = 0;
compile/compile.c:137:  current_ui->async = 0;
compile/compile.c:191:  current_ui->async = 0;
infcall.c:586:  current_ui->async = 0;
python/python.c:326:  current_ui->async = 0;
python/python.c:471:  current_ui->async = 0;
python/python.c:655:      current_ui->async = 0;
cli/cli-script.c:384:  current_ui->async = 0;
cli/cli-script.c:666:  current_ui->async = 0;
cli/cli-script.c:690:  current_ui->async = 0;
cli/cli-script.c:1697:  current_ui->async = 0;

... because these will only return when execution next stops,
so the cleanup runs after, and undoes the thread change.

E.g., this infcall stops due to a breakpoint on another
thread, but MI still reverts back to the "--thread" thread:

 (gdb)
 b usleep
 [...]
 ^done
 (gdb)

 thread 1
 [...]
 ^done
 =thread-selected,id="1"
 (gdb) 

 -interpreter-exec --thread 1 console "print sleep (10)"
 [...]
 ~"[Switching to Thread 0x7ffff6ffb700 (LWP 7818)]\n"
 ~"\n"
 ~"Thread 3 \"threads\" hit Breakpoint 2, usleep (useconds=1) at ../sysdeps/posix/usleep.c:26\n"
 ~"26\t  struct timespec ts = { .tv_sec = (long int) (useconds / 1000000),\n"
 *stopped,reason="breakpoint-hit",disp="keep",bkptno="2",frame={addr="0x00007ffff78f6030",func="usleep",args=[{name="useconds",value="1"}],file="../sysdeps/posix/usleep.c",fullname="/usr/src/debug/glibc-2.22/sysdeps/posix/usleep.c",line="26"},thread-id="3",stopped-threads="all",core="2"
 &"The program stopped in another thread while making a function call from GDB.\n"
 &"Evaluation of the expression containing the function\n"
 &"(__sleep) will be abandoned.\n"
 &"When the function is done executing, GDB will silently stop.\n"
 ^error,msg="The program stopped in another thread while making a function call from GDB.\nEvaluation of the expression containing the function\n(__sleep) will be abandoned.\nWhen the function is done executing, GDB will silently stop."
 (gdb)

 thread
 [...]
 ~"[Current thread is 1 (Thread 0x7ffff7fc8700 (LWP 7813))]\n"
 ^done
 (gdb) 

Above should have been 3.

E.g., this user-defined command starts the program and stops in
thread 2.1, but MI still reverts back to the "--thread" thread:

 (gdb) define foo
 ~">" inferior 2
 ~">" start
 ~">" end
 (gdb)

 info inferiors
 ~"  Num  Description       Executable        \n"
 ~"* 1    process 8216      /home/pedro/brno/pedro/gdb/tests/threads \n"
 ~"  2    <null>            /home/pedro/brno/pedro/gdb/tests/threads \n"
 ^done
 (gdb)

 info threads
 ~"  Id   Target Id         Frame \n"
 ~"* 1.1  Thread 0x7ffff7fc8700 (LWP 8216) \"threads\" main () at threads.c:54\n"
 ^done
 (gdb)

 -interpreter-exec --thread 1 console "foo"
 ~"[Switching to inferior 2 [<null>] (/home/pedro/brno/pedro/gdb/tests/threads)]\n"
 [...]
 ~"\n"
 ~"Thread 2.1 \"threads\" hit Temporary breakpoint 3, main () at threads.c:54\n"
 ~"54\t    long i = 0;\n"
 *stopped,reason="breakpoint-hit",disp="del",bkptno="3",frame={addr="0x000000000040076e",func="main",args=[],file="threads.c",fullname="/home/pedro/brno/pedro/gdb/tests/threads.c",line="54"},thread-id="3",stopped-threads="all",core="7"
 =breakpoint-deleted,id="3"
 (gdb)

 thread
 &"thread\n"
 ~"[Current thread is 1.1 (Thread 0x7ffff7fc8700 (LWP 8216))]\n"
 ^done
 (gdb) 


 While at it, "inferior 2" is a command that explicitly changes
 the thread, but misses setting the new flag:

-interpreter-exec --thread 1 console "inferior 2"
~"[Switching to inferior 2 [process 8603] (/home/pedro/brno/pedro/gdb/tests/threads)]\n"
~"[Switching to thread 2.1 (Thread 0x7ffff7fc8700 (LWP 8603))] \n"
~"#0  main () at threads.c:54\n"
~"54\t    long i = 0;\n"
^done
(gdb) 

 thread 
 &"thread \n"
 ~"[Current thread is 1.1 (Thread 0x7ffff7fc8700 (LWP 8599))]\n"
 ^done
 (gdb) 


It may be this requires setting the flag in most of the places
that we'd set the user-selected thread in the other approach,
not sure.

Thanks,
Pedro Alves

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

[Simon Marchi]

On 16-08-03 09:41 AM, Pedro Alves wrote:
> I think you need to consider infcalls, and canned sequence of commands
> Python/Scheme, etc., basically any place that sets ui->async = 0 ...
> 
> guile/guile.c:169:  current_ui->async = 0;
> guile/guile.c:202:  current_ui->async = 0;
> guile/guile.c:332:      current_ui->async = 0;
> guile/scm-ports.c:521:  current_ui->async = 0;
> top.c:700:  current_ui->async = 0;
> compile/compile.c:95:  current_ui->async = 0;
> compile/compile.c:137:  current_ui->async = 0;
> compile/compile.c:191:  current_ui->async = 0;
> infcall.c:586:  current_ui->async = 0;
> python/python.c:326:  current_ui->async = 0;
> python/python.c:471:  current_ui->async = 0;
> python/python.c:655:      current_ui->async = 0;
> cli/cli-script.c:384:  current_ui->async = 0;
> cli/cli-script.c:666:  current_ui->async = 0;
> cli/cli-script.c:690:  current_ui->async = 0;
> cli/cli-script.c:1697:  current_ui->async = 0;
>
> ... because these will only return when execution next stops,
> so the cleanup runs after, and undoes the thread change.

Ok, I have not tested all of these yet, but this is how far I have got today.

> E.g., this infcall stops due to a breakpoint on another
> thread, but MI still reverts back to the "--thread" thread:
> 
>  (gdb)
>  b usleep
>  [...]
>  ^done
>  (gdb)
> 
>  thread 1
>  [...]
>  ^done
>  =thread-selected,id="1"
>  (gdb) 
> 
>  -interpreter-exec --thread 1 console "print sleep (10)"
>  [...]
>  ~"[Switching to Thread 0x7ffff6ffb700 (LWP 7818)]\n"
>  ~"\n"
>  ~"Thread 3 \"threads\" hit Breakpoint 2, usleep (useconds=1) at ../sysdeps/posix/usleep.c:26\n"
>  ~"26\t  struct timespec ts = { .tv_sec = (long int) (useconds / 1000000),\n"
>  *stopped,reason="breakpoint-hit",disp="keep",bkptno="2",frame={addr="0x00007ffff78f6030",func="usleep",args=[{name="useconds",value="1"}],file="../sysdeps/posix/usleep.c",fullname="/usr/src/debug/glibc-2.22/sysdeps/posix/usleep.c",line="26"},thread-id="3",stopped-threads="all",core="2"
>  &"The program stopped in another thread while making a function call from GDB.\n"
>  &"Evaluation of the expression containing the function\n"
>  &"(__sleep) will be abandoned.\n"
>  &"When the function is done executing, GDB will silently stop.\n"
>  ^error,msg="The program stopped in another thread while making a function call from GDB.\nEvaluation of the expression containing the function\n(__sleep) will be abandoned.\nWhen the function is done executing, GDB will silently stop."
>  (gdb)
> 
>  thread
>  [...]
>  ~"[Current thread is 1 (Thread 0x7ffff7fc8700 (LWP 7813))]\n"
>  ^done
>  (gdb) 
> 
> Above should have been 3.

I tried to set command_changes_user_selected_thread in normal_stop, but it didn't work
right away.  That's because a breakpoint hit during an infcall calls error(), and the code
that discards the cleanup when command_changes_user_selected_thread is true is not executed.

I solved that by adding a try/catch in mi_cmd_execute that discards the cleanup and rethrows
the exception.

> E.g., this user-defined command starts the program and stops in
> thread 2.1, but MI still reverts back to the "--thread" thread:
> 
>  (gdb) define foo
>  ~">" inferior 2
>  ~">" start
>  ~">" end
>  (gdb)
> 
>  info inferiors
>  ~"  Num  Description       Executable        \n"
>  ~"* 1    process 8216      /home/pedro/brno/pedro/gdb/tests/threads \n"
>  ~"  2    <null>            /home/pedro/brno/pedro/gdb/tests/threads \n"
>  ^done
>  (gdb)
> 
>  info threads
>  ~"  Id   Target Id         Frame \n"
>  ~"* 1.1  Thread 0x7ffff7fc8700 (LWP 8216) \"threads\" main () at threads.c:54\n"
>  ^done
>  (gdb)
> 
>  -interpreter-exec --thread 1 console "foo"
>  ~"[Switching to inferior 2 [<null>] (/home/pedro/brno/pedro/gdb/tests/threads)]\n"
>  [...]
>  ~"\n"
>  ~"Thread 2.1 \"threads\" hit Temporary breakpoint 3, main () at threads.c:54\n"
>  ~"54\t    long i = 0;\n"
>  *stopped,reason="breakpoint-hit",disp="del",bkptno="3",frame={addr="0x000000000040076e",func="main",args=[],file="threads.c",fullname="/home/pedro/brno/pedro/gdb/tests/threads.c",line="54"},thread-id="3",stopped-threads="all",core="7"
>  =breakpoint-deleted,id="3"
>  (gdb)
> 
>  thread
>  &"thread\n"
>  ~"[Current thread is 1.1 (Thread 0x7ffff7fc8700 (LWP 8216))]\n"
>  ^done
>  (gdb) 

I was surprised this wasn't automatically solved with the fix in normal_stop.  That's
because "start" sets previous_inferior_ptid to the initial thread when it is started
(in init_wait_for_inferior).  Actually, I think that starting a new inferior is a case
where we want to change the user selected thread.  Setting command_changes_user_selected_thread
in init_wait_for_inferior fixes this.

It also fixes if you do just "-exec-run --thread-group i1" with mi-async=on and non-stop=on.
With master gdb, the first thread will be selected.  With my previous patch, no thread would
be selected.

>  While at it, "inferior 2" is a command that explicitly changes
>  the thread, but misses setting the new flag:
> 
> -interpreter-exec --thread 1 console "inferior 2"
> ~"[Switching to inferior 2 [process 8603] (/home/pedro/brno/pedro/gdb/tests/threads)]\n"
> ~"[Switching to thread 2.1 (Thread 0x7ffff7fc8700 (LWP 8603))] \n"
> ~"#0  main () at threads.c:54\n"
> ~"54\t    long i = 0;\n"
> ^done
> (gdb) 
> 
>  thread 
>  &"thread \n"
>  ~"[Current thread is 1.1 (Thread 0x7ffff7fc8700 (LWP 8599))]\n"
>  ^done
>  (gdb) 

Right, that's fixed by setting the command_changes_user_selected_thread in inferior_command.

> It may be this requires setting the flag in most of the places
> that we'd set the user-selected thread in the other approach,
> not sure.

It looks like it.

Here's an updated patch.  Notes/todo:

 - I did not test with compile yet.
 - I thought about that just before sending: I think there's also something to do about
   "frame X", "up" and "down", e.g.:

   -interpreter-exec --thread 1 console "up"

   won't work.

Thanks for the comments!

---

From baff2cfffe4ff36871320e41ddaf18b95c0eaa11 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Mon, 1 Aug 2016 17:12:51 -0400
Subject: [PATCH] mi: Restore original thread/frame when specifying --thread or
 --thread-group

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 patch adds a restore_current_thread cleanup whenever --thread or
--thread-group is used (using --frame implies using --thread).  To
support the case where the --thread flag would be used along with a
command that is meant to change the thread, it also adds a global
variable "command_changes_user_selected_ptid".  The GDB core can set
this variable to indicate that it wants to change the user-selected
thread.  When this variable is set, the cleanup is discarded so that the
newly selected thread is kept.  Because there is a single chain of
cleanups, I had to reorder things in mi_cmd_execute so that it was
possible to only discard this cleanup.

As a side effect, the code that checks whether a =thread-selected event
should be emitted was simplified.

This is intended to be a temporary solution, until we implement
something like what Pedro suggested in [2].

Regtested with --target_board=unix on Linux x86.

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

gdb/ChangeLog:

	* infcmd.c (command_changes_user_selected_ptid): New variable.
	* inferior.h (command_changes_user_selected_ptid): New
	declaration.
	* infrun.c (init_wait_for_inferior): Set
	command_changes_user_selected_ptid.
	(normal_stop): Likewise.
	* 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.  Move language and
	suppress_notification code blocks up.  Discard current thread
	cleanup if command_changes_user_selected_ptid is set.
	* thread.c (do_captured_thread_select): Set
	command_changes_user_selected_ptid.
	* top.c (prepare_execute_command): Unset
	command_changes_user_selected_ptid.
---
 gdb/infcmd.c     |  4 ++++
 gdb/inferior.c   |  2 ++
 gdb/inferior.h   |  5 +++++
 gdb/infrun.c     |  2 ++
 gdb/mi/mi-main.c | 63 ++++++++++++++++++++++++++++++++++----------------------
 gdb/thread.c     |  1 +
 6 files changed, 52 insertions(+), 25 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 58ba1cb..65adc63 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -124,6 +124,10 @@ static char *inferior_io_terminal_scratch;

 ptid_t inferior_ptid;

+/* See inferior.h.  */
+
+int command_changes_user_selected_ptid;
+
 /* Address at which inferior stopped.  */

 CORE_ADDR stop_pc;
diff --git a/gdb/inferior.c b/gdb/inferior.c
index 45b3141..cb7db1b 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -722,6 +722,8 @@ inferior_command (char *args, int from_tty)
   struct inferior *inf;
   int num;

+  command_changes_user_selected_ptid = 1;
+
   num = parse_and_eval_long (args);

   inf = find_inferior_id (num);
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 571d26a..edf829e 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -82,6 +82,11 @@ extern const char *get_inferior_io_terminal (void);

 extern ptid_t inferior_ptid;

+/* Flag indicating that we explicitly want to change the user selected
+   thread/ptid.  */
+
+extern int command_changes_user_selected_ptid;
+
 extern void generic_mourn_inferior (void);

 extern CORE_ADDR unsigned_pointer_to_address (struct gdbarch *gdbarch,
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 90841f4..894ccf9 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3247,6 +3247,7 @@ init_wait_for_inferior (void)
   target_last_wait_ptid = minus_one_ptid;

   previous_inferior_ptid = inferior_ptid;
+  command_changes_user_selected_ptid = 1;

   /* Discard any skipped inlined frames.  */
   clear_inline_frame_state (minus_one_ptid);
@@ -8313,6 +8314,7 @@ normal_stop (void)
 	  annotate_thread_changed ();
 	}
       previous_inferior_ptid = inferior_ptid;
+      command_changes_user_selected_ptid = 1;
     }

   if (last.kind == TARGET_WAITKIND_NO_RESUMED)
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 1913157..2001078 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -2170,18 +2170,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)
 	    {
@@ -2208,8 +2199,10 @@ static void
 mi_cmd_execute (struct mi_parse *parse)
 {
   struct cleanup *cleanup;
+  struct cleanup *thread_restore_cleanup = NULL;

   cleanup = prepare_execute_command ();
+  command_changes_user_selected_ptid = 0;

   if (parse->all && parse->thread_group != -1)
     error (_("Cannot specify --thread-group together with --all"));
@@ -2223,6 +2216,18 @@ mi_cmd_execute (struct mi_parse *parse)
   if (parse->frame != -1 && parse->thread == -1)
     error (_("Cannot specify --frame without --thread"));

+  if (parse->language != language_unknown)
+    {
+      make_cleanup_restore_current_language ();
+      set_language (parse->language);
+    }
+
+  if (parse->cmd->suppress_notification != NULL)
+    {
+      make_cleanup_restore_integer (parse->cmd->suppress_notification);
+      *parse->cmd->suppress_notification = 1;
+    }
+
   if (parse->thread_group != -1)
     {
       struct inferior *inf = find_inferior_id (parse->thread_group);
@@ -2231,6 +2236,8 @@ mi_cmd_execute (struct mi_parse *parse)
       if (!inf)
 	error (_("Invalid thread group for the --thread-group option"));

+      thread_restore_cleanup = 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
@@ -2253,6 +2260,8 @@ mi_cmd_execute (struct mi_parse *parse)
       if (is_exited (tp->ptid))
 	error (_("Thread id: %d has terminated"), parse->thread);

+      thread_restore_cleanup = make_cleanup_restore_current_thread ();
+
       switch_to_thread (tp->ptid);
     }

@@ -2269,23 +2278,23 @@ mi_cmd_execute (struct mi_parse *parse)
 	error (_("Invalid frame id: %d"), frame);
     }

-  if (parse->language != language_unknown)
-    {
-      make_cleanup_restore_current_language ();
-      set_language (parse->language);
-    }
-
   current_context = parse;

-  if (parse->cmd->suppress_notification != NULL)
-    {
-      make_cleanup_restore_integer (parse->cmd->suppress_notification);
-      *parse->cmd->suppress_notification = 1;
-    }
-
   if (parse->cmd->argv_func != NULL)
     {
-      parse->cmd->argv_func (parse->command, parse->argv, parse->argc);
+      TRY
+        {
+	  parse->cmd->argv_func (parse->command, parse->argv, parse->argc);
+        }
+      CATCH (exception, RETURN_MASK_ALL)
+        {
+          if (command_changes_user_selected_ptid && thread_restore_cleanup != NULL)
+            discard_cleanups (thread_restore_cleanup);
+
+          throw_exception (exception);
+        }
+      END_CATCH
+
     }
   else if (parse->cmd->cli.cmd != 0)
     {
@@ -2309,6 +2318,10 @@ mi_cmd_execute (struct mi_parse *parse)
       make_cleanup_ui_file_delete (stb);
       error_stream (stb);
     }
+
+  if (command_changes_user_selected_ptid && thread_restore_cleanup != NULL)
+    discard_cleanups (thread_restore_cleanup);
+
   do_cleanups (cleanup);
 }

diff --git a/gdb/thread.c b/gdb/thread.c
index ab98777..7aa1ce7 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -2055,6 +2055,7 @@ do_captured_thread_select (struct ui_out *uiout, void *tidstr_v)
     error (_("Thread ID %s has terminated."), tidstr);

   switch_to_thread (tp->ptid);
+  command_changes_user_selected_ptid = 1;

   annotate_thread_changed ();

-- 
2.9.2

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

[Pedro Alves]

On 08/03/2016 11:20 PM, Simon Marchi wrote:
> > On 16-08-03 09:41 AM, Pedro Alves wrote:

> I tried to set command_changes_user_selected_thread in normal_stop, but it didn't work
> right away.  That's because a breakpoint hit during an infcall calls error(), and the code
> that discards the cleanup when command_changes_user_selected_thread is true is not executed.
> 
> I solved that by adding a try/catch in mi_cmd_execute that discards the cleanup and rethrows
> the exception.

Yeah.  The approach of extending the previous_inferior_ptid's scope a bit would
avoid this, since it wouldn't need to revert back anything with a cleanup.
Might end up being that once all the spots are identified, switching to
the other approach ends up being a simpler patch.

> 
>> E.g., this user-defined command starts the program and stops in
>> thread 2.1, but MI still reverts back to the "--thread" thread:

...

> I was surprised this wasn't automatically solved with the fix in normal_stop.  That's
> because "start" sets previous_inferior_ptid to the initial thread when it is started
> (in init_wait_for_inferior).  Actually, I think that starting a new inferior is a case
> where we want to change the user selected thread.  Setting command_changes_user_selected_thread
> in init_wait_for_inferior fixes this.

Right.

> Here's an updated patch.  Notes/todo:
> 
>  - I did not test with compile yet.
>  - I thought about that just before sending: I think there's also something to do about
>    "frame X", "up" and "down", e.g.:
> 
>    -interpreter-exec --thread 1 console "up"
> 
>    won't work.

Sounds like it.  And don't we need a similar treatment for --frame ?

Thanks,
Pedro Alves

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

[Simon Marchi]

On 16-08-05 01:26 PM, Pedro Alves wrote:
> Yeah.  The approach of extending the previous_inferior_ptid's scope a bit would
> avoid this, since it wouldn't need to revert back anything with a cleanup.
> Might end up being that once all the spots are identified, switching to
> the other approach ends up being a simpler patch.

Hmm maybe.  I'll try to continue on that path and see what it gives.  I really feel
like I don't know what I am doing, but I'll try anyway, that's how we learn I suppose :).

> Sounds like it.  And don't we need a similar treatment for --frame ?

I don't think we do.  --frame can only be used with --thread, and if --thread is specified
we will restore the state.

Re: [PATCH 2/2] mi: Add launch-type={run,attach} in =thread-group-started

[Simon Marchi]

On 16-08-01 05:14 PM, Simon Marchi wrote:
> For Eclipse CDT to properly handle processes attached or started from
> the console, it would be useful to have a hint of whether the
> thread-group was started by gdb or attached.  In particular, it helps
> for inferior pty handling, since a process that is "ran" requires some
> special handling for its output to appear in a console in CDT, whereas
> an "attached" process doesn't require any intervention.
> 
> This patch adds launch-type="run" or launch-type="attach" to the
> =thread-group-started event.  "launch" comes from the Eclipse
> terminology, so it's perhaps not the best term.  It would be nice to
> have something that fits better in the gdb terminology.  Suggestions are
> welcome.  I also thought of using attached="{1,0}", but I think the
> other approach is more easily extensible if we ever need to.
> 
> Note that launch-type="run" will be present even "starting" a core file.
> That's because we display "run" if inferior->attach_flag is 0 and
> "attach" otherwise.  If that's a problem, we could find a way to omit
> the field completely when it's irrelevant.  For example, when none of
> the target present on the target stack know how to run or attach (which
> is the case when inspecting a core file I believe), then it's probably
> irrelevant.
> 
> I needed to do some little changes in various targets, so that struct
> inferior's attach_flag field is set before inferior_appeared is called.
> I can't test all of them, but the changes seem very low risk to me.
> 
> I have added a test for an attach in MI (it seems like there was none),
> in which I test =thread-group-started with launch-type="attach".
> Testing launch-type="run" could probably be done as well, but would be a
> bit more involved (we could perhaps test it as part of
> mi_run_cmd_full...).
> 
> Regtested on x86, unix/native-gdbserver/native-extended-gdbserver.
> 
> gdb/ChangeLog:
> 
> 	* NEWS: Announce new "launch-type" field in =thread-group-started.
> 	* mi/mi-interp.c (mi_inferior_appeared): Output "launch-type"
> 	attribute in =thread-group-started event.
> 	* darwin-nat.c (darwin_attach): Move setting of attach_flag
> 	before call to inferior_appeared.
> 	* gnu-nat.c (gnu_attach): Likewise.
> 	* inf-ptrace.c (inf_ptrace_attach): Likewise.
> 	* nto-procfs.c (procfs_attach): Likewise.
> 	* procfs.c (do_attach): Likewise.
> 	* remote.c (remote_add_inferior): Likewise.
> 	* windows-nat.c (do_initial_windows_stuff): Likewise.
> 
> gdb/doc/ChangeLog:
> 
> 	* gdb.texinfo (GDB/MI Async Records): Document new "launch-type" field
> 	in =thread-group-started.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.mi/mi-attach.exp: New file.
> 	* gdb.mi/mi-attach.c: New file.

I am abandoning this for now.

Based on this conversation [0], we concluded that the solution we had planned
might not be an appropriate user experience.  And even then, it had its
technical flaws, so it's not clear that this new field will be needed.

[0] https://sourceware.org/ml/gdb/2016-08/msg00036.html

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

[Simon Marchi]

On 16-08-05 04:58 PM, Simon Marchi wrote:
> On 16-08-05 01:26 PM, Pedro Alves wrote:
>> Yeah.  The approach of extending the previous_inferior_ptid's scope a bit would
>> avoid this, since it wouldn't need to revert back anything with a cleanup.
>> Might end up being that once all the spots are identified, switching to
>> the other approach ends up being a simpler patch.
> 
> Hmm maybe.  I'll try to continue on that path and see what it gives.  I really feel
> like I don't know what I am doing, but I'll try anyway, that's how we learn I suppose :).

Given the timeframe, I don't think it's realistic nor a good idea to push
to get this into 7.12.  Unless there's an obvious way to do it that I missed,
the changes are quite big and risky so close to the release.

Here's my work-in-progress branch in case you'd like to take a high level look.

  https://github.com/simark/binutils-gdb/commits/user-selected-ptid

It's not all pretty, but I got the testsuite to pass (at least with native x86
Linux, I haven't tried with remote yet...).  The "user_selection" object keeps
the currently selected inferior and thread, but not the frame yet.  That means
that if you are inspecting an arbitrary frame and there's an MI command with
--thread, the inferior/thread will be kept, but not your frame.