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

[Pedro Alves <palves@redhat.com>]

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