[prev in list] [next in list] [prev in thread] [next in thread]
List: gdb-cvs
Subject: [binutils-gdb/gdb-11-branch] gdb/remote: handle attach when stop packet lacks thread-id
From: Andrew Burgess via Gdb-cvs <gdb-cvs () sourceware ! org>
Date: 2021-12-23 13:17:45
Message-ID: 20211223131745.D9AAF3858418 () sourceware ! org
[Download RAW message or body]
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=6eccc2c811ad292ce3234d2a0cd71b2184ac40eb
commit 6eccc2c811ad292ce3234d2a0cd71b2184ac40eb
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date: Mon Oct 4 15:48:11 2021 +0100
gdb/remote: handle attach when stop packet lacks thread-id
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 an extra parameter to
remote_target::remote_add_thread. This new parameter will force the
new thread to be added with a call to add_thread_silent. We can now
call remote_add_thread from the ::attach method, the extra
actions (listed above) will now be performed, and the thread will be
left in the correct state.
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, we also run the tests with
target-async enabled, and disabled.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28405
This is a cherry pick of commit b622494ee378fd0a490 with a minor edit
in gdb.server/ext-attach.exp to disable some tests that fail due to
unrelated bugs. Those unrelated bugs have been fixed in the master
branch.
gdb/ChangeLog:
PR gdb/28405
* remote.c (remote_target::remote_add_thread): Add new silent_p
argument, use as needed.
(remote_target::remote_notice_new_inferior): Pass additional
argument to remote_add_thread.
(remote_target::remote_notice_new_inferior): Likewise.
(extended_remote_target::attach): Call remote_add_thread instead
of add_thred_silent directly.
(remote_target::select_thread_for_ambiguous_stop_reply): Move
assert earlier, before we use the thing we're asserting is not
nullptr.
gdb/testsuite/ChangeLog:
PR gdb/28405
* gdb.server/ext-attach.exp (run_test): New proc containing all of
the old code for running the core of the test. This proc is then
called multiple times from the global scope.
Diff:
---
gdb/ChangeLog | 14 +++++
gdb/remote.c | 31 +++++-----
gdb/testsuite/ChangeLog | 7 +++
gdb/testsuite/gdb.server/ext-attach.exp | 101 ++++++++++++++++++++------------
4 files changed, 101 insertions(+), 52 deletions(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index f7d8a35888b..4709e1a45f6 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,17 @@
+2021-12-23 Andrew Burgess <andrew.burgess@embecosm.com>
+
+ PR gdb/28405
+ * remote.c (remote_target::remote_add_thread): Add new silent_p
+ argument, use as needed.
+ (remote_target::remote_notice_new_inferior): Pass additional
+ argument to remote_add_thread.
+ (remote_target::remote_notice_new_inferior): Likewise.
+ (extended_remote_target::attach): Call remote_add_thread instead
+ of add_thred_silent directly.
+ (remote_target::select_thread_for_ambiguous_stop_reply): Move
+ assert earlier, before we use the thing we're asserting is not
+ nullptr.
+
2021-12-11 Bruno Larsen <blarsen@redhat.com>
PR gdb/28480
diff --git a/gdb/remote.c b/gdb/remote.c
index f2271ad3b50..c27cd51f00a 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -768,7 +768,8 @@ public: /* Remote specific methods. */
void process_initial_stop_replies (int from_tty);
- thread_info *remote_add_thread (ptid_t ptid, bool running, bool executing);
+ thread_info *remote_add_thread (ptid_t ptid, bool running, bool executing,
+ bool silent_p);
void btrace_sync_conf (const btrace_config *conf);
@@ -2522,10 +2523,13 @@ static remote_thread_info *get_remote_thread_info (remote_target *target,
ptid_t ptid);
/* Add thread PTID to GDB's thread list. Tag it as executing/running
- according to RUNNING. */
+ according to EXECUTING and RUNNING respectively. If SILENT_P (or the
+ remote_state::starting_up flag) is true then the new thread is added
+ silently, otherwise the new thread will be announced to the user. */
thread_info *
-remote_target::remote_add_thread (ptid_t ptid, bool running, bool executing)
+remote_target::remote_add_thread (ptid_t ptid, bool running, bool executing,
+ bool silent_p)
{
struct remote_state *rs = get_remote_state ();
struct thread_info *thread;
@@ -2536,7 +2540,7 @@ remote_target::remote_add_thread (ptid_t ptid, bool running, bool executing)
consider that a single-threaded target, mentioning a new thread
might be confusing to the user. Be silent then, preserving the
age old behavior. */
- if (rs->starting_up)
+ if (rs->starting_up || silent_p)
thread = add_thread_silent (this, ptid);
else
thread = add_thread (this, ptid);
@@ -2574,7 +2578,7 @@ remote_target::remote_notice_new_inferior (ptid_t currthread, bool executing)
{
/* We're seeing an event on a thread id we knew had exited.
This has to be a new thread reusing the old id. Add it. */
- remote_add_thread (currthread, running, executing);
+ remote_add_thread (currthread, running, executing, false);
return;
}
@@ -2596,7 +2600,7 @@ remote_target::remote_notice_new_inferior (ptid_t currthread, bool executing)
else
{
thread_info *thr
- = remote_add_thread (currthread, running, executing);
+ = remote_add_thread (currthread, running, executing, false);
switch_to_thread (thr);
}
return;
@@ -2628,7 +2632,7 @@ remote_target::remote_notice_new_inferior (ptid_t currthread, bool executing)
/* This is really a new thread. Add it. */
thread_info *new_thr
- = remote_add_thread (currthread, running, executing);
+ = remote_add_thread (currthread, running, executing, false);
/* If we found a new inferior, let the common code do whatever
it needs to with it (e.g., read shared libraries, insert
@@ -6049,14 +6053,11 @@ extended_remote_target::attach (const char *args, int from_tty)
ptid. */
ptid_t curr_ptid = remote_current_thread (ptid_t (pid));
- /* Add the main thread to the thread list. */
- thread_info *thr = add_thread_silent (this, curr_ptid);
+ /* Add the main thread to the thread list. We add the thread
+ silently in this case (the final true parameter). */
+ thread_info *thr = remote_add_thread (curr_ptid, true, true, true);
switch_to_thread (thr);
-
- /* Don't consider the thread stopped until we've processed the
- saved stop reply. */
- set_executing (this, thr->ptid, true);
}
/* Next, if the target can specify a description, read it. We do
@@ -7972,12 +7973,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/ChangeLog b/gdb/testsuite/ChangeLog
index 725f348db9f..8a3fa82c32f 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,10 @@
+2021-12-23 Andrew Burgess <andrew.burgess@embecosm.com>
+
+ PR gdb/28405
+ * gdb.server/ext-attach.exp (run_test): New proc containing all of
+ the old code for running the core of the test. This proc is then
+ called multiple times from the global scope.
+
2021-12-11 Bruno Larsen <blarsen@redhat.com>
PR gdb/28480
diff --git a/gdb/testsuite/gdb.server/ext-attach.exp b/gdb/testsuite/gdb.server/ext-attach.exp
index c9766e35317..fe936dedf5a 100644
--- a/gdb/testsuite/gdb.server/ext-attach.exp
+++ b/gdb/testsuite/gdb.server/ext-attach.exp
@@ -30,53 +30,80 @@ 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 {[build_executable "failed to prepare" $testfile $srcfile debug]} {
+ return -1
+}
- if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
- return -1
+# Run the test. TARGET_NON_STOP and TARGET_ASYNC 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_async target_non_stop to_disable } {
+ save_vars { ::GDBFLAGS } {
+ append ::GDBFLAGS " -ex \"maint set target-non-stop $target_non_stop\""
+ append ::GDBFLAGS " -ex \"maintenance set target-async ${target_async}\""
+
+ # 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\""
+ }
+
+ clean_restart $::binfile
}
-}
-# Make sure we're disconnected, in case we're testing with an
-# extended-remote board, therefore already connected.
-gdb_test "disconnect" ".*"
+ # Make sure we're disconnected, in case we're testing with an
+ # extended-remote board, therefore already connected.
+ gdb_test "disconnect" ".*"
-set target_exec [gdbserver_download_current_prog]
-gdbserver_start_extended
+ 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"
+ }
+ }
-gdb_test_no_output "set remote exec-file $target_exec" "set remote exec-file"
+ 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
-set test_spawn_id [spawn_wait_for_attach $binfile]
-set testpid [spawn_id_get_pid $test_spawn_id]
+ gdb_test_no_output "set remote exec-file $target_exec" "set remote exec-file"
-gdb_test "attach $testpid" \
- "Attaching to program: .*, process $testpid.*(in|at).*" \
- "attach to remote program 1"
+ set test_spawn_id [spawn_wait_for_attach $::binfile]
+ set testpid [spawn_id_get_pid $test_spawn_id]
-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"
- }
-}
+ gdb_test "attach $testpid" \
+ "Attaching to program: .*, process $testpid.*(in|at).*" \
+ "attach to remote program 1"
+
+ gdb_test "backtrace" ".*main.*" "backtrace 1"
-gdb_test "backtrace" ".*main.*" "backtrace 1"
+ gdb_test "detach" "Detaching from program.*process.*"
+ gdb_test "backtrace" "No stack\\." "backtrace with no program"
-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 2"
+ gdb_test "backtrace" ".*main.*" "backtrace 2"
-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"
-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
+}
-kill_wait_spawned_process $test_spawn_id
+# Don't run with target_async set to "off". There are bugs with the
+# extended-remote target when async mode is off and we try to attach.
+# These bugs have been fixed in the master branch.
+foreach_with_prefix target_async {"on"} {
+ foreach_with_prefix target_non_stop {"off" "on"} {
+ foreach_with_prefix to_disable { "" Tthread T } {
+ run_test ${target_async} ${target_non_stop} $to_disable
+ }
+ }
+}
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic