public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [RFC PATCH] gdb/mi: consistently notify user when GDB/MI client uses -thread-select
@ 2022-03-14 11:57 Jan Vrany
  2022-03-15 17:10 ` Andrew Burgess
  0 siblings, 1 reply; 4+ messages in thread
From: Jan Vrany @ 2022-03-14 11:57 UTC (permalink / raw)
  To: gdb-patches; +Cc: Jan Vrany, aburgess

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


^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-03-16 14:35 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-14 11:57 [RFC PATCH] gdb/mi: consistently notify user when GDB/MI client uses -thread-select Jan Vrany
2022-03-15 17:10 ` Andrew Burgess
2022-03-16 14:03   ` [PATCH v2] " Jan Vrany
2022-03-16 14:35     ` Andrew Burgess

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).