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

4 files changed, 51 insertions, 47 deletions

diff --git a/gdb/mi/mi-cmd-stack.c b/gdb/mi/mi-cmd-stack.c
index 1be8aa8..e894411 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 38fbe0d..60fec0a 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 47b90a2..05b702f 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 @@ private:
      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 73380f5..abd033b 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.  */