From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 1879) id 589C43845185; Mon, 28 Nov 2022 14:13:57 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 589C43845185 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1669644837; bh=dBMqoMOwIXmBgisDerIiFZAAVZ1A9NOBuH2AcN8oAz8=; h=From:To:Subject:Date:From; b=yasA/gU3aETYgfp1QUe7+eGBMNMAIoTLn9ukl3/Dd5PJAtAzX2L3QXO5Rr0qD0Y56 W00mEgvCxRqbKKlDb2WUkiLG/soxTg7utqdqUlGolquz6MesDORSGKREIBlKwZnHB/ d6X58TkGeJovy/UPWr0WA6k59GmxCvmk33NvEuRU= Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Simon Marchi To: gdb-cvs@sourceware.org Subject: [binutils-gdb] gdb: disable commit resumed in target_kill X-Act-Checkin: binutils-gdb X-Git-Author: Simon Marchi X-Git-Refname: refs/heads/master X-Git-Oldrev: 3295ff3bfb682c0c4c38e758272e5592f240da2c X-Git-Newrev: ed14d866a31625c6f8c2cb6d4a445a8372b46161 Message-Id: <20221128141357.589C43845185@sourceware.org> Date: Mon, 28 Nov 2022 14:13:57 +0000 (GMT) List-Id: https://sourceware.org/git/gitweb.cgi?p=3Dbinutils-gdb.git;h=3Ded14d866a316= 25c6f8c2cb6d4a445a8372b46161 commit ed14d866a31625c6f8c2cb6d4a445a8372b46161 Author: Simon Marchi Date: Mon Nov 21 12:12:13 2022 -0500 gdb: disable commit resumed in target_kill =20 New in this version: =20 - Better comment in target_kill - Uncomment line in test to avoid hanging when exiting, when testing on native-extended-gdbserver =20 PR 28275 shows that doing a sequence of: =20 - Run inferior in background (run &) - kill that inferior - Run again =20 We get into this assertion: =20 /home/smarchi/src/binutils-gdb/gdb/target.c:2590: internal-error: t= arget_wait: Assertion `!proc_target->commit_resumed_state' failed. =20 #0 internal_error_loc (file=3D0x5606b344e740 "/home/smarchi/src/bi= nutils-gdb/gdb/target.c", line=3D2590, fmt=3D0x5606b344d6a0 "%s: Assertion = `%s' failed.") at /home/smarchi/src/binutils-gdb/gdbsupport/errors.cc:54 #1 0x00005606b6296475 in target_wait (ptid=3D..., status=3D0x7fffb= 9390630, options=3D...) at /home/smarchi/src/binutils-gdb/gdb/target.c:2590 #2 0x00005606b5767a98 in startup_inferior (proc_target=3D0x5606bfc= cb2a0 , pid=3D3884857, ntraps=3D1, last_waitsta= tus=3D0x0, last_ptid=3D0x0) at /home/smarchi/src/binutils-gdb/gdb/nat/fork-= inferior.c:482 #3 0x00005606b4e6c9c5 in gdb_startup_inferior (pid=3D3884857, num_= traps=3D1) at /home/smarchi/src/binutils-gdb/gdb/fork-child.c:132 #4 0x00005606b50f14a5 in inf_ptrace_target::create_inferior (this= =3D0x5606bfccb2a0 , exec_file=3D0x604000039f50 = "/home/smarchi/build/binutils-gdb/gdb/test", allargs=3D"", env=3D0x61500000= a580, from_tty=3D1) at /home/smarchi/src/binutils-gdb/gdb/inf-ptrace.c:105 #5 0x00005606b53b6d23 in linux_nat_target::create_inferior (this= =3D0x5606bfccb2a0 , exec_file=3D0x604000039f50 = "/home/smarchi/build/binutils-gdb/gdb/test", allargs=3D"", env=3D0x61500000= a580, from_tty=3D1) at /home/smarchi/src/binutils-gdb/gdb/linux-nat.c:978 #6 0x00005606b512b79b in run_command_1 (args=3D0x0, from_tty=3D1, = run_how=3DRUN_NORMAL) at /home/smarchi/src/binutils-gdb/gdb/infcmd.c:468 #7 0x00005606b512c236 in run_command (args=3D0x0, from_tty=3D1) at= /home/smarchi/src/binutils-gdb/gdb/infcmd.c:526 =20 When running the kill command, commit_resumed_state for the process_stratum_target (linux-nat, here) is true. After the kill, when there are no more threads, commit_resumed_state is still true, as nothing touches this flag during the kill operation. During the subsequent run command, run_command_1 does: =20 scoped_disable_commit_resumed disable_commit_resumed ("running"); =20 We would think that this would clear the commit_resumed_state flag of our native target, but that's not the case, because scoped_disable_commit_resumed iterates on non-exited inferiors in order to find active process targets. And after the kill, the inferior is exited, and the native target was unpushed from it anyway. So scoped_disable_commit_resumed doesn't touch the commit_resumed_state flag of the native target, it stays true. When reaching target_wait, in startup_inferior (to consume the initial expect stop events while the inferior is starting up and working its way through the shell), commit_resumed_state is true, breaking the contract saying that commit_resumed_state is always false when calling the targets' wait method. =20 (note: to be correct, I think that startup_inferior should toggle commit_resumed between the target_wait and target_resume calls, but I'll ignore that for now) =20 I can see multiple ways to fix this. In the end, we need commit_resumed_state to be cleared by the time we get to that target_wait. It could be done at the end of the kill command, or at the beginning of the run command. =20 To keep things in a coherent state, I'd like to make it so that after the kill command, when the target is left with no threads, its commit_resumed_state flag is left to false. This way, we can keep working with the assumption that a target with no threads (and therefore no running threads) has commit_resumed_state =3D=3D false. =20 Do this by adding a scoped_disable_commit_resumed in target_kill. It clears the target's commit_resumed_state on entry, and leaves it false if the target does not have any resumed thread on exit. That means, even if the target has another inferior with stopped threads, commit_resumed_state will be left to false, which makes sense. =20 Add a test that tries to cover various combinations of actions done while an inferior is running (and therefore while commit_resumed_state is true on the process target). =20 Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=3D28275 Change-Id: I8e6fe6dc1f475055921520e58cab68024039a1e9 Approved-By: Andrew Burgess Diff: --- gdb/target.c | 9 ++ .../gdb.base/run-control-while-bg-execution.c | 33 ++++++ .../gdb.base/run-control-while-bg-execution.exp | 118 +++++++++++++++++= ++++ 3 files changed, 160 insertions(+) diff --git a/gdb/target.c b/gdb/target.c index 9b937b0e8b4..1dd0f42af7d 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -907,6 +907,15 @@ add_deprecated_target_alias (const target_info &tinfo,= const char *alias) void target_kill (void) { + + /* If the commit_resume_state of the to-be-killed-inferior's process str= atum + is true, and this inferior is the last live inferior with resumed thr= eads + of that target, then we want to leave commit_resume_state to false, a= s the + target won't have any resumed threads anymore. We achieve this with + this scoped_disable_commit_resumed. On construction, it will set the= flag + to false. On destruction, it will only set it to true if there are r= esumed + threads left. */ + scoped_disable_commit_resumed disable ("killing"); current_inferior ()->top_target ()->kill (); } =20 diff --git a/gdb/testsuite/gdb.base/run-control-while-bg-execution.c b/gdb/= testsuite/gdb.base/run-control-while-bg-execution.c new file mode 100644 index 00000000000..8092fadc8b9 --- /dev/null +++ b/gdb/testsuite/gdb.base/run-control-while-bg-execution.c @@ -0,0 +1,33 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2020-2022 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see . = */ + // +#include + +static pid_t mypid =3D -1; + +static void +after_getpid (void) +{ +} + +int +main (void) +{ + mypid =3D getpid (); + after_getpid (); + sleep (30); +} diff --git a/gdb/testsuite/gdb.base/run-control-while-bg-execution.exp b/gd= b/testsuite/gdb.base/run-control-while-bg-execution.exp new file mode 100644 index 00000000000..2f6dde06fa6 --- /dev/null +++ b/gdb/testsuite/gdb.base/run-control-while-bg-execution.exp @@ -0,0 +1,118 @@ +# Copyright 2022 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . + +# This test aims at testing various operations after getting rid of an inf= erior +# that was running in background, or while we have an inferior running in +# background. The original intent was to expose cases where the commit-re= sumed +# state of the process stratum target was not reset properly after killing= an +# inferior running in background, which would be a problem when trying to = run +# again. The test was expanded to test various combinations of +# run-control-related actions done with an inferior running in background. + +if {[use_gdb_stub]} { + unsupported "test requires running" + return +} + +standard_testfile + +if {[build_executable "failed to prepare" $testfile $srcfile]} { + return +} + +# Run one variation of the test: +# +# 1. Start an inferior in the background with "run &" +# 2. Do action 1 +# 3. Do action 2 +# +# Action 1 indicates what to do with the inferior running in background: +# +# - kill: kill it +# - detach: detach it +# - add: add a new inferior and switch to it, leave the inferior running = in +# background alone +# - none: do nothing, leave the inferior running in background alone +# +# Action 2 indicates what to do after that: +# +# - start: use the start command +# - run: use the run command +# - attach: start a process outside of GDB and attach it +proc do_test { action1 action2 } { + save_vars { ::GDBFLAGS } { + append ::GDBFLAGS " -ex \"maintenance set target-non-stop on\"" + clean_restart $::binfile + } + + # Ensure we are at least after the getpid call, should we need it. + if { ![runto "after_getpid"] } { + return + } + + # Some commands below ask for confirmation. Turn that off for simplic= ity. + gdb_test "set confirm off" + gdb_test -no-prompt-anchor "continue &" + + if { $action1 =3D=3D "kill" } { + gdb_test "kill" "Inferior 1 .* killed.*" + } elseif { $action1 =3D=3D "detach" } { + set child_pid [get_integer_valueof "mypid" -1] + if { $child_pid =3D=3D -1 } { + fail "failed to extract child pid" + return + } + + gdb_test "detach" "Inferior 1 .* detached.*" "detach from first instance" + + # Kill the detached process, to avoid hanging when exiting GDBserver, + # when testing with the native-extended-gdbserver board. + remote_exec target "kill $child_pid" + } elseif { $action1 =3D=3D "add" } { + gdb_test "add-inferior -exec $::binfile" \ + "Added inferior 2 on connection 1.*" "add-inferior" + gdb_test "inferior 2" "Switching to inferior 2 .*" + } elseif { $action1 =3D=3D "none" } { + + } else { + error "invalid action 1" + } + + if { $action2 =3D=3D "start" } { + gdb_test "start" "Temporary breakpoint $::decimal\(?:\.$::decimal\)?, mai= n .*" + } elseif { $action2 =3D=3D "run" } { + gdb_test "break main" "Breakpoint $::decimal at $::hex.*" + gdb_test "run" "Breakpoint $::decimal\(?:\.$::decimal\)?, main .*" + } elseif { $action2 =3D=3D "attach" } { + set test_spawn_id [spawn_wait_for_attach $::binfile] + set test_pid [spawn_id_get_pid $test_spawn_id] + + if { [gdb_attach $test_pid] } { + gdb_test "detach" "Inferior $::decimal .* detached.*" \ + "detach from second instance" + } + + # Detach and kill this inferior so we don't leave it around. + kill_wait_spawned_process $test_spawn_id + } else { + error "invalid action 2" + } +} + +foreach_with_prefix action1 { kill detach add none } { + foreach_with_prefix action2 { start run attach } { + do_test $action1 $action2 + } +}