mirror/binutils-gdb
2
0
Fork 0
mirror of https://sourceware.org/git/binutils-gdb.git synced 2024-12-15 04:31:49 +08:00
Code Issues Packages Projects Releases Wiki Activity

gdb/remote: Restore support for 'S' stop reply packet

Browse Source 
With this commit:

  commit 5b6d1e4fa4
  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
targets (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 target.

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 3cada74087
  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 target 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 target.

The problem is that after adding the new threads we don't reset
inferior_ptid, and the code path we use to wait for a stop event from
the target also doesn't reset inferior_ptid, so it 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 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 all 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_event 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 target and use that.  This will be fine for single threaded
 targets.  A multi-thread or multi-inferior aware remote target
 should be using T packets with a thread-id, so we give a warning if
 the target 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.
This commit is contained in:
Andrew Burgess 2020-01-30 14:35:40 +00:00
parent 442131c1be
commit 24ed6739b6
5 changed files with 103 additions and 47 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
gdb/ChangeLog
View File

@ -1,3 +1,13 @@
2020-03-02 Andrew Burgess <andrew.burgess@embecosm.com>
* 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.
2020-02-29 Jon Turney <jon.turney@dronecode.org.uk>
* debuginfod-support.c: Include defs.h first.

8
gdb/infrun.c
View File

@ -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 the target of 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 none 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);

43
gdb/remote.c
View File

@ -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

5
gdb/testsuite/ChangeLog
View File

@ -1,3 +1,8 @@
2020-03-02 Andrew Burgess <andrew.burgess@embecosm.com>
* gdb.server/stop-reply-no-thread.exp: Add test where T packet is
disabled.
2020-03-02 Pedro Alves <palves@redhat.com>
Tom de Vries <tdevries@suse.de>

84
gdb/testsuite/gdb.server/stop-reply-no-thread.exp
View File

@ -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]
# 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
}
# 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"
# 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"
}
# 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
}
# 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
}
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"