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

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

[Jan Vrany]

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: [pushed] gdb/mi: consistently notify user when GDB/MI client uses -thread-select

[Simon Marchi]

On 2022-03-16 11:09, Jan Vrany via Gdb-patches wrote:
> 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

Hi,

I see this on Ubuntu 20.04, am I the only one?

FAIL: gdb.mi/user-selected-context-sync.exp: mode=all-stop: test_mi_thread_select: thread 1.2: -thread-select again, event on CLI, ensure no output CLI
FAIL: gdb.mi/user-selected-context-sync.exp: mode=all-stop: test_mi_thread_select: thread 1.3: -thread-select again, event on CLI, ensure no output CLI
FAIL: gdb.mi/user-selected-context-sync.exp: mode=non-stop: test_mi_thread_select: thread 1.2: -thread-select again, event on CLI, ensure no output CLI

Just the first one here:

-thread-select 2
^done,new-thread-id="2",frame={level="0",addr="0x00005555555551b8",func="child_sub_function",args=[],file="/home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.mi/user-selected-context-
sync.c",fullname="/home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c",line="33",arch="i386:x86-64"}
(gdb)
PASS: gdb.mi/user-selected-context-sync.exp: mode=all-stop: test_mi_thread_select: thread 1.2: -thread-select again
[Switching to thread 1.2 (Thread 0x7ffff7d99700 (LWP 1763981))]
#0  child_sub_function () at /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:33
33          dummy = !dummy; /* thread loop line */
print 666
$9 = 666
(gdb) FAIL: gdb.mi/user-selected-context-sync.exp: mode=all-stop: test_mi_thread_select: thread 1.2: -thread-select again, event on CLI, ensure no output CLI

Simon

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

[Jan Vrany]

On Mon, 2022-03-21 at 16:35 -0400, Simon Marchi wrote:
> On 2022-03-16 11:09, Jan Vrany via Gdb-patches wrote:
> > GDB notifies users about user selected thread changes somewhat
> > inconsistently as mentioned on gdb-patches mailing list here:
> > 
> >   https://urldefense.proofpoint.com/v2/url?u=https-3A__sourceware.org_pipermail_gdb-2Dpatches_2022-2DFebruary_185989.html&d=DwICaQ&c=sPZ6DeHLiehUHQWKIrsNwWp3t7snrE-az24ztT0w7Jc&r=WpFFGgYa98Yp-c29WHTCwU1wAGFBvszA6a4RzgpMSqc&m=qn1ryn0GnzccCli0TKe7kc6WpMiz1opcXfyT-fLWkIk&s=XoLasKCD9agRgS73PoGDnzPlKbo4_lL616HCUvldty0&e= 
> > 
> > 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://urldefense.proofpoint.com/v2/url?u=https-3A__sourceware.org_bugzilla_show-5Fbug.cgi-3Fid-3D20631&d=DwICaQ&c=sPZ6DeHLiehUHQWKIrsNwWp3t7snrE-az24ztT0w7Jc&r=WpFFGgYa98Yp-c29WHTCwU1wAGFBvszA6a4RzgpMSqc&m=qn1ryn0GnzccCli0TKe7kc6WpMiz1opcXfyT-fLWkIk&s=_9JOeNLk-p5CQtAtasdJbRyV5KK5tq1qLb_IxeKdmMw&e= 
> 
> Hi,
> 
> I see this on Ubuntu 20.04, am I the only one?

Weird. I just compiled GDB commit f55649cc and see no such failures:

> make check RUNTESTFLAGS='TRANSCRIPT=y  gdb.mi/user-select*.exp'
...
Running /home/jv/Projects/gdb/origin_master/gdb/testsuite/gdb.mi/user-selected-context-sync.exp ...

		=== gdb Summary ===

# of expected passes		419

I'll try on Ubuntu 20.04 tomorrow. 

Jan

> 
> FAIL: gdb.mi/user-selected-context-sync.exp: mode=all-stop: test_mi_thread_select: thread 1.2: -thread-select again, event on CLI, ensure no output CLI
> FAIL: gdb.mi/user-selected-context-sync.exp: mode=all-stop: test_mi_thread_select: thread 1.3: -thread-select again, event on CLI, ensure no output CLI
> FAIL: gdb.mi/user-selected-context-sync.exp: mode=non-stop: test_mi_thread_select: thread 1.2: -thread-select again, event on CLI, ensure no output CLI
> 
> Just the first one here:
> 
> -thread-select 2
> ^done,new-thread-id="2",frame={level="0",addr="0x00005555555551b8",func="child_sub_function",args=[],file="/home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.mi/user-selected-context-
> sync.c",fullname="/home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c",line="33",arch="i386:x86-64"}
> (gdb)
> PASS: gdb.mi/user-selected-context-sync.exp: mode=all-stop: test_mi_thread_select: thread 1.2: -thread-select again
> [Switching to thread 1.2 (Thread 0x7ffff7d99700 (LWP 1763981))]
> #0  child_sub_function () at /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:33
> 33          dummy = !dummy; /* thread loop line */
> print 666
> $9 = 666
> (gdb) FAIL: gdb.mi/user-selected-context-sync.exp: mode=all-stop: test_mi_thread_select: thread 1.2: -thread-select again, event on CLI, ensure no output CLI
> 
> Simon

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

[Jan Vrany]

On Mon, 2022-03-21 at 21:01 +0000, Jan Vrany wrote:
> On Mon, 2022-03-21 at 16:35 -0400, Simon Marchi wrote:
> > On 2022-03-16 11:09, Jan Vrany via Gdb-patches wrote:
> > > GDB notifies users about user selected thread changes somewhat
> > > inconsistently as mentioned on gdb-patches mailing list here:
> > > 
> > >   https://urldefense.proofpoint.com/v2/url?u=https-3A__sourceware.org_pipermail_gdb-2Dpatches_2022-2DFebruary_185989.html&d=DwICaQ&c=sPZ6DeHLiehUHQWKIrsNwWp3t7snrE-az24ztT0w7Jc&r=WpFFGgYa98Yp-c29WHTCwU1wAGFBvszA6a4RzgpMSqc&m=qn1ryn0GnzccCli0TKe7kc6WpMiz1opcXfyT-fLWkIk&s=XoLasKCD9agRgS73PoGDnzPlKbo4_lL616HCUvldty0&e= 
> > > 
> > > 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://urldefense.proofpoint.com/v2/url?u=https-3A__sourceware.org_bugzilla_show-5Fbug.cgi-3Fid-3D20631&d=DwICaQ&c=sPZ6DeHLiehUHQWKIrsNwWp3t7snrE-az24ztT0w7Jc&r=WpFFGgYa98Yp-c29WHTCwU1wAGFBvszA6a4RzgpMSqc&m=qn1ryn0GnzccCli0TKe7kc6WpMiz1opcXfyT-fLWkIk&s=_9JOeNLk-p5CQtAtasdJbRyV5KK5tq1qLb_IxeKdmMw&e= 
> > 
> > Hi,
> > 
> > I see this on Ubuntu 20.04, am I the only one?
> 
> Weird. I just compiled GDB commit f55649cc and see no such failures:
> 
> > make check RUNTESTFLAGS='TRANSCRIPT=y  gdb.mi/user-select*.exp'
> ...
> Running /home/jv/Projects/gdb/origin_master/gdb/testsuite/gdb.mi/user-selected-context-sync.exp ...
> 
> 		=== gdb Summary ===
> 
> # of expected passes		419
> 
> I'll try on Ubuntu 20.04 tomorrow. 
> 

Just tried on Ubuntu 20.04 and I do not any problem. 

Jan

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

[Simon Marchi]

> Just tried on Ubuntu 20.04 and I do not any problem. 

I'm trying to look a bit more into it.  The state is that we have thread
2 already selected, issue a `-thread-select 2` on the MI UI, and expect
to not have a CLI notification to say that the thread changed, since we
try to select the already selected thread.

Before your patch I get:

    -thread-select 2

    ^done,new-thread-id="2",frame={level="0",addr="0x00005555555551b8",func="child_sub_function",args=[],file="/home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c",fullname="/home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c",line="33",arch="i386:x86-64"}

    (gdb) 

    PASS: gdb.mi/user-selected-context-sync.exp: mode=all-stop: test_mi_thread_select: thread 1.2: -thread-select again
    print 666

    $9 = 666

    (gdb) PASS: gdb.mi/user-selected-context-sync.exp: mode=all-stop: test_mi_thread_select: thread 1.2: -thread-select again, event on CLI, ensure no output CLI

After:

    -thread-select 2

    ^done,new-thread-id="2",frame={level="0",addr="0x00005555555551b8",func="child_sub_function",args=[],file="/home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c",fullname="/home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c",line="33",arch="i386:x86-64"}

    (gdb) 

    PASS: gdb.mi/user-selected-context-sync.exp: mode=all-stop: test_mi_thread_select: thread 1.2: -thread-select again
    [Switching to thread 1.2 (Thread 0x7ffff7d99700 (LWP 1885257))]

    #0  child_sub_function () at /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:33

    33	    dummy = !dummy; /* thread loop line */

    print 666

    $9 = 666

    (gdb) FAIL: gdb.mi/user-selected-context-sync.exp: mode=all-stop: test_mi_thread_select: thread 1.2: -thread-select again, event on CLI, ensure no output CLI

So there is really a spurious "Switching to thread 1.2" notification in
my case.

Trying a simpler case, just an empty main program, I do this (/dev/pts/8
being the tty for my MI UI):

    $ ./gdb -q -nx --data-directory=data-directory a.out -ex "new-ui mi /dev/pts/8" -ex start

If I then type `-thread-select 1` on the MI UI, I do get:

    [Switching to thread 1 (process 1891548)]

... which is unexpected.  Before your patch, I don't get it.  Can you
give this small test a try?

Simon

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

[Jan Vrany]

Hi,

On Wed, 2022-03-23 at 08:36 -0400, Simon Marchi wrote:
> > Just tried on Ubuntu 20.04 and I do not any problem. 
> 
> I'm trying to look a bit more into it.  The state is that we have thread
> 2 already selected, issue a `-thread-select 2` on the MI UI, and expect
> to not have a CLI notification to say that the thread changed, since we
> try to select the already selected thread.
> 
> Before your patch I get:
> 
>     -thread-select 2
> 
>     ^done,new-thread-id="2",frame={level="0",addr="0x00005555555551b8",func="child_sub_function",args=[],file="/home/smarchi/src/binutils-
> gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c",fullname="/home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.mi/user-selected-context-
> sync.c",line="33",arch="i386:x86-64"}
> 
>     (gdb) 
> 
>     PASS: gdb.mi/user-selected-context-sync.exp: mode=all-stop: test_mi_thread_select: thread 1.2: -thread-select again
>     print 666
> 
>     $9 = 666
> 
>     (gdb) PASS: gdb.mi/user-selected-context-sync.exp: mode=all-stop: test_mi_thread_select: thread 1.2: -thread-select again, event on CLI, ensure no output
> CLI
> 
> After:
> 
>     -thread-select 2
> 
>     ^done,new-thread-id="2",frame={level="0",addr="0x00005555555551b8",func="child_sub_function",args=[],file="/home/smarchi/src/binutils-
> gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c",fullname="/home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.mi/user-selected-context-
> sync.c",line="33",arch="i386:x86-64"}
> 
>     (gdb) 
> 
>     PASS: gdb.mi/user-selected-context-sync.exp: mode=all-stop: test_mi_thread_select: thread 1.2: -thread-select again
>     [Switching to thread 1.2 (Thread 0x7ffff7d99700 (LWP 1885257))]
> 
>     #0  child_sub_function () at /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:33
> 
>     33	    dummy = !dummy; /* thread loop line */
> 
>     print 666
> 
>     $9 = 666
> 
>     (gdb) FAIL: gdb.mi/user-selected-context-sync.exp: mode=all-stop: test_mi_thread_select: thread 1.2: -thread-select again, event on CLI, ensure no output
> CLI
> 
> So there is really a spurious "Switching to thread 1.2" notification in
> my case.
> 
> Trying a simpler case, just an empty main program, I do this (/dev/pts/8
> being the tty for my MI UI):
> 
>     $ ./gdb -q -nx --data-directory=data-directory a.out -ex "new-ui mi /dev/pts/8" -ex start
> 
> If I then type `-thread-select 1` on the MI UI, I do get:
> 
>     [Switching to thread 1 (process 1891548)]
> 
> ... which is unexpected.  Before your patch, I don't get it.  Can you
> give this small test a try?
> 

I gave it a go but I'm sorry: everything seem to work as expected 
here. I do not get "[Switching to thread 1 (process 1891548)]" message
on CLI when -thread-select already selected thread. 

I must be doing something differently. What commit is failing for you?
What's your ./configure incantation?

I'm happy to sit down with you and have a closer look at this, maybe using 
IRC or something. 

Jan




> Simon

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

[Simon Marchi]

> I gave it a go but I'm sorry: everything seem to work as expected 
> here. I do not get "[Switching to thread 1 (process 1891548)]" message
> on CLI when -thread-select already selected thread. 

Ok, strange.

> I must be doing something differently. What commit is failing for you?
> What's your ./configure incantation?

My configure arguments are:

'CC=ccache gcc-11' 'CXX=ccache g++-11' '--disable-binutils' '--disable-gold' '--disable-ld' '--disable-gprof' '--disable-gas' '--with-guile' 'CFLAGS=-g3 -O0 -fdiagnostics-color=always -fmax-errors=1 -fsanitize=address' 'CXXFLAGS=-std=c++11 -g3 -O0 -fdiagnostics-color=always -fmax-errors=1 -fsanitize=address -D_GLIBCXX_DEBUG=1 -D_GLIBCXX_DEBUG_PEDANTIC=1 -D_GLIBCXX_SANITIZE_VECTOR=1' 'LDFLAGS=-fuse-ld=lld -fsanitize=address' '--prefix=/usr' '--enable-ubsan' '--with-python=python3' '--with-debuginfod' '--enable-silent-rules' '--enable-werror'


> I'm happy to sit down with you and have a closer look at this, maybe using 
> IRC or something. 

I'll try to dig deeper on my side first, thanks.

Simon

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

[Jan Vrany]

On Wed, 2022-03-23 at 11:16 -0400, Simon Marchi wrote:
> > I gave it a go but I'm sorry: everything seem to work as expected 
> > here. I do not get "[Switching to thread 1 (process 1891548)]" message
> > on CLI when -thread-select already selected thread. 
> 
> Ok, strange.
> 
> > I must be doing something differently. What commit is failing for you?
> > What's your ./configure incantation?
> 
> My configure arguments are:
> 
> 'CC=ccache gcc-11' 'CXX=ccache g++-11' '--disable-binutils' '--disable-gold' '--disable-ld' '--disable-gprof' '--disable-gas' '--with-guile' 'CFLAGS=-g3 -O0
> -fdiagnostics-color=always -fmax-errors=1 -fsanitize=address' 'CXXFLAGS=-std=c++11 -g3 -O0 -fdiagnostics-color=always -fmax-errors=1 -fsanitize=address -
> D_GLIBCXX_DEBUG=1 -D_GLIBCXX_DEBUG_PEDANTIC=1 -D_GLIBCXX_SANITIZE_VECTOR=1' 'LDFLAGS=-fuse-ld=lld -fsanitize=address' '--prefix=/usr' '--enable-ubsan' '--
> with-python=python3' '--with-debuginfod' '--enable-silent-rules' '--enable-werror'
> 

Thanks! Now it is failing for me too! 
I just recompiled GDB using the above command. I'll have a 
look tomorrow. 

Jan

> 
> > I'm happy to sit down with you and have a closer look at this, maybe using 
> > IRC or something. 
> 
> I'll try to dig deeper on my side first, thanks.
> 
> Simon

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

[Andrew Burgess]

Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes:

> On Wed, 2022-03-23 at 11:16 -0400, Simon Marchi wrote:
>> > I gave it a go but I'm sorry: everything seem to work as expected 
>> > here. I do not get "[Switching to thread 1 (process 1891548)]" message
>> > on CLI when -thread-select already selected thread. 
>> 
>> Ok, strange.
>> 
>> > I must be doing something differently. What commit is failing for you?
>> > What's your ./configure incantation?
>> 
>> My configure arguments are:
>> 
>> 'CC=ccache gcc-11' 'CXX=ccache g++-11' '--disable-binutils' '--disable-gold' '--disable-ld' '--disable-gprof' '--disable-gas' '--with-guile' 'CFLAGS=-g3 -O0
>> -fdiagnostics-color=always -fmax-errors=1 -fsanitize=address' 'CXXFLAGS=-std=c++11 -g3 -O0 -fdiagnostics-color=always -fmax-errors=1 -fsanitize=address -
>> D_GLIBCXX_DEBUG=1 -D_GLIBCXX_DEBUG_PEDANTIC=1 -D_GLIBCXX_SANITIZE_VECTOR=1' 'LDFLAGS=-fuse-ld=lld -fsanitize=address' '--prefix=/usr' '--enable-ubsan' '--
>> with-python=python3' '--with-debuginfod' '--enable-silent-rules' '--enable-werror'
>> 
>
> Thanks! Now it is failing for me too! 
> I just recompiled GDB using the above command. I'll have a 
> look tomorrow. 

These failures are totally my fault.  Sorry!

The patch below should fix the problem.  Let me know what you think.

Thanks,
Andrew

---

commit fcac0d17e64a18e0600372c2bed51f9e2d9e9d6a
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Wed Mar 23 19:00:35 2022 +0000

    gdb/mi: fix use after free of frame_info causing spurious notifications
    
    In commit:
    
      commit a2757c4ed693cef4ecc4dcdcb2518353eb6b3c3f
      Date:   Wed Mar 16 15:08:22 2022 +0000
    
          gdb/mi: consistently notify user when GDB/MI client uses -thread-select
    
    Changes were made to GDB to address some inconsistencies in when
    notifications are sent from a MI terminal to a CLI terminal (when
    multiple terminals are in use, see new-ui command).
    
    Unfortunately, in order to track when the currently selected frame has
    changed, that commit grabs a frame_info pointer before and after an MI
    command has executed, and compares the pointers to see if the frame
    has changed.
    
    This is not safe.
    
    If the frame cache is deleted for any reason then the frame_info
    pointer captured before the command started, is no longer valid, and
    any comparisons based on that pointer are undefined.
    
    This was leading to random test failures for some folk, see:
    
      https://sourceware.org/pipermail/gdb-patches/2022-March/186867.html
    
    This commit changes GDB so we no longer hold frame_info pointers, but
    instead store the frame_id and frame_level, this is safe even when the
    frame cache is flushed.

diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index abd033b22ae..4f32bb58939 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -1974,27 +1974,40 @@ struct user_selected_context
 {
   /* Constructor.  */
   user_selected_context ()
-    : m_previous_ptid (inferior_ptid),
-      m_previous_frame (deprecated_safe_get_selected_frame ())
-  { /* Nothing.  */ }
+    : m_previous_ptid (inferior_ptid)
+  {
+    save_selected_frame (&m_previous_frame_id, &m_previous_frame_level);
+  }
 
   /* Return true if the user selected context has changed since this object
      was created.  */
   bool has_changed () const
   {
+    /* Grab details of the currently selected frame, for comparison.  */
+    frame_id current_frame_id;
+    int current_frame_level;
+    save_selected_frame (&current_frame_id, &current_frame_level);
+
+    /* If we end up trying to compare two invalid frame-id's then these
+       will always report themselves as not equal.  However, if we are at
+       the top-most level of the stack then we don't want to consider this
+       as a frame change.  */
     return ((m_previous_ptid != null_ptid
 	     && inferior_ptid != null_ptid
 	     && m_previous_ptid != inferior_ptid)
-	    || m_previous_frame != deprecated_safe_get_selected_frame ());
+	    || current_frame_level != m_previous_frame_level
+	    || (current_frame_level != -1
+		&& !frame_id_eq (current_frame_id, m_previous_frame_id)));
   }
 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;
+  /* The previously selected frame.  The frame_id might be null_frame_id
+     if no frame is currently selected.  */
+  frame_id m_previous_frame_id;
+  int m_previous_frame_level;
 };
 
 static void

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

[Andrew Burgess]

Andrew Burgess <aburgess@redhat.com> writes:

> Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes:
>
>> On Wed, 2022-03-23 at 11:16 -0400, Simon Marchi wrote:
>>> > I gave it a go but I'm sorry: everything seem to work as expected 
>>> > here. I do not get "[Switching to thread 1 (process 1891548)]" message
>>> > on CLI when -thread-select already selected thread. 
>>> 
>>> Ok, strange.
>>> 
>>> > I must be doing something differently. What commit is failing for you?
>>> > What's your ./configure incantation?
>>> 
>>> My configure arguments are:
>>> 
>>> 'CC=ccache gcc-11' 'CXX=ccache g++-11' '--disable-binutils' '--disable-gold' '--disable-ld' '--disable-gprof' '--disable-gas' '--with-guile' 'CFLAGS=-g3 -O0
>>> -fdiagnostics-color=always -fmax-errors=1 -fsanitize=address' 'CXXFLAGS=-std=c++11 -g3 -O0 -fdiagnostics-color=always -fmax-errors=1 -fsanitize=address -
>>> D_GLIBCXX_DEBUG=1 -D_GLIBCXX_DEBUG_PEDANTIC=1 -D_GLIBCXX_SANITIZE_VECTOR=1' 'LDFLAGS=-fuse-ld=lld -fsanitize=address' '--prefix=/usr' '--enable-ubsan' '--
>>> with-python=python3' '--with-debuginfod' '--enable-silent-rules' '--enable-werror'
>>> 
>>
>> Thanks! Now it is failing for me too! 
>> I just recompiled GDB using the above command. I'll have a 
>> look tomorrow. 
>
> These failures are totally my fault.  Sorry!
>
> The patch below should fix the problem.  Let me know what you think.

Also, this patch should definitely be back ported to the GDB 12 branch.

Thanks,
Andrew

>
> Thanks,
> Andrew
>
> ---
>
> commit fcac0d17e64a18e0600372c2bed51f9e2d9e9d6a
> Author: Andrew Burgess <aburgess@redhat.com>
> Date:   Wed Mar 23 19:00:35 2022 +0000
>
>     gdb/mi: fix use after free of frame_info causing spurious notifications
>     
>     In commit:
>     
>       commit a2757c4ed693cef4ecc4dcdcb2518353eb6b3c3f
>       Date:   Wed Mar 16 15:08:22 2022 +0000
>     
>           gdb/mi: consistently notify user when GDB/MI client uses -thread-select
>     
>     Changes were made to GDB to address some inconsistencies in when
>     notifications are sent from a MI terminal to a CLI terminal (when
>     multiple terminals are in use, see new-ui command).
>     
>     Unfortunately, in order to track when the currently selected frame has
>     changed, that commit grabs a frame_info pointer before and after an MI
>     command has executed, and compares the pointers to see if the frame
>     has changed.
>     
>     This is not safe.
>     
>     If the frame cache is deleted for any reason then the frame_info
>     pointer captured before the command started, is no longer valid, and
>     any comparisons based on that pointer are undefined.
>     
>     This was leading to random test failures for some folk, see:
>     
>       https://sourceware.org/pipermail/gdb-patches/2022-March/186867.html
>     
>     This commit changes GDB so we no longer hold frame_info pointers, but
>     instead store the frame_id and frame_level, this is safe even when the
>     frame cache is flushed.
>
> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
> index abd033b22ae..4f32bb58939 100644
> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c
> @@ -1974,27 +1974,40 @@ struct user_selected_context
>  {
>    /* Constructor.  */
>    user_selected_context ()
> -    : m_previous_ptid (inferior_ptid),
> -      m_previous_frame (deprecated_safe_get_selected_frame ())
> -  { /* Nothing.  */ }
> +    : m_previous_ptid (inferior_ptid)
> +  {
> +    save_selected_frame (&m_previous_frame_id, &m_previous_frame_level);
> +  }
>  
>    /* Return true if the user selected context has changed since this object
>       was created.  */
>    bool has_changed () const
>    {
> +    /* Grab details of the currently selected frame, for comparison.  */
> +    frame_id current_frame_id;
> +    int current_frame_level;
> +    save_selected_frame (&current_frame_id, &current_frame_level);
> +
> +    /* If we end up trying to compare two invalid frame-id's then these
> +       will always report themselves as not equal.  However, if we are at
> +       the top-most level of the stack then we don't want to consider this
> +       as a frame change.  */
>      return ((m_previous_ptid != null_ptid
>  	     && inferior_ptid != null_ptid
>  	     && m_previous_ptid != inferior_ptid)
> -	    || m_previous_frame != deprecated_safe_get_selected_frame ());
> +	    || current_frame_level != m_previous_frame_level
> +	    || (current_frame_level != -1
> +		&& !frame_id_eq (current_frame_id, m_previous_frame_id)));
>    }
>  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;
> +  /* The previously selected frame.  The frame_id might be null_frame_id
> +     if no frame is currently selected.  */
> +  frame_id m_previous_frame_id;
> +  int m_previous_frame_level;
>  };
>  
>  static void

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

[Jan Vrany]

On Wed, 2022-03-23 at 19:09 +0000, Andrew Burgess wrote:
> Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> > On Wed, 2022-03-23 at 11:16 -0400, Simon Marchi wrote:
> > > > I gave it a go but I'm sorry: everything seem to work as expected 
> > > > here. I do not get "[Switching to thread 1 (process 1891548)]" message
> > > > on CLI when -thread-select already selected thread. 
> > > 
> > > Ok, strange.
> > > 
> > > > I must be doing something differently. What commit is failing for you?
> > > > What's your ./configure incantation?
> > > 
> > > My configure arguments are:
> > > 
> > > 'CC=ccache gcc-11' 'CXX=ccache g++-11' '--disable-binutils' '--disable-gold' '--disable-ld' '--disable-gprof' '--disable-gas' '--with-guile' 'CFLAGS=-g3 -O0
> > > -fdiagnostics-color=always -fmax-errors=1 -fsanitize=address' 'CXXFLAGS=-std=c++11 -g3 -O0 -fdiagnostics-color=always -fmax-errors=1 -fsanitize=address -
> > > D_GLIBCXX_DEBUG=1 -D_GLIBCXX_DEBUG_PEDANTIC=1 -D_GLIBCXX_SANITIZE_VECTOR=1' 'LDFLAGS=-fuse-ld=lld -fsanitize=address' '--prefix=/usr' '--enable-ubsan' '--
> > > with-python=python3' '--with-debuginfod' '--enable-silent-rules' '--enable-werror'
> > > 
> > 
> > Thanks! Now it is failing for me too! 
> > I just recompiled GDB using the above command. I'll have a 
> > look tomorrow. 
> 
> These failures are totally my fault.  Sorry!
> 
> The patch below should fix the problem.  Let me know what you think.

I wrote patch pretty much the same as yours when I noticed your email!

"deprecated_SAFE_get_selected_frame()" turned out not to be as safe
as we both thought it is :-)

Thanks a lot!
 
Jan


> 
> Thanks,
> Andrew
> 
> ---
> 
> commit fcac0d17e64a18e0600372c2bed51f9e2d9e9d6a
> Author: Andrew Burgess <aburgess@redhat.com>
> Date:   Wed Mar 23 19:00:35 2022 +0000
> 
>     gdb/mi: fix use after free of frame_info causing spurious notifications
>     
>     In commit:
>     
>       commit a2757c4ed693cef4ecc4dcdcb2518353eb6b3c3f
>       Date:   Wed Mar 16 15:08:22 2022 +0000
>     
>           gdb/mi: consistently notify user when GDB/MI client uses -thread-select
>     
>     Changes were made to GDB to address some inconsistencies in when
>     notifications are sent from a MI terminal to a CLI terminal (when
>     multiple terminals are in use, see new-ui command).
>     
>     Unfortunately, in order to track when the currently selected frame has
>     changed, that commit grabs a frame_info pointer before and after an MI
>     command has executed, and compares the pointers to see if the frame
>     has changed.
>     
>     This is not safe.
>     
>     If the frame cache is deleted for any reason then the frame_info
>     pointer captured before the command started, is no longer valid, and
>     any comparisons based on that pointer are undefined.
>     
>     This was leading to random test failures for some folk, see:
>     
>       https://urldefense.proofpoint.com/v2/url?u=https-3A__sourceware.org_pipermail_gdb-2Dpatches_2022-2DMarch_186867.html&d=DwIFaQ&c=sPZ6DeHLiehUHQWKIrsNwWp3t7snrE-az24ztT0w7Jc&r=WpFFGgYa98Yp-c29WHTCwU1wAGFBvszA6a4RzgpMSqc&m=fuC61Jpzxz_BRT-kfZ5C3exaG0I5TSRJt2JTG-bPdV8&s=yf5HLpnQ2WSg3e1_HMVDT5XNWSyoRYDpTsxso1TMQOE&e= 
>     
>     This commit changes GDB so we no longer hold frame_info pointers, but
>     instead store the frame_id and frame_level, this is safe even when the
>     frame cache is flushed.
> 
> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
> index abd033b22ae..4f32bb58939 100644
> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c
> @@ -1974,27 +1974,40 @@ struct user_selected_context
>  {
>    /* Constructor.  */
>    user_selected_context ()
> -    : m_previous_ptid (inferior_ptid),
> -      m_previous_frame (deprecated_safe_get_selected_frame ())
> -  { /* Nothing.  */ }
> +    : m_previous_ptid (inferior_ptid)
> +  {
> +    save_selected_frame (&m_previous_frame_id, &m_previous_frame_level);
> +  }
>  
>    /* Return true if the user selected context has changed since this object
>       was created.  */
>    bool has_changed () const
>    {
> +    /* Grab details of the currently selected frame, for comparison.  */
> +    frame_id current_frame_id;
> +    int current_frame_level;
> +    save_selected_frame (&current_frame_id, &current_frame_level);
> +
> +    /* If we end up trying to compare two invalid frame-id's then these
> +       will always report themselves as not equal.  However, if we are at
> +       the top-most level of the stack then we don't want to consider this
> +       as a frame change.  */
>      return ((m_previous_ptid != null_ptid
>  	     && inferior_ptid != null_ptid
>  	     && m_previous_ptid != inferior_ptid)
> -	    || m_previous_frame != deprecated_safe_get_selected_frame ());
> +	    || current_frame_level != m_previous_frame_level
> +	    || (current_frame_level != -1
> +		&& !frame_id_eq (current_frame_id, m_previous_frame_id)));
>    }
>  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;
> +  /* The previously selected frame.  The frame_id might be null_frame_id
> +     if no frame is currently selected.  */
> +  frame_id m_previous_frame_id;
> +  int m_previous_frame_level;
>  };
>  
>  static void
> 

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

[Simon Marchi]

On 2022-03-23 15:09, Andrew Burgess wrote:
> Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes:
>
>> On Wed, 2022-03-23 at 11:16 -0400, Simon Marchi wrote:
>>>> I gave it a go but I'm sorry: everything seem to work as expected
>>>> here. I do not get "[Switching to thread 1 (process 1891548)]" message
>>>> on CLI when -thread-select already selected thread.
>>>
>>> Ok, strange.
>>>
>>>> I must be doing something differently. What commit is failing for you?
>>>> What's your ./configure incantation?
>>>
>>> My configure arguments are:
>>>
>>> 'CC=ccache gcc-11' 'CXX=ccache g++-11' '--disable-binutils' '--disable-gold' '--disable-ld' '--disable-gprof' '--disable-gas' '--with-guile' 'CFLAGS=-g3 -O0
>>> -fdiagnostics-color=always -fmax-errors=1 -fsanitize=address' 'CXXFLAGS=-std=c++11 -g3 -O0 -fdiagnostics-color=always -fmax-errors=1 -fsanitize=address -
>>> D_GLIBCXX_DEBUG=1 -D_GLIBCXX_DEBUG_PEDANTIC=1 -D_GLIBCXX_SANITIZE_VECTOR=1' 'LDFLAGS=-fuse-ld=lld -fsanitize=address' '--prefix=/usr' '--enable-ubsan' '--
>>> with-python=python3' '--with-debuginfod' '--enable-silent-rules' '--enable-werror'
>>>
>>
>> Thanks! Now it is failing for me too!
>> I just recompiled GDB using the above command. I'll have a
>> look tomorrow.
>
> These failures are totally my fault.  Sorry!
>
> The patch below should fix the problem.  Let me know what you think.
>
> Thanks,
> Andrew
>
> ---
>
> commit fcac0d17e64a18e0600372c2bed51f9e2d9e9d6a
> Author: Andrew Burgess <aburgess@redhat.com>
> Date:   Wed Mar 23 19:00:35 2022 +0000
>
>     gdb/mi: fix use after free of frame_info causing spurious notifications
>
>     In commit:
>
>       commit a2757c4ed693cef4ecc4dcdcb2518353eb6b3c3f
>       Date:   Wed Mar 16 15:08:22 2022 +0000
>
>           gdb/mi: consistently notify user when GDB/MI client uses -thread-select
>
>     Changes were made to GDB to address some inconsistencies in when
>     notifications are sent from a MI terminal to a CLI terminal (when
>     multiple terminals are in use, see new-ui command).
>
>     Unfortunately, in order to track when the currently selected frame has
>     changed, that commit grabs a frame_info pointer before and after an MI
>     command has executed, and compares the pointers to see if the frame
>     has changed.
>
>     This is not safe.
>
>     If the frame cache is deleted for any reason then the frame_info
>     pointer captured before the command started, is no longer valid, and
>     any comparisons based on that pointer are undefined.
>
>     This was leading to random test failures for some folk, see:
>
>       https://sourceware.org/pipermail/gdb-patches/2022-March/186867.html
>
>     This commit changes GDB so we no longer hold frame_info pointers, but
>     instead store the frame_id and frame_level, this is safe even when the
>     frame cache is flushed.
>
> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
> index abd033b22ae..4f32bb58939 100644
> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c
> @@ -1974,27 +1974,40 @@ struct user_selected_context
>  {
>    /* Constructor.  */
>    user_selected_context ()
> -    : m_previous_ptid (inferior_ptid),
> -      m_previous_frame (deprecated_safe_get_selected_frame ())
> -  { /* Nothing.  */ }
> +    : m_previous_ptid (inferior_ptid)
> +  {
> +    save_selected_frame (&m_previous_frame_id, &m_previous_frame_level);
> +  }
>
>    /* Return true if the user selected context has changed since this object
>       was created.  */
>    bool has_changed () const
>    {
> +    /* Grab details of the currently selected frame, for comparison.  */
> +    frame_id current_frame_id;
> +    int current_frame_level;
> +    save_selected_frame (&current_frame_id, &current_frame_level);
> +
> +    /* If we end up trying to compare two invalid frame-id's then these
> +       will always report themselves as not equal.  However, if we are at
> +       the top-most level of the stack then we don't want to consider this
> +       as a frame change.  */
>      return ((m_previous_ptid != null_ptid
>  	     && inferior_ptid != null_ptid
>  	     && m_previous_ptid != inferior_ptid)
> -	    || m_previous_frame != deprecated_safe_get_selected_frame ());
> +	    || current_frame_level != m_previous_frame_level
> +	    || (current_frame_level != -1
> +		&& !frame_id_eq (current_frame_id, m_previous_frame_id)));

That's a subjective request, but could you rewrite this as separate
statements, like this?  I find this easier to parse and debug than one
big statement.

  /* Did the selected thread change?  */
  if (m_previous_ptid != null_ptid && inferior_ptid != null_ptid
      && m_previous_ptid != inferior_ptid)
    return true;

  /* Did the selected frame level change?  */
  if (current_frame_level != m_previous_frame_level)
    return true;

  /* Did the selected frame id change?  */
  if (current_frame_level != -1
      && !frame_id_eq (current_frame_id, m_previous_frame_id))
    return true;

  return false;


>    }
>  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;
> +  /* The previously selected frame.  The frame_id might be null_frame_id
> +     if no frame is currently selected.  */

Maybe keep using the past tense?  "if no frame was selected"
(it implicitly says: "at the time of construction", although that could
be made explicit).

I would also use "is" instead of "might": if there was no selected frame
at the time of construction, then this is null_frame_id, there's no
doubt about it.

Simon

> +  frame_id m_previous_frame_id;
> +  int m_previous_frame_level;
>  };
>
>  static void
>

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

[Simon Marchi]

On 2022-03-23 12:09, Jan Vrany wrote:
> Thanks! Now it is failing for me too!
> I just recompiled GDB using the above command. I'll have a
> look tomorrow.
>
> Jan

Probably because of ASan?  I initially ruled that out, because it's not
an ASan crash.  But it's possible that it is impacting memory
allocation, such that the old and new frame_info objects are allocated
at different places.

Glad there was an actual bug and I was not crazy!

Simon

[PATCHv2] gdb/mi: fix use after free of frame_info causing spurious notifications

[Andrew Burgess]

Simon,

Thanks for the feedback.  I believe this addresses all your points.
What do you think?

Thanks,
Andrew

---

In commit:

  commit a2757c4ed693cef4ecc4dcdcb2518353eb6b3c3f
  Date:   Wed Mar 16 15:08:22 2022 +0000

      gdb/mi: consistently notify user when GDB/MI client uses -thread-select

Changes were made to GDB to address some inconsistencies in when
notifications are sent from a MI terminal to a CLI terminal (when
multiple terminals are in use, see new-ui command).

Unfortunately, in order to track when the currently selected frame has
changed, that commit grabs a frame_info pointer before and after an MI
command has executed, and compares the pointers to see if the frame
has changed.

This is not safe.

If the frame cache is deleted for any reason then the frame_info
pointer captured before the command started, is no longer valid, and
any comparisons based on that pointer are undefined.

This was leading to random test failures for some folk, see:

  https://sourceware.org/pipermail/gdb-patches/2022-March/186867.html

This commit changes GDB so we no longer hold frame_info pointers, but
instead store the frame_id and frame_level, this is safe even when the
frame cache is flushed.
---
 gdb/mi/mi-main.c | 43 +++++++++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index abd033b22ae..28d4afc28c0 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -1974,27 +1974,50 @@ struct user_selected_context
 {
   /* Constructor.  */
   user_selected_context ()
-    : m_previous_ptid (inferior_ptid),
-      m_previous_frame (deprecated_safe_get_selected_frame ())
-  { /* Nothing.  */ }
+    : m_previous_ptid (inferior_ptid)
+  {
+    save_selected_frame (&m_previous_frame_id, &m_previous_frame_level);
+  }
 
   /* 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 ());
+    /* Did the selected thread change?  */
+    if (m_previous_ptid != null_ptid && inferior_ptid != null_ptid
+	&& m_previous_ptid != inferior_ptid)
+      return true;
+
+    /* Grab details of the currently selected frame, for comparison.  */
+    frame_id current_frame_id;
+    int current_frame_level;
+    save_selected_frame (&current_frame_id, &current_frame_level);
+
+    /* Did the selected frame level change?  */
+    if (current_frame_level != m_previous_frame_level)
+      return true;
+
+    /* Did the selected frame id change?  If the innermost frame is
+       selected then the level will be -1, and the frame-id will be
+       null_frame_id.  As comparing null_frame_id with itself always
+       reports not-equal, we only do the equality test if we have something
+       other than the innermost frame selected.  */
+    if (current_frame_level != -1
+	&& !frame_id_eq (current_frame_id, m_previous_frame_id))
+      return true;
+
+    /* Nothing changed!  */
+    return false;
   }
 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;
+  /* The previously selected frame.  If the innermost frame is selected
+     then the frame_id will be null_frame_id, and the level will be -1.  */
+  frame_id m_previous_frame_id;
+  int m_previous_frame_level;
 };
 
 static void
-- 
2.25.4

Re: [PATCHv2] gdb/mi: fix use after free of frame_info causing spurious notifications

[Simon Marchi]

On 2022-03-24 14:52, Andrew Burgess via Gdb-patches wrote:
> Simon,
>
> Thanks for the feedback.  I believe this addresses all your points.
> What do you think?
>
> Thanks,
> Andrew
>
> ---
>
> In commit:
>
>   commit a2757c4ed693cef4ecc4dcdcb2518353eb6b3c3f
>   Date:   Wed Mar 16 15:08:22 2022 +0000
>
>       gdb/mi: consistently notify user when GDB/MI client uses -thread-select
>
> Changes were made to GDB to address some inconsistencies in when
> notifications are sent from a MI terminal to a CLI terminal (when
> multiple terminals are in use, see new-ui command).
>
> Unfortunately, in order to track when the currently selected frame has
> changed, that commit grabs a frame_info pointer before and after an MI
> command has executed, and compares the pointers to see if the frame
> has changed.
>
> This is not safe.
>
> If the frame cache is deleted for any reason then the frame_info
> pointer captured before the command started, is no longer valid, and
> any comparisons based on that pointer are undefined.
>
> This was leading to random test failures for some folk, see:
>
>   https://sourceware.org/pipermail/gdb-patches/2022-March/186867.html
>
> This commit changes GDB so we no longer hold frame_info pointers, but
> instead store the frame_id and frame_level, this is safe even when the
> frame cache is flushed.
> ---
>  gdb/mi/mi-main.c | 43 +++++++++++++++++++++++++++++++++----------
>  1 file changed, 33 insertions(+), 10 deletions(-)
>
> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
> index abd033b22ae..28d4afc28c0 100644
> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c
> @@ -1974,27 +1974,50 @@ struct user_selected_context
>  {
>    /* Constructor.  */
>    user_selected_context ()
> -    : m_previous_ptid (inferior_ptid),
> -      m_previous_frame (deprecated_safe_get_selected_frame ())
> -  { /* Nothing.  */ }
> +    : m_previous_ptid (inferior_ptid)
> +  {
> +    save_selected_frame (&m_previous_frame_id, &m_previous_frame_level);
> +  }
>
>    /* 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 ());
> +    /* Did the selected thread change?  */
> +    if (m_previous_ptid != null_ptid && inferior_ptid != null_ptid
> +	&& m_previous_ptid != inferior_ptid)
> +      return true;
> +
> +    /* Grab details of the currently selected frame, for comparison.  */
> +    frame_id current_frame_id;
> +    int current_frame_level;
> +    save_selected_frame (&current_frame_id, &current_frame_level);
> +
> +    /* Did the selected frame level change?  */
> +    if (current_frame_level != m_previous_frame_level)
> +      return true;
> +
> +    /* Did the selected frame id change?  If the innermost frame is
> +       selected then the level will be -1, and the frame-id will be
> +       null_frame_id.  As comparing null_frame_id with itself always
> +       reports not-equal, we only do the equality test if we have something
> +       other than the innermost frame selected.  */
> +    if (current_frame_level != -1
> +	&& !frame_id_eq (current_frame_id, m_previous_frame_id))
> +      return true;
> +
> +    /* Nothing changed!  */
> +    return false;
>    }
>  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;
> +  /* The previously selected frame.  If the innermost frame is selected
> +     then the frame_id will be null_frame_id, and the level will be -1.  */

I noticed you changed this from "if no frame is currently selected" to
"if the innermost frame is selected".  I think that both can happen,
actually:

 - if you execute an MI command while the target is running, there is no
   selected frame
 - if you execute an MI command while the innermost frame is selected

So my understanding is that you could mention both (although it might
become a bit redundant with the big comment on top of selected_frame_id,
in frame.c).

LGTM in any case.

Simon

Re: [PATCHv2] gdb/mi: fix use after free of frame_info causing spurious notifications

[Andrew Burgess]

Simon Marchi <simark@simark.ca> writes:

> On 2022-03-24 14:52, Andrew Burgess via Gdb-patches wrote:
>> Simon,
>>
>> Thanks for the feedback.  I believe this addresses all your points.
>> What do you think?
>>
>> Thanks,
>> Andrew
>>
>> ---
>>
>> In commit:
>>
>>   commit a2757c4ed693cef4ecc4dcdcb2518353eb6b3c3f
>>   Date:   Wed Mar 16 15:08:22 2022 +0000
>>
>>       gdb/mi: consistently notify user when GDB/MI client uses -thread-select
>>
>> Changes were made to GDB to address some inconsistencies in when
>> notifications are sent from a MI terminal to a CLI terminal (when
>> multiple terminals are in use, see new-ui command).
>>
>> Unfortunately, in order to track when the currently selected frame has
>> changed, that commit grabs a frame_info pointer before and after an MI
>> command has executed, and compares the pointers to see if the frame
>> has changed.
>>
>> This is not safe.
>>
>> If the frame cache is deleted for any reason then the frame_info
>> pointer captured before the command started, is no longer valid, and
>> any comparisons based on that pointer are undefined.
>>
>> This was leading to random test failures for some folk, see:
>>
>>   https://sourceware.org/pipermail/gdb-patches/2022-March/186867.html
>>
>> This commit changes GDB so we no longer hold frame_info pointers, but
>> instead store the frame_id and frame_level, this is safe even when the
>> frame cache is flushed.
>> ---
>>  gdb/mi/mi-main.c | 43 +++++++++++++++++++++++++++++++++----------
>>  1 file changed, 33 insertions(+), 10 deletions(-)
>>
>> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
>> index abd033b22ae..28d4afc28c0 100644
>> --- a/gdb/mi/mi-main.c
>> +++ b/gdb/mi/mi-main.c
>> @@ -1974,27 +1974,50 @@ struct user_selected_context
>>  {
>>    /* Constructor.  */
>>    user_selected_context ()
>> -    : m_previous_ptid (inferior_ptid),
>> -      m_previous_frame (deprecated_safe_get_selected_frame ())
>> -  { /* Nothing.  */ }
>> +    : m_previous_ptid (inferior_ptid)
>> +  {
>> +    save_selected_frame (&m_previous_frame_id, &m_previous_frame_level);
>> +  }
>>
>>    /* 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 ());
>> +    /* Did the selected thread change?  */
>> +    if (m_previous_ptid != null_ptid && inferior_ptid != null_ptid
>> +	&& m_previous_ptid != inferior_ptid)
>> +      return true;
>> +
>> +    /* Grab details of the currently selected frame, for comparison.  */
>> +    frame_id current_frame_id;
>> +    int current_frame_level;
>> +    save_selected_frame (&current_frame_id, &current_frame_level);
>> +
>> +    /* Did the selected frame level change?  */
>> +    if (current_frame_level != m_previous_frame_level)
>> +      return true;
>> +
>> +    /* Did the selected frame id change?  If the innermost frame is
>> +       selected then the level will be -1, and the frame-id will be
>> +       null_frame_id.  As comparing null_frame_id with itself always
>> +       reports not-equal, we only do the equality test if we have something
>> +       other than the innermost frame selected.  */
>> +    if (current_frame_level != -1
>> +	&& !frame_id_eq (current_frame_id, m_previous_frame_id))
>> +      return true;
>> +
>> +    /* Nothing changed!  */
>> +    return false;
>>    }
>>  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;
>> +  /* The previously selected frame.  If the innermost frame is selected
>> +     then the frame_id will be null_frame_id, and the level will be -1.  */
>
> I noticed you changed this from "if no frame is currently selected" to
> "if the innermost frame is selected".  I think that both can happen,
> actually:
>
>  - if you execute an MI command while the target is running, there is no
>    selected frame
>  - if you execute an MI command while the innermost frame is selected
>
> So my understanding is that you could mention both (although it might
> become a bit redundant with the big comment on top of selected_frame_id,
> in frame.c).
>
> LGTM in any case.

I updated the comment as you suggested and pushed this patch to mater
and gdb-12-branch.

Thanks,
Andrew