[PATCHv2] gdb/remote: Restore support for 'S' stop reply packet
Commit Message
Pedro,
Thanks for your feedback. I agree with all your points, this version
of the patch:
1. Doesn't query the target for the current thread, but just uses
the first non-exited thread.
2. Gives a warning if the target has more than one non-exited
thread.
3. Commit message is revised to reflect new implementation.
How's this?
Thanks,
Andrew
---
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. 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 solution I propose in this commit is to use the first non exited
thread of the inferior. Additionally, GDB will give a warning if a
multi-threaded target stops without passing a thread-id.
There's no test included with this commit, if anyone has any ideas for
how we could test this aspect of the remote protocol (short of writing
a new gdbserver) then I'd love to know.
It is possible to trigger this bug by attaching GDB to a running GDB,
place a breakpoint on remote_parse_stop_reply, and manually change the
contents of buf - when we get a 'T' based stop packet, replace it with
an 'S' based packet, like this:
(gdb) call memset (buf, "S05\0", 4)
After this the GDB that is performing the remote debugging will crash
with this error:
inferior.c:279: internal-error: inferior* find_inferior_pid(process_stratum_target*, int): Assertion `pid != 0' failed.
gdb/ChangeLog:
* remote.c (remote_target::process_stop_reply): Use the first
non-exited thread if the target didn't pass a thread-id.
---
gdb/ChangeLog | 5 +++++
gdb/remote.c | 18 +++++++++++++++---
2 files changed, 20 insertions(+), 3 deletions(-)
Comments
On 2/27/20 4:17 PM, Andrew Burgess wrote:
> The solution I propose in this commit is to use the first non exited
> thread of the inferior.
s/of the inferior/of the target/
> Additionally, GDB will give a warning if a
> multi-threaded target stops without passing a thread-id.
>
> There's no test included with this commit, if anyone has any ideas for
> how we could test this aspect of the remote protocol (short of writing
> a new gdbserver) then I'd love to know.
I forgot to mention this earlier, but I think we could test this by using
gdbserver's --disable-packet=Tthread option (or --disable-packet=threads
to disable vCont as well.). This sets the disable_packet_Tthread global in
gdbserver, which makes it skip including the thread in the T stop reply.
$ gdbserver --disable-packet
Disableable packets:
vCont All vCont packets
qC Querying the current thread
qfThreadInfo Thread listing
Tthread Passing the thread specifier in the T stop reply packet
threads All of the above
I added these options several years ago exactly to exercise support
for old non-threaded protocol. ISTR having documented them, ...
... ah, here:
https://sourceware.org/ml/gdb-patches/2008-06/msg00501.html
doesn't look like that patch was ever merged. :-/
>
> - /* 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 inferior. */
s/current inferior/current target/
> if (ptid == null_ptid)
> - ptid = inferior_ptid;
> + {
> + for (thread_info *thr : all_non_exited_threads (this))
> + {
> + if (ptid != null_ptid)
> + {
> + warning (_("multi-threaded target stopped without sending "
> + "a thread-id, using first non-exited thread"));
I wonder about making this only warn once, to avoid a flood of warnings.
Like:
if (!warned)
{
warned = true;
warning (....);
}
We use that same pattern in other places we only want to warn once.
E.g., parse_memory_map.
Otherwise LGTM.
Thanks,
Pedro Alves
@@ -7668,10 +7668,22 @@ 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 inferior. */
if (ptid == null_ptid)
- ptid = inferior_ptid;
+ {
+ for (thread_info *thr : all_non_exited_threads (this))
+ {
+ if (ptid != null_ptid)
+ {
+ warning (_("multi-threaded target stopped without sending "
+ "a thread-id, using first non-exited thread"));
+ break;
+ }
+ ptid = thr->ptid;
+ }
+ gdb_assert (ptid != null_ptid);
+ }
if (status->kind != TARGET_WAITKIND_EXITED
&& status->kind != TARGET_WAITKIND_SIGNALLED