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

  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(-)
  

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
>
  

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