From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 2A7C7385B180 for ; Mon, 28 Nov 2022 13:44:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 2A7C7385B180 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1669643045; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=bnWyAkZhPSE5r8qganyRDc2WzOZi8y8B3B3p7zQrdMw=; b=FHBCsb66HOOAmy0GvkhRJmEWYyZXDBLi0VUt00TVrY5sPHQCosrB+a0kUGMXbtDrF4FvVc /ecu35rJV84UTdcFXpZh0XPSRfRQPQxsLgeAQyjnAXaFtpDKFRlwTsLMPpoi3OU9SvprE4 b4tGFOt32upczOe1M/PZ450RuJ6aQlY= Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-60-f63Z2T8wNmq2odPcQpZrrw-1; Mon, 28 Nov 2022 08:44:04 -0500 X-MC-Unique: f63Z2T8wNmq2odPcQpZrrw-1 Received: by mail-wr1-f71.google.com with SMTP id o10-20020adfa10a000000b00241f603af8dso1958621wro.11 for ; Mon, 28 Nov 2022 05:44:04 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=bnWyAkZhPSE5r8qganyRDc2WzOZi8y8B3B3p7zQrdMw=; b=BUqQm/ybhphKkpY+VMYpS3Tx0qMy+lehQSidF5DJZAUZiRvevu8IjxnutZXR9Y3QQz U1urDz7W1qbapQEBGPepBznPS1hqa3waPGhplPI/GxAj0iXsXEWSYLspClJVMqy+bqLD a5ZjXL3HILywLINy50Q0tqBQEZCLcdvVtNYduLxt4aJxFxheHrpuTqzJRRwMyKRvgd9z /gFG/eVvfTmnfHtfzlD8UQem+Pm15I7cs/NSXDcady+4SHcirN9Pnjl3vy+aHE05wftq 471uTgl9ORA+eXNFNgtMfXQg1jeb+8ePyoJMPptGgkEqVVOybg6tYCzzcjAOrSHOs+nK 36pQ== X-Gm-Message-State: ANoB5pmLwH+7kKLHAnD63gDJbfhJGRMuDja0PLiu6D/IvJZFPqQDk5Fo /xl9fGt9CVhJDrHy/NDZo1YXDqa3CLDfLvspi2dq1JvR+Vf4LQDSirzpyMu/C6qBmbOSAHVl4w7 gC6z6ljaohJ3SowMriMB/JQ== X-Received: by 2002:a05:600c:220b:b0:3cf:f747:71f with SMTP id z11-20020a05600c220b00b003cff747071fmr22856542wml.147.1669643043311; Mon, 28 Nov 2022 05:44:03 -0800 (PST) X-Google-Smtp-Source: AA0mqf6Iq5kygxeGz7YsADHOKJs3ZZhws1ExLDOlbE28IahSB0IHoT+H6AHBExAW4pi/iPhEbQUIhg== X-Received: by 2002:a05:600c:220b:b0:3cf:f747:71f with SMTP id z11-20020a05600c220b00b003cff747071fmr22856521wml.147.1669643042970; Mon, 28 Nov 2022 05:44:02 -0800 (PST) Received: from localhost ([31.111.84.238]) by smtp.gmail.com with ESMTPSA id e5-20020a05600c4e4500b003b492753826sm14693883wmq.43.2022.11.28.05.44.01 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 28 Nov 2022 05:44:01 -0800 (PST) From: Andrew Burgess To: Simon Marchi , gdb-patches@sourceware.org Cc: Simon Marchi Subject: Re: [PATCH v2 5/5] gdb: disable commit resumed in target_kill In-Reply-To: <20221121171213.1414366-6-simon.marchi@polymtl.ca> References: <20221121171213.1414366-1-simon.marchi@polymtl.ca> <20221121171213.1414366-6-simon.marchi@polymtl.ca> Date: Mon, 28 Nov 2022 13:44:00 +0000 Message-ID: <87a64b2nz3.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-8.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPAM_BODY,SPF_HELO_NONE,SPF_NONE,TXREP,WEIRD_PORT autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Simon Marchi via Gdb-patches writes: > From: Simon Marchi > > New in this version: > > - Better comment in target_kill > - Uncomment line in test to avoid hanging when exiting, when testing on > native-extended-gdbserver > > PR 28275 shows that doing a sequence of: > > - Run inferior in background (run &) > - kill that inferior > - Run again > > We get into this assertion: > > /home/smarchi/src/binutils-gdb/gdb/target.c:2590: internal-error: target_wait: Assertion `!proc_target->commit_resumed_state' failed. > > #0 internal_error_loc (file=0x5606b344e740 "/home/smarchi/src/binutils-gdb/gdb/target.c", line=2590, fmt=0x5606b344d6a0 "%s: Assertion `%s' failed.") at /home/smarchi/src/binutils-gdb/gdbsupport/errors.cc:54 > #1 0x00005606b6296475 in target_wait (ptid=..., status=0x7fffb9390630, options=...) at /home/smarchi/src/binutils-gdb/gdb/target.c:2590 > #2 0x00005606b5767a98 in startup_inferior (proc_target=0x5606bfccb2a0 , pid=3884857, ntraps=1, last_waitstatus=0x0, last_ptid=0x0) at /home/smarchi/src/binutils-gdb/gdb/nat/fork-inferior.c:482 > #3 0x00005606b4e6c9c5 in gdb_startup_inferior (pid=3884857, num_traps=1) at /home/smarchi/src/binutils-gdb/gdb/fork-child.c:132 > #4 0x00005606b50f14a5 in inf_ptrace_target::create_inferior (this=0x5606bfccb2a0 , exec_file=0x604000039f50 "/home/smarchi/build/binutils-gdb/gdb/test", allargs="", env=0x61500000a580, from_tty=1) > at /home/smarchi/src/binutils-gdb/gdb/inf-ptrace.c:105 > #5 0x00005606b53b6d23 in linux_nat_target::create_inferior (this=0x5606bfccb2a0 , exec_file=0x604000039f50 "/home/smarchi/build/binutils-gdb/gdb/test", allargs="", env=0x61500000a580, from_tty=1) > at /home/smarchi/src/binutils-gdb/gdb/linux-nat.c:978 > #6 0x00005606b512b79b in run_command_1 (args=0x0, from_tty=1, run_how=RUN_NORMAL) at /home/smarchi/src/binutils-gdb/gdb/infcmd.c:468 > #7 0x00005606b512c236 in run_command (args=0x0, from_tty=1) at /home/smarchi/src/binutils-gdb/gdb/infcmd.c:526 > > 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: > > scoped_disable_commit_resumed disable_commit_resumed ("running"); > > 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. > > (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) > > 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. > > 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 == false. > > 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. > > 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). > > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28275 > Change-Id: I8e6fe6dc1f475055921520e58cab68024039a1e9 > --- > gdb/target.c | 9 ++ > .../gdb.base/run-control-while-bg-execution.c | 33 +++++ > .../run-control-while-bg-execution.exp | 118 ++++++++++++++++++ > 3 files changed, 160 insertions(+) > create mode 100644 gdb/testsuite/gdb.base/run-control-while-bg-execution.c > create mode 100644 gdb/testsuite/gdb.base/run-control-while-bg-execution.exp > > diff --git a/gdb/target.c b/gdb/target.c > index 4a22885b82f1..08b47f52d2e0 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 stratum > + is true, and this inferior is the last live inferior with resumed threads > + of that target, then we want to leave commit_resume_state to false, as 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 resumed > + threads left. */ > + scoped_disable_commit_resumed disable ("killing"); > current_inferior ()->top_target ()->kill (); > } > > 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 000000000000..8092fadc8b96 > --- /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 = -1; > + > +static void > +after_getpid (void) > +{ > +} > + > +int > +main (void) > +{ > + mypid = getpid (); > + after_getpid (); > + sleep (30); > +} > diff --git a/gdb/testsuite/gdb.base/run-control-while-bg-execution.exp b/gdb/testsuite/gdb.base/run-control-while-bg-execution.exp > new file mode 100644 > index 000000000000..d1ce561b15d2 > --- /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 inferior > +# that was running in background, or while we have an inferior running in > +# background. The original intent was to expose cases where the commit-resumed > +# 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, shall we need it. s/shall/should/ > + if { ![runto "after_getpid"] } { > + return > + } > + > + # Some commands below ask for confirmation. Turn that off for simplicity. > + gdb_test "set confirm off" > + gdb_test -no-prompt-anchor "continue &" > + > + if { $action1 == "kill" } { > + gdb_test "kill" "Inferior 1 .* killed.*" > + } elseif { $action1 == "detach" } { > + set child_pid [get_integer_valueof "mypid" -1] > + if { $child_pid == -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 == "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 == "none" } { > + > + } else { > + error "invalid action 1" > + } > + > + if { $action2 == "start" } { > + gdb_test "start" "Temporary breakpoint $::decimal, main .*" When I tested using a recent version of master, I needed to change this line to: gdb_test "start" "Temporary breakpoint $::decimal\(?:\.$::decimal\)?, main .*" this is a result of commit 78805ff8aecf0a8c828fb1e2c344fa3a56655120. > + } elseif { $action2 == "run" } { > + gdb_test "break main" "Breakpoint $::decimal at $::hex.*" > + gdb_test "run" "Breakpoint $::decimal, main .*" and I ran into the same issue here: gdb_test "run" "Breakpoint $::decimal\(?:\.$::decimal\)?, main .*" With those fixes, this LGTM. Thanks, Andrew > + } elseif { $action2 == "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 > + } > +} > -- > 2.38.1