[PATCH 33/34] Windows gdb: Watchpoints while running (internal vs external stops)

[Pedro Alves <pedro@palves.net>]

Teach the Windows target to temporarily pause all threads when we
change the debug registers for a watchpoint.  Implements the same
logic as Linux uses:

    ~~~
      /*                             (...) if threads are running when the
         mirror changes, a temporary and transparent stop on all threads
         is forced so they can get their copy of the debug registers
         updated on re-resume.   (...)  */
    ~~~

On Linux, we send each thread a SIGSTOP to step them.  On Windows,
SuspendThread itself doesn't cause any asynchronous debug event to be
reported.  However, we've implemented windows_nat_target::stop such
that it uses SuspendThread, and then queues a pending GDB_SIGNAL_0
stop on the thread.  That results in a user-visible stop, while here
we want a non-user-visible stop.  So what we do is re-use that
windows_nat_target::stop stopping mechanism, but add an external vs
internal stopping kind distinction.  An internal stop results in
windows_nat_target::wait immediately re-resuming the thread.

Note we don't make the debug registers poking code SuspendThread ->
write debug registers -> ContinueThread itself, because SuspendThread
is actually asynchronous and may take a bit to stop the thread (a
following GetThreadContext blocks until the thread is actually
suspended), and, there will be several debug register writes when a
watchpoint is set, because we have to set all of DR0, DR1, DR2, DR3,
and DR7.  Defering the actualy writes to ::wait avoids a bunch of
SuspendThread/ResumeThread sequences, so in principle should be
faster.

Change-Id: I39c2492c7aac06d23ef8f287f4afe3747b7bc53f
---
 gdb/nat/windows-nat.h |  27 ++++++++--
 gdb/windows-nat.c     | 116 ++++++++++++++++++++++++++++++++++--------
 2 files changed, 119 insertions(+), 24 deletions(-)

diff --git a/gdb/nat/windows-nat.h b/gdb/nat/windows-nat.h
index c6d0c4e98dd..8399ed7b3b7 100644
--- a/gdb/nat/windows-nat.h
+++ b/gdb/nat/windows-nat.h
@@ -34,6 +34,24 @@ namespace windows_nat
 
 struct windows_process_info;
 
+/* The reason for explicitly stopping a thread.  Note the enumerators
+   are ordered such that when comparing two stopping_kind's numerical
+   value, the highest should prevail.  */
+enum stopping_kind
+  {
+    /* Not really stopping the thread.  */
+    SK_NOT_STOPPING = 0,
+
+    /* We're stopping the thread for internal reasons, the stop should
+       not be reported as an event to the core.  */
+    SK_INTERNAL = 1,
+
+    /* We're stopping the thread for external reasons, meaning, the
+       core/user asked us to stop the thread, so we must report a stop
+       event to the core.  */
+    SK_EXTERNAL = 2,
+  };
+
 /* Thread information structure used to track extra information about
    each thread.  */
 struct windows_thread_info
@@ -117,9 +135,10 @@ struct windows_thread_info
   int suspended = 0;
 
   /* This flag indicates whether we are explicitly stopping this
-     thread in response to a target_stop request.  This allows
-     distinguishing between threads that are explicitly stopped by the
-     debugger and threads that are stopped due to other reasons.
+     thread in response to a target_stop request or for
+     backend-internal reasons.  This allows distinguishing between
+     threads that are explicitly stopped by the debugger and threads
+     that are stopped due to other reasons.
 
      Typically, when we want to stop a thread, we suspend it, enqueue
      a pending GDB_SIGNAL_0 stop status on the thread, and then set
@@ -128,7 +147,7 @@ struct windows_thread_info
      already has an event to report.  In such case, we simply set the
      'stopping' flag without suspending the thread or enqueueing a
      pending stop.  See stop_one_thread.  */
-  bool stopping = false;
+  stopping_kind stopping = SK_NOT_STOPPING;
 
 /* Info about a potential pending stop.
 
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index a33ef868566..cb1d030d12b 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -261,6 +261,10 @@ enum windows_continue_flag
        all-stop mode.  This flag indicates that windows_continue
        should call ContinueDebugEvent even in non-stop mode.  */
     WCONT_CONTINUE_DEBUG_EVENT = 4,
+
+    /* Skip calling ContinueDebugEvent even in all-stop mode.  This is
+       the default in non-stop mode.  */
+    WCONT_DONT_CONTINUE_DEBUG_EVENT = 8,
   };
 
 DEF_ENUM_FLAGS_TYPE (windows_continue_flag, windows_continue_flags);
@@ -521,6 +525,8 @@ struct windows_nat_target final : public x86_nat_target<inf_child_target>
     return serial_event_fd (m_wait_event);
   }
 
+  void debug_registers_changed_all_threads ();
+
 private:
 
   windows_thread_info *add_thread (ptid_t ptid, HANDLE h, void *tlb,
@@ -528,7 +534,8 @@ struct windows_nat_target final : public x86_nat_target<inf_child_target>
   void delete_thread (ptid_t ptid, DWORD exit_code, bool main_thread_p);
   DWORD fake_create_process (const DEBUG_EVENT &current_event);
 
-  void stop_one_thread (windows_thread_info *th);
+  void stop_one_thread (windows_thread_info *th,
+			enum stopping_kind stopping_kind);
 
   BOOL windows_continue (DWORD continue_status, int id,
 			 windows_continue_flags cont_flags = 0);
@@ -1294,7 +1301,7 @@ windows_per_inferior::handle_output_debug_string
 		 a pending event.  It will be picked up by
 		 windows_nat_target::wait.  */
 	      th->suspend ();
-	      th->stopping = true;
+	      th->stopping = SK_EXTERNAL;
 	      th->last_event = {};
 	      th->pending_status.set_stopped (gotasig);
 
@@ -1605,7 +1612,7 @@ windows_per_inferior::continue_one_thread (windows_thread_info *th,
     }
 
   th->resume ();
-  th->stopping = false;
+  th->stopping = SK_NOT_STOPPING;
   th->last_sig = GDB_SIGNAL_0;
 }
 
@@ -1680,8 +1687,19 @@ windows_nat_target::windows_continue (DWORD continue_status, int id,
 #endif
       }
 
-  if (!target_is_non_stop_p ()
-      || (cont_flags & WCONT_CONTINUE_DEBUG_EVENT) != 0)
+  /* WCONT_DONT_CONTINUE_DEBUG_EVENT and WCONT_CONTINUE_DEBUG_EVENT
+     can't both be enabled at the same time.  */
+  gdb_assert ((cont_flags & WCONT_DONT_CONTINUE_DEBUG_EVENT) == 0
+	      || (cont_flags & WCONT_CONTINUE_DEBUG_EVENT) == 0);
+
+  bool continue_debug_event;
+  if ((cont_flags & WCONT_CONTINUE_DEBUG_EVENT) != 0)
+    continue_debug_event = true;
+  else if ((cont_flags & WCONT_DONT_CONTINUE_DEBUG_EVENT) != 0)
+    continue_debug_event = false;
+  else
+    continue_debug_event = !target_is_non_stop_p ();
+  if (continue_debug_event)
     {
       DEBUG_EVENTS ("windows_continue -> continue_last_debug_event");
       continue_last_debug_event_main_thread
@@ -1876,11 +1894,13 @@ windows_nat_target::interrupt ()
 	     "Press Ctrl-c in the program console."));
 }
 
-/* Stop thread TH.  This leaves a GDB_SIGNAL_0 pending in the thread,
-   which is later consumed by windows_nat_target::wait.  */
+/* Stop thread TH, for STOPPING_KIND reason.  This leaves a
+   GDB_SIGNAL_0 pending in the thread, which is later consumed by
+   windows_nat_target::wait.  */
 
 void
-windows_nat_target::stop_one_thread (windows_thread_info *th)
+windows_nat_target::stop_one_thread (windows_thread_info *th,
+				     enum stopping_kind stopping_kind)
 {
   ptid_t thr_ptid (windows_process.process_id, th->tid);
 
@@ -1896,12 +1916,18 @@ windows_nat_target::stop_one_thread (windows_thread_info *th)
 #ifdef __CYGWIN__
   else if (th->suspended
 	   && th->signaled_thread != nullptr
-	   && th->pending_status.kind () == TARGET_WAITKIND_IGNORE)
+	   && th->pending_status.kind () == TARGET_WAITKIND_IGNORE
+	   /* If doing an internal stop to update debug registers,
+	      then just leave the "sig" thread suspended.  Otherwise
+	      windows_nat_target::wait would incorrectly break the
+	      signaled_thread lock when it later processes the pending
+	      stop and calls windows_continue on this thread.  */
+	   && stopping_kind == SK_EXTERNAL)
     {
       DEBUG_EVENTS ("explict stop for \"sig\" thread %s held for signal",
 		    thr_ptid.to_string ().c_str ());
 
-      th->stopping = true;
+      th->stopping = stopping_kind;
       th->pending_status.set_stopped (GDB_SIGNAL_0);
       th->last_event = {};
       serial_event_set (m_wait_event);
@@ -1915,7 +1941,9 @@ windows_nat_target::stop_one_thread (windows_thread_info *th)
 		    thr_ptid.to_string ().c_str (),
 		    th->suspended, th->stopping);
 
-      th->stopping = true;
+      /* Upgrade stopping.  */
+      if (stopping_kind > th->stopping)
+	th->stopping = stopping_kind;
     }
   else
     {
@@ -1924,9 +1952,13 @@ windows_nat_target::stop_one_thread (windows_thread_info *th)
       th->suspend ();
       gdb_assert (th->suspended);
 
-      th->stopping = true;
-      th->pending_status.set_stopped (GDB_SIGNAL_0);
-      th->last_event = {};
+      if (stopping_kind > th->stopping)
+	{
+	  th->stopping = stopping_kind;
+	  th->pending_status.set_stopped (GDB_SIGNAL_0);
+	  th->last_event = {};
+	}
+
       serial_event_set (m_wait_event);
     }
 }
@@ -1940,7 +1972,7 @@ windows_nat_target::stop (ptid_t ptid)
     {
       ptid_t thr_ptid (windows_process.process_id, th->tid);
       if (thr_ptid.matches (ptid))
-	stop_one_thread (th.get ());
+	stop_one_thread (th.get (), SK_EXTERNAL);
     }
 }
 
@@ -2367,6 +2399,17 @@ windows_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 	    {
 	      windows_thread_info *th = windows_process.find_thread (result);
 
+	      /* If this thread was temporarily stopped just so we
+		 could update its debug registers on the next
+		 resumption, do it now.  */
+	      if (th->stopping == SK_INTERNAL)
+		{
+		  gdb_assert (fake);
+		  windows_continue (DBG_CONTINUE, th->tid,
+				    WCONT_DONT_CONTINUE_DEBUG_EVENT);
+		  continue;
+		}
+
 	      th->stopped_at_software_breakpoint = false;
 	      if (current_event.dwDebugEventCode
 		  == EXCEPTION_DEBUG_EVENT
@@ -2419,7 +2462,7 @@ windows_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 		for (auto &thr : windows_process.thread_list)
 		  thr->suspend ();
 
-	      th->stopping = false;
+	      th->stopping = SK_NOT_STOPPING;
 	    }
 
 	  /* If something came out, assume there may be more.  This is
@@ -3902,6 +3945,41 @@ Use \"file\" or \"dll\" command to load executable/libraries directly."));
     }
 }
 
+/* For each thread, set the debug_registers_changed flag, and
+   temporarily stop it so we can update its debug registers.  */
+
+void
+windows_nat_target::debug_registers_changed_all_threads ()
+{
+  for (auto &th : windows_process.thread_list)
+    {
+      th->debug_registers_changed = true;
+
+      /* Note we don't SuspendThread => update debug regs =>
+	 ResumeThread, because SuspendThread is actually asynchronous
+	 (and GetThreadContext blocks until the thread really
+	 suspends), and doing that for all threads may take a bit.
+	 Also, the core does one call per DR register update, so that
+	 would result in a lot of suspend-resumes.  So instead, we
+	 suspend the thread if it wasn't already suspended, and queue
+	 a pending stop to be handled by windows_nat_target::wait.
+	 This means we only stop each thread once, and, we don't block
+	 waiting for each individual thread stop.  */
+      stop_one_thread (th.get (), SK_INTERNAL);
+    }
+}
+
+/* Trampoline helper to get at the
+   windows_nat_target::debug_registers_changed_all_threads method in
+   the native target.  */
+
+static void
+debug_registers_changed_all_threads ()
+{
+  auto *win_tgt = static_cast<windows_nat_target *> (get_native_target ());
+  win_tgt->debug_registers_changed_all_threads ();
+}
+
 /* Hardware watchpoint support, adapted from go32-nat.c code.  */
 
 /* Pass the address ADDR to the inferior in the I'th debug register.
@@ -3913,8 +3991,7 @@ windows_set_dr (int i, CORE_ADDR addr)
   if (i < 0 || i > 3)
     internal_error (_("Invalid register %d in windows_set_dr.\n"), i);
 
-  for (auto &th : windows_process.thread_list)
-    th->debug_registers_changed = true;
+  debug_registers_changed_all_threads ();
 }
 
 /* Pass the value VAL to the inferior in the DR7 debug control
@@ -3923,8 +4000,7 @@ windows_set_dr (int i, CORE_ADDR addr)
 static void
 windows_set_dr7 (unsigned long val)
 {
-  for (auto &th : windows_process.thread_list)
-    th->debug_registers_changed = true;
+  debug_registers_changed_all_threads ();
 }
 
 /* Get the value of debug register I from the inferior.  */
-- 
2.43.2