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

[Andrew Burgess <aburgess@redhat.com>]

* 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" {