[review,v2] Handle pending stops from the Windows kernel

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 14cb238..bb441a1 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,27 @@ 
 2019-11-26  Tom Tromey  <tromey@adacore.com>
 
+	PR gdb/22992
+	* windows-nat.c (current_event): Update comment.
+	(last_wait_event, desired_stop_thread_id): New globals.
+	(struct pending_stop): New.
+	(pending_stops): New global.
+	(windows_nat_target) <stopped_by_sw_breakpoint>
+	<supports_stopped_by_sw_breakpoint>: New methods.
+	(windows_fetch_one_register): Add assertions.  Adjust PC.
+	(windows_continue): Handle pending stops.  Suspend other threads
+	when stepping.  Use last_wait_event
+	(wait_for_debug_event): New function.
+	(get_windows_debug_event): Use wait_for_debug_event.  Handle
+	pending stops.  Queue spurious stops.
+	(windows_nat_target::wait): Set stopped_at_breakpoint.
+	(windows_nat_target::kill): Use wait_for_debug_event.
+	* nat/windows-nat.h (struct windows_thread_info)
+	<stopped_at_breakpoint>: New field.
+	* nat/windows-nat.c (windows_thread_info::resume): Clear
+	stopped_at_breakpoint.
+
+2019-11-26  Tom Tromey  <tromey@adacore.com>
+
 	* windows-nat.c (enum thread_disposition_type): New.
 	(thread_rec): Replace "get_context" parameter with "disposition";
 	change type.
diff --git a/gdb/nat/windows-nat.c b/gdb/nat/windows-nat.c
index a98ff42..59b0fac 100644
--- a/gdb/nat/windows-nat.c
+++ b/gdb/nat/windows-nat.c
@@ -49,6 +49,8 @@ 
 {
   if (suspended > 0)
     {
+      stopped_at_breakpoint = false;
+
       if (ResumeThread (h) == (DWORD) -1)
 	{
 	  DWORD err = GetLastError ();
diff --git a/gdb/nat/windows-nat.h b/gdb/nat/windows-nat.h
index 488c9fb..d64ef8e 100644
--- a/gdb/nat/windows-nat.h
+++ b/gdb/nat/windows-nat.h
@@ -71,6 +71,10 @@ 
      inferior thread.  */
   bool reload_context = false;
 
+  /* True if this thread is currently stopped at a breakpoint.  This
+     is used to offset the PC when needed.  */
+  bool stopped_at_breakpoint = false;
+
   /* The name of the thread, allocated by xmalloc.  */
   gdb::unique_xmalloc_ptr<char> name;
 };
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index abd9d55..8448d1c 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -218,8 +218,17 @@ 
 
 /* The process and thread handles for the above context.  */
 
-static DEBUG_EVENT current_event;	/* The current debug event from
-					   WaitForDebugEvent */
+/* The current debug event from WaitForDebugEvent or from a pending
+   stop.  */
+static DEBUG_EVENT current_event;
+
+/* The most recent event from WaitForDebugEvent.  Unlike
+   current_event, this is guaranteed never to come from a pending
+   stop.  This is important because only data from the most recent
+   event from WaitForDebugEvent can be used when calling
+   ContinueDebugEvent.  */
+static DEBUG_EVENT last_wait_event;
+
 static HANDLE current_process_handle;	/* Currently executing process */
 static windows_thread_info *current_thread;	/* Info on currently selected thread */
 
@@ -289,6 +298,37 @@ 
 
 #endif /* 0 */
 
+/* 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.  */
+static DWORD desired_stop_thread_id = -1;
+
+/* 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;
+};
+
+/* 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.  */
+static std::vector<pending_stop> pending_stops;
+
 struct windows_nat_target final : public x86_nat_target<inf_child_target>
 {
   void close () override;
@@ -307,6 +347,16 @@ 
   void fetch_registers (struct regcache *, int) override;
   void store_registers (struct regcache *, int) override;
 
+  bool stopped_by_sw_breakpoint () override
+  {
+    return current_thread->stopped_at_breakpoint;
+  }
+
+  bool supports_stopped_by_sw_breakpoint () override
+  {
+    return true;
+  }
+
   enum target_xfer_status xfer_partial (enum target_object object,
 					const char *annex,
 					gdb_byte *readbuf,
@@ -542,6 +592,10 @@ 
   struct gdbarch *gdbarch = regcache->arch ();
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
 
+  gdb_assert (!gdbarch_read_pc_p (gdbarch));
+  gdb_assert (gdbarch_pc_regnum (gdbarch) >= 0);
+  gdb_assert (!gdbarch_write_pc_p (gdbarch));
+
   if (r == I387_FISEG_REGNUM (tdep))
     {
       long l = *((long *) context_offset) & 0xffff;
@@ -561,7 +615,28 @@ 
       regcache->raw_supply (r, (char *) &l);
     }
   else
-    regcache->raw_supply (r, context_offset);
+    {
+      if (th->stopped_at_breakpoint && r == gdbarch_pc_regnum (gdbarch))
+	{
+	  int size = register_size (gdbarch, r);
+	  if (size == 4)
+	    {
+	      uint32_t value;
+	      memcpy (&value, context_offset, size);
+	      value -= gdbarch_decr_pc_after_break (gdbarch);
+	      memcpy (context_offset, &value, size);
+	    }
+	  else
+	    {
+	      gdb_assert (size == 8);
+	      uint64_t value;
+	      memcpy (&value, context_offset, size);
+	      value -= gdbarch_decr_pc_after_break (gdbarch);
+	      memcpy (context_offset, &value, size);
+	    }
+	}
+      regcache->raw_supply (r, context_offset);
+    }
 }
 
 void
@@ -1297,16 +1372,36 @@ 
 {
   BOOL res;
 
+  desired_stop_thread_id = id;
+
+  /* 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 (("windows_continue - pending stop anticipated, "
+			 "desired=0x%x, item=0x%x\n",
+			 desired_stop_thread_id, item.thread_id));
+	  return TRUE;
+	}
+    }
+
   DEBUG_EVENTS (("ContinueDebugEvent (cpid=%d, ctid=0x%x, %s);\n",
-		  (unsigned) current_event.dwProcessId,
-		  (unsigned) current_event.dwThreadId,
+		  (unsigned) last_wait_event.dwProcessId,
+		  (unsigned) last_wait_event.dwThreadId,
 		  continue_status == DBG_CONTINUE ?
 		  "DBG_CONTINUE" : "DBG_EXCEPTION_NOT_HANDLED"));
 
   for (windows_thread_info *th : thread_list)
-    if ((id == -1 || id == (int) th->tid)
-	&& th->suspended)
+    if (id == -1 || id == (int) th->tid)
       {
+	if (!th->suspended)
+	  continue;
 	if (debug_registers_changed)
 	  {
 	    th->context.ContextFlags |= CONTEXT_DEBUG_REGISTERS;
@@ -1333,9 +1428,15 @@ 
 	  }
 	th->resume ();
       }
+    else
+      {
+	/* When single-stepping a specific thread, other threads must
+	   be suspended.  */
+	th->suspend ();
+      }
 
-  res = ContinueDebugEvent (current_event.dwProcessId,
-			    current_event.dwThreadId,
+  res = ContinueDebugEvent (last_wait_event.dwProcessId,
+			    last_wait_event.dwThreadId,
 			    continue_status);
 
   if (!res)
@@ -1487,6 +1588,17 @@ 
   return TRUE;
 }
 
+/* A wrapper for WaitForDebugEvent that sets "last_wait_event"
+   appropriately.  */
+static BOOL
+wait_for_debug_event (DEBUG_EVENT *event, DWORD timeout)
+{
+  BOOL result = WaitForDebugEvent (event, timeout);
+  if (result)
+    last_wait_event = *event;
+  return result;
+}
+
 /* Get the next event from the child.  Returns a non-zero thread id if the event
    requires handling by WFI (or whatever).  */
 static int
@@ -1499,9 +1611,36 @@ 
   static windows_thread_info dummy_thread_info (0, 0, 0);
   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.  */
+  for (auto iter = pending_stops.begin ();
+       iter != pending_stops.end ();
+       ++iter)
+    {
+      if (desired_stop_thread_id == -1
+	  || desired_stop_thread_id == iter->thread_id)
+	{
+	  thread_id = iter->thread_id;
+	  *ourstatus = iter->status;
+	  current_event = iter->event;
+
+	  inferior_ptid = ptid_t (current_event.dwProcessId, thread_id, 0);
+	  current_thread = thread_rec (thread_id, INVALIDATE_CONTEXT);
+	  current_thread->reload_context = 1;
+
+	  DEBUG_EVENTS (("get_windows_debug_event - "
+			 "pending stop found in 0x%x (desired=0x%x)\n",
+			 thread_id, desired_stop_thread_id));
+
+	  pending_stops.erase (iter);
+	  return thread_id;
+	}
+    }
+
   last_sig = GDB_SIGNAL_0;
 
-  if (!(debug_event = WaitForDebugEvent (&current_event, 1000)))
+  if (!(debug_event = wait_for_debug_event (&current_event, 1000)))
     goto out;
 
   event_count++;
@@ -1671,7 +1810,27 @@ 
 
   if (!thread_id || saw_create != 1)
     {
-      CHECK (windows_continue (continue_status, -1, 0));
+      CHECK (windows_continue (continue_status, desired_stop_thread_id, 0));
+    }
+  else if (desired_stop_thread_id != -1 && desired_stop_thread_id != thread_id)
+    {
+      /* 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)\n",
+		     thread_id, desired_stop_thread_id));
+
+      if (current_event.dwDebugEventCode == EXCEPTION_DEBUG_EVENT
+	  && (current_event.u.Exception.ExceptionRecord.ExceptionCode
+	      == EXCEPTION_BREAKPOINT)
+	  && windows_initialization_done)
+	{
+	  th = thread_rec (thread_id, INVALIDATE_CONTEXT);
+	  th->stopped_at_breakpoint = true;
+	}
+      pending_stops.push_back ({thread_id, *ourstatus, current_event});
+      thread_id = 0;
+      CHECK (windows_continue (continue_status, desired_stop_thread_id, 0));
     }
   else
     {
@@ -1733,7 +1892,21 @@ 
       SetConsoleCtrlHandler (&ctrl_c_handler, FALSE);
 
       if (retval)
-	return ptid_t (current_event.dwProcessId, retval, 0);
+	{
+	  ptid_t result = ptid_t (current_event.dwProcessId, retval, 0);
+
+	  if (current_thread != nullptr)
+	    {
+	      current_thread->stopped_at_breakpoint = false;
+	      if (current_event.dwDebugEventCode == EXCEPTION_DEBUG_EVENT
+		  && (current_event.u.Exception.ExceptionRecord.ExceptionCode
+		      == EXCEPTION_BREAKPOINT)
+		  && windows_initialization_done)
+		current_thread->stopped_at_breakpoint = true;
+	    }
+
+	  return result;
+	}
       else
 	{
 	  int detach = 0;
@@ -2873,7 +3046,7 @@ 
     {
       if (!windows_continue (DBG_CONTINUE, -1, 1))
 	break;
-      if (!WaitForDebugEvent (&current_event, INFINITE))
+      if (!wait_for_debug_event (&current_event, INFINITE))
 	break;
       if (current_event.dwDebugEventCode == EXIT_PROCESS_DEBUG_EVENT)
 	break;