Re: [PATCHv2] gdb/remote: Restore support for 'S' stop reply packet

[Pedro Alves <palves@redhat.com>]

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