[PATCHv2] gdb/remote: Restore support for 'S' stop reply packet
Pedro Alves
palves@redhat.com
Fri Feb 28 17:46:00 GMT 2020
On 2/28/20 2:02 PM, Andrew Burgess wrote:
> * Pedro Alves <palves@redhat.com> [2020-02-27 19:46:12 +0000]:
>
>> 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
>
> Thanks, this looks super useful. I thought I'd write a test using
> this feature but it turns out there's already
> gdb.server/stop-reply-no-thread.exp which does
> --disable-packet=Tthreads.
>
> So, I run the test (without my patch) and ... it passes. The reason:
>
> commit 3cada74087687907311b52781354ff551e10a0ed
> Author: Pedro Alves <palves@redhat.com>
> Date: Thu Jan 11 00:23:04 2018 +0000
>
> Fix backwards compatibility with old GDBservers (PR remote/22597)
Ah... I did have a feeling of deja vu...
Thanks for digging it out.
>
> Specifically, this hunk:
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 81c772a5451..a1cd9ae1df3 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -6940,7 +6940,13 @@ Packet: '%s'\n"),
> event->ptid = read_ptid (thr + strlen (";thread:"),
> NULL);
> else
> - event->ptid = magic_null_ptid;
> + {
> + /* 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)
>
> This code sets the stop_reply::ptid to the general_thread if we have
> no other thread specified in a 'T' packet. I haven't looked at the
> gdbserver code yet, but from your description I don't think I can stop
> gdbserver sending a 'T' and revert back to an 'S'.
Right, you can't.
>
> It seems a little weird that we use general_thread here, I don't see
> why that would be any more valid than the continue_thread, and you
> might think that if we set a continue_thread just before we execute
> the inferior then the stop event is possibly going to be for the
> continue thread.
>
> Still, like you say, the design of the old mechanism is horribly
> broken anyway, so I suspect there's little point worrying too much
> about changing this stuff.
>
> So, my next thought was maybe I should be doing something similar,
> instead of "fixing" a missing ptid in process_stop_event, fill in a
> valid ptid earlier, so I did this:
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 4ac359ad5d9..67c5f298ee6 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -7492,6 +7492,8 @@ Packet: '%s'\n"),
> event->ws.value.sig = (enum gdb_signal) sig;
> else
> event->ws.value.sig = GDB_SIGNAL_UNKNOWN;
> + if (event->ptid == null_ptid)
> + event->ptid = event->rs->general_thread;
> }
> break;
> case 'w': /* Thread exited. */
>
> And this seems to do the trick.
>
> However, I'm still worried by the code in ::process_stop_event that
> says:
>
> if (ptid == null_ptid)
> ptid = inferior_ptid;
>
> After all, we know that as part of do_target_wait the inferior_ptid is
> reset to null_ptid, so this code should be pointless now, right...
Right. Relying on inferior_ptid when processing events is just wrong
and should be removed.
>
> So I tried deleting it, and then both the stop-reply-no-thread.exp,
> and my local testing with my minimal gdb stub failed because once
> again we end up in ::process_stop_event with stop_reply->ptid being
> null_ptid - which now is coming from general_thread.
>
> Out of interest I tried replacing both the uses of general_thread (one
> in the 'T' packet and one I just added in the 'S' packet processing)
> with continue_thread, and this didn't do any better, though now the
> assertion that triggers is complaining that we end up using
> minus_one_ptid, and this leads to my next thought...
Right.
>
> I think that general_thread and continue_thread are more
> representative of what GDB has asked of the inferior, rather than what
> the inferior is telling GDB (maybe?). Let me explain my thinking:
> GDB updates the general_thread to a valid (something other than
> null_ptid, minus_one_ptid, etc) in only two places:
>
> (a) _AFTER_ processing a stop reply packet, to make the
> general_thread match the thread that stopped, this is based on the
> ptid that was parsed out of the stop reply, or
>
> (b) When we send a 'H' packet to the remote, setting either the
> general 'g' or continue 'c' thread variable, which is done by GDB
> just before an action.
>
> Now relying on (a) to set general_thread so we can read it on the
> _next_ stop clearly makes no sense - what stopped this time has no
> relevance to what might stop next time, and similarly, relying on the
> value of general_thread set via the 'H' packet is clearly just wrong,
> as GDB must send a 'Hc' (and sets the continue_thread) to pick which
> thread should continue.
The theory of the original fix was that if the stub isn't sending
a thread id in the stop reply, then there's only one thread in
the remote target, so the thread recorded in general_thread is
going to be the right thread anyway. As you noticed, continue_thread
is going to be minus_one_ptid when we tell the stub to resume
all threads, so continue_thread was a better choice. I think.
>
> I think what I'm arguing is that using general_thread as a fall-back
> when processing 'T' (and 'S') is wrong, we should instead in these
> cases deliberately leave the stop event ptid as null_ptid (indicating
> that the stop didn't name a thread), and then use my patch (with your
> fixes) to pick a non-exited thread.
That sounds good to me. Seems safer / better than relying on
general_thread pointing to a thread, since in some cases it can
point to a while process, or to no process/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 send an 'S' stop
You mean "send" -> "sent", I think.
Thanks,
Pedro Alves
More information about the Gdb-patches
mailing list