From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.120]) by sourceware.org (Postfix) with ESMTP id 72A8E385B835 for ; Thu, 16 Apr 2020 17:07:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 72A8E385B835 Received: from mail-ed1-f71.google.com (mail-ed1-f71.google.com [209.85.208.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-208-mzrU-5TxMKmAxY49Im0QAQ-1; Thu, 16 Apr 2020 13:06:54 -0400 X-MC-Unique: mzrU-5TxMKmAxY49Im0QAQ-1 Received: by mail-ed1-f71.google.com with SMTP id d24so3937804edp.5 for ; Thu, 16 Apr 2020 10:06:53 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=TF8BpkItWL9XB9s9ExB1+nzrKO+UEJ52ZuQFu6eKVZY=; b=iJnG4p1GhNxjWqBhp/YKb3h06L6LttUGr+TDrlgpmA0mCl/tzXBgOd0aRE1/IvBd2G mSLNul/1gBqk8i5ojfyOQykAKjeygXEMvDr/6IRKoVvkzM71t9Z/tP79tk9k0ONE+CBs Ko3ja7lBrkus8zcPROQ9d8Q/Wy5h2W/0ksmCRxwdeBJ7+2pfUNtNCpovMMu4HUzWvNt/ HuKRTdPTKLOZe599+t92u3b1H0WLRUuXuIdfYVNmAvAMupLH2KH1+iZg8QMbP/7OZue1 OT/qfgjJ2SXLhOm4yfCfu+TAjn8sq5T/70iZjVXAv4j1qrJQrfhpsh9aXelhtmNy0ikF os+g== X-Gm-Message-State: AGi0PuaS71/Sab4lkQvEMbcEkLdjHA/q+SYr5WwwTETUEXCZ19YAqsaY dWVHnqpumFLbcsD0jpxorfQD0zfKftmHi9E54NQ5zmlIIS+yD0wuiiJSUVXPVUv30G+ZBW+o6DA a81Du9RtBZazEUFPXX4hMLw== X-Received: by 2002:a05:6402:1505:: with SMTP id f5mr28900108edw.208.1587056812079; Thu, 16 Apr 2020 10:06:52 -0700 (PDT) X-Google-Smtp-Source: APiQypIFAoWfXO5xetdhmYZHhyWkmLe7nZHGob7Oq9Zwv1INla1FST/nyna9U+lsc9c8o/+e9mxSQw== X-Received: by 2002:a05:6402:1505:: with SMTP id f5mr28900052edw.208.1587056811547; Thu, 16 Apr 2020 10:06:51 -0700 (PDT) Received: from ?IPv6:2001:8a0:f909:7b00:56ee:75ff:fe8d:232b? ([2001:8a0:f909:7b00:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id a17sm2574826edj.53.2020.04.16.10.06.50 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 16 Apr 2020 10:06:50 -0700 (PDT) Subject: Re: [PATCH v5 5/5] gdb/infrun: handle already-exited threads when attempting to stop To: Tankut Baris Aktemur , gdb-patches@sourceware.org References: From: Pedro Alves Message-ID: <5a9fe2a5-61d7-a5e6-d5e4-b1097883f6cc@redhat.com> Date: Thu, 16 Apr 2020 18:06:49 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-23.6 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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-List-Received-Date: Thu, 16 Apr 2020 17:07:12 -0000 On 4/6/20 4:45 PM, Tankut Baris Aktemur via Gdb-patches wrote: > In stop_all_threads, GDB sends signals to other threads in an attempt > to stop them. While in a typical scenario the expected wait status is > TARGET_WAITKIND_STOPPED, it is possible that the thread GDB attempted > to stop has already terminated. If so, a waitstatus other than > TARGET_WAITKIND_STOPPED would be received. Handle this case > appropriately. > > If a wait status that denotes thread termination is ignored, GDB goes > into an infinite loop in stop_all_threads. > E.g.: > > $ gdb ./a.out > (gdb) start > ... > (gdb) add-inferior -exec ./a.out > ... > (gdb) inferior 2 > ... > (gdb) start > ... > (gdb) set schedule-multiple on > (gdb) set debug infrun 2 > (gdb) continue > Continuing. > infrun: clear_proceed_status_thread (process 10449) > infrun: clear_proceed_status_thread (process 10453) > infrun: proceed (addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT) > infrun: proceed: resuming process 10449 > infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 10449] at 0x55555555514e > infrun: infrun_async(1) > infrun: prepare_to_wait > infrun: proceed: resuming process 10453 > infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 10453] at 0x55555555514e > infrun: prepare_to_wait > infrun: Found 2 inferiors, starting at #0 > infrun: target_wait (-1.0.0, status) = > infrun: 10449.10449.0 [process 10449], > infrun: status->kind = exited, status = 0 > infrun: handle_inferior_event status->kind = exited, status = 0 > [Inferior 1 (process 10449) exited normally] > infrun: stop_waiting > infrun: stop_all_threads > infrun: stop_all_threads, pass=0, iterations=0 > infrun: process 10453 executing, need stop > infrun: target_wait (-1.0.0, status) = > infrun: 10453.10453.0 [process 10453], > infrun: status->kind = exited, status = 0 > infrun: stop_all_threads status->kind = exited, status = 0 process 10453 > infrun: process 10453 executing, already stopping > infrun: target_wait (-1.0.0, status) = > infrun: -1.0.0 [process -1], > infrun: status->kind = no-resumed > infrun: infrun_async(0) > infrun: stop_all_threads status->kind = no-resumed process -1 > infrun: process 10453 executing, already stopping > infrun: stop_all_threads status->kind = no-resumed process -1 > infrun: process 10453 executing, already stopping > infrun: stop_all_threads status->kind = no-resumed process -1 > infrun: process 10453 executing, already stopping > infrun: stop_all_threads status->kind = no-resumed process -1 > infrun: process 10453 executing, already stopping > infrun: stop_all_threads status->kind = no-resumed process -1 > infrun: process 10453 executing, already stopping > infrun: stop_all_threads status->kind = no-resumed process -1 > infrun: process 10453 executing, already stopping > infrun: stop_all_threads status->kind = no-resumed process -1 > infrun: process 10453 executing, already stopping > infrun: stop_all_threads status->kind = no-resumed process -1 > infrun: process 10453 executing, already stopping > infrun: stop_all_threads status->kind = no-resumed process -1 > infrun: process 10453 executing, already stopping > infrun: stop_all_threads status->kind = no-resumed process -1 > infrun: process 10453 executing, already stopping > ... > > And this polling goes on forever. This patch prevents the infinite > looping behavior. For the same scenario above, we obtain the > following behavior: > > ... > (gdb) continue > Continuing. > infrun: clear_proceed_status_thread (process 31229) > infrun: clear_proceed_status_thread (process 31233) > infrun: proceed (addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT) > infrun: proceed: resuming process 31229 > infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 31229] at 0x55555555514e > infrun: infrun_async(1) > infrun: prepare_to_wait > infrun: proceed: resuming process 31233 > infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 31233] at 0x55555555514e > infrun: prepare_to_wait > infrun: Found 2 inferiors, starting at #0 > infrun: target_wait (-1.0.0, status) = > infrun: 31229.31229.0 [process 31229], > infrun: status->kind = exited, status = 0 > infrun: handle_inferior_event status->kind = exited, status = 0 > [Inferior 1 (process 31229) exited normally] > infrun: stop_waiting > infrun: stop_all_threads > infrun: stop_all_threads, pass=0, iterations=0 > infrun: process 31233 executing, need stop > infrun: target_wait (-1.0.0, status) = > infrun: 31233.31233.0 [process 31233], > infrun: status->kind = exited, status = 0 > infrun: stop_all_threads status->kind = exited, status = 0 process 31233 > infrun: saving status status->kind = exited, status = 0 for 31233.31233.0 > infrun: process 31233 not executing > infrun: stop_all_threads, pass=1, iterations=1 > infrun: process 31233 not executing > infrun: stop_all_threads done > (gdb) > > The exit event from Inferior 1 is received and shown to the user. > The exit event from Inferior 2 is not displayed, but kept pending. > > (gdb) info inferiors > Num Description Connection Executable > * 1 a.out > 2 process 31233 1 (native) a.out > (gdb) inferior 2 > [Switching to inferior 2 [process 31233] (a.out)] > [Switching to thread 2.1 (process 31233)] > Couldn't get registers: No such process. > (gdb) > > Note the "Couldn't get regsiters" error above. As of writing this patch, > GDB normally goes into another hang, infinitely trying register access. > A patch such as > > https://sourceware.org/pipermail/gdb-patches/2020-March/166982.html > I've reviewed that patch today. Once that is in, remember to update this commit log. > eliminates the infinite polling. Resuming Inferior 2 processes the > pending exit event. > > (gdb) continue > Continuing. > infrun: clear_proceed_status_thread (process 31233) > infrun: clear_proceed_status_thread: thread process 31233 has pending wait status status->kind = exited, status = 0 (currently_stepping=0). > infrun: proceed (addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT) > infrun: proceed: resuming process 31233 > infrun: resume: thread process 31233 has pending wait status status->kind = exited, status = 0 (currently_stepping=0). > infrun: prepare_to_wait > infrun: Using pending wait status status->kind = exited, status = 0 for process 31233. > infrun: target_wait (-1.0.0, status) = > infrun: 31233.31233.0 [process 31233], > infrun: status->kind = exited, status = 0 > infrun: handle_inferior_event status->kind = exited, status = 0 > [Inferior 2 (process 31233) exited normally] > infrun: stop_waiting > (gdb) info inferiors > Num Description Connection Executable > 1 a.out > * 2 a.out > (gdb) > > Regression-tested on X86_64 Linux. Awesome, thanks much for tackling this. > > gdb/ChangeLog: > 2020-02-05 Tankut Baris Aktemur > Tom de Vries > > PR threads/25478 > * infrun.c (stop_all_threads): Do NOT ignore > TARGET_WAITKIND_NO_RESUMED, TARGET_WAITKIND_THREAD_EXITED, > TARGET_WAITKIND_EXITED, TARGET_WAITKIND_SIGNALLED wait statuses > received from threads we attempt to stop. > > gdb/testsuite/ChangeLog: > 2019-11-04 Tankut Baris Aktemur > > * gdb.multi/multi-exit.c: New file. > * gdb.multi/multi-exit.exp: New file. > * gdb.multi/multi-kill.c: New file. > * gdb.multi/multi-kill.exp: New file. > > Change-Id: I7cec98f40283773b79255d998511da434e9cd408 > --- > gdb/infrun.c | 59 +++++++++- > gdb/testsuite/gdb.multi/multi-exit.c | 22 ++++ > gdb/testsuite/gdb.multi/multi-exit.exp | 147 +++++++++++++++++++++++++ > gdb/testsuite/gdb.multi/multi-kill.c | 34 ++++++ > gdb/testsuite/gdb.multi/multi-kill.exp | 104 +++++++++++++++++ > 5 files changed, 360 insertions(+), 6 deletions(-) > create mode 100644 gdb/testsuite/gdb.multi/multi-exit.c > create mode 100644 gdb/testsuite/gdb.multi/multi-exit.exp > create mode 100644 gdb/testsuite/gdb.multi/multi-kill.c > create mode 100644 gdb/testsuite/gdb.multi/multi-kill.exp > > diff --git a/gdb/infrun.c b/gdb/infrun.c > index ce8b544ab8d..ead60a0d152 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -4854,13 +4854,60 @@ stop_all_threads (void) > target_pid_to_str (event.ptid).c_str ()); > } > > - if (event.ws.kind == TARGET_WAITKIND_NO_RESUMED > - || event.ws.kind == TARGET_WAITKIND_THREAD_EXITED > - || event.ws.kind == TARGET_WAITKIND_EXITED > - || event.ws.kind == TARGET_WAITKIND_SIGNALLED) > + if (event.ws.kind == TARGET_WAITKIND_NO_RESUMED) > + { > + /* All resumed threads exited. */ > + } > + else if (event.ws.kind == TARGET_WAITKIND_THREAD_EXITED > + || event.ws.kind == TARGET_WAITKIND_EXITED > + || event.ws.kind == TARGET_WAITKIND_SIGNALLED) > { > - /* All resumed threads exited > - or one thread/process exited/signalled. */ > + /* One thread/process exited/signalled. */ > + > + thread_info *t = nullptr; > + > + /* The target may have reported just a pid. If so, try > + the first non-exited thread. */ > + if (event.ptid.is_pid ()) > + { > + int pid = event.ptid.pid (); > + inferior *inf = find_inferior_pid (event.target, pid); > + for (thread_info *tp : inf->non_exited_threads ()) > + { > + t = tp; > + break; > + } > + > + /* FIXME: If there is no available thread, the event > + would have to be appended to a per-inferior event > + list, which, unfortunately, does not exist yet. We > + assert here instead of going into an infinite loop. */ > + gdb_assert (t != nullptr); > + > + if (debug_infrun) > + fprintf_unfiltered (gdb_stdlog, > + "infrun: stop_all_threads, using %s\n", > + target_pid_to_str (t->ptid).c_str ()); > + } > + else > + { > + t = find_thread_ptid (event.target, event.ptid); > + /* Check if this is the first time we see this thread. > + Don't bother adding if it individually exited. */ > + if (t == nullptr > + && event.ws.kind != TARGET_WAITKIND_THREAD_EXITED) > + t = add_thread (event.target, event.ptid); > + } > + > + if (t != nullptr) > + { > + /* Set the threads as non-executing to avoid > + another stop attempt on them. */ > + mark_non_executing_threads (event.target, event.ptid, > + event.ws); > + save_waitstatus (t, &event.ws); > + t->stop_requested = false; > + } > } > else > { > diff --git a/gdb/testsuite/gdb.multi/multi-exit.c b/gdb/testsuite/gdb.multi/multi-exit.c > new file mode 100644 > index 00000000000..f4825c8a7c1 > --- /dev/null > +++ b/gdb/testsuite/gdb.multi/multi-exit.c > @@ -0,0 +1,22 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright 2020 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 . */ > + > +int > +main () > +{ > + return 0; > +} > diff --git a/gdb/testsuite/gdb.multi/multi-exit.exp b/gdb/testsuite/gdb.multi/multi-exit.exp > new file mode 100644 > index 00000000000..2236243461d > --- /dev/null > +++ b/gdb/testsuite/gdb.multi/multi-exit.exp > @@ -0,0 +1,147 @@ > +# This testcase is part of GDB, the GNU debugger. > + > +# Copyright 2020 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 receiving TARGET_WAITKIND_EXITED events from multiple > +# inferiors. In all stop-mode, upon receiving the exit event from one > +# of the inferiors, GDB will try to stop the other inferior, too. So, > +# a stop request will be sent. Receiving a TARGET_WAITKIND_EXITED > +# status kind as a response to that stop request instead of a > +# TARGET_WAITKIND_STOPPED should be handled by GDB without problems. > + > +load_lib gdbserver-support.exp > + > +if {[skip_gdbserver_tests]} { > + return 0 > +} > + > +standard_testfile > + > +# The plain remote target can't do multiple inferiors. > +if {[target_info gdb_protocol] != ""} { Use: if [use_gdb_stub] { instead. multi-target.exp is a bit special, in that it really needs to control which targets are run. It is not the case in this testcase, AFAICT. See more below. > + return 0 > +} > + > +if {[build_executable "failed to build" $testfile $srcfile {debug}] == -1} { > + return -1 > +} > + > +# Set up the current inferior with a gdbserver in multi mode as its > +# target, if TARGET is "extended-remote". Otherwise the target is native. > + > +proc setup_inferior {target infnum} { > + global binfile > + > + gdb_test "file ${binfile}" ".*" "load file in inferior $infnum" > + > + if {$target == "extended-remote"} { > + gdb_test_no_output "set remote exec-file ${binfile}" \ > + "set remote-exec file in inferior $infnum" > + set res [gdbserver_start "--multi" ""] > + set gdbserver_gdbport [lindex $res 1] > + if {[gdb_target_cmd "extended-remote" $gdbserver_gdbport]} { > + return 0 > + } > + } > + > + if {![runto_main]} { > + fail "starting inferior $infnum" > + return 0 > + } > + return 1 > +} > + > +# Set up two inferiors and start the processes. The underlying target > +# of each inferior is determined by the TARGET argument. > + > +proc setup {target} { > + clean_restart > + > + # This is a test specific for GDB's ability to stop all threads. > + gdb_test_no_output "maint set target-non-stop on" > + > + if {![setup_inferior $target 1]} { > + return 0 > + } > + > + gdb_test "add-inferior -no-connection" "Added inferior 2" \ > + "add the second inferior" > + gdb_test "inferior 2" "Switching to inferior 2.*" \ > + "switch to inferior 2" > + > + if {![setup_inferior $target 2]} { > + return 0 > + } > + > + # We want to continue both processes. > + gdb_test_no_output "set schedule-multiple on" > + > + return 1 > +} > + > +# Run the test using TARGET as the target of the inferiors. > + > +proc test {target} { > + if {![setup $target]} { > + untested "could not do setup" > + return > + } > + > + # We want GDB to complete the command and return the prompt > + # instead of going into an infinite loop. > + global decimal gdb_prompt inferior_exited_re > + gdb_test_multiple "continue" "first continue" { > + -re "Inferior ($decimal) \[^\n\r\]+ exited normally.*$gdb_prompt $" { > + set exited_inferior $expect_out(1,string) > + pass $gdb_test_name > + } > + } > + > + if {![info exists exited_inferior]} { > + fail "first continue" > + return 0 > + } > + > + if {$exited_inferior == 1} { > + set other_inferior 2 > + } else { > + set other_inferior 1 > + } > + > + # Switch to the other inferior and check it, too, continues to the end. > + gdb_test "inferior $other_inferior" \ > + ".*Switching to inferior $other_inferior.*" \ > + "switch to the other inferior" > + > + gdb_continue_to_end > + > + # Finally, check if we can re-run both inferiors. > + delete_breakpoints > + > + gdb_test "run" "$inferior_exited_re normally\]" \ > + "re-run the other inferior" > + > + gdb_test "inferior $exited_inferior" \ > + ".*Switching to inferior $exited_inferior.*" \ > + "switch to the first exited inferior" > + > + gdb_test "run" "$inferior_exited_re normally\]" \ > + "re-run the first exited inferior" > +} > + > +foreach_with_prefix target {"native" "extended-remote"} { This is really usually not the right thing to do. Better write the testcase in a target-neutral form, and then let make check RUNTESTFLAGS="--target_board=native-extended-gdbserver" cover testing with extended-remote. AFAICT, when testing with extended-remote, you're spawning a gdbserver for each inferior. You don't really need that, right? A single gdbserver with both inferiors should trigger the problem as well. I.e., remove this specific target stuff, and just use regular commands. Instead of issuing the "file" + "set remote exec-file", use gdb_load. Instead of the "run" command, try gdb_run_cmd. > +gdb_test_multiple "continue" "continue processes" { > + -re "Continuing.\[\r\n\]+" { > + # Kill both processes at once. > + remote_exec build "kill -9 ${testpid1} ${testpid2}" Pedantically, "remote_exec target" > +# Find the current inferior's id. > +set current_inferior 1 > +gdb_test_multiple "info inferiors" "find the current inf id" { > + -re "\\* 1 .*$gdb_prompt $" { > + set current_inferior 1 > + set other_inferior 2 > + pass $gdb_test_name > + } > + -re "\\* 2 .*$gdb_prompt $" { > + set current_inferior 2 > + set other_inferior 1 > + pass $gdb_test_name > + } Watch out for tabs vs spaces above. Does multi-kill.exp really need to attach to processes instead of spawning them via GDB? If we do the latter, then the testcase will run on more configurations. Thanks, Pedro Alves