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

  Pedro,

Thanks for your feedback.  I agree with all your points, this version
of the patch:

  1. Doesn't query the target for the current thread, but just uses
  the first non-exited thread.

  2. Gives a warning if the target has more than one non-exited
  thread.

  3. Commit message is revised to reflect new implementation.

How's this?

Thanks,
Andrew

---

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 to use the first non exited
thread of the inferior.  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.

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): Use the first
	non-exited thread if the target didn't pass a thread-id.
---
 gdb/ChangeLog |  5 +++++
 gdb/remote.c  | 18 +++++++++++++++---
 2 files changed, 20 insertions(+), 3 deletions(-)
  

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

doesn't look like that patch was ever merged.  :-/

>  
> -  /* 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 inferior.  */

s/current inferior/current target/

>    if (ptid == null_ptid)
> -    ptid = inferior_ptid;
> +    {
> +      for (thread_info *thr : all_non_exited_threads (this))
> +	{
> +	  if (ptid != null_ptid)
> +	    {
> +	      warning (_("multi-threaded target stopped without sending "
> +			 "a thread-id, using first non-exited thread"));

I wonder about making this only warn once, to avoid a flood of warnings.
Like:

    if (!warned)
      {
        warned = true;
        warning (....);
      }

We use that same pattern in other places we only want to warn once.
E.g., parse_memory_map.

Otherwise LGTM.

Thanks,
Pedro Alves