From 622e0ee9fd0d047111aad9724f864dd5df29992b Mon Sep 17 00:00:00 2001 From: Andrew Burgess Date: Wed, 16 Jul 2025 20:01:00 +0100 Subject: [PATCH] gdb: disable commit resumed in wait_for_inferior This patch proposes a fix for PR gdb/33147. The bug can be reproduced like this: gdb -q -ex 'file /bin/ls' \ -ex 'run &' \ -ex 'add-inferior' \ -ex 'infer 2' \ -ex 'set sysroot' \ -ex 'target remote | gdbserver - ls' Which will trigger an assertion failure: target.c:3760: internal-error: target_stop: Assertion `!proc_target->commit_resumed_state' failed. The problem is that target_stop is being called for a target when commit_resumed_state is true, the comment on process_stratum_target::commit_resumed_state is pretty clear: To simplify the implementation of targets, the following methods are guaranteed to be called with COMMIT_RESUMED_STATE set to false: - resume - stop - wait So clearly we're breaking a precondition of target_stop. In this example there are two target, the native target (inferior 1), and the remote target (inferior 2). It is the first, the native target, for which commit_resumed_state is set incorrectly. At the point target_stop is called looks like this: #11 0x00000000009a3c19 in target_stop (ptid=...) at ../../src/gdb/target.c:3760 #12 target_stop (ptid=...) at ../../src/gdb/target.c:3756 #13 0x00000000007042f2 in stop_all_threads (reason=, inf=) at ../../src/gdb/infrun.c:5739 #14 0x0000000000711d3a in wait_for_inferior (inf=0x2b90fd0) at ../../src/gdb/infrun.c:4412 #15 start_remote (from_tty=from_tty@entry=1) at ../../src/gdb/infrun.c:3829 #16 0x0000000000897014 in remote_target::start_remote_1 (this=this@entry=0x2c4a520, from_tty=from_tty@entry=1, extended_p=extended_p@entry=0) at ../../src/gdb/remote.c:5350 #17 0x00000000008976e7 in remote_target::start_remote (extended_p=0, from_tty=1, this=0x2c4a520) at ../../src/gdb/remote.c:5441 #18 remote_target::open_1 (name=, from_tty=1, extended_p=0) at ../../src/gdb/remote.c:6312 #19 0x00000000009a815f in open_target (args=0x7fffffffa93c "| gdbserver - ls", from_tty=1, command=) at ../../src/gdb/target.c:838 For new inferiors commit_resumed_state starts set to false, for this reason, if we only start a remote inferior, then when wait_for_inferior is called commit_resumed_state will be false, and everything will work. Further, as target_stop is only called for running threads, if, when the remote inferior is started, all other threads (in other targets) are already stopped, then GDB will never need to call target_stop for the other targets, and so GDB will not notice that commit_resumed_state for those target is set to true. In this case though, as the first (native) inferior is left running in the background while the remote inferior is created, and because GDB is running in all-stop mode (so needs to stop all threads in all targets), then GDB does call target_stop for the other targets, and so spots that commit_resumed_state is not set correctly and asserts. The fix is to add scoped_disable_commit_resumed somewhere in the call stack. Initially I planned to add the scoped_disable_commit_resumed in `wait_for_inferior`, however, this isn't good enough. This location would solve the problem as described in the bug, but when writing the test I extended the problem to also cover non-stop mode, and this runs into a second problem, the same assertion, but triggered from a different call path. For this new case the stack looks like this: #1 0x0000000000fb0e50 in target_stop (ptid=...) at ../../src/gdb/target.c:3771 #2 0x0000000000a7f0ae in stop_all_threads (reason=0x1d0ff74 "remote connect in all-stop", inf=0x0) at ../../src/gdb/infrun.c:5756 #3 0x0000000000d9c028 in remote_target::process_initial_stop_replies (this=0x3e10670, from_tty=1) at ../../src/gdb/remote.c:5017 #4 0x0000000000d9cdf0 in remote_target::start_remote_1 (this=0x3e10670, from_tty=1, extended_p=0) at ../../src/gdb/remote.c:5405 #5 0x0000000000d9d0d4 in remote_target::start_remote (this=0x3e10670, from_tty=1, extended_p=0) at ../../src/gdb/remote.c:5457 #6 0x0000000000d9e8ac in remote_target::open_1 (name=0x7fffffffa931 "| gdbserver - /bin/ls", from_tty=1, extended_p=0) at ../../src/gdb/remote.c:6329 #7 0x0000000000d9d167 in remote_target::open (name=0x7fffffffa931 "| gdbserver - /bin/ls", from_tty=1) at ../../src/gdb/remote.c:5479 #8 0x0000000000f9914d in open_target (args=0x7fffffffa931 "| gdbserver - /bin/ls", from_tty=1, command=0x35d1a40) at ../../src/gdb/target.c:838 So I'm now thinking that stop_all_threads would be the best place for the scoped_disable_commit_resumed. I did leave an assert in wait_for_inferior as, having thought about the assert some, I do still think the logic of it is true, and it doesn't hurt to leave it in place I think. However, it's not quite that simple, the test throws up yet another bug when we 'maint set target-non-stop on', but then 'set non-stop off'. This bug leaves a stopped thread marked as "(running)" in the 'info threads' output. I have a fix for this issue, but I'm leaving that for the next commit. For now I've just disabled part of the test in the problem case. I've also tagged this patch with PR gdb/27322. That bug was created before the above assert was added, but if you follow the steps to reproduce for that bug today you will hit the above assert. The actual issue described in PR gdb/27322 is fixed in the next patch. Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27322 Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33147 --- gdb/infrun.c | 7 + .../gdb.multi/remote-with-running-inferior.c | 38 ++++ .../remote-with-running-inferior.exp | 180 ++++++++++++++++++ 3 files changed, 225 insertions(+) create mode 100644 gdb/testsuite/gdb.multi/remote-with-running-inferior.c create mode 100644 gdb/testsuite/gdb.multi/remote-with-running-inferior.exp diff --git a/gdb/infrun.c b/gdb/infrun.c index eeef01ae66d..82e7284065b 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -4393,6 +4393,11 @@ wait_for_inferior (inferior *inf) scoped_finish_thread_state finish_state (inf->process_target (), minus_one_ptid); + /* The commit_resumed_state of INF should already be false at this point + as INF will be a newly started remote target. This might not be true + for other targets but this will be handled in stop_all_threads. */ + gdb_assert (!inf->process_target ()->commit_resumed_state); + while (1) { execution_control_state ecs; @@ -5699,6 +5704,8 @@ stop_all_threads (const char *reason, inferior *inf) debug_prefixed_printf ("infrun", "stop_all_threads", "done"); }; + scoped_disable_commit_resumed disable_commit_resumed ("stop all threads"); + /* Request threads to stop, and then wait for the stops. Because threads we already know about can spawn more threads while we're trying to stop them, and we only learn about new threads when we diff --git a/gdb/testsuite/gdb.multi/remote-with-running-inferior.c b/gdb/testsuite/gdb.multi/remote-with-running-inferior.c new file mode 100644 index 00000000000..a610edaf30f --- /dev/null +++ b/gdb/testsuite/gdb.multi/remote-with-running-inferior.c @@ -0,0 +1,38 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2025 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 . */ + +#include + +int global_var = 123; + +void +breakpt (void) +{ + /* Nothing. */ +} + +int +main (void) +{ + for (int i = 0; i < 30; ++i) + { + sleep (1); + breakpt (); + } + + return 0; +} diff --git a/gdb/testsuite/gdb.multi/remote-with-running-inferior.exp b/gdb/testsuite/gdb.multi/remote-with-running-inferior.exp new file mode 100644 index 00000000000..c08122379de --- /dev/null +++ b/gdb/testsuite/gdb.multi/remote-with-running-inferior.exp @@ -0,0 +1,180 @@ +# This testcase is part of GDB, the GNU debugger. +# +# Copyright 2025 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 . + +# Set an inferior running in the background (using "run&"), then +# connect to gdbserver in a second inferior. When this test was added +# there were two bugs in GDB. First, just connecting to gdbserver +# while an inferior was running on a different connection type +# (e.g. inferior 1 was native, and inferior 2 was gdbserver) would +# trigger an assertion. +# +# Then, once the assertion was fixed, GDB would stop the thread from +# the first inferior, but would fail to update it's state in GDB core, +# this would leave the thread stopped, but GDB core thinking the +# thread was running. Check this is fixed by looking at the 'info +# threads' output after connecting to the remote target. + +# This tests the use of native and remote targets, and also depends on use +# of the 'run' command. If we try to run it with a board that forces native +# targets to become remote, then this test isn't going to work, especially +# for 'remote' targets where 'run' is not supported. +require {string equal [target_info gdb_protocol] ""} + +load_lib gdbserver-support.exp + +require allow_gdbserver_tests + +standard_testfile + +if { [build_executable "failed to build" $testfile $srcfile] } { + return +} + +# Set non-stop mode based on NON_STOP. Start a native inferior running in +# the background, then start a second, remote inferior. Based on the value +# of NON_STOP we might expect the inferior thread to have been stopped. +# Confirm inferior one is in the correct state, and that it can be +# interrupted and/or resumed. +proc run_test { target_non_stop non_stop } { + clean_restart $::testfile + + # Setup non-stop settings. + gdb_test_no_output "maint set target-non-stop $target_non_stop" + gdb_test_no_output "set non-stop $non_stop" + + # Start the first inferior running in the background. + gdb_test -no-prompt-anchor "run&" "Starting program: .*" "start background inferior" + + # Add a second inferior. + gdb_test "add-inferior" "Added inferior 2.*" + gdb_test "inferior 2" "Switching to inferior 2.*" + + # Setup the sysroot if possible. This will make connecting to + # gdbserver quicker. + if { ![is_remote host] && ![is_remote target] } { + gdb_test "set sysroot" + } + + # Setup, and connect to, a remote target. + set target_exec [gdbserver_download_current_prog] + set res [gdbserver_start "" $target_exec] + set gdbserver_protocol [lindex $res 0] + set gdbserver_gdbport [lindex $res 1] + set res [gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport] + gdb_assert {$res == 0} "connect to remote target" + + # FIXME: A bug in GDB means that the state of thread 1.1 will be wrong, + # GDB's frontend thinks that the thread is running when really the + # thread is stopped. This will be fixed in the next commit, at which + # point this whole 'if' will be removed. + if { $target_non_stop == "on" && $non_stop == "off" } { + gdb_test "p 1 + 2" " = 3" "check GDB is still alive" + return + } + + # Check the info threads output. We're checking that we see the two + # threads we expect, that the correct thread (inferior two's thread) + # is current, and that none of the threads are running. + set state_inferior_1 "" + set state_inferior_2 "" + gdb_test_multiple "info threads" "" { + -re "^info threads\r\n" { + exp_continue + } + + -re "^\\s+Id\\s+Target Id\\s+Frame\\s*\r\n" { + exp_continue + } + + -re "^\\s+1\\.1\\s+\[^\r\n\]+\\(running\\)\r\n" { + set state_inferior_1 "running" + exp_continue + } + + -re "^\\*\\s+2\\.1\\s+\[^\r\n\]+\\(running\\)\r\n" { + set state_inferior_2 "running" + exp_continue + } + + -re "^\\s+1\\.1\\s+\[^\r\n\]+\r\n" { + set state_inferior_1 "stopped" + exp_continue + } + + -re "^\\*\\s+2\\.1\\s+\[^\r\n\]+\r\n" { + set state_inferior_2 "stopped" + exp_continue + } + + -re "^$::gdb_prompt $" { + if { $non_stop } { + gdb_assert { $state_inferior_1 == "running" \ + && $state_inferior_2 == "stopped" } \ + $gdb_test_name + } else { + gdb_assert { $state_inferior_1 == "stopped" \ + && $state_inferior_2 == "stopped" } \ + $gdb_test_name + } + } + } + + # Allow inferior 2 to reach main. The confirms that inferior 2 can be + # set running again. + gdb_breakpoint main + gdb_continue_to_breakpoint "breakpoint in main" + gdb_test "bt 1" \ + "#0\\s+main \\(\\) at\[^\r\n\]+" \ + "check inferior 2 is in main" + + # Switch to inferior 1 and allow it to continue. This is a + # critical part of the test. When the test was added a bug (in + # all-stop mode) would leave inferior 1 stopped, but GDB code + # would think the thread was running. As such. the thread + # couldn't be resumed again. + gdb_test "inferior 1" "Switching to inferior 1.*" + + # In non-stop mode, thread 1.1 is correctly left running, so we + # need to stop it now. + if { $non_stop } { + gdb_test -no-prompt-anchor "interrupt" + gdb_test "p 1 + 1" " = 2" \ + "simple print to resync output" + } + + gdb_breakpoint breakpt + gdb_continue_to_breakpoint "continue to breakpoint in breakpt" + gdb_test "bt 1" \ + [multi_line \ + "#0\\s+breakpt \\(\\) at\[^\r\n\]+" \ + "\\(More stack frames follow\\.\\.\\.\\)"] \ + "check inferior 1 is in breakpt" + + # Switch back to inferior 2. The testing infrastructure will try to + # use 'monitor exit' to close gdbserver. It helps if we are in the + # gdbserver inferior when the script finishes. + gdb_test "inferior 2" "Switching to inferior 2.*" \ + "switch back to inferior 2" +} + +# Multi-inferior support requires non-stop targets. +foreach_with_prefix target_non_stop { auto on } { + # But it's OK if we're emulating all-stop mode on top of non-stop. + foreach_with_prefix non_stop { on off } { + run_test $target_non_stop $non_stop + } +} -- 2.47.3