[PATCH v2 20/47] Windows gdb+gdbserver: Elim desired_stop_thread_id / rework pending_stops

[Pedro Alves <pedro@palves.net>]

windows_process.desired_stop_thread_id doesn't work for non-stop, as
in that case every thread will have its own independent
WaitForDebugEvent event.

Instead, detect whether we have been reported a stop that was not
supposed to be reported by simply checking whether the thread that is
reporting the event is suspended.  This is now easilly possible since
each thread's suspend state is kept in sync with whether infrun wants
the thread executing or not.

windows_process.desired_stop_thread_id was also used as thread to pass
to windows_continue.  However, we don't really need that either.
windows_continue is used to update the thread's registers, unsuspend
them, and then finally call ContinueDebugEvent.  In most cases, we
only need the ContinueDebugEvent step, so we can convert the
windows_continue calls to continue_last_debug_event_main_thread calls.
The exception is when we see a thread creation event -- in that case,
we need to update the debug registers of the new thread.  We can use
continue_one_thread for that.

Since the pending stop is now stored in windows_thread_info,
get_windows_debug_event needs to avoid reaching the bottom code if
there's no thread associated with the event anymore (i.e.,
EXIT_THREAD_DEBUG_EVENT / EXIT_PROCESS_DEBUG_EVENT).

I considered whether it would be possible to keep the pending_stop
handling code shared in gdb/nat/windows-nat.c, in this patch and
throughout the series, but I conclused that it isn't worth it, until
gdbserver is taught about async and non-stop as well.

The pending_stop struct will eventually be eliminated later down the
series.

Change-Id: Ib7c8e8d16edc0900b7c411976c5d70cf93931c1c
---
 gdb/nat/windows-nat.c  |  51 -------------------
 gdb/nat/windows-nat.h  |  71 ++++++++++----------------
 gdb/windows-nat.c      | 112 +++++++++++++++++++++++------------------
 gdbserver/win32-low.cc |  55 ++++++++++++--------
 4 files changed, 125 insertions(+), 164 deletions(-)

diff --git a/gdb/nat/windows-nat.c b/gdb/nat/windows-nat.c
index 40a58a0afd9..fbb48f8e3ee 100644
--- a/gdb/nat/windows-nat.c
+++ b/gdb/nat/windows-nat.c
@@ -637,57 +637,6 @@ windows_process_info::add_all_dlls ()
 
 /* See nat/windows-nat.h.  */
 
-bool
-windows_process_info::matching_pending_stop (bool debug_events)
-{
-  /* If there are pending stops, and we might plausibly hit one of
-     them, we don't want to actually continue the inferior -- we just
-     want to report the stop.  In this case, we just pretend to
-     continue.  See the comment by the definition of "pending_stops"
-     for details on why this is needed.  */
-  for (const auto &item : pending_stops)
-    {
-      if (desired_stop_thread_id == -1
-	  || desired_stop_thread_id == item.thread_id)
-	{
-	  DEBUG_EVENTS ("pending stop anticipated, desired=0x%x, item=0x%x",
-			desired_stop_thread_id, item.thread_id);
-	  return true;
-	}
-    }
-
-  return false;
-}
-
-/* See nat/windows-nat.h.  */
-
-std::optional<pending_stop>
-windows_process_info::fetch_pending_stop (bool debug_events)
-{
-  std::optional<pending_stop> result;
-  for (auto iter = pending_stops.begin ();
-       iter != pending_stops.end ();
-       ++iter)
-    {
-      if (desired_stop_thread_id == -1
-	  || desired_stop_thread_id == iter->thread_id)
-	{
-	  result = *iter;
-	  current_event = iter->event;
-
-	  DEBUG_EVENTS ("pending stop found in 0x%x (desired=0x%x)",
-			iter->thread_id, desired_stop_thread_id);
-
-	  pending_stops.erase (iter);
-	  break;
-	}
-    }
-
-  return result;
-}
-
-/* See nat/windows-nat.h.  */
-
 BOOL
 continue_last_debug_event (DWORD continue_status, bool debug_events)
 {
diff --git a/gdb/nat/windows-nat.h b/gdb/nat/windows-nat.h
index 9f1ecb4429b..fec6becb594 100644
--- a/gdb/nat/windows-nat.h
+++ b/gdb/nat/windows-nat.h
@@ -38,6 +38,21 @@
 namespace windows_nat
 {
 
+/* Info about a potential pending stop.  Each thread holds one of
+   these.  See "windows_thread_info::pending_stop" for more
+   information.  */
+struct pending_stop
+{
+  /* The target waitstatus we computed.  TARGET_WAITKIND_IGNORE if the
+     thread does not have a pending stop.  */
+  target_waitstatus status;
+
+  /* The event.  A few fields of this can be referenced after a stop,
+     and it seemed simplest to store the entire event.  */
+  DEBUG_EVENT event;
+};
+
+
 /* Thread information structure used to track extra information about
    each thread.  */
 struct windows_thread_info
@@ -77,6 +92,18 @@ struct windows_thread_info
      was not.  */
   int suspended = 0;
 
+/* Info about a potential pending stop.
+
+   Sometimes, Windows will report a stop on a thread that has been
+   ostensibly suspended.  We believe what happens here is that two
+   threads hit a breakpoint simultaneously, and the Windows kernel
+   queues the stop events.  However, this can result in the strange
+   effect of trying to single step thread A -- leaving all other
+   threads suspended -- and then seeing a stop in thread B.  To handle
+   this scenario, we queue all such "pending" stops here, and then
+   process them once the step has completed.  See PR gdb/22992.  */
+  struct pending_stop pending_stop {};
+
   /* The context of the thread, including any manipulations.  */
   union
   {
@@ -103,22 +130,6 @@ struct windows_thread_info
   gdb::unique_xmalloc_ptr<char> name;
 };
 
-
-/* A single pending stop.  See "pending_stops" for more
-   information.  */
-struct pending_stop
-{
-  /* The thread id.  */
-  DWORD thread_id;
-
-  /* The target waitstatus we computed.  */
-  target_waitstatus status;
-
-  /* The event.  A few fields of this can be referenced after a stop,
-     and it seemed simplest to store the entire event.  */
-  DEBUG_EVENT event;
-};
-
 enum handle_exception_result
 {
   HANDLE_EXCEPTION_UNHANDLED = 0,
@@ -142,22 +153,6 @@ struct windows_process_info
      stop.  */
   DEBUG_EVENT current_event {};
 
-  /* The ID of the thread for which we anticipate a stop event.
-     Normally this is -1, meaning we'll accept an event in any
-     thread.  */
-  DWORD desired_stop_thread_id = -1;
-
-  /* A vector of pending stops.  Sometimes, Windows will report a stop
-     on a thread that has been ostensibly suspended.  We believe what
-     happens here is that two threads hit a breakpoint simultaneously,
-     and the Windows kernel queues the stop events.  However, this can
-     result in the strange effect of trying to single step thread A --
-     leaving all other threads suspended -- and then seeing a stop in
-     thread B.  To handle this scenario, we queue all such "pending"
-     stops here, and then process them once the step has completed.  See
-     PR gdb/22992.  */
-  std::vector<pending_stop> pending_stops;
-
   /* Contents of $_siginfo */
   EXCEPTION_RECORD siginfo_er {};
 
@@ -223,18 +218,6 @@ struct windows_process_info
 
   void add_all_dlls ();
 
-  /* Return true if there is a pending stop matching
-     desired_stop_thread_id.  If DEBUG_EVENTS is true, logging will be
-     enabled.  */
-
-  bool matching_pending_stop (bool debug_events);
-
-  /* See if a pending stop matches DESIRED_STOP_THREAD_ID.  If so,
-     remove it from the list of pending stops, set 'current_event', and
-     return it.  Otherwise, return an empty optional.  */
-
-  std::optional<pending_stop> fetch_pending_stop (bool debug_events);
-
   const char *pid_to_exec_file (int);
 
   template<typename Function>
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 2bed29edc50..f86e6ae9a28 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -1262,15 +1262,22 @@ BOOL
 windows_nat_target::windows_continue (DWORD continue_status, int id,
 				      windows_continue_flags cont_flags)
 {
-  windows_process.desired_stop_thread_id = id;
-
-  if (windows_process.matching_pending_stop (debug_events))
+  for (auto &th : windows_process.thread_list)
     {
-      /* There's no need to really continue, because there's already
-	 another event pending.  However, we do need to inform the
-	 event loop of this.  */
-      serial_event_set (m_wait_event);
-      return TRUE;
+      if ((id == -1 || id == (int) th->tid)
+	  && !th->suspended
+	  && th->pending_stop.status.kind () != TARGET_WAITKIND_IGNORE)
+	{
+	  DEBUG_EVENTS ("got matching pending stop event "
+			"for 0x%x, not resuming",
+			th->tid);
+
+	  /* There's no need to really continue, because there's already
+	     another event pending.  However, we do need to inform the
+	     event loop of this.  */
+	  serial_event_set (m_wait_event);
+	  return TRUE;
+	}
     }
 
   for (auto &th : windows_process.thread_list)
@@ -1444,21 +1451,24 @@ windows_nat_target::get_windows_debug_event
   DWORD thread_id = 0;
 
   /* If there is a relevant pending stop, report it now.  See the
-     comment by the definition of "pending_stops" for details on why
-     this is needed.  */
-  std::optional<pending_stop> stop
-    = windows_process.fetch_pending_stop (debug_events);
-  if (stop.has_value ())
+     comment by the definition of "windows_thread_info::pending_stop"
+     for details on why this is needed.  */
+  for (auto &th : windows_process.thread_list)
     {
-      thread_id = stop->thread_id;
-      *ourstatus = stop->status;
-      windows_process.current_event = stop->event;
+      if (!th->suspended
+	  && th->pending_stop.status.kind () != TARGET_WAITKIND_IGNORE)
+	{
+	  DEBUG_EVENTS ("reporting pending event for 0x%x", th->tid);
+
+	  thread_id = th->tid;
+	  *ourstatus = th->pending_stop.status;
+	  th->pending_stop.status.set_ignore ();
+	  windows_process.current_event = th->pending_stop.event;
 
-      ptid_t ptid (windows_process.current_event.dwProcessId, thread_id);
-      windows_thread_info *th = windows_process.find_thread (ptid);
-      if (th != nullptr)
-	windows_process.invalidate_context (th);
-      return ptid;
+	  ptid_t ptid (windows_process.process_id, thread_id);
+	  windows_process.invalidate_context (th.get ());
+	  return ptid;
+	}
     }
 
   windows_process.last_sig = GDB_SIGNAL_0;
@@ -1500,12 +1510,18 @@ windows_nat_target::get_windows_debug_event
 	}
       /* Record the existence of this thread.  */
       thread_id = current_event->dwThreadId;
-      add_thread
-	(ptid_t (current_event->dwProcessId, current_event->dwThreadId, 0),
-	 current_event->u.CreateThread.hThread,
-	 current_event->u.CreateThread.lpThreadLocalBase,
-	 false /* main_thread_p */);
 
+      {
+	windows_thread_info *th
+	  = (add_thread
+	     (ptid_t (current_event->dwProcessId, current_event->dwThreadId, 0),
+	      current_event->u.CreateThread.hThread,
+	      current_event->u.CreateThread.lpThreadLocalBase,
+	      false /* main_thread_p */));
+
+	/* This updates debug registers if necessary.  */
+	windows_process.continue_one_thread (th, 0);
+      }
       break;
 
     case EXIT_THREAD_DEBUG_EVENT:
@@ -1517,6 +1533,7 @@ windows_nat_target::get_windows_debug_event
 			     current_event->dwThreadId, 0),
 		     current_event->u.ExitThread.dwExitCode,
 		     false /* main_thread_p */);
+      thread_id = 0;
       break;
 
     case CREATE_PROCESS_DEBUG_EVENT:
@@ -1567,8 +1584,7 @@ windows_nat_target::get_windows_debug_event
 	    ourstatus->set_exited (exit_status);
 	  else
 	    ourstatus->set_signalled (gdb_signal_from_host (exit_signal));
-
-	  thread_id = current_event->dwThreadId;
+	  return ptid_t (current_event->dwProcessId);
 	}
       break;
 
@@ -1658,17 +1674,22 @@ windows_nat_target::get_windows_debug_event
 
   if (!thread_id || windows_process.saw_create != 1)
     {
-      CHECK (windows_continue (continue_status,
-			       windows_process.desired_stop_thread_id, 0));
+      continue_last_debug_event_main_thread
+	(_("Failed to resume program execution"), continue_status);
+      ourstatus->set_ignore ();
+      return null_ptid;
     }
-  else if (windows_process.desired_stop_thread_id != -1
-	   && windows_process.desired_stop_thread_id != thread_id)
+
+  const ptid_t ptid = ptid_t (current_event->dwProcessId, thread_id, 0);
+  windows_thread_info *th = windows_process.find_thread (ptid);
+
+  if (th->suspended)
     {
       /* Pending stop.  See the comment by the definition of
 	 "pending_stops" for details on why this is needed.  */
       DEBUG_EVENTS ("get_windows_debug_event - "
-		    "unexpected stop in 0x%x (expecting 0x%x)",
-		    thread_id, windows_process.desired_stop_thread_id);
+		    "unexpected stop in suspended thread 0x%x",
+		    thread_id);
 
       if (current_event->dwDebugEventCode == EXCEPTION_DEBUG_EVENT
 	  && ((current_event->u.Exception.ExceptionRecord.ExceptionCode
@@ -1677,24 +1698,19 @@ windows_nat_target::get_windows_debug_event
 		  == STATUS_WX86_BREAKPOINT))
 	  && windows_process.windows_initialization_done)
 	{
-	  ptid_t ptid = ptid_t (current_event->dwProcessId, thread_id, 0);
-	  windows_thread_info *th = windows_process.find_thread (ptid);
 	  th->stopped_at_software_breakpoint = true;
 	  th->pc_adjusted = false;
 	}
-      windows_process.pending_stops.push_back
-	({thread_id, *ourstatus, windows_process.current_event});
-      thread_id = 0;
-      CHECK (windows_continue (continue_status,
-			       windows_process.desired_stop_thread_id, 0));
-    }
-
-  if (thread_id == 0)
-    {
+      th->pending_stop.status = *ourstatus;
+      th->pending_stop.event = *current_event;
       ourstatus->set_ignore ();
+
+      continue_last_debug_event_main_thread
+	(_("Failed to resume program execution"), continue_status);
       return null_ptid;
     }
-  return ptid_t (windows_process.current_event.dwProcessId, thread_id, 0);
+
+  return ptid;
 }
 
 /* Wait for interesting events to occur in the target process.  */
@@ -1720,8 +1736,8 @@ windows_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 
       if (ourstatus->kind () == TARGET_WAITKIND_SPURIOUS)
 	{
-	  CHECK (windows_continue (DBG_CONTINUE,
-				   windows_process.desired_stop_thread_id, 0));
+	  continue_last_debug_event_main_thread
+	    (_("Failed to resume program execution"), DBG_CONTINUE);
 	}
       else if (ourstatus->kind () != TARGET_WAITKIND_IGNORE)
 	{
diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc
index a23a41b9c02..32464024722 100644
--- a/gdbserver/win32-low.cc
+++ b/gdbserver/win32-low.cc
@@ -408,9 +408,15 @@ continue_one_thread (thread_info *thread, int thread_id)
 static BOOL
 child_continue (DWORD continue_status, int thread_id)
 {
-  windows_process.desired_stop_thread_id = thread_id;
-  if (windows_process.matching_pending_stop (debug_threads))
-    return TRUE;
+  process_info *proc = find_process_pid (windows_process.process_id);
+  for (thread_info &thread : proc->thread_list ())
+    {
+      auto *th = static_cast<windows_thread_info *> (thread.target_data ());
+      if ((thread_id == -1 || thread_id == (int) th->tid)
+	  && !th->suspended
+	  && th->pending_stop.status.kind () != TARGET_WAITKIND_IGNORE)
+	return TRUE;
+    }
 
   /* The inferior will only continue after the ContinueDebugEvent
      call.  */
@@ -980,15 +986,21 @@ get_child_debug_event (DWORD *continue_status,
 
   windows_process.attaching = 0;
   {
-    std::optional<pending_stop> stop
-      = windows_process.fetch_pending_stop (debug_threads);
-    if (stop.has_value ())
+    process_info *proc = find_process_pid (windows_process.process_id);
+    for (thread_info &thread : proc->thread_list ())
       {
-	*ourstatus = stop->status;
-	windows_process.current_event = stop->event;
-	ptid = debug_event_ptid (&windows_process.current_event);
-	switch_to_thread (find_thread_ptid (ptid));
-	return 1;
+	auto *th = static_cast<windows_thread_info *> (thread.target_data ());
+
+	if (!th->suspended
+	    && th->pending_stop.status.kind () != TARGET_WAITKIND_IGNORE)
+	  {
+	    *ourstatus = th->pending_stop.status;
+	    th->pending_stop.status.set_ignore ();
+	    windows_process.current_event = th->pending_stop.event;
+	    ptid = debug_event_ptid (&windows_process.current_event);
+	    switch_to_thread (find_thread_ptid (ptid));
+	    return 1;
+	  }
       }
 
     /* Keep the wait time low enough for comfortable remote
@@ -1080,7 +1092,7 @@ get_child_debug_event (DWORD *continue_status,
 	else
 	  ourstatus->set_signalled (gdb_signal_from_host (exit_signal));
       }
-      child_continue (DBG_CONTINUE, windows_process.desired_stop_thread_id);
+      continue_last_debug_event (DBG_CONTINUE, debug_threads);
       break;
 
     case LOAD_DLL_DEBUG_EVENT:
@@ -1137,17 +1149,19 @@ get_child_debug_event (DWORD *continue_status,
 
   ptid = debug_event_ptid (&windows_process.current_event);
 
-  if (windows_process.desired_stop_thread_id != -1
-      && windows_process.desired_stop_thread_id != ptid.lwp ())
+  windows_thread_info *th = windows_process.find_thread (ptid);
+
+  if (th != nullptr && th->suspended)
     {
       /* Pending stop.  See the comment by the definition of
-	 "pending_stops" for details on why this is needed.  */
+	 "windows_thread_info::pending_stop" for details on why this
+	 is needed.  */
       OUTMSG2 (("get_windows_debug_event - "
-		"unexpected stop in 0x%lx (expecting 0x%x)\n",
-		ptid.lwp (), windows_process.desired_stop_thread_id));
+		"unexpected stop in suspended thread 0x%x\n",
+		th->tid));
       maybe_adjust_pc ();
-      windows_process.pending_stops.push_back
-	({(DWORD) ptid.lwp (), *ourstatus, *current_event});
+      th->pending_stop.status = *ourstatus;
+      th->pending_stop.event = *current_event;
       ourstatus->set_spurious ();
     }
   else
@@ -1207,8 +1221,7 @@ win32_process_target::wait (ptid_t ptid, target_waitstatus *ourstatus,
 	  [[fallthrough]];
 	case TARGET_WAITKIND_SPURIOUS:
 	  /* do nothing, just continue */
-	  child_continue (continue_status,
-			  windows_process.desired_stop_thread_id);
+	  continue_last_debug_event (continue_status, debug_threads);
 	  break;
 	}
     }
-- 
2.49.0