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

[Andrew Burgess <andrew.burgess@embecosm.com>]

Any feedback on this patch?  I'd like to get this, or something like
this merged soon-ish.

Thanks,
Andrew

* Andrew Burgess <andrew.burgess@embecosm.com> [2020-01-31 00:06:00 +0000]:

> 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.
> 
> 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.  */
>    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 ());
> +	  gdb_assert (thread != nullptr);
> +	  ptid = thread->ptid;
> +	}
> +    }
>  
>    if (status->kind != TARGET_WAITKIND_EXITED
>        && status->kind != TARGET_WAITKIND_SIGNALLED
> -- 
> 2.14.5
>