Windows gdb: Watchpoints while running (internal vs external stops)

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

diff --git a/gdb/nat/windows-nat.h b/gdb/nat/windows-nat.h
index fe377fcde34434546d2e4b72dae5adea9e8ac9bb..568c7c46a5136b10857b084bdad839be9b175bf2 100644 (file)
--- 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 25b025755bde8d6699017f4a4e0de1d434da7bf4..2d8b5582dc0b3f55741ad34fb362ddbaa6dcaf4a 100644 (file)
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -303,6 +303,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);
@@ -571,6 +575,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,
@@ -578,7 +584,8 @@ private:
   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);
 
   DWORD continue_status_for_event_detaching
     (const DEBUG_EVENT &event, size_t *reply_later_events_left = nullptr);
@@ -1373,7 +1380,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);
 
@@ -1633,7 +1640,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;
 }
 
@@ -1708,8 +1715,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
@@ -1918,11 +1936,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);
 
@@ -1938,12 +1958,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);
@@ -1957,7 +1983,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
     {
@@ -1972,14 +2000,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);
     }
 }
@@ -1992,7 +2026,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);
     }
 }
 
@@ -2497,6 +2531,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
@@ -2543,7 +2588,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
@@ -4162,6 +4207,41 @@ 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.  */
+
+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.
@@ -4173,8 +4253,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;
+  debug_registers_changed_all_threads ();
 }
 
 /* Pass the value VAL to the inferior in the DR7 debug control
@@ -4183,8 +4262,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;
+  debug_registers_changed_all_threads ();
 }
 
 /* Get the value of debug register I from the inferior.  */