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

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

* Tom Tromey <tromey@adacore.com> [2020-03-09 11:35:52 -0600]:

> Andrew> There was a regression in GDB's support for older aspects of the
> Andrew> remote protocol.  Specifically, when a target sends the 'S' stop reply
> Andrew> packet (which doesn't include a thread-id) then GDB has to figure out
> Andrew> which thread actually stopped.
> 
> With this patch, I started seeing this warning when running the AdaCore
> internal test suite using qemu.
> 
> I investigated a little and the warning comes from this packet:
> 
>     Packet received: W00
>     warning: multi-threaded target stopped without sending a thread-id, using first non-exited thread
> 
> It seems to me that the warning must not be correct in this case, though
> I am not 100% sure, so I thought I would ask.
> 
> My reason is that this is a whole-process exit (so reporting a thread
> doesn't make sense); and that in this run, qemu is not reporting that it
> is multi-process-capable (so reporting a PID doesn't make sense).
> 
> So, I wonder if process_stop_reply should be modified like (beware,
> untested patch):
> 
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 9b73faf9a34..810df658c54 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -7666,7 +7666,7 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply,
>  
>    /* If no thread/process was reported by the stub then use the first
>       non-exited thread in the current target.  */
> -  if (ptid == null_ptid)
> +  if (ptid == null_ptid && stop_reply->stop_reason != TARGET_WAITKIND_EXITED)
>      {
>        for (thread_info *thr : all_non_exited_threads (this))
>  	{
> 
> 
> Perhaps this also applies to some of the other TARGET_WAITKIND_ values
> as well?  TARGET_WAITKIND_SIGNALLED seems similar.

Sorry for the breakage.

Looking through the list I think only W and X stop packets are going
to have this problem, which correspond to TARGET_WAITKIND_EXITED and
TARGET_WAITKIND_SIGNALLED respectively.

I think the patch below will fix your problem, though it would be
great if you could give it a try.

I've only tested this against the specific issue you're seeing, I
still need to clean this up, and add a test, so this is not the final
version.  Still, thoughts and feedback welcome.

My motivation here, is to have ::process_stop_reply return some kind
of valid ptid_t, so in this case, for a process wide event, I'm
creating something like `ptid_t (pid)` where pid comes from the first
thread we find in the target.

Then I only give a warning for process wide events when the pids don't
match.

I'll try to post a cleaned up version with test tomorrow.

Thanks,
Andrew

---

diff --git a/gdb/remote.c b/gdb/remote.c
index 9b73faf9a34..4e201d58cd6 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -7670,7 +7670,10 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply,
     {
       for (thread_info *thr : all_non_exited_threads (this))
 	{
-	  if (ptid != null_ptid)
+	  if (ptid != null_ptid
+	      && ((!(status->kind == TARGET_WAITKIND_EXITED
+		     || status->kind == TARGET_WAITKIND_SIGNALLED))
+		  || (ptid.pid () != thr->ptid.pid ())))
 	    {
 	      static bool warned = false;
 
@@ -7689,7 +7692,12 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply,
 		}
 	      break;
 	    }
-	  ptid = thr->ptid;
+
+	  if (status->kind == TARGET_WAITKIND_EXITED
+	      || status->kind == TARGET_WAITKIND_SIGNALLED)
+	    ptid = ptid_t (thr->ptid.pid ());
+	  else
+	    ptid = thr->ptid;
 	}
       gdb_assert (ptid != null_ptid);
     }