Re: [PATCH] gdb/remote: Ask target for current thread if it doesn't tell us

[Pedro Alves <palves@redhat.com>]

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