gdb: better handling of 'S' packets

This commit builds on work started in the following two commits:

  commit 24ed6739b699f329c2c45aedee5f8c7d2f54e493
  Date:   Thu Jan 30 14:35:40 2020 +0000

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

  commit cada5fc921e39a1945c422eea055c8b326d8d353
  Date:   Wed Mar 11 12:30:13 2020 +0000

      gdb: Handle W and X remote packets without giving a warning

This is related to how GDB handles remote targets that send back 'S'
packets.

In the first of the above commits we fixed GDB's ability to handle a
single process, single threaded target that sends back 'S' packets.
Although the 'T' packet would always be preferred to 'S' these days,
there's nothing really wrong with 'S' for this situation.

The second commit above fixed an oversight in the first commit, a
single-process, multi-threaded target can send back a process wide
event, for example the process exited event 'W' without including a
process-id, this also is fine as there is no ambiguity in this case.

In PR gdb/26819 we run into yet another problem with the above
commits.  In this case we have a single process with two threads, GDB
hits a breakpoint in thread 2 and then performs a stepi:

  (gdb) b main
  Breakpoint 1 at 0x1212340830: file infinite_loop.S, line 10.
  (gdb) c
  Continuing.

  Thread 2 hit Breakpoint 1, main () at infinite_loop.S:10
  10    in infinite_loop.S
  (gdb) set debug remote 1
  (gdb) stepi
  Sending packet: $vCont;s:2#24...Packet received: S05
  ../binutils-gdb/gdb/infrun.c:5807: internal-error: int finish_step_over(execution_control_state*): Assertion `ecs->event_thread->control.trap_expected' failed.

What happens in this case is that on the RISC-V target displaced
stepping is not supported, so when the stepi is issued GDB steps just
thread 2.  As only a single thread was set running the target decides
that is can get away with sending back an 'S' packet without a
thread-id.  GDB then associates the stop with thread 1 (the first
non-exited thread), but as thread 1 was not previously set executing
the assertion seen above triggers.

As an aside I am surprised that the target sends pack 'S' in this
situation.  The target is happy to send back 'T' (including thread-id)
when multiple threads are set running, so (to me) it would seem easier
to just always use the 'T' packet when multiple threads are in use.
However, the target only uses 'T' when multiple threads are actually
executing, otherwise an 'S' packet it used.

Still, when looking at the above situation we can see that GDB should
be able to understand which thread the 'S' reply is referring too.

The problem is that is that in commit 24ed6739b699 (above) when a stop
reply comes in with no thread-id we look for the first non-exited
thread and select that as the thread the stop applies too.

What we should really do is select the first non-exited, resumed thread,
and associate the stop event with this thread.  In the above example
both thread 1 and 2 are non-exited, but only thread 2 is resumed, so
this is what we should use.

There's a test for this issue included which works with stock
gdbserver by disabling use of the 'T' packet, and enabling
'scheduler-locking' within GDB so only one thread is set running.

gdb/ChangeLog:

	PR gdb/26819
	* remote.c
	(remote_target::select_thread_for_ambiguous_stop_reply): New
	member function.
	(remote_target::process_stop_reply): Call
	select_thread_for_ambiguous_stop_reply.

gdb/testsuite/ChangeLog:

	PR gdb/26819
	* gdb.server/stop-reply-no-thread-multi.c: New file.
	* gdb.server/stop-reply-no-thread-multi.exp: New file.

Change-Id: I9b49d76c2a99063dcc76203fa0f5270a72825d15

1 files changed, 108 insertions, 55 deletions

diff --git a/gdb/remote.c b/gdb/remote.c
index a657902..74ebbf9 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -747,6 +747,9 @@ public: /* Remote specific methods.  */
   ptid_t process_stop_reply (struct stop_reply *stop_reply,
 			     target_waitstatus *status);
 
+  ptid_t select_thread_for_ambiguous_stop_reply
+    (const struct target_waitstatus *status);
+
   void remote_notice_new_inferior (ptid_t currthread, int executing);
 
   void process_initial_stop_replies (int from_tty);
@@ -7753,75 +7756,125 @@ remote_notif_get_pending_events (remote_target *remote, notif_client *nc)
   remote->remote_notif_get_pending_events (nc);
 }
 
-/* Called when it is decided that STOP_REPLY holds the info of the
-   event that is to be returned to the core.  This function always
-   destroys STOP_REPLY.  */
+/* Called from process_stop_reply when the stop packet we are responding
+   to didn't include a process-id or thread-id.  STATUS is the stop event
+   we are responding to.
+
+   It is the task of this function to select a suitable thread (or process)
+   and return its ptid, this is the thread (or process) we will assume the
+   stop event came from.
+
+   In some cases there isn't really any choice about which thread (or
+   process) is selected, a basic remote with a single process containing a
+   single thread might choose not to send any process-id or thread-id in
+   its stop packets, this function will select and return the one and only
+   thread.
+
+   However, if a target supports multiple threads (or processes) and still
+   doesn't include a thread-id (or process-id) in its stop packet then
+   first, this is a badly behaving target, and second, we're going to have
+   to select a thread (or process) at random and use that.  This function
+   will print a warning to the user if it detects that there is the
+   possibility that GDB is guessing which thread (or process) to
+   report.
+
+   Note that this is called before GDB fetches the updated thread list from the
+   target.  So it's possible for the stop reply to be ambiguous and for GDB to
+   not realize it.  For example, if there's initially one thread, the target
+   spawns a second thread, and then sends a stop reply without an id that
+   concerns the first thread.  GDB will assume the stop reply is about the
+   first thread - the only thread it knows about - without printing a warning.
+   Anyway, if the remote meant for the stop reply to be about the second thread,
+   then it would be really broken, because GDB doesn't know about that thread
+   yet.  */
 
 ptid_t
-remote_target::process_stop_reply (struct stop_reply *stop_reply,
-				   struct target_waitstatus *status)
+remote_target::select_thread_for_ambiguous_stop_reply
+  (const struct target_waitstatus *status)
 {
-  ptid_t ptid;
+  /* Some stop events apply to all threads in an inferior, while others
+     only apply to a single thread.  */
+  bool process_wide_stop
+    = (status->kind == TARGET_WAITKIND_EXITED
+       || status->kind == TARGET_WAITKIND_SIGNALLED);
 
-  *status = stop_reply->ws;
-  ptid = stop_reply->ptid;
+  thread_info *first_resumed_thread = nullptr;
+  bool ambiguous = false;
 
-  /* If no thread/process was reported by the stub then use the first
-     non-exited thread in the current target.  */
-  if (ptid == null_ptid)
+  /* Consider all non-exited threads of the target, find the first resumed
+     one.  */
+  for (thread_info *thr : all_non_exited_threads (this))
     {
-      /* Some stop events apply to all threads in an inferior, while others
-	 only apply to a single thread.  */
-      bool is_stop_for_all_threads
-	= (status->kind == TARGET_WAITKIND_EXITED
-	   || status->kind == TARGET_WAITKIND_SIGNALLED);
+      remote_thread_info *remote_thr = get_remote_thread_info (thr);
 
-      for (thread_info *thr : all_non_exited_threads (this))
+      if (remote_thr->resume_state () != resume_state::RESUMED)
+	continue;
+
+      if (first_resumed_thread == nullptr)
+	first_resumed_thread = thr;
+      else if (!process_wide_stop
+	       || first_resumed_thread->ptid.pid () != thr->ptid.pid ())
+	ambiguous = true;
+    }
+
+  gdb_assert (first_resumed_thread != nullptr);
+
+  /* Warn if the remote target is sending ambiguous stop replies.  */
+  if (ambiguous)
+    {
+      static bool warned = false;
+
+      if (!warned)
 	{
-	  if (ptid != null_ptid
-	      && (!is_stop_for_all_threads
-		  || ptid.pid () != thr->ptid.pid ()))
-	    {
-	      static bool warned = false;
+	  /* If you are seeing this warning then the remote target has
+	     stopped without specifying a thread-id, but the target
+	     does have multiple threads (or inferiors), and so GDB is
+	     having to guess which thread stopped.
 
-	      if (!warned)
-		{
-		  /* If you are seeing this warning then the remote target
-		     has stopped without specifying a thread-id, but the
-		     target does have multiple threads (or inferiors), and
-		     so GDB is having to guess which thread stopped.
-
-		     Examples of what might cause this are the target
-		     sending and 'S' stop packet, or a 'T' stop packet and
-		     not including a thread-id.
-
-		     Additionally, the target might send a 'W' or 'X
-		     packet without including a process-id, when the target
-		     has multiple running inferiors.  */
-		  if (is_stop_for_all_threads)
-		    warning (_("multi-inferior target stopped without "
-			       "sending a process-id, using first "
-			       "non-exited inferior"));
-		  else
-		    warning (_("multi-threaded target stopped without "
-			       "sending a thread-id, using first "
-			       "non-exited thread"));
-		  warned = true;
-		}
-	      break;
-	    }
+	     Examples of what might cause this are the target sending
+	     and 'S' stop packet, or a 'T' stop packet and not
+	     including a thread-id.
 
-	  /* If this is a stop for all threads then don't use a particular
-	     threads ptid, instead create a new ptid where only the pid
-	     field is set.  */
-	  if (is_stop_for_all_threads)
-	    ptid = ptid_t (thr->ptid.pid ());
+	     Additionally, the target might send a 'W' or 'X packet
+	     without including a process-id, when the target has
+	     multiple running inferiors.  */
+	  if (process_wide_stop)
+	    warning (_("multi-inferior target stopped without "
+		       "sending a process-id, using first "
+		       "non-exited inferior"));
 	  else
-	    ptid = thr->ptid;
+	    warning (_("multi-threaded target stopped without "
+		       "sending a thread-id, using first "
+		       "non-exited thread"));
+	  warned = true;
 	}
-      gdb_assert (ptid != null_ptid);
     }
 
+  /* If this is a stop for all threads then don't use a particular threads
+     ptid, instead create a new ptid where only the pid field is set.  */
+  if (process_wide_stop)
+    return ptid_t (first_resumed_thread->ptid.pid ());
+  else
+    return first_resumed_thread->ptid;
+}
+
+/* Called when it is decided that STOP_REPLY holds the info of the
+   event that is to be returned to the core.  This function always
+   destroys STOP_REPLY.  */
+
+ptid_t
+remote_target::process_stop_reply (struct stop_reply *stop_reply,
+				   struct target_waitstatus *status)
+{
+  *status = stop_reply->ws;
+  ptid_t ptid = stop_reply->ptid;
+
+  /* If no thread/process was reported by the stub then select a suitable
+     thread/process.  */
+  if (ptid == null_ptid)
+    ptid = select_thread_for_ambiguous_stop_reply (status);
+  gdb_assert (ptid != null_ptid);
+
   if (status->kind != TARGET_WAITKIND_EXITED
       && status->kind != TARGET_WAITKIND_SIGNALLED
       && status->kind != TARGET_WAITKIND_NO_RESUMED)