From patchwork Tue May 2 20:49:41 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Marchi X-Patchwork-Id: 68648 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 7074C3857017 for ; Tue, 2 May 2023 20:51:06 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 7074C3857017 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1683060666; bh=ZGvodSLpL/eL20Fmh7Yq6pc+ki4nl897PvJX/3Xi22I=; h=To:Cc:Subject:Date:In-Reply-To:References:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=dKNc0sgA2x790I7P7r2C+8YYzllI/EVdaToQ/mmNmjSpfkZVt6EasbxOz27w7l+Hi 7oc45rCjikZX5hueDrlvTD6PkQykXC9diw8gQg3uhglTQNs+P+6n9NAoK5N3PGUMDx +Jcpr8X2SkiT6didg9bP4nf6aiA11fbJpML35Jto= X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 54E713858D20 for ; Tue, 2 May 2023 20:50:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 54E713858D20 Received: from smarchi-efficios.internal.efficios.com (192-222-143-198.qc.cable.ebox.net [192.222.143.198]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id D87351E114; Tue, 2 May 2023 16:50:13 -0400 (EDT) To: gdb-patches@sourceware.org Cc: Simon Marchi Subject: [PATCH 01/30] gdb/mi: fix ^running record with multiple MI interpreters Date: Tue, 2 May 2023 16:49:41 -0400 Message-Id: <20230502205011.132151-2-simon.marchi@efficios.com> X-Mailer: git-send-email 2.40.1 In-Reply-To: <20230502205011.132151-1-simon.marchi@efficios.com> References: <20230502205011.132151-1-simon.marchi@efficios.com> MIME-Version: 1.0 X-Spam-Status: No, score=-3497.3 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_NONE, KAM_DMARC_STATUS, KAM_SHORT, SPF_HELO_PASS, SPF_SOFTFAIL, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Simon Marchi via Gdb-patches From: Simon Marchi Reply-To: Simon Marchi Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" 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 Tested-By: Alexandra Petlanova Hajkova --- 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 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 (¤t_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 + +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 . + +# 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"