[PATCH] gdb/remote: handle attach when stop packet lacks thread-id
Andrew Burgess
andrew.burgess@embecosm.com
Tue Oct 5 09:04:15 GMT 2021
Bug PR gdb/28405 reports a regression when using attach with an
extended-remote target. In this case the target is not including a
thread-id in the stop packet it sends back after the attach.
The regression was introduced with this commit:
commit 8f66807b98f7634c43149ea62e454ea8f877691d
Date: Wed Jan 13 20:26:58 2021 -0500
gdb: better handling of 'S' packets
The problem is that when GDB processes the stop packet, it sees that
there is no thread-id and so has to "guess" which thread the stop
should apply to.
In this case the target only has one thread, so really, there's no
guessing needed, but GDB still runs through the same process, this
shouldn't cause us any problems.
However, after the above commit, GDB now expects itself to be more
internally consistent, specifically, only a thread that GDB thinks is
resumed, can be a candidate for having stopped.
It turns out that, when GDB attaches to a process through an
extended-remote target, the threads of the process being attached too,
are not, initially, marked as resumed.
And so, when GDB tries to figure out which thread the stop might apply
too, it finds no threads in the processes marked resumed, and so an
assert triggers.
In extended_remote_target::attach we create a new thread with a call
to add_thread_silent, rather than remote_target::remote_add_thread,
the reason is that calling the latter will result in a call to
'add_thread' rather than 'add_thread_silent'. However,
remote_target::remote_add_thread includes additional
actions (i.e. calling remote_thread_info::set_resumed and set_running)
which are missing from extended_remote_target::attach. These missing
calls are what would serve to mark the new thread as resumed.
In this commit, I propose that we add the extra calls into
extended_remote_target::attach, this solves the problem at hand.
Additionally, in PR gdb/28405, a segfault is reported. This segfault
triggers when 'set debug remote 1' is used before trying to reproduce
the original assertion failure. The cause of this is in
remote_target::select_thread_for_ambiguous_stop_reply, where we do
this:
remote_debug_printf ("first resumed thread is %s",
pid_to_str (first_resumed_thread->ptid).c_str ());
remote_debug_printf ("is this guess ambiguous? = %d", ambiguous);
gdb_assert (first_resumed_thread != nullptr);
Notice that when debug printing is on we dereference
first_resumed_thread before we assert that the pointer is not
nullptr. This is the cause of the segfault, and is resolved by moving
the assert before the debug printing code.
I've extended an existing test, ext-attach.exp, so that the original
test is run multiple times; we run in the original mode, as normal,
but also, we now run with different packets disabled in gdbserver. In
particular, disabling Tthread would trigger the assertion as it was
reported in the original bug. I also run the test in all-stop and
non-stop modes now for extra coverage.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28405
---
gdb/remote.c | 7 +-
gdb/testsuite/gdb.server/ext-attach.exp | 94 +++++++++++++++----------
2 files changed, 62 insertions(+), 39 deletions(-)
diff --git a/gdb/remote.c b/gdb/remote.c
index d5eb40ce578..4db17ed7fb5 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -6075,6 +6075,9 @@ extended_remote_target::attach (const char *args, int from_tty)
/* Don't consider the thread stopped until we've processed the
saved stop reply. */
+ get_remote_thread_info (thr)->set_resumed ();
+ set_running (this, thr->ptid, true);
+ set_resumed (this, thr->ptid, true);
set_executing (this, thr->ptid, true);
}
@@ -7992,12 +7995,12 @@ remote_target::select_thread_for_ambiguous_stop_reply
ambiguous = true;
}
+ gdb_assert (first_resumed_thread != nullptr);
+
remote_debug_printf ("first resumed thread is %s",
pid_to_str (first_resumed_thread->ptid).c_str ());
remote_debug_printf ("is this guess ambiguous? = %d", ambiguous);
- gdb_assert (first_resumed_thread != nullptr);
-
/* Warn if the remote target is sending ambiguous stop replies. */
if (ambiguous)
{
diff --git a/gdb/testsuite/gdb.server/ext-attach.exp b/gdb/testsuite/gdb.server/ext-attach.exp
index c9766e35317..abd8c9b5b6d 100644
--- a/gdb/testsuite/gdb.server/ext-attach.exp
+++ b/gdb/testsuite/gdb.server/ext-attach.exp
@@ -30,53 +30,73 @@ if {![can_spawn_for_attach]} {
return 0
}
-save_vars { GDBFLAGS } {
- # If GDB and GDBserver are both running locally, set the sysroot to avoid
- # reading files via the remote protocol.
- if { ![is_remote host] && ![is_remote target] } {
- set GDBFLAGS "$GDBFLAGS -ex \"set sysroot\""
- }
-
- if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
- return -1
- }
+if {[build_executable "failed to prepare" $testfile $srcfile debug]} {
+ return -1
}
-# Make sure we're disconnected, in case we're testing with an
-# extended-remote board, therefore already connected.
-gdb_test "disconnect" ".*"
+# Run the test. TARGET_NON_STOP should be 'on' or 'off'. TO_DISABLE
+# should be either the empty string, or something that can be passed
+# to gdbserver's --disable-packet command line option.
+proc run_test { target_non_stop to_disable } {
+ save_vars { ::GDBFLAGS } {
+ append ::GDBFLAGS " -ex \"maint set target-non-stop $target_non_stop\""
-set target_exec [gdbserver_download_current_prog]
-gdbserver_start_extended
+ # If GDB and GDBserver are both running locally, set the sysroot to avoid
+ # reading files via the remote protocol.
+ if { ![is_remote host] && ![is_remote target] } {
+ set ::GDBFLAGS "$::GDBFLAGS -ex \"set sysroot\""
+ }
-gdb_test_no_output "set remote exec-file $target_exec" "set remote exec-file"
+ clean_restart $::binfile
+ }
-set test_spawn_id [spawn_wait_for_attach $binfile]
-set testpid [spawn_id_get_pid $test_spawn_id]
+ # Make sure we're disconnected, in case we're testing with an
+ # extended-remote board, therefore already connected.
+ gdb_test "disconnect" ".*"
-gdb_test "attach $testpid" \
- "Attaching to program: .*, process $testpid.*(in|at).*" \
- "attach to remote program 1"
+ if { [gdb_target_supports_trace] } then {
+ # Test predefined TSVs are uploaded.
+ gdb_test_sequence "info tvariables" "check uploaded tsv" {
+ "\[\r\n\]+Name\[\t \]+Initial\[\t \]+Current"
+ "\[\r\n\]+\\\$trace_timestamp 0"
+ }
+ }
-if { [gdb_target_supports_trace] } then {
- # Test predefined TSVs are uploaded.
- gdb_test_sequence "info tvariables" "check uploaded tsv" {
- "\[\r\n\]+Name\[\t \]+Initial\[\t \]+Current"
- "\[\r\n\]+\\\$trace_timestamp 0"
+ set target_exec [gdbserver_download_current_prog]
+ if { $to_disable != "" } {
+ set gdbserver_opts "--disable-packet=${to_disable}"
+ } else {
+ set gdbserver_opts ""
}
-}
+ gdbserver_start_extended $gdbserver_opts
+
+ gdb_test_no_output "set remote exec-file $target_exec" "set remote exec-file"
-gdb_test "backtrace" ".*main.*" "backtrace 1"
+ set test_spawn_id [spawn_wait_for_attach $::binfile]
+ set testpid [spawn_id_get_pid $test_spawn_id]
-gdb_test "detach" "Detaching from program.*process.*"
-gdb_test "backtrace" "No stack\\." "backtrace with no program"
+ gdb_test "attach $testpid" \
+ "Attaching to program: .*, process $testpid.*(in|at).*" \
+ "attach to remote program 1"
-gdb_test "attach $testpid" \
- "Attaching to program: .*, process $testpid.*(in|at).*" \
- "attach to remote program 2"
-gdb_test "backtrace" ".*main.*" "backtrace 2"
+ gdb_test "backtrace" ".*main.*" "backtrace 1"
-gdb_test "kill" "" "kill" "Kill the program being debugged. .y or n. " "y"
-gdb_test_no_output "monitor exit"
+ gdb_test "detach" "Detaching from program.*process.*"
+ gdb_test "backtrace" "No stack\\." "backtrace with no program"
-kill_wait_spawned_process $test_spawn_id
+ gdb_test "attach $testpid" \
+ "Attaching to program: .*, process $testpid.*(in|at).*" \
+ "attach to remote program 2"
+ gdb_test "backtrace" ".*main.*" "backtrace 2"
+
+ gdb_test "kill" "" "kill" "Kill the program being debugged. .y or n. " "y"
+ gdb_test_no_output "monitor exit"
+
+ kill_wait_spawned_process $test_spawn_id
+}
+
+foreach_with_prefix target_non_stop {"off" "on"} {
+ foreach_with_prefix to_disable { "" Tthread T } {
+ run_test ${target_non_stop} $to_disable
+ }
+}
--
2.25.4
More information about the Gdb-patches
mailing list