[RFC PATCH] gdb/mi: consistently notify user when GDB/MI client uses -thread-select

[RFC PATCH] gdb/mi: consistently notify user when GDB/MI client uses -thread-select

[Jan Vrany]

Hi Andrew,

this is a follow-up patch, trying to address inconsistency in GDB/MI
notifications you pointed out the other day [1].

The below patch seems to fix it when I manually test it, but I did not
manage to write a test. I tried several things, but always get timeouts.
See my (failed) attempt in below patch (note, that the expected output is
wrong by purpose to ensure the test actually works).

At this point I'd like to take you offer of help - if you have an idea how
to write the test, I'd be great.

[1]: https://sourceware.org/pipermail/gdb-patches/2022-February/185989.html

-- >8 --

GDB notifies users about user selected thread changes somewhat
inconsistently as mentioned on gdb-patches mailing list here:

  https://sourceware.org/pipermail/gdb-patches/2022-February/185989.html

Consider GDB debugging a multi-threaded inferior with both CLI and GDB/MI
interfaces connected to separate terminals.

Assuming inferior is stopped and thread 1 is selected, when a thread
2 is selected using '-thread-select 2' command on GDB/MI terminal:

    -thread-select 2
    ^done,new-thread-id="2",frame={level="0",addr="0x00005555555551cd",func="child_sub_function",args=[],file="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.mi/user-selected-context-sync.c",fullname="/home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c",line="30",arch="i386:x86-64"}
    (gdb)

and on CLI terminal we get the notification (as expected):

    [Switching to thread 2 (Thread 0x7ffff7daa640 (LWP 389659))]
    #0  child_sub_function () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:30
    30        volatile int dummy = 0;

However, now that thread 2 is selected, if thread 1 is selected
using 'thread-select --thread 1 1' command on GDB/MI terminal
terminal:

   -thread-select --thread 1 1
   ^done,new-thread-id="1",frame={level="0",addr="0x0000555555555294",func="main",args=[],file="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.mi/user-selected-context-sync.c",fullname="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.mi/user-selected-context-sync.c",line="66",arch="i386:x86-64"}
   (gdb)

but no notification is printed on CLI terminal, despite the fact
that user selected thread has changed.

The problem is that when `-thread-select --thread 1 1` is executed
then thread is switched to thread 1 before mi_cmd_thread_select () is
called, therefore the condition "inferior_ptid != previous_ptid"
there does not hold.

To address this problem, we have to move notification logic up to
mi_cmd_execute () where --thread option is processed and notify
user selected contents observers there if context changes.

However, this in itself breaks GDB/MI because it would cause context
notification to be sent on MI channel. This is because by the time
we notify, MI notification suppression is already restored (done in
mi_command::invoke(). Therefore we had to lift notification suppression
logic also up to mi_cmd_execute ().

With this change, all gdb.mi tests pass, tested on x86_64-linux.

TODO:
 * fix test
---
 gdb/mi/mi-cmd-stack.c                         | 10 ---
 gdb/mi/mi-cmds.c                              |  2 -
 gdb/mi/mi-cmds.h                              | 16 ++--
 gdb/mi/mi-main.c                              | 32 +++++---
 ...mi-user-selected-context-notifications.exp | 78 +++++++++++++++++++
 5 files changed, 109 insertions(+), 29 deletions(-)
 create mode 100644 gdb/testsuite/gdb.mi/mi-user-selected-context-notifications.exp

diff --git a/gdb/mi/mi-cmd-stack.c b/gdb/mi/mi-cmd-stack.c
index 1be8aa81c3d..afe584b7fca 100644
--- a/gdb/mi/mi-cmd-stack.c
+++ b/gdb/mi/mi-cmd-stack.c
@@ -757,17 +757,7 @@ mi_cmd_stack_select_frame (const char *command, char **argv, int argc)
 {
   if (argc == 0 || argc > 1)
     error (_("-stack-select-frame: Usage: FRAME_SPEC"));
-
-  ptid_t previous_ptid = inferior_ptid;
-
   select_frame_for_mi (parse_frame_specification (argv[0]));
-
-  /* Notify if the thread has effectively changed.  */
-  if (inferior_ptid != previous_ptid)
-    {
-      gdb::observers::user_selected_context_changed.notify
-	(USER_SELECTED_THREAD | USER_SELECTED_FRAME);
-    }
 }
 
 void
diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index cd7cabdda9b..957dfb4e03d 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -171,8 +171,6 @@ mi_command::mi_command (const char *name, int *suppress_notification)
 void
 mi_command::invoke (struct mi_parse *parse) const
 {
-  gdb::optional<scoped_restore_tmpl<int>> restore
-    = do_suppress_notification ();
   this->do_invoke (parse);
 }
 
diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
index 785652ee1c9..da1598a3f32 100644
--- a/gdb/mi/mi-cmds.h
+++ b/gdb/mi/mi-cmds.h
@@ -175,14 +175,6 @@ struct mi_command
     return m_suppress_notification != &mi_suppress_notification.user_selected_context;
   }
 
-protected:
-
-  /* The core of command invocation, this needs to be overridden in each
-     base class.  PARSE is the parsed command line from the user.  */
-  virtual void do_invoke (struct mi_parse *parse) const = 0;
-
-private:
-
   /* If this command was created with a suppress notifications pointer,
      then this function will set the suppress flag and return a
      gdb::optional with its value set to an object that will restore the
@@ -192,6 +184,14 @@ struct mi_command
      then this function returns an empty gdb::optional.  */
   gdb::optional<scoped_restore_tmpl<int>> do_suppress_notification () const;
 
+protected:
+
+  /* The core of command invocation, this needs to be overridden in each
+     base class.  PARSE is the parsed command line from the user.  */
+  virtual void do_invoke (struct mi_parse *parse) const = 0;
+
+private:
+
   /* The name of the command.  */
   const char *m_name;
 
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 73380f5e668..a5b847c7039 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -556,19 +556,10 @@ mi_cmd_thread_select (const char *command, char **argv, int argc)
   if (thr == NULL)
     error (_("Thread ID %d not known."), num);
 
-  ptid_t previous_ptid = inferior_ptid;
-
   thread_select (argv[0], thr);
 
   print_selected_thread_frame (current_uiout,
 			       USER_SELECTED_THREAD | USER_SELECTED_FRAME);
-
-  /* Notify if the thread has effectively changed.  */
-  if (inferior_ptid != previous_ptid)
-    {
-      gdb::observers::user_selected_context_changed.notify
-	(USER_SELECTED_THREAD | USER_SELECTED_FRAME);
-    }
 }
 
 void
@@ -1975,6 +1966,16 @@ mi_execute_command (const char *cmd, int from_tty)
     }
 }
 
+/* Determine whether the thread has changed.  */
+
+static bool
+command_changed_user_selected_thread (ptid_t previous_ptid, ptid_t current_ptid)
+{
+  return (previous_ptid != null_ptid
+	  && current_ptid != previous_ptid
+	  && current_ptid != null_ptid);
+}
+
 static void
 mi_cmd_execute (struct mi_parse *parse)
 {
@@ -2015,6 +2016,8 @@ mi_cmd_execute (struct mi_parse *parse)
       set_current_program_space (inf->pspace);
     }
 
+  ptid_t previous_ptid = inferior_ptid;
+
   gdb::optional<scoped_restore_current_thread> thread_saver;
   if (parse->thread != -1)
     {
@@ -2060,7 +2063,18 @@ mi_cmd_execute (struct mi_parse *parse)
   current_context = parse;
 
   gdb_assert (parse->cmd != nullptr);
+
+  gdb::optional<scoped_restore_tmpl<int>> restore_suppress_notification
+    = parse->cmd->do_suppress_notification ();
+
   parse->cmd->invoke (parse);
+
+  if (!parse->cmd->preserve_user_selected_context ()
+      && command_changed_user_selected_thread (previous_ptid, inferior_ptid))
+    {
+      gdb::observers::user_selected_context_changed.notify
+      (USER_SELECTED_THREAD | USER_SELECTED_FRAME);
+    }
 }
 
 /* See mi-main.h.  */
diff --git a/gdb/testsuite/gdb.mi/mi-user-selected-context-notifications.exp b/gdb/testsuite/gdb.mi/mi-user-selected-context-notifications.exp
new file mode 100644
index 00000000000..cf9c02e54a4
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-user-selected-context-notifications.exp
@@ -0,0 +1,78 @@
+# Copyright 2022-2022 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/>.
+
+# Test that user is propery notified on CLI channel when.
+# selected thread and/or frame is changed using GDB/MI.
+#
+# See https://sourceware.org/pipermail/gdb-patches/2022-February/185989.html
+
+load_lib mi-support.exp
+
+standard_testfile user-selected-context-sync.c
+
+# Multiple threads are needed, therefore only native gdb and extended
+# gdbserver modes are supported.
+if [use_gdb_stub] {
+	untested "using gdb stub"
+	return
+}
+
+if {[build_executable $testfile.exp $testfile ${srcfile} "debug pthreads"] == -1} {
+	untested "failed to compile"
+	return -1
+}
+
+if [eval mi_gdb_start { "separate-mi-tty" }] {
+	return
+}
+
+# Useful for debugging:
+verbose -log "Channels:"
+verbose -log " inferior_spawn_id=$inferior_spawn_id"
+verbose -log " gdb_spawn_id=$gdb_spawn_id"
+verbose -log " gdb_main_spawn_id=$gdb_main_spawn_id"
+verbose -log " mi_spawn_id=$mi_spawn_id"
+
+mi_gdb_load $binfile
+if { [mi_runto_main] < 0 } {
+	return
+}
+
+switch_gdb_spawn_id $gdb_main_spawn_id
+
+gdb_test "break $srcfile:child_function" \
+    "Breakpoint.*at.* file .*$srcfile, line.*" \
+    "breakpoint function in file"
+
+gdb_test "continue" \
+	".*Thread 2.*hit Breakpoint 2.*" \
+	"continue to child_function"
+
+switch_gdb_spawn_id $mi_spawn_id
+
+send_gdb "111-thread-select 1"
+gdb_expect {
+	-i "$mi_spawn_id"
+	-re "\\^done,new-thread-id=\"2\".*" {
+		fail "111-thread-select 1 (MI output)"
+	}
+	-i "$gdb_main_spawn_id"
+	-re "\\\[Switching to thread 1.*" {
+		fail "111-thread-select 1 (CLI output)"
+	}
+	timeout {
+	    fail "111-thread-select 1 (timeout)"
+	}
+}
-- 
2.35.1

Re: [RFC PATCH] gdb/mi: consistently notify user when GDB/MI client uses -thread-select

[Andrew Burgess]

* Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> [2022-03-14 11:57:41 +0000]:

> Hi Andrew,
> 
> this is a follow-up patch, trying to address inconsistency in GDB/MI
> notifications you pointed out the other day [1].
> 
> The below patch seems to fix it when I manually test it, but I did not
> manage to write a test. I tried several things, but always get timeouts.
> See my (failed) attempt in below patch (note, that the expected output is
> wrong by purpose to ensure the test actually works).
> 
> At this point I'd like to take you offer of help - if you have an idea how
> to write the test, I'd be great.

No problem.

Thanks for looking at this, the work so far looks great.  As for
testing, I can't claim too much credit, I ended up dropping the test
you'd started, and just extended the existing test script
gdb.mi/user-selected-context-sync.exp as this covers all the related
cases, just not the specific cases you are testing.

Except, it does actually cover one of the cases you are fixing - the
test includes a kfail for bug gdb/20631 - which is fixed by your
commit - good job!

So while I was writing the tests I was also adding tests for selecting
the frame, as this seemed like a related problem, checking things like
this:

  # Currently in thread 2, frame 0, then...
  -stack-select-frame --thread 2 --frame 1 1

After this I would expect a change of frame announcement in the CLI,
which didn't work with your original patch.

The patch below includes a fix for this problem too, as well as some
tests for this.

I don't know if you had any other changes you wanted to make to this
patch or not, feel free to review the changes I've made, and if you're
happy, just post this to the list as a v2 for review.

Thanks,
Andrew

---

commit ae700a9c07c3573e1e7adde0905156a75aa22aac
Author: Jan Vrany <jan.vrany@labware.com>
Date:   Mon Mar 14 11:57:41 2022 +0000

    gdb/mi: consistently notify user when GDB/MI client uses -thread-select
    
    GDB notifies users about user selected thread changes somewhat
    inconsistently as mentioned on gdb-patches mailing list here:
    
      https://sourceware.org/pipermail/gdb-patches/2022-February/185989.html
    
    Consider GDB debugging a multi-threaded inferior with both CLI and GDB/MI
    interfaces connected to separate terminals.
    
    Assuming inferior is stopped and thread 1 is selected, when a thread
    2 is selected using '-thread-select 2' command on GDB/MI terminal:
    
        -thread-select 2
        ^done,new-thread-id="2",frame={level="0",addr="0x00005555555551cd",func="child_sub_function",args=[],file="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.mi/user-selected-context-sync.c",fullname="/home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c",line="30",arch="i386:x86-64"}
        (gdb)
    
    and on CLI terminal we get the notification (as expected):
    
        [Switching to thread 2 (Thread 0x7ffff7daa640 (LWP 389659))]
        #0  child_sub_function () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:30
        30        volatile int dummy = 0;
    
    However, now that thread 2 is selected, if thread 1 is selected
    using 'thread-select --thread 1 1' command on GDB/MI terminal
    terminal:
    
       -thread-select --thread 1 1
       ^done,new-thread-id="1",frame={level="0",addr="0x0000555555555294",func="main",args=[],file="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.mi/user-selected-context-sync.c",fullname="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.mi/user-selected-context-sync.c",line="66",arch="i386:x86-64"}
       (gdb)
    
    but no notification is printed on CLI terminal, despite the fact
    that user selected thread has changed.
    
    The problem is that when `-thread-select --thread 1 1` is executed
    then thread is switched to thread 1 before mi_cmd_thread_select () is
    called, therefore the condition "inferior_ptid != previous_ptid"
    there does not hold.
    
    To address this problem, we have to move notification logic up to
    mi_cmd_execute () where --thread option is processed and notify
    user selected contents observers there if context changes.
    
    However, this in itself breaks GDB/MI because it would cause context
    notification to be sent on MI channel. This is because by the time
    we notify, MI notification suppression is already restored (done in
    mi_command::invoke(). Therefore we had to lift notification suppression
    logic also up to mi_cmd_execute ().
    
    With this change, all gdb.mi tests pass, tested on x86_64-linux.
    
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=20631

diff --git a/gdb/mi/mi-cmd-stack.c b/gdb/mi/mi-cmd-stack.c
index 1be8aa81c3d..e894411765a 100644
--- a/gdb/mi/mi-cmd-stack.c
+++ b/gdb/mi/mi-cmd-stack.c
@@ -757,17 +757,7 @@ mi_cmd_stack_select_frame (const char *command, char **argv, int argc)
 {
   if (argc == 0 || argc > 1)
     error (_("-stack-select-frame: Usage: FRAME_SPEC"));
-
-  ptid_t previous_ptid = inferior_ptid;
-
-  select_frame_for_mi (parse_frame_specification (argv[0]));
-
-  /* Notify if the thread has effectively changed.  */
-  if (inferior_ptid != previous_ptid)
-    {
-      gdb::observers::user_selected_context_changed.notify
-	(USER_SELECTED_THREAD | USER_SELECTED_FRAME);
-    }
+  select_frame (parse_frame_specification (argv[0]));
 }
 
 void
diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index 38fbe0d8a32..d21b5d48548 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -176,8 +176,6 @@ mi_command::mi_command (const char *name, int *suppress_notification)
 void
 mi_command::invoke (struct mi_parse *parse) const
 {
-  gdb::optional<scoped_restore_tmpl<int>> restore
-    = do_suppress_notification ();
   this->do_invoke (parse);
 }
 
diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
index 47b90a26064..41806f1d334 100644
--- a/gdb/mi/mi-cmds.h
+++ b/gdb/mi/mi-cmds.h
@@ -175,14 +175,6 @@ struct mi_command
     return m_suppress_notification != &mi_suppress_notification.user_selected_context;
   }
 
-protected:
-
-  /* The core of command invocation, this needs to be overridden in each
-     base class.  PARSE is the parsed command line from the user.  */
-  virtual void do_invoke (struct mi_parse *parse) const = 0;
-
-private:
-
   /* If this command was created with a suppress notifications pointer,
      then this function will set the suppress flag and return a
      gdb::optional with its value set to an object that will restore the
@@ -192,6 +184,14 @@ struct mi_command
      then this function returns an empty gdb::optional.  */
   gdb::optional<scoped_restore_tmpl<int>> do_suppress_notification () const;
 
+protected:
+
+  /* The core of command invocation, this needs to be overridden in each
+     base class.  PARSE is the parsed command line from the user.  */
+  virtual void do_invoke (struct mi_parse *parse) const = 0;
+
+private:
+
   /* The name of the command.  */
   const char *m_name;
 
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 73380f5e668..abd033b22ae 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -556,19 +556,10 @@ mi_cmd_thread_select (const char *command, char **argv, int argc)
   if (thr == NULL)
     error (_("Thread ID %d not known."), num);
 
-  ptid_t previous_ptid = inferior_ptid;
-
   thread_select (argv[0], thr);
 
   print_selected_thread_frame (current_uiout,
 			       USER_SELECTED_THREAD | USER_SELECTED_FRAME);
-
-  /* Notify if the thread has effectively changed.  */
-  if (inferior_ptid != previous_ptid)
-    {
-      gdb::observers::user_selected_context_changed.notify
-	(USER_SELECTED_THREAD | USER_SELECTED_FRAME);
-    }
 }
 
 void
@@ -1975,6 +1966,37 @@ mi_execute_command (const char *cmd, int from_tty)
     }
 }
 
+/* Captures the current user selected context state, that is the current
+   thread and frame.  Later we can then check if the user selected context
+   has changed at all.  */
+
+struct user_selected_context
+{
+  /* Constructor.  */
+  user_selected_context ()
+    : m_previous_ptid (inferior_ptid),
+      m_previous_frame (deprecated_safe_get_selected_frame ())
+  { /* Nothing.  */ }
+
+  /* Return true if the user selected context has changed since this object
+     was created.  */
+  bool has_changed () const
+  {
+    return ((m_previous_ptid != null_ptid
+	     && inferior_ptid != null_ptid
+	     && m_previous_ptid != inferior_ptid)
+	    || m_previous_frame != deprecated_safe_get_selected_frame ());
+  }
+private:
+  /* The previously selected thread.  This might be null_ptid if there was
+     no previously selected thread.  */
+  ptid_t m_previous_ptid;
+
+  /* The previously selected frame.  This might be nullptr if there was no
+     previously selected frame.  */
+  frame_info *m_previous_frame;
+};
+
 static void
 mi_cmd_execute (struct mi_parse *parse)
 {
@@ -2015,6 +2037,8 @@ mi_cmd_execute (struct mi_parse *parse)
       set_current_program_space (inf->pspace);
     }
 
+  user_selected_context current_user_selected_context;
+
   gdb::optional<scoped_restore_current_thread> thread_saver;
   if (parse->thread != -1)
     {
@@ -2060,7 +2084,16 @@ mi_cmd_execute (struct mi_parse *parse)
   current_context = parse;
 
   gdb_assert (parse->cmd != nullptr);
+
+  gdb::optional<scoped_restore_tmpl<int>> restore_suppress_notification
+    = parse->cmd->do_suppress_notification ();
+
   parse->cmd->invoke (parse);
+
+  if (!parse->cmd->preserve_user_selected_context ()
+      && current_user_selected_context.has_changed ())
+    gdb::observers::user_selected_context_changed.notify
+      (USER_SELECTED_THREAD | USER_SELECTED_FRAME);
 }
 
 /* See mi-main.h.  */
diff --git a/gdb/stack.c b/gdb/stack.c
index 8855ed8fe70..10da88b88e5 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -1841,14 +1841,6 @@ select_frame_command_core (struct frame_info *fi, bool ignored)
     gdb::observers::user_selected_context_changed.notify (USER_SELECTED_FRAME);
 }
 
-/* See stack.h.  */
-
-void
-select_frame_for_mi (struct frame_info *fi)
-{
-  select_frame_command_core (fi, false /* Ignored.  */);
-}
-
 /* The core of all the "frame" sub-commands.  Select frame FI, and if this
    means we change frame send out a change notification (otherwise, just
    reprint the current frame summary).   */
diff --git a/gdb/stack.h b/gdb/stack.h
index c49d2e2cbef..f78aedf1c85 100644
--- a/gdb/stack.h
+++ b/gdb/stack.h
@@ -20,12 +20,6 @@
 #ifndef STACK_H
 #define STACK_H
 
-/* Access method used by the MI -stack-select-frame command to switch to
-   frame FI.  This differs from SELECT_FRAME in that the observers for a
-   user selected context change will be triggered.  */
-
-void select_frame_for_mi (struct frame_info *fi);
-
 gdb::unique_xmalloc_ptr<char> find_frame_funname (struct frame_info *frame,
 						  enum language *funlang,
 						  struct symbol **funcp);
diff --git a/gdb/testsuite/gdb.mi/user-selected-context-sync.exp b/gdb/testsuite/gdb.mi/user-selected-context-sync.exp
index 3b4ff0bf2e4..9444ca5acf4 100644
--- a/gdb/testsuite/gdb.mi/user-selected-context-sync.exp
+++ b/gdb/testsuite/gdb.mi/user-selected-context-sync.exp
@@ -916,7 +916,7 @@ proc_with_prefix test_mi_thread_select { mode } {
 	}
     }
 
-    with_test_prefix "thread 1.2 with --thread" {
+    with_test_prefix "thread 1.2 with --thread 2" {
 	# Test selecting a thread from MI with a --thread option.  This test
 	# verifies that even if the thread GDB would switch to is the same as
 	# the thread specified with --thread, an event is still sent to CLI.
@@ -930,10 +930,28 @@ proc_with_prefix test_mi_thread_select { mode } {
 	}
 
 	with_spawn_id $gdb_main_spawn_id {
-	    # This doesn't work as of now, no event is sent on CLI.  It is
-	    # commented out so we don't have to wait for the timeout every time.
-	    # match_re_or_ensure_no_output "$cli_re\r\n" "-thread-select, event on cli"
-	    kfail "gdb/20631" "thread-select, event on cli"
+	    match_re_or_ensure_no_output "$cli_re\r\n" "-thread-select, event on cli"
+	}
+    }
+
+    with_test_prefix "thread 1.2 with --thread 3" {
+	# Test selecting a thread from MI with a --thread option.
+	# This test verifies that when different thread numbers are
+	# passed to the --thread option and the underlying
+	# -thread-select command, the correct thread is selected.
+	# In this case this is thread 1.2
+
+	reset_selection "1.1"
+
+	set mi_re [make_mi_re $mode 2 0 response]
+	set cli_re [make_cli_re $mode -1 1.2 0]
+
+	with_spawn_id $mi_spawn_id {
+	    mi_gdb_test "-thread-select --thread 3 2" $mi_re "-thread-select"
+	}
+
+	with_spawn_id $gdb_main_spawn_id {
+	    match_re_or_ensure_no_output "$cli_re\r\n" "-thread-select, event on cli"
 	}
     }
 
@@ -974,6 +992,36 @@ proc_with_prefix test_mi_stack_select_frame { mode } {
 	with_spawn_id $gdb_main_spawn_id {
 	    match_re_or_ensure_no_output $cli_re "-stack-select-frame again, event on CLI"
 	}
+
+	# Now use the '-stack-select-frame' command with the --frame
+	# option, this verifies that even when the frame GDB would
+	# swith to is the same as the frame specified with --frame, an
+	# event is still sent to the CLI.
+
+	set cli_re [make_cli_re $mode -1 -1 0]
+
+	with_spawn_id $mi_spawn_id {
+	    mi_gdb_test "-stack-select-frame --thread 2 --frame 0 0" $mi_re
+	}
+
+	with_spawn_id $gdb_main_spawn_id {
+	    match_re_or_ensure_no_output "$cli_re\r\n" "-stack-select-frame with --frame 0, event on CLI"
+	}
+
+	# Now use the '-stack-select-frame' command with the --frame
+	# option, this verifies that the correct event is sent to the
+	# CLI when the frame specified with --frame is different to
+	# the actual frame selected.
+
+	set cli_re [make_cli_re $mode -1 -1 1]
+
+	with_spawn_id $mi_spawn_id {
+	    mi_gdb_test "-stack-select-frame --thread 2 --frame 2 1" $mi_re
+	}
+
+	with_spawn_id $gdb_main_spawn_id {
+	    match_re_or_ensure_no_output "$cli_re\r\n" "-stack-select-frame with --frame 2, event on CLI"
+	}
     }
 
     with_test_prefix "thread 1.3" {

[PATCH v2] gdb/mi: consistently notify user when GDB/MI client uses -thread-select

[Jan Vrany]

Changes since v1:

* all improvements and tests by Andrew - thanks a lot!
* merged mi_command::invoke() and mi_command::do_invoke() into
single method (mi_command::invoke()). See the commit message
for rationale.

-- >8 --
GDB notifies users about user selected thread changes somewhat
inconsistently as mentioned on gdb-patches mailing list here:

  https://sourceware.org/pipermail/gdb-patches/2022-February/185989.html

Consider GDB debugging a multi-threaded inferior with both CLI and GDB/MI
interfaces connected to separate terminals.

Assuming inferior is stopped and thread 1 is selected, when a thread
2 is selected using '-thread-select 2' command on GDB/MI terminal:

    -thread-select 2
    ^done,new-thread-id="2",frame={level="0",addr="0x00005555555551cd",func="child_sub_function",args=[],file="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.mi/user-selected-context-sync.c",fullname="/home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c",line="30",arch="i386:x86-64"}
    (gdb)

and on CLI terminal we get the notification (as expected):

    [Switching to thread 2 (Thread 0x7ffff7daa640 (LWP 389659))]
    #0  child_sub_function () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:30
    30        volatile int dummy = 0;

However, now that thread 2 is selected, if thread 1 is selected
using 'thread-select --thread 1 1' command on GDB/MI terminal
terminal:

   -thread-select --thread 1 1
   ^done,new-thread-id="1",frame={level="0",addr="0x0000555555555294",func="main",args=[],file="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.mi/user-selected-context-sync.c",fullname="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.mi/user-selected-context-sync.c",line="66",arch="i386:x86-64"}
   (gdb)

but no notification is printed on CLI terminal, despite the fact
that user selected thread has changed.

The problem is that when `-thread-select --thread 1 1` is executed
then thread is switched to thread 1 before mi_cmd_thread_select () is
called, therefore the condition "inferior_ptid != previous_ptid"
there does not hold.

To address this problem, we have to move notification logic up to
mi_cmd_execute () where --thread option is processed and notify
user selected contents observers there if context changes.

However, this in itself breaks GDB/MI because it would cause context
notification to be sent on MI channel. This is because by the time
we notify, MI notification suppression is already restored (done in
mi_command::invoke(). Therefore we had to lift notification suppression
logic also up to mi_cmd_execute (). This change in made distinction
between mi_command::invoke() and mi_command::do_invoke() unnecessary
as all mi_command::invoke() did (after the change) was to call
do_invoke(). So this patches removes do_invoke() and moves the command
execution logic directly to invoke().

With this change, all gdb.mi tests pass, tested on x86_64-linux.

Co-authored-by: Andrew Burgess <aburgess@redhat.com>
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=20631
---
 gdb/mi/mi-cmd-stack.c                         | 12 +---
 gdb/mi/mi-cmds.c                              | 18 +-----
 gdb/mi/mi-cmds.h                              | 17 ++----
 gdb/mi/mi-main.c                              | 51 +++++++++++++---
 gdb/python/py-micmd.c                         |  5 +-
 gdb/stack.c                                   |  8 ---
 gdb/stack.h                                   |  6 --
 .../gdb.mi/user-selected-context-sync.exp     | 58 +++++++++++++++++--
 8 files changed, 106 insertions(+), 69 deletions(-)

diff --git a/gdb/mi/mi-cmd-stack.c b/gdb/mi/mi-cmd-stack.c
index 1be8aa81c3d..e894411765a 100644
--- a/gdb/mi/mi-cmd-stack.c
+++ b/gdb/mi/mi-cmd-stack.c
@@ -757,17 +757,7 @@ mi_cmd_stack_select_frame (const char *command, char **argv, int argc)
 {
   if (argc == 0 || argc > 1)
     error (_("-stack-select-frame: Usage: FRAME_SPEC"));
-
-  ptid_t previous_ptid = inferior_ptid;
-
-  select_frame_for_mi (parse_frame_specification (argv[0]));
-
-  /* Notify if the thread has effectively changed.  */
-  if (inferior_ptid != previous_ptid)
-    {
-      gdb::observers::user_selected_context_changed.notify
-	(USER_SELECTED_THREAD | USER_SELECTED_FRAME);
-    }
+  select_frame (parse_frame_specification (argv[0]));
 }
 
 void
diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index 38fbe0d8a32..60fec0a0b85 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -45,11 +45,9 @@ struct mi_command_mi : public mi_command
     gdb_assert (func != nullptr);
   }
 
-protected:
-
   /* Called when this MI command has been invoked, calls m_argv_function
      with arguments contained within PARSE.  */
-  void do_invoke (struct mi_parse *parse) const override
+  void invoke (struct mi_parse *parse) const override
   {
     mi_parse_argv (parse->args, parse);
 
@@ -83,13 +81,11 @@ struct mi_command_cli : public mi_command
       m_args_p (args_p)
   { /* Nothing.  */ }
 
-protected:
-
   /* Called when this MI command has been invoked, calls the m_cli_name
      CLI function.  In m_args_p is true then the argument string from
      within PARSE is passed through to the CLI function, otherwise nullptr
      is passed through to the CLI function as its argument string.  */
-  void do_invoke (struct mi_parse *parse) const override
+  void invoke (struct mi_parse *parse) const override
   {
     const char *args = m_args_p ? parse->args : nullptr;
     mi_execute_cli_command (m_cli_name, m_args_p, args);
@@ -173,16 +169,6 @@ mi_command::mi_command (const char *name, int *suppress_notification)
 
 /* See mi-cmds.h.  */
 
-void
-mi_command::invoke (struct mi_parse *parse) const
-{
-  gdb::optional<scoped_restore_tmpl<int>> restore
-    = do_suppress_notification ();
-  this->do_invoke (parse);
-}
-
-/* See mi-cmds.h.  */
-
 gdb::optional<scoped_restore_tmpl<int>>
 mi_command::do_suppress_notification () const
 {
diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
index 47b90a26064..05b702f83ec 100644
--- a/gdb/mi/mi-cmds.h
+++ b/gdb/mi/mi-cmds.h
@@ -160,9 +160,10 @@ struct mi_command
   const char *name () const
   { return m_name; }
 
-  /* Execute the MI command.  Can throw an exception if something goes
-     wrong.  */
-  void invoke (struct mi_parse *parse) const;
+  /* Execute the MI command.  this needs to be overridden in each
+     base class.  PARSE is the parsed command line from the user.
+     Can throw an exception if something goes wrong.  */
+  virtual void invoke (struct mi_parse *parse) const = 0;
 
   /* Return whether this command preserves user selected context (thread
      and frame).  */
@@ -175,14 +176,6 @@ struct mi_command
     return m_suppress_notification != &mi_suppress_notification.user_selected_context;
   }
 
-protected:
-
-  /* The core of command invocation, this needs to be overridden in each
-     base class.  PARSE is the parsed command line from the user.  */
-  virtual void do_invoke (struct mi_parse *parse) const = 0;
-
-private:
-
   /* If this command was created with a suppress notifications pointer,
      then this function will set the suppress flag and return a
      gdb::optional with its value set to an object that will restore the
@@ -192,6 +185,8 @@ struct mi_command
      then this function returns an empty gdb::optional.  */
   gdb::optional<scoped_restore_tmpl<int>> do_suppress_notification () const;
 
+private:
+
   /* The name of the command.  */
   const char *m_name;
 
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 73380f5e668..abd033b22ae 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -556,19 +556,10 @@ mi_cmd_thread_select (const char *command, char **argv, int argc)
   if (thr == NULL)
     error (_("Thread ID %d not known."), num);
 
-  ptid_t previous_ptid = inferior_ptid;
-
   thread_select (argv[0], thr);
 
   print_selected_thread_frame (current_uiout,
 			       USER_SELECTED_THREAD | USER_SELECTED_FRAME);
-
-  /* Notify if the thread has effectively changed.  */
-  if (inferior_ptid != previous_ptid)
-    {
-      gdb::observers::user_selected_context_changed.notify
-	(USER_SELECTED_THREAD | USER_SELECTED_FRAME);
-    }
 }
 
 void
@@ -1975,6 +1966,37 @@ mi_execute_command (const char *cmd, int from_tty)
     }
 }
 
+/* Captures the current user selected context state, that is the current
+   thread and frame.  Later we can then check if the user selected context
+   has changed at all.  */
+
+struct user_selected_context
+{
+  /* Constructor.  */
+  user_selected_context ()
+    : m_previous_ptid (inferior_ptid),
+      m_previous_frame (deprecated_safe_get_selected_frame ())
+  { /* Nothing.  */ }
+
+  /* Return true if the user selected context has changed since this object
+     was created.  */
+  bool has_changed () const
+  {
+    return ((m_previous_ptid != null_ptid
+	     && inferior_ptid != null_ptid
+	     && m_previous_ptid != inferior_ptid)
+	    || m_previous_frame != deprecated_safe_get_selected_frame ());
+  }
+private:
+  /* The previously selected thread.  This might be null_ptid if there was
+     no previously selected thread.  */
+  ptid_t m_previous_ptid;
+
+  /* The previously selected frame.  This might be nullptr if there was no
+     previously selected frame.  */
+  frame_info *m_previous_frame;
+};
+
 static void
 mi_cmd_execute (struct mi_parse *parse)
 {
@@ -2015,6 +2037,8 @@ mi_cmd_execute (struct mi_parse *parse)
       set_current_program_space (inf->pspace);
     }
 
+  user_selected_context current_user_selected_context;
+
   gdb::optional<scoped_restore_current_thread> thread_saver;
   if (parse->thread != -1)
     {
@@ -2060,7 +2084,16 @@ mi_cmd_execute (struct mi_parse *parse)
   current_context = parse;
 
   gdb_assert (parse->cmd != nullptr);
+
+  gdb::optional<scoped_restore_tmpl<int>> restore_suppress_notification
+    = parse->cmd->do_suppress_notification ();
+
   parse->cmd->invoke (parse);
+
+  if (!parse->cmd->preserve_user_selected_context ()
+      && current_user_selected_context.has_changed ())
+    gdb::observers::user_selected_context_changed.notify
+      (USER_SELECTED_THREAD | USER_SELECTED_FRAME);
 }
 
 /* See mi-main.h.  */
diff --git a/gdb/python/py-micmd.c b/gdb/python/py-micmd.c
index 4665fcc75c7..399e89b0298 100644
--- a/gdb/python/py-micmd.c
+++ b/gdb/python/py-micmd.c
@@ -134,9 +134,8 @@ struct mi_command_py : public mi_command
     m_pyobj = new_pyobj;
   }
 
-protected:
   /* Called when the MI command is invoked.  */
-  virtual void do_invoke(struct mi_parse *parse) const override;
+  virtual void invoke(struct mi_parse *parse) const override;
 
 private:
   /* The Python object representing this MI command.  */
@@ -311,7 +310,7 @@ serialize_mi_result (PyObject *result)
    command line arguments from the user.  */
 
 void
-mi_command_py::do_invoke (struct mi_parse *parse) const
+mi_command_py::invoke (struct mi_parse *parse) const
 {
   PYMICMD_SCOPED_DEBUG_ENTER_EXIT;
 
diff --git a/gdb/stack.c b/gdb/stack.c
index 8855ed8fe70..10da88b88e5 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -1841,14 +1841,6 @@ select_frame_command_core (struct frame_info *fi, bool ignored)
     gdb::observers::user_selected_context_changed.notify (USER_SELECTED_FRAME);
 }
 
-/* See stack.h.  */
-
-void
-select_frame_for_mi (struct frame_info *fi)
-{
-  select_frame_command_core (fi, false /* Ignored.  */);
-}
-
 /* The core of all the "frame" sub-commands.  Select frame FI, and if this
    means we change frame send out a change notification (otherwise, just
    reprint the current frame summary).   */
diff --git a/gdb/stack.h b/gdb/stack.h
index c49d2e2cbef..f78aedf1c85 100644
--- a/gdb/stack.h
+++ b/gdb/stack.h
@@ -20,12 +20,6 @@
 #ifndef STACK_H
 #define STACK_H
 
-/* Access method used by the MI -stack-select-frame command to switch to
-   frame FI.  This differs from SELECT_FRAME in that the observers for a
-   user selected context change will be triggered.  */
-
-void select_frame_for_mi (struct frame_info *fi);
-
 gdb::unique_xmalloc_ptr<char> find_frame_funname (struct frame_info *frame,
 						  enum language *funlang,
 						  struct symbol **funcp);
diff --git a/gdb/testsuite/gdb.mi/user-selected-context-sync.exp b/gdb/testsuite/gdb.mi/user-selected-context-sync.exp
index 3b4ff0bf2e4..9444ca5acf4 100644
--- a/gdb/testsuite/gdb.mi/user-selected-context-sync.exp
+++ b/gdb/testsuite/gdb.mi/user-selected-context-sync.exp
@@ -916,7 +916,7 @@ proc_with_prefix test_mi_thread_select { mode } {
 	}
     }
 
-    with_test_prefix "thread 1.2 with --thread" {
+    with_test_prefix "thread 1.2 with --thread 2" {
 	# Test selecting a thread from MI with a --thread option.  This test
 	# verifies that even if the thread GDB would switch to is the same as
 	# the thread specified with --thread, an event is still sent to CLI.
@@ -930,10 +930,28 @@ proc_with_prefix test_mi_thread_select { mode } {
 	}
 
 	with_spawn_id $gdb_main_spawn_id {
-	    # This doesn't work as of now, no event is sent on CLI.  It is
-	    # commented out so we don't have to wait for the timeout every time.
-	    # match_re_or_ensure_no_output "$cli_re\r\n" "-thread-select, event on cli"
-	    kfail "gdb/20631" "thread-select, event on cli"
+	    match_re_or_ensure_no_output "$cli_re\r\n" "-thread-select, event on cli"
+	}
+    }
+
+    with_test_prefix "thread 1.2 with --thread 3" {
+	# Test selecting a thread from MI with a --thread option.
+	# This test verifies that when different thread numbers are
+	# passed to the --thread option and the underlying
+	# -thread-select command, the correct thread is selected.
+	# In this case this is thread 1.2
+
+	reset_selection "1.1"
+
+	set mi_re [make_mi_re $mode 2 0 response]
+	set cli_re [make_cli_re $mode -1 1.2 0]
+
+	with_spawn_id $mi_spawn_id {
+	    mi_gdb_test "-thread-select --thread 3 2" $mi_re "-thread-select"
+	}
+
+	with_spawn_id $gdb_main_spawn_id {
+	    match_re_or_ensure_no_output "$cli_re\r\n" "-thread-select, event on cli"
 	}
     }
 
@@ -974,6 +992,36 @@ proc_with_prefix test_mi_stack_select_frame { mode } {
 	with_spawn_id $gdb_main_spawn_id {
 	    match_re_or_ensure_no_output $cli_re "-stack-select-frame again, event on CLI"
 	}
+
+	# Now use the '-stack-select-frame' command with the --frame
+	# option, this verifies that even when the frame GDB would
+	# swith to is the same as the frame specified with --frame, an
+	# event is still sent to the CLI.
+
+	set cli_re [make_cli_re $mode -1 -1 0]
+
+	with_spawn_id $mi_spawn_id {
+	    mi_gdb_test "-stack-select-frame --thread 2 --frame 0 0" $mi_re
+	}
+
+	with_spawn_id $gdb_main_spawn_id {
+	    match_re_or_ensure_no_output "$cli_re\r\n" "-stack-select-frame with --frame 0, event on CLI"
+	}
+
+	# Now use the '-stack-select-frame' command with the --frame
+	# option, this verifies that the correct event is sent to the
+	# CLI when the frame specified with --frame is different to
+	# the actual frame selected.
+
+	set cli_re [make_cli_re $mode -1 -1 1]
+
+	with_spawn_id $mi_spawn_id {
+	    mi_gdb_test "-stack-select-frame --thread 2 --frame 2 1" $mi_re
+	}
+
+	with_spawn_id $gdb_main_spawn_id {
+	    match_re_or_ensure_no_output "$cli_re\r\n" "-stack-select-frame with --frame 2, event on CLI"
+	}
     }
 
     with_test_prefix "thread 1.3" {
-- 
2.35.1

Re: [PATCH v2] gdb/mi: consistently notify user when GDB/MI client uses -thread-select

[Andrew Burgess]

Jan Vrany <jan.vrany@labware.com> writes:

> Changes since v1:
>
> * all improvements and tests by Andrew - thanks a lot!
> * merged mi_command::invoke() and mi_command::do_invoke() into
> single method (mi_command::invoke()). See the commit message
> for rationale.
>
> -- >8 --
> GDB notifies users about user selected thread changes somewhat
> inconsistently as mentioned on gdb-patches mailing list here:
>
>   https://sourceware.org/pipermail/gdb-patches/2022-February/185989.html
>
> Consider GDB debugging a multi-threaded inferior with both CLI and GDB/MI
> interfaces connected to separate terminals.
>
> Assuming inferior is stopped and thread 1 is selected, when a thread
> 2 is selected using '-thread-select 2' command on GDB/MI terminal:
>
>     -thread-select 2
>     ^done,new-thread-id="2",frame={level="0",addr="0x00005555555551cd",func="child_sub_function",args=[],file="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.mi/user-selected-context-sync.c",fullname="/home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c",line="30",arch="i386:x86-64"}
>     (gdb)
>
> and on CLI terminal we get the notification (as expected):
>
>     [Switching to thread 2 (Thread 0x7ffff7daa640 (LWP 389659))]
>     #0  child_sub_function () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:30
>     30        volatile int dummy = 0;
>
> However, now that thread 2 is selected, if thread 1 is selected
> using 'thread-select --thread 1 1' command on GDB/MI terminal
> terminal:
>
>    -thread-select --thread 1 1
>    ^done,new-thread-id="1",frame={level="0",addr="0x0000555555555294",func="main",args=[],file="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.mi/user-selected-context-sync.c",fullname="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.mi/user-selected-context-sync.c",line="66",arch="i386:x86-64"}
>    (gdb)
>
> but no notification is printed on CLI terminal, despite the fact
> that user selected thread has changed.
>
> The problem is that when `-thread-select --thread 1 1` is executed
> then thread is switched to thread 1 before mi_cmd_thread_select () is
> called, therefore the condition "inferior_ptid != previous_ptid"
> there does not hold.
>
> To address this problem, we have to move notification logic up to
> mi_cmd_execute () where --thread option is processed and notify
> user selected contents observers there if context changes.
>
> However, this in itself breaks GDB/MI because it would cause context
> notification to be sent on MI channel. This is because by the time
> we notify, MI notification suppression is already restored (done in
> mi_command::invoke(). Therefore we had to lift notification suppression
> logic also up to mi_cmd_execute (). This change in made distinction
> between mi_command::invoke() and mi_command::do_invoke() unnecessary
> as all mi_command::invoke() did (after the change) was to call
> do_invoke(). So this patches removes do_invoke() and moves the command
> execution logic directly to invoke().
>
> With this change, all gdb.mi tests pass, tested on x86_64-linux.
>
> Co-authored-by: Andrew Burgess <aburgess@redhat.com>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=20631
> ---
>  gdb/mi/mi-cmd-stack.c                         | 12 +---
>  gdb/mi/mi-cmds.c                              | 18 +-----
>  gdb/mi/mi-cmds.h                              | 17 ++----
>  gdb/mi/mi-main.c                              | 51 +++++++++++++---
>  gdb/python/py-micmd.c                         |  5 +-
>  gdb/stack.c                                   |  8 ---
>  gdb/stack.h                                   |  6 --
>  .../gdb.mi/user-selected-context-sync.exp     | 58 +++++++++++++++++--
>  8 files changed, 106 insertions(+), 69 deletions(-)

Thanks this looks great.  Please go ahead and push this.

Thanks,
Andrew

>
> diff --git a/gdb/mi/mi-cmd-stack.c b/gdb/mi/mi-cmd-stack.c
> index 1be8aa81c3d..e894411765a 100644
> --- a/gdb/mi/mi-cmd-stack.c
> +++ b/gdb/mi/mi-cmd-stack.c
> @@ -757,17 +757,7 @@ mi_cmd_stack_select_frame (const char *command, char **argv, int argc)
>  {
>    if (argc == 0 || argc > 1)
>      error (_("-stack-select-frame: Usage: FRAME_SPEC"));
> -
> -  ptid_t previous_ptid = inferior_ptid;
> -
> -  select_frame_for_mi (parse_frame_specification (argv[0]));
> -
> -  /* Notify if the thread has effectively changed.  */
> -  if (inferior_ptid != previous_ptid)
> -    {
> -      gdb::observers::user_selected_context_changed.notify
> -	(USER_SELECTED_THREAD | USER_SELECTED_FRAME);
> -    }
> +  select_frame (parse_frame_specification (argv[0]));
>  }
>  
>  void
> diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
> index 38fbe0d8a32..60fec0a0b85 100644
> --- a/gdb/mi/mi-cmds.c
> +++ b/gdb/mi/mi-cmds.c
> @@ -45,11 +45,9 @@ struct mi_command_mi : public mi_command
>      gdb_assert (func != nullptr);
>    }
>  
> -protected:
> -
>    /* Called when this MI command has been invoked, calls m_argv_function
>       with arguments contained within PARSE.  */
> -  void do_invoke (struct mi_parse *parse) const override
> +  void invoke (struct mi_parse *parse) const override
>    {
>      mi_parse_argv (parse->args, parse);
>  
> @@ -83,13 +81,11 @@ struct mi_command_cli : public mi_command
>        m_args_p (args_p)
>    { /* Nothing.  */ }
>  
> -protected:
> -
>    /* Called when this MI command has been invoked, calls the m_cli_name
>       CLI function.  In m_args_p is true then the argument string from
>       within PARSE is passed through to the CLI function, otherwise nullptr
>       is passed through to the CLI function as its argument string.  */
> -  void do_invoke (struct mi_parse *parse) const override
> +  void invoke (struct mi_parse *parse) const override
>    {
>      const char *args = m_args_p ? parse->args : nullptr;
>      mi_execute_cli_command (m_cli_name, m_args_p, args);
> @@ -173,16 +169,6 @@ mi_command::mi_command (const char *name, int *suppress_notification)
>  
>  /* See mi-cmds.h.  */
>  
> -void
> -mi_command::invoke (struct mi_parse *parse) const
> -{
> -  gdb::optional<scoped_restore_tmpl<int>> restore
> -    = do_suppress_notification ();
> -  this->do_invoke (parse);
> -}
> -
> -/* See mi-cmds.h.  */
> -
>  gdb::optional<scoped_restore_tmpl<int>>
>  mi_command::do_suppress_notification () const
>  {
> diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
> index 47b90a26064..05b702f83ec 100644
> --- a/gdb/mi/mi-cmds.h
> +++ b/gdb/mi/mi-cmds.h
> @@ -160,9 +160,10 @@ struct mi_command
>    const char *name () const
>    { return m_name; }
>  
> -  /* Execute the MI command.  Can throw an exception if something goes
> -     wrong.  */
> -  void invoke (struct mi_parse *parse) const;
> +  /* Execute the MI command.  this needs to be overridden in each
> +     base class.  PARSE is the parsed command line from the user.
> +     Can throw an exception if something goes wrong.  */
> +  virtual void invoke (struct mi_parse *parse) const = 0;
>  
>    /* Return whether this command preserves user selected context (thread
>       and frame).  */
> @@ -175,14 +176,6 @@ struct mi_command
>      return m_suppress_notification != &mi_suppress_notification.user_selected_context;
>    }
>  
> -protected:
> -
> -  /* The core of command invocation, this needs to be overridden in each
> -     base class.  PARSE is the parsed command line from the user.  */
> -  virtual void do_invoke (struct mi_parse *parse) const = 0;
> -
> -private:
> -
>    /* If this command was created with a suppress notifications pointer,
>       then this function will set the suppress flag and return a
>       gdb::optional with its value set to an object that will restore the
> @@ -192,6 +185,8 @@ struct mi_command
>       then this function returns an empty gdb::optional.  */
>    gdb::optional<scoped_restore_tmpl<int>> do_suppress_notification () const;
>  
> +private:
> +
>    /* The name of the command.  */
>    const char *m_name;
>  
> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
> index 73380f5e668..abd033b22ae 100644
> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c
> @@ -556,19 +556,10 @@ mi_cmd_thread_select (const char *command, char **argv, int argc)
>    if (thr == NULL)
>      error (_("Thread ID %d not known."), num);
>  
> -  ptid_t previous_ptid = inferior_ptid;
> -
>    thread_select (argv[0], thr);
>  
>    print_selected_thread_frame (current_uiout,
>  			       USER_SELECTED_THREAD | USER_SELECTED_FRAME);
> -
> -  /* Notify if the thread has effectively changed.  */
> -  if (inferior_ptid != previous_ptid)
> -    {
> -      gdb::observers::user_selected_context_changed.notify
> -	(USER_SELECTED_THREAD | USER_SELECTED_FRAME);
> -    }
>  }
>  
>  void
> @@ -1975,6 +1966,37 @@ mi_execute_command (const char *cmd, int from_tty)
>      }
>  }
>  
> +/* Captures the current user selected context state, that is the current
> +   thread and frame.  Later we can then check if the user selected context
> +   has changed at all.  */
> +
> +struct user_selected_context
> +{
> +  /* Constructor.  */
> +  user_selected_context ()
> +    : m_previous_ptid (inferior_ptid),
> +      m_previous_frame (deprecated_safe_get_selected_frame ())
> +  { /* Nothing.  */ }
> +
> +  /* Return true if the user selected context has changed since this object
> +     was created.  */
> +  bool has_changed () const
> +  {
> +    return ((m_previous_ptid != null_ptid
> +	     && inferior_ptid != null_ptid
> +	     && m_previous_ptid != inferior_ptid)
> +	    || m_previous_frame != deprecated_safe_get_selected_frame ());
> +  }
> +private:
> +  /* The previously selected thread.  This might be null_ptid if there was
> +     no previously selected thread.  */
> +  ptid_t m_previous_ptid;
> +
> +  /* The previously selected frame.  This might be nullptr if there was no
> +     previously selected frame.  */
> +  frame_info *m_previous_frame;
> +};
> +
>  static void
>  mi_cmd_execute (struct mi_parse *parse)
>  {
> @@ -2015,6 +2037,8 @@ mi_cmd_execute (struct mi_parse *parse)
>        set_current_program_space (inf->pspace);
>      }
>  
> +  user_selected_context current_user_selected_context;
> +
>    gdb::optional<scoped_restore_current_thread> thread_saver;
>    if (parse->thread != -1)
>      {
> @@ -2060,7 +2084,16 @@ mi_cmd_execute (struct mi_parse *parse)
>    current_context = parse;
>  
>    gdb_assert (parse->cmd != nullptr);
> +
> +  gdb::optional<scoped_restore_tmpl<int>> restore_suppress_notification
> +    = parse->cmd->do_suppress_notification ();
> +
>    parse->cmd->invoke (parse);
> +
> +  if (!parse->cmd->preserve_user_selected_context ()
> +      && current_user_selected_context.has_changed ())
> +    gdb::observers::user_selected_context_changed.notify
> +      (USER_SELECTED_THREAD | USER_SELECTED_FRAME);
>  }
>  
>  /* See mi-main.h.  */
> diff --git a/gdb/python/py-micmd.c b/gdb/python/py-micmd.c
> index 4665fcc75c7..399e89b0298 100644
> --- a/gdb/python/py-micmd.c
> +++ b/gdb/python/py-micmd.c
> @@ -134,9 +134,8 @@ struct mi_command_py : public mi_command
>      m_pyobj = new_pyobj;
>    }
>  
> -protected:
>    /* Called when the MI command is invoked.  */
> -  virtual void do_invoke(struct mi_parse *parse) const override;
> +  virtual void invoke(struct mi_parse *parse) const override;
>  
>  private:
>    /* The Python object representing this MI command.  */
> @@ -311,7 +310,7 @@ serialize_mi_result (PyObject *result)
>     command line arguments from the user.  */
>  
>  void
> -mi_command_py::do_invoke (struct mi_parse *parse) const
> +mi_command_py::invoke (struct mi_parse *parse) const
>  {
>    PYMICMD_SCOPED_DEBUG_ENTER_EXIT;
>  
> diff --git a/gdb/stack.c b/gdb/stack.c
> index 8855ed8fe70..10da88b88e5 100644
> --- a/gdb/stack.c
> +++ b/gdb/stack.c
> @@ -1841,14 +1841,6 @@ select_frame_command_core (struct frame_info *fi, bool ignored)
>      gdb::observers::user_selected_context_changed.notify (USER_SELECTED_FRAME);
>  }
>  
> -/* See stack.h.  */
> -
> -void
> -select_frame_for_mi (struct frame_info *fi)
> -{
> -  select_frame_command_core (fi, false /* Ignored.  */);
> -}
> -
>  /* The core of all the "frame" sub-commands.  Select frame FI, and if this
>     means we change frame send out a change notification (otherwise, just
>     reprint the current frame summary).   */
> diff --git a/gdb/stack.h b/gdb/stack.h
> index c49d2e2cbef..f78aedf1c85 100644
> --- a/gdb/stack.h
> +++ b/gdb/stack.h
> @@ -20,12 +20,6 @@
>  #ifndef STACK_H
>  #define STACK_H
>  
> -/* Access method used by the MI -stack-select-frame command to switch to
> -   frame FI.  This differs from SELECT_FRAME in that the observers for a
> -   user selected context change will be triggered.  */
> -
> -void select_frame_for_mi (struct frame_info *fi);
> -
>  gdb::unique_xmalloc_ptr<char> find_frame_funname (struct frame_info *frame,
>  						  enum language *funlang,
>  						  struct symbol **funcp);
> diff --git a/gdb/testsuite/gdb.mi/user-selected-context-sync.exp b/gdb/testsuite/gdb.mi/user-selected-context-sync.exp
> index 3b4ff0bf2e4..9444ca5acf4 100644
> --- a/gdb/testsuite/gdb.mi/user-selected-context-sync.exp
> +++ b/gdb/testsuite/gdb.mi/user-selected-context-sync.exp
> @@ -916,7 +916,7 @@ proc_with_prefix test_mi_thread_select { mode } {
>  	}
>      }
>  
> -    with_test_prefix "thread 1.2 with --thread" {
> +    with_test_prefix "thread 1.2 with --thread 2" {
>  	# Test selecting a thread from MI with a --thread option.  This test
>  	# verifies that even if the thread GDB would switch to is the same as
>  	# the thread specified with --thread, an event is still sent to CLI.
> @@ -930,10 +930,28 @@ proc_with_prefix test_mi_thread_select { mode } {
>  	}
>  
>  	with_spawn_id $gdb_main_spawn_id {
> -	    # This doesn't work as of now, no event is sent on CLI.  It is
> -	    # commented out so we don't have to wait for the timeout every time.
> -	    # match_re_or_ensure_no_output "$cli_re\r\n" "-thread-select, event on cli"
> -	    kfail "gdb/20631" "thread-select, event on cli"
> +	    match_re_or_ensure_no_output "$cli_re\r\n" "-thread-select, event on cli"
> +	}
> +    }
> +
> +    with_test_prefix "thread 1.2 with --thread 3" {
> +	# Test selecting a thread from MI with a --thread option.
> +	# This test verifies that when different thread numbers are
> +	# passed to the --thread option and the underlying
> +	# -thread-select command, the correct thread is selected.
> +	# In this case this is thread 1.2
> +
> +	reset_selection "1.1"
> +
> +	set mi_re [make_mi_re $mode 2 0 response]
> +	set cli_re [make_cli_re $mode -1 1.2 0]
> +
> +	with_spawn_id $mi_spawn_id {
> +	    mi_gdb_test "-thread-select --thread 3 2" $mi_re "-thread-select"
> +	}
> +
> +	with_spawn_id $gdb_main_spawn_id {
> +	    match_re_or_ensure_no_output "$cli_re\r\n" "-thread-select, event on cli"
>  	}
>      }
>  
> @@ -974,6 +992,36 @@ proc_with_prefix test_mi_stack_select_frame { mode } {
>  	with_spawn_id $gdb_main_spawn_id {
>  	    match_re_or_ensure_no_output $cli_re "-stack-select-frame again, event on CLI"
>  	}
> +
> +	# Now use the '-stack-select-frame' command with the --frame
> +	# option, this verifies that even when the frame GDB would
> +	# swith to is the same as the frame specified with --frame, an
> +	# event is still sent to the CLI.
> +
> +	set cli_re [make_cli_re $mode -1 -1 0]
> +
> +	with_spawn_id $mi_spawn_id {
> +	    mi_gdb_test "-stack-select-frame --thread 2 --frame 0 0" $mi_re
> +	}
> +
> +	with_spawn_id $gdb_main_spawn_id {
> +	    match_re_or_ensure_no_output "$cli_re\r\n" "-stack-select-frame with --frame 0, event on CLI"
> +	}
> +
> +	# Now use the '-stack-select-frame' command with the --frame
> +	# option, this verifies that the correct event is sent to the
> +	# CLI when the frame specified with --frame is different to
> +	# the actual frame selected.
> +
> +	set cli_re [make_cli_re $mode -1 -1 1]
> +
> +	with_spawn_id $mi_spawn_id {
> +	    mi_gdb_test "-stack-select-frame --thread 2 --frame 2 1" $mi_re
> +	}
> +
> +	with_spawn_id $gdb_main_spawn_id {
> +	    match_re_or_ensure_no_output "$cli_re\r\n" "-stack-select-frame with --frame 2, event on CLI"
> +	}
>      }
>  
>      with_test_prefix "thread 1.3" {
> -- 
> 2.35.1