RE: [PATCH v8 6/6] gdb/infrun: handle already-exited threads when attempting to stop

["Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com>]

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