[PATCH v3 06/11] 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 actual writes to ::wait avoids a bunch of
SuspendThread/ResumeThread sequences, so in principle should be
faster.

Reviewed-By: Tom Tromey <tom@tromey.com>
Change-Id: I39c2492c7aac06d23ef8f287f4afe3747b7bc53f
commit-id:22d7a7e0
---
 gdb/aarch64-windows-nat.c |   3 +-
 gdb/nat/windows-nat.h     |  27 ++++++++--
 gdb/windows-nat.c         | 102 ++++++++++++++++++++++++++++++++------
 gdb/windows-nat.h         |  11 +++-
 gdb/x86-windows-nat.c     |   6 +--
 5 files changed, 123 insertions(+), 26 deletions(-)

diff --git a/gdb/aarch64-windows-nat.c b/gdb/aarch64-windows-nat.c
index e6b596a38cb..29a6c95d485 100644
--- a/gdb/aarch64-windows-nat.c
+++ b/gdb/aarch64-windows-nat.c
@@ -317,8 +317,7 @@ aarch64_notify_debug_reg_change (ptid_t ptid,
     = aarch64_get_debug_reg_state (inferior_ptid.pid ());
   aarch64_windows_process.dr_state = *state;
 
-  for (auto &th : aarch64_windows_process.thread_list)
-    th->debug_registers_changed = true;
+  windows_debug_registers_changed_all_threads ();
 }
 
 INIT_GDB_FILE (aarch64_windows_nat)
diff --git a/gdb/nat/windows-nat.h b/gdb/nat/windows-nat.h
index f61eab766ac..52378765438 100644
--- a/gdb/nat/windows-nat.h
+++ b/gdb/nat/windows-nat.h
@@ -40,6 +40,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
@@ -123,9 +141,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
@@ -134,7 +153,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 d56510c23f6..6f3803c887b 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -855,7 +855,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);
 
@@ -923,7 +923,7 @@ windows_nat_target::continue_one_thread (windows_thread_info *th,
   thread_context_continue (th, killed);
 
   th->resume ();
-  th->stopping = false;
+  th->stopping = SK_NOT_STOPPING;
   th->last_sig = GDB_SIGNAL_0;
 }
 
@@ -997,8 +997,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
@@ -1201,11 +1212,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);
 
@@ -1221,12 +1234,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);
@@ -1240,7 +1259,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
     {
@@ -1255,14 +1276,20 @@ windows_nat_target::stop_one_thread (windows_thread_info *th)
 	{
 	  DEBUG_EVENTS ("suspension of %s failed, expect thread exit event",
 			thr_ptid.to_string ().c_str ());
+	  if (stopping_kind > th->stopping)
+	    th->stopping = stopping_kind;
 	  return;
 	}
 
       gdb_assert (th->suspended == 1);
 
-      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);
     }
 }
@@ -1275,7 +1302,7 @@ windows_nat_target::stop (ptid_t ptid)
   for (thread_info &thr : all_non_exited_threads (this))
     {
       if (thr.ptid.matches (ptid))
-	stop_one_thread (as_windows_thread_info (&thr));
+	stop_one_thread (as_windows_thread_info (&thr), SK_EXTERNAL);
     }
 }
 
@@ -1777,6 +1804,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
@@ -1821,7 +1859,7 @@ windows_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 		for (auto *thr : all_windows_threads ())
 		  thr->suspend ();
 
-	      th->stopping = false;
+	      th->stopping = SK_NOT_STOPPING;
 	    }
 
 	  /* If something came out, assume there may be more.  This is
@@ -3404,6 +3442,40 @@ Use \"%ps\" or \"%ps\" 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 : all_windows_threads ())
+    {
+      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, SK_INTERNAL);
+    }
+}
+
+/* Trampoline helper to get at the
+   windows_nat_target::debug_registers_changed_all_threads method in
+   the native target.  */
+
+void
+windows_debug_registers_changed_all_threads ()
+{
+  get_windows_nat_target ()->debug_registers_changed_all_threads ();
+}
+
 /* Determine if the thread referenced by "ptid" is alive
    by "polling" it.  If WaitForSingleObject returns WAIT_OBJECT_0
    it means that the thread has died.  Otherwise it is assumed to be alive.  */
diff --git a/gdb/windows-nat.h b/gdb/windows-nat.h
index 7ce71690e20..5ef68dff274 100644
--- a/gdb/windows-nat.h
+++ b/gdb/windows-nat.h
@@ -78,6 +78,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);
@@ -258,6 +262,8 @@ struct windows_nat_target : public inf_child_target
     return serial_event_fd (m_wait_event);
   }
 
+  void debug_registers_changed_all_threads ();
+
 protected:
 
   /* Initialize arch-specific data for a new inferior (debug registers,
@@ -303,7 +309,8 @@ struct windows_nat_target : public 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 windows_nat::stopping_kind stopping_kind);
 
   DWORD continue_status_for_event_detaching
     (const DEBUG_EVENT &event, size_t *reply_later_events_left = nullptr);
@@ -470,4 +477,6 @@ all_windows_threads ()
 	  (all_non_exited_threads_range (win_tgt, minus_one_ptid)));
 }
 
+extern void windows_debug_registers_changed_all_threads ();
+
 #endif /* GDB_WINDOWS_NAT_H */
diff --git a/gdb/x86-windows-nat.c b/gdb/x86-windows-nat.c
index 8ad9d3bedea..61764da8deb 100644
--- a/gdb/x86-windows-nat.c
+++ b/gdb/x86-windows-nat.c
@@ -350,8 +350,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 : all_windows_threads ())
-    th->debug_registers_changed = true;
+  windows_debug_registers_changed_all_threads ();
 }
 
 /* Pass the value VAL to the inferior in the DR7 debug control
@@ -360,8 +359,7 @@ windows_set_dr (int i, CORE_ADDR addr)
 static void
 windows_set_dr7 (unsigned long val)
 {
-  for (auto *th : all_windows_threads ())
-    th->debug_registers_changed = true;
+  windows_debug_registers_changed_all_threads ();
 }
 
 /* Get the value of debug register I from the inferior.  */
-- 
2.53.0