This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
[PATCHv3 2/2] gdb/remote: Restore support for 'S' stop reply packet
- From: Andrew Burgess <andrew dot burgess at embecosm dot com>
- To: gdb-patches at sourceware dot org
- Cc: Pedro Alves <palves at redhat dot com>, Andrew Burgess <andrew dot burgess at embecosm dot com>
- Date: Mon, 2 Mar 2020 11:54:10 +0000
- Subject: [PATCHv3 2/2] gdb/remote: Restore support for 'S' stop reply packet
- References: <cover.1583149853.git.andrew.burgess@embecosm.com>
- References: <137d09cf-9f97-fa5e-19a0-71231a3f760a@redhat.com> <cover.1583149853.git.andrew.burgess@embecosm.com>
With this commit:
commit 5b6d1e4fa4fc6827c7b3f0e99ff120dfa14d65d2
Date: Fri Jan 10 20:06:08 2020 +0000
Multi-target support
There was a regression in GDB's support for older aspects of the
remote protocol. Specifically, when a target sends the 'S' stop reply
packet (which doesn't include a thread-id) then GDB has to figure out
which thread actually stopped.
Before the above commit GDB figured this out by using inferior_ptid in
process_stop_reply, which contained the ptid of the current
process/thread. This would be fine for single threaded
inferiors (which is the only place using an S packet makes sense), but
in the general case, relying on inferior_ptid for processing a stop is
wrong - there's no reason to believe that what was GDB's current
thread will be the same thread that just stopped in the inferior.
With the above commit the inferior_ptid now has the value null_ptid
inside process_stop_reply, this can be seen in do_target_wait, where
we call switch_to_inferior_no_thread before calling do_target_wait_1.
The problem this causes can be seen in the new test that runs
gdbserver using the flag --disable-packet=T, and causes GDB to throw
this assertion:
inferior.c:279: internal-error: inferior* find_inferior_pid(process_stratum_target*, int): Assertion `pid != 0' failed.
A similar problem was fixed in this commit:
commit 3cada74087687907311b52781354ff551e10a0ed
Date: Thu Jan 11 00:23:04 2018 +0000
Fix backwards compatibility with old GDBservers (PR remote/22597)
However, this commit deals with the case where the T packet doesn't
include a thread-id, not the S packet case. This commit solves the
problem providing a thread-id at the GDB side if the remote target
doesn't provide one. The thread-id provided comes from
remote_state::general_thread, however, though this does work, I don't
think it is the ideal solution.
The remote_state tracks two threads, the continue_thread and the
general_thread, these are updated when GDB asks the remote target to
switch threads. The general_thread is set before performing things
like register or memory accesses, and the continue_thread is set
before things like continue or step commands. Further, the
general_thread is updated after a target stops to reference the thread
that stopped.
The first thing to note from the above description is that we have a
cycle of dependency, when a T packet arrives without a thread-id we
fill in the thread-id from the general_thread data. The thread-id
from the stop event is then used to set the general_thread. This in
itself feels a little weird.
The second question is why use the general_thread at all? You'd think
given how they are originally set that the continue thread would be a
better choice. The problem with this is that the continue_thread, if
the user just does "continue", will be set to the minus_one_ptid, in
the remote protocol this means all threads. When the stop arrives
with no thread-id and we use continue_thread we end up with a very
similar assertion to before because we now end up trying to lookup a
thread using the minus_one_ptid. By contrast, once GDB has connected
to a remote target the general_thread will be set to a valid
thread-id, after which, if the inferior is single threaded, and stop
events arrive without a thread-id, everything works fine.
There is one slight weirdness with the above behaviour though. When
GDB first connects to the remote target inferior_ptid is null_ptid,
however, upon connecting we query the remote for its threads. As the
thread information arrives GDB adds the threads to its internal
database, and this process involves setting inferior_ptid to the id of
each new thread in turn. Once we know about all the threads we wait
for a stop event from the remote target to indicate that GDB is now in
control of the inferior.
The problem is that after adding the new threads we don't reset
inferior_ptid, and the code path we use to wait for the inferior for
this first wait also doesn't reset inferior_ptid, it just turns out
that during the initial connection inferior_ptid is not null_ptid.
This is lucky, because during the initial connection the
general_thread variable _is_ set to null_ptid.
So, during the initial connection, if the first stop event is missing
a thread-id then we "provide" a thead-id from general_thread. This
turns out to also be null_ptid meaning no thread-id is known, and then
during ::process_stop_reply we fill in the missing thread-id using
inferior_ptid.
This was all discussed on the mailing list here:
https://sourceware.org/ml/gdb-patches/2020-02/msg01011.html
My proposal for a fix then is:
1. Move the call to switch_to_inferior_no_thread into
do_target_wait_1, this means that in call cases where we are waiting
for an inferior the inferior_ptid will be set to null_ptid. This is
good as no wait code should rely on inferior_ptid.
2. Remove the use of general_thread from the 'T' packet processing.
The general_thread read here was only ever correct by chance, and we
shouldn't be using it this way.
3. Remove use of inferior_ptid from ::process_stop_event as this is
wrong, and will always be null_ptid now anyway.
4. When a stop_even has null_ptid due to a lack of thread-id (either
from a T packet or an S packet) then pick the first non exited thread
in the inferior and use that. This will be fine for single threaded
inferiors. A multi-threaded inferior really should be using T
packets with a thread-id, so we give a warning if the inferior is
multi-threaded, and we are still missing a thread-id.
5. Extend the existing test that covered the T packet with missing
thread-id to also cover the S packet.
gdb/ChangeLog:
* remote.c (remote_target::remote_parse_stop_reply): Don't use the
general_thread if the stop reply is missing a thread-id.
(remote_target::process_stop_reply): Use the first non-exited
thread if the target didn't pass a thread-id.
* infrun.c (do_target_wait): Move call to
switch_to_inferior_no_thread to ....
(do_target_wait_1): ... here.
gdb/testsuite/ChangeLog:
* gdb.server/stop-reply-no-thread.exp: Add test where T packet is
disabled.
---
gdb/ChangeLog | 10 +++
gdb/infrun.c | 8 ++-
gdb/remote.c | 43 ++++++++----
gdb/testsuite/ChangeLog | 5 ++
gdb/testsuite/gdb.server/stop-reply-no-thread.exp | 80 ++++++++++++++---------
5 files changed, 101 insertions(+), 45 deletions(-)
diff --git a/gdb/infrun.c b/gdb/infrun.c
index d9a6f733519..43199b17b05 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3456,6 +3456,12 @@ do_target_wait_1 (inferior *inf, ptid_t ptid,
ptid_t event_ptid;
struct thread_info *tp;
+ /* We know that we are looking for an event in inferior INF, but we don't
+ know which thread the event might come from. As such we want to make
+ sure that INFERIOR_PTID is reset so that non of the wait code relies
+ on it - doing so is always a mistake. */
+ switch_to_inferior_no_thread (inf);
+
/* First check if there is a resumed thread with a wait status
pending. */
if (ptid == minus_one_ptid || ptid.is_pid ())
@@ -3651,8 +3657,6 @@ do_target_wait (ptid_t wait_ptid, execution_control_state *ecs, int options)
auto do_wait = [&] (inferior *inf)
{
- switch_to_inferior_no_thread (inf);
-
ecs->ptid = do_target_wait_1 (inf, wait_ptid, &ecs->ws, options);
ecs->target = inf->process_target ();
return (ecs->ws.kind != TARGET_WAITKIND_IGNORE);
diff --git a/gdb/remote.c b/gdb/remote.c
index 4a70ab3fb0d..9b73faf9a34 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -7402,18 +7402,14 @@ Packet: '%s'\n"),
reported expedited registers. */
if (event->ptid == null_ptid)
{
+ /* If there is no thread-id information then leave
+ the event->ptid as null_ptid. Later in
+ process_stop_reply we will pick a suitable
+ thread. */
const char *thr = strstr (p1 + 1, ";thread:");
if (thr != NULL)
event->ptid = read_ptid (thr + strlen (";thread:"),
NULL);
- else
- {
- /* Either the current thread hasn't changed,
- or the inferior is not multi-threaded.
- The event must be for the thread we last
- set as (or learned as being) current. */
- event->ptid = event->rs->general_thread;
- }
}
if (rsa == NULL)
@@ -7668,10 +7664,35 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply,
*status = stop_reply->ws;
ptid = stop_reply->ptid;
- /* If no thread/process was reported by the stub, assume the current
- inferior. */
+ /* If no thread/process was reported by the stub then use the first
+ non-exited thread in the current target. */
if (ptid == null_ptid)
- ptid = inferior_ptid;
+ {
+ for (thread_info *thr : all_non_exited_threads (this))
+ {
+ if (ptid != null_ptid)
+ {
+ static bool warned = false;
+
+ if (!warned)
+ {
+ /* If you are seeing this warning then the remote target
+ has multiple threads and either sent an 'S' stop
+ packet, or a 'T' stop packet without a thread-id. In
+ both of these cases GDB is unable to know which thread
+ just stopped and is now having to guess. The correct
+ action is to fix the remote target to send the correct
+ packet (a 'T' packet and include a thread-id). */
+ warning (_("multi-threaded target stopped without sending "
+ "a thread-id, using first non-exited thread"));
+ warned = true;
+ }
+ break;
+ }
+ ptid = thr->ptid;
+ }
+ gdb_assert (ptid != null_ptid);
+ }
if (status->kind != TARGET_WAITKIND_EXITED
&& status->kind != TARGET_WAITKIND_SIGNALLED
diff --git a/gdb/testsuite/gdb.server/stop-reply-no-thread.exp b/gdb/testsuite/gdb.server/stop-reply-no-thread.exp
index 45407bc31de..ffc1c27dcb4 100644
--- a/gdb/testsuite/gdb.server/stop-reply-no-thread.exp
+++ b/gdb/testsuite/gdb.server/stop-reply-no-thread.exp
@@ -32,43 +32,59 @@ if [prepare_for_testing "failed to prepare" $testfile $srcfile] {
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 tests with different features of GDBserver disabled.
+proc run_test { disable_feature } {
+ global binfile gdb_prompt decimal
-# Start GDBserver, with ";thread:NNN" in T stop replies disabled,
-# emulating old gdbservers when debugging single-threaded programs.
-set res [gdbserver_start "--disable-packet=Tthread" $binfile]
-set gdbserver_protocol [lindex $res 0]
-set gdbserver_gdbport [lindex $res 1]
+ clean_restart ${binfile}
-# Disable XML-based thread listing, and multi-process extensions.
-gdb_test_no_output "set remote threads-packet off"
-gdb_test_no_output "set remote multiprocess-feature-packet off"
+ # Make sure we're disconnected, in case we're testing with an
+ # extended-remote board, therefore already connected.
+ gdb_test "disconnect" ".*"
-set res [gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport]
-if ![gdb_assert {$res == 0} "connect"] {
- return
-}
+ set res [gdbserver_start "--disable-packet=${disable_feature}" $binfile]
+ set gdbserver_protocol [lindex $res 0]
+ set gdbserver_gdbport [lindex $res 1]
-# There should be only one thread listed.
-set test "info threads"
-gdb_test_multiple $test $test {
- -re "2 Thread.*$gdb_prompt $" {
- fail $test
- }
- -re "has terminated.*$gdb_prompt $" {
- fail $test
+ # Disable XML-based thread listing, and multi-process extensions.
+ gdb_test_no_output "set remote threads-packet off"
+ gdb_test_no_output "set remote multiprocess-feature-packet off"
+
+ set res [gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport]
+ if ![gdb_assert {$res == 0} "connect"] {
+ return
}
- -re "\\\* 1\[\t \]*Thread\[^\r\n\]*\r\n$gdb_prompt $" {
- pass $test
+
+ # There should be only one thread listed.
+ set test "info threads"
+ gdb_test_multiple $test $test {
+ -re "2 Thread.*$gdb_prompt $" {
+ fail $test
+ }
+ -re "has terminated.*$gdb_prompt $" {
+ fail $test
+ }
+ -re "\\\* 1\[\t \]*Thread\[^\r\n\]*\r\n$gdb_prompt $" {
+ pass $test
+ }
}
-}
-gdb_breakpoint "main"
+ gdb_breakpoint "main"
-# Bad GDB behaved like this:
-# (gdb) c
-# Cannot execute this command without a live selected thread.
-# (gdb)
-gdb_test "c" "Breakpoint $decimal, main.*" "continue to main"
+ # Bad GDB behaved like this:
+ # (gdb) c
+ # Cannot execute this command without a live selected thread.
+ # (gdb)
+ gdb_test "c" "Breakpoint $decimal, main.*" "continue to main"
+}
+
+# Disable different features within gdbserver:
+#
+# Tthread: Start GDBserver, with ";thread:NNN" in T stop replies disabled,
+# emulating old gdbservers when debugging single-threaded programs.
+#
+# T: Start GDBserver with the entire 'T' stop reply packet disabled,
+# GDBserver will instead send the 'S' stop reply.
+foreach_with_prefix to_disable { Tthread T } {
+ run_test $to_disable
+}
--
2.14.5