[PATCH v8 6/6] gdb/infrun: handle already-exited threads when attempting to stop
Aktemur, Tankut Baris
tankut.baris.aktemur@intel.com
Thu May 14 08:47:53 GMT 2020
On Wednesday, May 13, 2020 10:54 PM, Pedro Alves wrote:
>
> When a process exits and we leave the process exit event pending, we
> need to make sure that at least one thread is left listed in the
> inferior's thread list. This is necessary in order to make sure we
> have a thread that we can later resume, so the process exit event can
> be collected/reported.
>
> When native debugging, the GNU/Linux back end already makes sure that
> the last LWP isn't deleted.
>
> When remote debugging against GNU/Linux GDBserver, the GNU/Linux
> GDBserver backend also makes sure that the last thread isn't deleted
> until the process exit event is reported to GDBserver core.
>
> However, between the backend reporting the process exit event to
> GDBserver core, and GDB consuming the event, GDB may update the thread
> list and find no thread left in the process. The process exit event
> will be pending somewhere in GDBserver's stop reply queue, or
> gdb/remote.c's queue, or whathever other event queue inbetween
> GDBserver and infrun.c's handle_inferior_event.
>
> This patch tweaks remote.c's target_update_thread_list implementation
> to avoid deleting the last thread of an inferior.
>
> In the past, this case of inferior-with-no-threads lead to a special
"lead" -> "led"?
> case at the bottom of handle_no_resumed, where it reads:
>
> /* Note however that we may find no resumed thread because the whole
> process exited meanwhile (thus updating the thread list results
> in an empty thread list). In this case we know we'll be getting
> a process exit event shortly. */
> for (inferior *inf : all_non_exited_inferiors (ecs->target))
>
> In current master, that code path is still reacheable with the
"reacheable" -> "reachable"
> gdb.threads/continue-pending-after-query.exp testcase, when tested
> against GDBserver, with "maint set target-non-stop" forced "on".
>
> With this patch, the scenario that loop was concerned about is still
> properly handled, because the loop above it finds the process's last
> thread with "executing" set to true, and thus the handle_no_resumed
> function still returns true.
>
> Since GNU/Linux native and remote are the only targets that support
> non-stop mode, and with this patch, we always make sure the inferior
> has at least one thread, this patch also removes that "inferior with
> no threads" special case handling from handle_no_resumed.
>
> Since remote.c now has a special case where we treat a thread that has
> already exited as if it was still alive, we might need to tweak
> remote.c's target_thread_alive implementation to return true for that
> thread without querying the remote side (which would say "no, not
> alive"). After inspecting all the target_thread_alive calls in the
> codebase, it seems that only the one from prune_threads could result
> in that thread being accidentaly deleted. There's only one call to
"accidentaly" -> "accidentally"
> prune_threads in GDB's common code, so this patch handles this by
> replacing the prune_threads call with a delete_exited_threads call.
> This seems like an improvement anyway, because we'll still be doing
> what the comment suggests we want to do, and, we avoid remote protocol
> traffic.
The whole explanation above is very useful. Thank you.
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 6602bc28d5e..c9a092e4943 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -4791,7 +4791,11 @@ stop_all_threads (void)
> {
> int need_wait = 0;
>
> - update_thread_list ();
> + for (auto *target : all_non_exited_process_targets ())
> + {
> + switch_to_target_no_thread (target);
> + update_thread_list ();
> + }
I'm glad that the artificial thread addition/resurrection piece is gone
and the thread lists are now updated straightforwardly. I'm also glad
that all_non_exited_process_targets found itself another use :).
> + /* If there is no available thread, the event would
> + have to be appended to a per-inferior event list,
> + which, does not exist (and if it did, we'd have
> + to adjust run control command to be able to
> + resume such an inferior). We assert here instead
> + of going into an infinite loop. */
Just a nit: no comma after "which".
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 5db406e045c..8e12ba9603e 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -3785,6 +3785,18 @@ remote_target::remote_get_threads_with_qthreadinfo (threads_listing_context *con
> return 0;
> }
>
> +/* Return true if INF only has one non-exited thread. */
> +
> +static bool
> +single_non_exited_thread_p (inferior *inf)
Given that the return type is bool and not int, can the '_p' in the name be
discarded and a question phrase, like 'has_single_non_exited_thread', be
used?
> +{
> + int count = 0;
> + for (thread_info *tp ATTRIBUTE_UNUSED : inf->non_exited_threads ())
> + if (++count > 1)
> + break;
> + return count == 1;
> +}
> +
> diff --git a/gdb/testsuite/gdb.multi/multi-exit.exp b/gdb/testsuite/gdb.multi/multi-exit.exp
> new file mode 100644
> index 00000000000..3c4e99164ed
> --- /dev/null
> +++ b/gdb/testsuite/gdb.multi/multi-exit.exp
> @@ -0,0 +1,138 @@
> +# 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 <http://www.gnu.org/licenses/>.
> +
> +# 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.
> +
> +standard_testfile
> +
> +if {[use_gdb_stub]} {
> + return 0
> +}
> +
> +if {[build_executable "failed to prepare" $testfile $srcfile]} {
> + return -1
> +}
> +
> +# We are testing GDB's ability to stop all threads.
> +# Hence, go with the all-stop-on-top-of-non-stop mode.
> +save_vars { GDBFLAGS } {
> + append GDBFLAGS " -ex \"maint set target-non-stop on\""
> + clean_restart ${binfile}
> +}
> +
> +with_test_prefix "inf 1" {
> + gdb_load $binfile
> +
> + if {[gdb_start_cmd] < 0} {
> + fail "could not start"
> + return -1
> + }
> + gdb_test "" ".*reakpoint .*, main .*${srcfile}.*" "start"
> +}
Similar to what you did in multi-kill.exp, to eliminate code duplication,
this 'with_test_prefix' code block can be removed, and the start_inferior
procedure can be used for inferior 1, too, by ...
> +
> +# Start inferior NUM.
> +
> +proc start_inferior {num} {
> + global srcfile binfile
> +
> + gdb_test "add-inferior" "Added inferior $num.*" \
> + "add empty inferior $num"
> + gdb_test "inferior $num" "Switching to inferior $num.*" \
> + "switch to inferior $num"
... guarding the statements above with 'if {$num != 1}'. And then ...
> +
> + with_test_prefix "inf $num" {
> + gdb_load $binfile
> +
> + if {[gdb_start_cmd] < 0} {
> + fail "could not start"
> + return -1
> + }
> + gdb_test "" ".*reakpoint .*, main .*${srcfile}.*" "start"
> + }
> +
> + return 0
> +}
> +
> +# Sufficient inferiors to make sure that at least some other inferior
> +# exits while we're handling a process exit event.
> +set NUM_INFS 10
> +
> +for {set i 2} {$i <= $NUM_INFS} {incr i} {
... this loop could start from 1.
> + if {[start_inferior $i] < 0} {
> + return -1
> + }
> +}
> +
> +# We want to continue all processes.
> +gdb_test_no_output "set schedule-multiple on"
> +
> +# Check that "continue" continues to the end of an inferior, as many
> +# times as we have inferiors.
> +
> +for {set i 1} {$i <= $NUM_INFS} {incr i} {
> + with_test_prefix "inf $i" {
> + set live_inferior ""
> +
> + # Pick any live inferior.
> + gdb_test_multiple "info inferiors" "" {
> + -re "($decimal) *process.*$gdb_prompt $" {
> + set live_inferior $expect_out(1,string)
> + }
> + }
> +
> + if {$live_inferior == ""} {
> + return -1
> + }
> +
> + gdb_test "inferior $live_inferior" \
> + ".*Switching to inferior $live_inferior.*" \
> + "switch to another inferior"
> +
> + set exited_inferior ""
> +
> + # We want GDB to complete the command and return the prompt
> + # instead of going into an infinite loop.
> + gdb_test_multiple "continue" "first continue" {
Just a nit: Now that there is no "second" continue, the "first" in the
test name can be discarded, I think. How about just "continue"?
> + -re "Inferior ($decimal) \[^\n\r\]+ exited normally.*$gdb_prompt $" {
> + set exited_inferior $expect_out(1,string)
> + pass $gdb_test_name
> + }
> + }
> +
> + if {$exited_inferior == ""} {
> + return -1
> + }
> + }
> +}
> +
> +# Finally, check that we can re-run all inferiors. Note that if any
> +# inferior was still alive this would catch it, as "run" would query
> +# "Start it from the beginning?".
> +
> +delete_breakpoints
> +
> +for {set i 1} {$i <= $NUM_INFS} {incr i} {
> + with_test_prefix "inf $i" {
I think we need to switch to inferior $i here, otherwise we re-run
the latest inferior from the previous loop for ten times.
> + gdb_test "run" "$inferior_exited_re normally\]" \
> + "re-run inferior"
> + }
> +}
> diff --git a/gdb/testsuite/gdb.multi/multi-kill.exp b/gdb/testsuite/gdb.multi/multi-kill.exp
> new file mode 100644
> index 00000000000..7deaadc68e8
> --- /dev/null
> +++ b/gdb/testsuite/gdb.multi/multi-kill.exp
> @@ -0,0 +1,127 @@
> +# 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 <http://www.gnu.org/licenses/>.
> +
> +# Test receiving TARGET_WAITKIND_SIGNALLED 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_SIGNALLED
> +# status kind as a response to that stop request instead of a
> +# TARGET_WAITKIND_STOPPED should be handled by GDB without problems.
> +
> +standard_testfile
> +
> +if {[use_gdb_stub]} {
> + return 0
> +}
> +
> +if {[build_executable "failed to prepare" $testfile $srcfile {debug}]} {
> + return -1
> +}
> +
> +# We are testing GDB's ability to stop all threads.
> +# Hence, go with the all-stop-on-top-of-non-stop mode.
> +save_vars { GDBFLAGS } {
> + append GDBFLAGS " -ex \"maint set target-non-stop on\""
> + clean_restart ${binfile}
> +}
> +
> +# Start inferior NUM and record its PID in the TESTPID array.
> +
> +proc start_inferior {num} {
> + with_test_prefix "start_inferior $num" {
> + global testpid binfile srcfile
> +
> + if {$num != 1} {
> + gdb_test "add-inferior" "Added inferior .*" \
> + "add empty inferior"
> + gdb_test "inferior $num" "Switching to inferior .*" \
> + "switch to inferior"
> + }
> +
> + gdb_load $binfile
> +
> + gdb_breakpoint "initialized" {temporary}
> + gdb_run_cmd
> + gdb_test "" ".*reakpoint .*, initialized .*${srcfile}.*" "run"
> +
> + set testpid($num) [get_integer_valueof "pid" -1]
> + if {$testpid($num) == -1} {
> + return -1
> + }
> +
> + return 0
> + }
> +}
> +
> +# Sufficient inferiors to make sure that at least some other inferior
> +# is killed while we're handling a killed event.
> +set NUM_INFS 10
> +
> +for {set i 1} {$i <= $NUM_INFS} {incr i} {
> + if {[start_inferior $i] < 0} {
> + return -1
> + }
> +}
> +
> +# We want to continue all processes.
> +gdb_test_no_output "set schedule-multiple on"
> +
> +# Resume, but then kill all from outside.
> +gdb_test_multiple "continue" "continue processes" {
> + -re "Continuing.\[\r\n\]+" {
> + # Kill all processes at once.
> +
> + set kill_cmd "kill -9"
> + for {set i 1} {$i <= $NUM_INFS} {incr i} {
> + append kill_cmd " $testpid($i)"
> + }
> +
> + remote_exec target $kill_cmd
> + exp_continue
> + }
> + -re "Program terminated with signal.*$gdb_prompt" {
I'm not sure if it really matters, but for consistency with the other
similar regexps here and multi-exit.exp, how about ending this regexp
with " $"? That is:
-re "Program terminated with signal.*$gdb_prompt $" {
Thanks.
-Baris
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
More information about the Gdb-patches
mailing list