diff mbox series

[01/30] gdb/mi: fix ^running record with multiple MI interpreters

Message ID 20230502205011.132151-2-simon.marchi@efficios.com
State New
Headers
Series Switch interpreters to use virtual methods |

Commit Message

Simon Marchi May 2, 2023, 8:49 p.m. UTC 
  I stumbled on the mi_proceeded and running_result_record_printed
globals, which are shared by all MI interpreter instances (it's unlikely
that people use multiple MI interpreter instances, but it's possible).
After poking at it, I found this bug:

1. Start GDB in MI mode
2. Add a second MI interpreter with the new-ui command
3. Use -exec-run on the second interpreter

This is the output I get on the first interpreter:

    =thread-group-added,id="i1"
    ~"Reading symbols from a.out...\n"
    ~"New UI allocated\n"
    (gdb)
    =thread-group-started,id="i1",pid="94718"
    =thread-created,id="1",group-id="i1"
    ^running
    *running,thread-id="all"

And this is the output I get on the second intepreter:

    =thread-group-added,id="i1"
    (gdb)
    -exec-run
    =thread-group-started,id="i1",pid="94718"
    =thread-created,id="1",group-id="i1"
    *running,thread-id="all"

The problem here is that the `^running` reply to the -exec-run command
is printed on the wrong UI.  It is printed on the first one, it should
be printed on the second (the one on which we sent the -exec-run).

What happens under the hood is that captured_mi_execute_command, while
executing a command for the second intepreter, clears the
running_result_record_printed and mi_proceeded globals.
mi_about_to_proceed then sets mi_proceeded.  Then, mi_on_resume_1 gets
called for the first intepreter first.  Since the

    !running_result_record_printed && mi_proceeded

condition is true, it prints a ^running, and sets
running_result_record_printed.  When mi_on_resume_1 gets called for the
second interpreter, running_result_record_printed is already set, so
^running is not printed there.

It took me a while to understand the relationship between these two
variables.  I think that in the end, this is what we want to track:

 1. When executing an MI command, take note if that command causes a
    "proceed".  This is done in mi_about_to_proceed.
 2. In mi_on_resume_1, if the command indeed caused a "proceed", we want
    to output a ^running record.  And we want to remember that we did,
    because...
 3. Back in captured_mi_execute_command, if we did not output a
    ^running, we want to output a ^done.

Moving those two variables to the mi_interp struture appears to fix it.
Only for the interpreter doing the -exec-run command does the
running_result_record_printed flag get cleared, and therefore only or
that one does the ^running record get printed.

Add a new test for this, that does pretty much what the reproducer above
shows.  Without the fix, the test fails because
mi_send_resuming_command_raw never sees the ^running record.

Change-Id: I63ea30e6cb61a8e1dd5ef03377e6003381a9209b
---
 gdb/mi/mi-interp.c                           | 13 ++--
 gdb/mi/mi-interp.h                           |  6 ++
 gdb/mi/mi-main.c                             | 14 ++--
 gdb/mi/mi-main.h                             |  3 -
 gdb/testsuite/gdb.mi/run-with-two-mi-uis.c   |  7 ++
 gdb/testsuite/gdb.mi/run-with-two-mi-uis.exp | 67 ++++++++++++++++++++
 gdb/testsuite/lib/mi-support.exp             | 26 +++++---
 7 files changed, 111 insertions(+), 25 deletions(-)
 create mode 100644 gdb/testsuite/gdb.mi/run-with-two-mi-uis.c
 create mode 100644 gdb/testsuite/gdb.mi/run-with-two-mi-uis.exp

Comments

Alexandra Petlanova Hajkova May 3, 2023, 2:42 p.m. UTC | #1 
On Tue, May 2, 2023 at 10:50 PM Simon Marchi via Gdb-patches <
gdb-patches@sourceware.org> wrote:

> I stumbled on the mi_proceeded and running_result_record_printed
> globals, which are shared by all MI interpreter instances (it's unlikely
> that people use multiple MI interpreter instances, but it's possible).
> After poking at it, I found this bug:
>
> 1. Start GDB in MI mode
> 2. Add a second MI interpreter with the new-ui command
> 3. Use -exec-run on the second interpreter
>
> This is the output I get on the first interpreter:
>
>     =thread-group-added,id="i1"
>     ~"Reading symbols from a.out...\n"
>     ~"New UI allocated\n"
>     (gdb)
>     =thread-group-started,id="i1",pid="94718"
>     =thread-created,id="1",group-id="i1"
>     ^running
>     *running,thread-id="all"
>
> And this is the output I get on the second intepreter:
>
>     =thread-group-added,id="i1"
>     (gdb)
>     -exec-run
>     =thread-group-started,id="i1",pid="94718"
>     =thread-created,id="1",group-id="i1"
>     *running,thread-id="all"
>
> The problem here is that the `^running` reply to the -exec-run command
> is printed on the wrong UI.  It is printed on the first one, it should
> be printed on the second (the one on which we sent the -exec-run).
>
> What happens under the hood is that captured_mi_execute_command, while
> executing a command for the second intepreter, clears the
> running_result_record_printed and mi_proceeded globals.
> mi_about_to_proceed then sets mi_proceeded.  Then, mi_on_resume_1 gets
> called for the first intepreter first.  Since the
>
>     !running_result_record_printed && mi_proceeded
>
> condition is true, it prints a ^running, and sets
> running_result_record_printed.  When mi_on_resume_1 gets called for the
> second interpreter, running_result_record_printed is already set, so
> ^running is not printed there.
>
> It took me a while to understand the relationship between these two
> variables.  I think that in the end, this is what we want to track:
>
>  1. When executing an MI command, take note if that command causes a
>     "proceed".  This is done in mi_about_to_proceed.
>  2. In mi_on_resume_1, if the command indeed caused a "proceed", we want
>     to output a ^running record.  And we want to remember that we did,
>     because...
>  3. Back in captured_mi_execute_command, if we did not output a
>     ^running, we want to output a ^done.
>
> Moving those two variables to the mi_interp struture appears to fix it.
> Only for the interpreter doing the -exec-run command does the
> running_result_record_printed flag get cleared, and therefore only or
> that one does the ^running record get printed.
>
> Add a new test for this, that does pretty much what the reproducer above
> shows.  Without the fix, the test fails because
> mi_send_resuming_command_raw never sees the ^running record.
>
> Change-Id: I63ea30e6cb61a8e1dd5ef03377e6003381a9209b
>
>
I can confirm the new test gdb.mi/run-with-two-mi-uis.exp, passes on
ppc64le.
Simon Marchi May 3, 2023, 3:02 p.m. UTC | #2 
> I can confirm the new test gdb.mi/run-with-two-mi-uis.exp, passes on ppc64le.

Thanks for testing.  I will add:

Tested-By: Alexandra Petlanova Hajkova <ahajkova@redhat.com>

Simon
Simon Marchi May 29, 2023, 2:53 p.m. UTC | #3 
On 5/2/23 16:49, Simon Marchi via Gdb-patches wrote:
> I stumbled on the mi_proceeded and running_result_record_printed
> globals, which are shared by all MI interpreter instances (it's unlikely
> that people use multiple MI interpreter instances, but it's possible).
> After poking at it, I found this bug:
> 
> 1. Start GDB in MI mode
> 2. Add a second MI interpreter with the new-ui command
> 3. Use -exec-run on the second interpreter
> 
> This is the output I get on the first interpreter:
> 
>     =thread-group-added,id="i1"
>     ~"Reading symbols from a.out...\n"
>     ~"New UI allocated\n"
>     (gdb)
>     =thread-group-started,id="i1",pid="94718"
>     =thread-created,id="1",group-id="i1"
>     ^running
>     *running,thread-id="all"
> 
> And this is the output I get on the second intepreter:
> 
>     =thread-group-added,id="i1"
>     (gdb)
>     -exec-run
>     =thread-group-started,id="i1",pid="94718"
>     =thread-created,id="1",group-id="i1"
>     *running,thread-id="all"
> 
> The problem here is that the `^running` reply to the -exec-run command
> is printed on the wrong UI.  It is printed on the first one, it should
> be printed on the second (the one on which we sent the -exec-run).
> 
> What happens under the hood is that captured_mi_execute_command, while
> executing a command for the second intepreter, clears the
> running_result_record_printed and mi_proceeded globals.
> mi_about_to_proceed then sets mi_proceeded.  Then, mi_on_resume_1 gets
> called for the first intepreter first.  Since the
> 
>     !running_result_record_printed && mi_proceeded
> 
> condition is true, it prints a ^running, and sets
> running_result_record_printed.  When mi_on_resume_1 gets called for the
> second interpreter, running_result_record_printed is already set, so
> ^running is not printed there.
> 
> It took me a while to understand the relationship between these two
> variables.  I think that in the end, this is what we want to track:
> 
>  1. When executing an MI command, take note if that command causes a
>     "proceed".  This is done in mi_about_to_proceed.
>  2. In mi_on_resume_1, if the command indeed caused a "proceed", we want
>     to output a ^running record.  And we want to remember that we did,
>     because...
>  3. Back in captured_mi_execute_command, if we did not output a
>     ^running, we want to output a ^done.
> 
> Moving those two variables to the mi_interp struture appears to fix it.
> Only for the interpreter doing the -exec-run command does the
> running_result_record_printed flag get cleared, and therefore only or
> that one does the ^running record get printed.
> 
> Add a new test for this, that does pretty much what the reproducer above
> shows.  Without the fix, the test fails because
> mi_send_resuming_command_raw never sees the ^running record.
> 
> Change-Id: I63ea30e6cb61a8e1dd5ef03377e6003381a9209b

I pushed just this patch on its own.  The rest of the series conflicts
in non-trivial ways with new stuff in master.

Simon
diff mbox series

Patch

diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index ad33a21374ab..7d8dfd830a4f 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -676,7 +676,12 @@  mi_about_to_proceed (void)
 	return;
     }
 
-  mi_proceeded = 1;
+  mi_interp *mi = as_mi_interp (top_level_interpreter ());
+
+  if (mi == nullptr)
+    return;
+
+  mi->mi_proceeded = 1;
 }
 
 /* When the element is non-zero, no MI notifications will be emitted in
@@ -960,7 +965,7 @@  mi_on_resume_1 (struct mi_interp *mi,
      will make it impossible for frontend to know what's going on.
 
      In future (MI3), we'll be outputting "^done" here.  */
-  if (!running_result_record_printed && mi_proceeded)
+  if (!mi->running_result_record_printed && mi->mi_proceeded)
     {
       gdb_printf (mi->raw_stdout, "%s^running\n",
 		  current_token ? current_token : "");
@@ -976,9 +981,9 @@  mi_on_resume_1 (struct mi_interp *mi,
     for (thread_info *tp : all_non_exited_threads (targ, ptid))
       mi_output_running (tp);
 
-  if (!running_result_record_printed && mi_proceeded)
+  if (!mi->running_result_record_printed && mi->mi_proceeded)
     {
-      running_result_record_printed = 1;
+      mi->running_result_record_printed = 1;
       /* This is what gdb used to do historically -- printing prompt
 	 even if it cannot actually accept any input.  This will be
 	 surely removed for MI3, and may be removed even earlier.  */
diff --git a/gdb/mi/mi-interp.h b/gdb/mi/mi-interp.h
index e07be12f87ac..eb81cbe6cada 100644
--- a/gdb/mi/mi-interp.h
+++ b/gdb/mi/mi-interp.h
@@ -64,6 +64,12 @@  class mi_interp final : public interp
 
   /* MI's CLI builder (wraps OUT).  */
   struct ui_out *cli_uiout;
+
+  int running_result_record_printed = 1;
+
+  /* Flag indicating that the target has proceeded since the last
+     command was issued.  */
+  int mi_proceeded;
 };
 
 /* Output the shared object attributes to UIOUT.  */
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 35c74c407ee8..ab9184bc3585 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -84,12 +84,6 @@  char *current_token;
    command including all option, and make it possible.  */
 static struct mi_parse *current_context;
 
-int running_result_record_printed = 1;
-
-/* Flag indicating that the target has proceeded since the last
-   command was issued.  */
-int mi_proceeded;
-
 static void mi_cmd_execute (struct mi_parse *parse);
 
 static void mi_execute_async_cli_command (const char *cli_command,
@@ -1803,8 +1797,8 @@  captured_mi_execute_command (struct ui_out *uiout, struct mi_parse *context)
   scoped_restore save_token = make_scoped_restore (&current_token,
 						   context->token);
 
-  running_result_record_printed = 0;
-  mi_proceeded = 0;
+  mi->running_result_record_printed = 0;
+  mi->mi_proceeded = 0;
   switch (context->op)
     {
     case MI_COMMAND:
@@ -1822,7 +1816,7 @@  captured_mi_execute_command (struct ui_out *uiout, struct mi_parse *context)
 	 to directly use the mi_interp's uiout, since the command
 	 could have reset the interpreter, in which case the current
 	 uiout will most likely crash in the mi_out_* routines.  */
-      if (!running_result_record_printed)
+      if (!mi->running_result_record_printed)
 	{
 	  gdb_puts (context->token, mi->raw_stdout);
 	  /* There's no particularly good reason why target-connect results
@@ -1861,7 +1855,7 @@  captured_mi_execute_command (struct ui_out *uiout, struct mi_parse *context)
 	    || current_interp_named_p (INTERP_MI3)
 	    || current_interp_named_p (INTERP_MI4))
 	  {
-	    if (!running_result_record_printed)
+	    if (!mi->running_result_record_printed)
 	      {
 		gdb_puts (context->token, mi->raw_stdout);
 		gdb_puts ("^done", mi->raw_stdout);
diff --git a/gdb/mi/mi-main.h b/gdb/mi/mi-main.h
index ff1d4ed84bc8..b9fe295d3306 100644
--- a/gdb/mi/mi-main.h
+++ b/gdb/mi/mi-main.h
@@ -36,9 +36,6 @@  extern int mi_async_p (void);
 
 extern char *current_token;
 
-extern int running_result_record_printed;
-extern int mi_proceeded;
-
 struct mi_suppress_notification
 {
   /* Breakpoint notification suppressed?  */
diff --git a/gdb/testsuite/gdb.mi/run-with-two-mi-uis.c b/gdb/testsuite/gdb.mi/run-with-two-mi-uis.c
new file mode 100644
index 000000000000..1e2428e0d544
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/run-with-two-mi-uis.c
@@ -0,0 +1,7 @@ 
+#include <unistd.h>
+
+int
+main (void)
+{
+  sleep (1234);
+}
diff --git a/gdb/testsuite/gdb.mi/run-with-two-mi-uis.exp b/gdb/testsuite/gdb.mi/run-with-two-mi-uis.exp
new file mode 100644
index 000000000000..9a049c98ea7a
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/run-with-two-mi-uis.exp
@@ -0,0 +1,67 @@ 
+# Copyright 2023 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 doing an -exec-run while there are two MI UIs.
+
+load_lib mi-support.exp
+
+standard_testfile
+
+if {[build_executable $testfile.exp $testfile ${srcfile} "debug"] == -1} {
+    untested "failed to compile"
+    return
+}
+
+# Run one configuration of the test.
+#
+# UI_TO_RUN is the UI that should issue the run command.
+
+proc do_test { ui_to_run } {
+    if {[mi_clean_restart $::binfile "separate-mi-tty"] != 0} {
+	fail "could not start gdb"
+	return
+    }
+
+    with_spawn_id $::gdb_main_spawn_id {
+	lassign [create_mi_ui] second_mi_spawn_id second_mi_tty_name
+    }
+
+    with_spawn_id $second_mi_spawn_id {
+	gdb_expect {
+	    -re "=thread-group-added,id=\"i1\"\r\n$::mi_gdb_prompt$" {
+		pass "consume"
+	    }
+	}
+    }
+
+    if { $ui_to_run == "first" } {
+	set spawn_id_to_run $::mi_spawn_id
+    } elseif { $ui_to_run == "second" } {
+	set spawn_id_to_run $second_mi_spawn_id
+    } else {
+	error "invalid ui_to_run value"
+    }
+
+    with_spawn_id $spawn_id_to_run {
+	# mi_runto_main implicitly verifies that the UI doing the -exec-run gets
+	# the expected ^running record.
+	mi_runto_main
+    }
+}
+
+foreach_with_prefix ui_to_run {first second} {
+    do_test $ui_to_run
+}
+
diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
index 0d830d8e4ae5..2ff4ab93ea82 100644
--- a/gdb/testsuite/lib/mi-support.exp
+++ b/gdb/testsuite/lib/mi-support.exp
@@ -131,6 +131,21 @@  proc mi_create_inferior_pty {} {
     }
 }
 
+# Create a new pty, and reate a new MI UI (using the new-ui command) on it.
+#
+# Return a list with the spawn id for that pty and the pty file name.
+
+proc create_mi_ui {} {
+    spawn -pty
+    set tty_name $spawn_out(slave,name)
+    gdb_test_multiple "new-ui mi $tty_name" "new-ui" {
+	-re "New UI allocated\r\n$::gdb_prompt $" {
+	}
+    }
+
+    return [list $spawn_id $tty_name]
+}
+
 #
 # Like default_mi_gdb_start below, but the MI is created as a separate
 # ui in a new tty.  The global MI_SPAWN_ID is updated to point at the
@@ -154,13 +169,7 @@  proc mi_gdb_start_separate_mi_tty { { flags {} } } {
     gdb_start
 
     # Create the new PTY for the MI UI.
-    spawn -pty
-    set mi_spawn_id $spawn_id
-    set mi_tty_name $spawn_out(slave,name)
-    gdb_test_multiple "new-ui mi $mi_tty_name" "new-ui" {
-	-re "New UI allocated\r\n$gdb_prompt $" {
-	}
-    }
+    lassign [create_mi_ui] mi_spawn_id mi_tty_name
 
     # Switch to the MI channel.
     set gdb_main_spawn_id $gdb_spawn_id
@@ -822,7 +831,7 @@  proc mi_gdb_test { args } {
 	    fail "$errmsg"
 	    return -1
 	}
-	 -re ".*$mi_gdb_prompt\[ \]*$" {
+	 -re "(.*$mi_gdb_prompt\[ \]*)$" {
 	    if {![string match "" $message]} {
 		fail "$message (unexpected output)"
 	    }
@@ -1082,6 +1091,7 @@  proc mi_runto_helper {func run_or_continue args} {
       # file.", etc. to the CLI stream.
       set extra_output "&\"\[^\r\n\]+\"\r\n"
   }
+
   mi_gdb_test "200-break-insert [join $extra_opts " "] -t $func" "${extra_output}200\\^done,$bp" \
       "breakpoint at $func"