[PATCH] gdb/remote: Ask target for current thread if it doesn't tell us
Pedro Alves
palves@redhat.com
Wed Feb 26 19:28:00 GMT 2020
On 1/31/20 12:06 AM, Andrew Burgess wrote:
> 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 that, if we don't get a
> thread id in the stop reply, then we should just ask the target for
> the current thread.
I'm really not sure it is worth it to add an extra remote roundtrip for
every single-step. If the stub uses the S stop reply, then it must be
it doesn't really support multiple threads. Otherwise, even before
the multi-target patch, multi-threading support would be broken
when the target stopped for a thread different from GDB's current
thread (the inferior_ptid). The "Hc" ("set continue thread") + "s/c/S/C"
(step/continue) mechanism was broken by design for multi-threading and
led to the creation of vCont + T many years ago.
So I think that making GDB use the first thread of the target is
sufficient and basically restores GDB to its previous behavior.
>
> 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): Don't use
> inferior_ptid, instead ask the remote for the current thread.
>
> Change-Id: I06f6fe5cc9a5cfc6d7157772aa7582f44c413bd5
> ---
> gdb/ChangeLog | 5 +++++
> gdb/remote.c | 18 +++++++++++++++---
> 2 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index be2987707ff..94ed57ebf33 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -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
> + thread in the current inferior. */
I suspect this comment was written before you made the code query the
remote side? I was more expecting that this comment here would say
something like
/* If no thread/process was included in the stop reply, then query
the remote current thread. */
> if (ptid == null_ptid)
> - ptid = inferior_ptid;
> + {
> + ptid = remote_current_thread (null_ptid);
> + if (ptid == null_ptid)
> + {
> + /* We didn't get a useful answer back from the remote target so
> + we need to pick something - we can't just report null_ptid.
> + Lets just pick the first thread in GDB's current inferior. */
> + struct thread_info *thread
> + = first_thread_of_inferior (current_inferior ());
To be extra pedantic, it should be the first non-exited thread of the
target instead of the first thread of the current inferior.
Something around:
for (thread_info *thr : all_non_exited_threads (this))
{
ptid = thr->ptid;
break;
}
gdb_assert (ptid != null_ptid);
> + gdb_assert (thread != nullptr);
> + ptid = thread->ptid;
> + }
> + }
>
Thanks,
Pedro Alves
More information about the Gdb-patches
mailing list