[binutils-gdb] Windows gdb: Avoid writing debug registers if watchpoint hit pending

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=1ba40beb4c5fe20652fd886e42146e9fa5bb01b1

commit 1ba40beb4c5fe20652fd886e42146e9fa5bb01b1
Author: Pedro Alves <[email protected]>
Date:   Tue May 7 20:41:37 2024 +0100

    Windows gdb: Avoid writing debug registers if watchpoint hit pending
    
    Several watchpoint-related testcases, such as
    gdb.threads/watchthreads.exp for example, when tested with the backend
    in non-stop mode, exposed an interesting detail of the Windows debug
    API that wasn't considered before.  The symptom observed is spurious
    SIGTRAPs, like:
    
      Thread 1 "watchthreads" received signal SIGTRAP, Trace/breakpoint trap.
      0x00000001004010b1 in main () at .../src/gdb/testsuite/gdb.threads/watchthreads.c:48
      48              args[i] = 1; usleep (1); /* Init value.  */
    
    After a good amount of staring at logs and headscratching, I realized
    the problem:
    
     #0 - It all starts with the fact that multiple threads can hit an
          event at the same time.  Say, a watchpoint for thread A, and a
          breakpoint for thread B.
    
     #1 - Say, WaitForDebugEvent reports the breakpoint hit for thread B
          first, then GDB for some reason decides to update debug
          registers, and continue.  Updating debug registers means writing
          the debug registers to _all_ threads, with SetThreadContext.
    
     #2 - WaitForDebugEvent reports the watchpoint hit for thread A.
          Watchpoint hits are reported as EXCEPTION_SINGLE_STEP.
    
     #3 - windows-nat checks the Dr6 debug register to check if the step
          was a watchpoint or hardware breakpoint stop, and finds that Dr6
          is completely cleared.  So windows-nat reports a plain SIGTRAP
          (given EXCEPTION_SINGLE_STEP) to the core.
    
     #4 - Thread A was not supposed to be stepping, so infrun reports the
          SIGTRAP to the user as a random signal.
    
    The strange part is #3 above.  Why was Dr6 cleared?
    
    Turns out that (at least in Windows 10 & 11), writing to _any_ debug
    register has the side effect of clearing Dr6, even if you write the
    same values the registers already had, back to the registers.
    
    I confirmed it clearly by adding this hack to GDB:
    
      if (th->context.ContextFlags == 0)
        {
          th->context.ContextFlags = CONTEXT_DEBUGGER_DR;
    
          /* Get current values of debug registers.  */
          CHECK (GetThreadContext (th->h, &th->context));
    
          DEBUG_EVENTS ("For 0x%x (once),  Dr6=0x%llx", th->tid, th->context.Dr6);
    
          /* Write debug registers back to thread, same values,
             and re-read them.  */
          CHECK (SetThreadContext (th->h, &th->context));
          CHECK (GetThreadContext (th->h, &th->context));
    
          DEBUG_EVENTS ("For 0x%x (twice), Dr6=0x%llx", th->tid, th->context.Dr6);
        }
    
    Which showed Dr6=0 after the write + re-read:
    
      [windows events] fill_thread_context: For 0x6a0 (once),  Dr6=0xffff0ff1
      [windows events] fill_thread_context: For 0x6a0 (twice), Dr6=0x0
    
    This commit fixes the issue by detecting that a thread has a pending
    watchpoint hit to report (Dr6 has interesting bits set), and if so,
    avoid modifying any debug register.  Instead, let the pending
    watchpoint hit be reported by WaitForDebugEvent.  If infrun did want
    to modify watchpoints, it will still be done when the thread is
    eventually re-resumed after the pending watchpoint hit is reported.
    (infrun knows how to gracefully handle the case of a watchpoint hit
    for a watchpoint that has since been deleted.)
    
    Move the fill_thread_context method from windows_nat_target to
    windows_per_inferior so it can be used by gdbserver too.
    
    Approved-By: Tom Tromey <[email protected]>
    Change-Id: I21a3daa9e34eecfa054f0fea706e5ab40aabe70a
    commit-id:a28f8d4e

Diff:
---
 gdb/aarch64-windows-nat.c |  7 +++---
 gdb/nat/windows-nat.h     |  6 +++++
 gdb/windows-nat.c         |  2 +-
 gdb/windows-nat.h         |  3 ---
 gdb/x86-windows-nat.c     | 64 +++++++++++++++++++++++++++++++++++++++--------
 gdbserver/win32-low.cc    |  8 ++++++
 gdbserver/win32-low.h     |  2 ++
 7 files changed, 73 insertions(+), 19 deletions(-)

diff --git a/gdb/aarch64-windows-nat.c b/gdb/aarch64-windows-nat.c
index 871531bb93a..2b082805708 100644
--- a/gdb/aarch64-windows-nat.c
+++ b/gdb/aarch64-windows-nat.c
@@ -30,6 +30,7 @@ struct aarch64_windows_per_inferior : public windows_per_inferior
 {
   aarch64_debug_reg_state dr_state;
 
+  void fill_thread_context (windows_thread_info *th) override;
   void invalidate_thread_context (windows_thread_info *th) override;
 };
 
@@ -42,8 +43,6 @@ struct aarch64_windows_nat_target final
   void initialize_windows_arch (bool attaching) override;
   void cleanup_windows_arch () override;
 
-  void fill_thread_context (windows_thread_info *th) override;
-
   void thread_context_continue (windows_thread_info *th, int killed) override;
   void thread_context_step (windows_thread_info *th) override;
 
@@ -176,10 +175,10 @@ aarch64_windows_nat_target::cleanup_windows_arch ()
   aarch64_remove_debug_reg_state (inferior_ptid.pid ());
 }
 
-/* See windows-nat.h.  */
+/* See nat/windows-nat.h.  */
 
 void
-aarch64_windows_nat_target::fill_thread_context (windows_thread_info *th)
+aarch64_windows_per_inferior::fill_thread_context (windows_thread_info *th)
 {
   CONTEXT *context = &th->context;
 
diff --git a/gdb/nat/windows-nat.h b/gdb/nat/windows-nat.h
index 5258ce52a0f..45d186f771a 100644
--- a/gdb/nat/windows-nat.h
+++ b/gdb/nat/windows-nat.h
@@ -174,6 +174,12 @@ struct windows_process_info
      This function must be supplied by the embedding application.  */
   virtual windows_thread_info *find_thread (ptid_t ptid) = 0;
 
+  /* Fill in the thread's CONTEXT/WOW64_CONTEXT, if it wasn't filled
+     in yet.
+
+     This function must be supplied by the embedding application.  */
+  virtual void fill_thread_context (windows_thread_info *th) = 0;
+
   /* Handle OUTPUT_DEBUG_STRING_EVENT from child process.  Updates
      OURSTATUS and returns the thread id if this represents a thread
      change (this is specific to Cygwin), otherwise 0.
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index fba1ae6c40b..17166fdcfad 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -386,7 +386,7 @@ windows_nat_target::fetch_registers (struct regcache *regcache, int r)
   if (th == NULL)
     return;
 
-  fill_thread_context (th);
+  windows_process->fill_thread_context (th);
 
   if (r < 0)
     for (r = 0; r < gdbarch_num_regs (regcache->arch()); r++)
diff --git a/gdb/windows-nat.h b/gdb/windows-nat.h
index e5084631e5d..235844a7c95 100644
--- a/gdb/windows-nat.h
+++ b/gdb/windows-nat.h
@@ -226,9 +226,6 @@ protected:
   /* Cleanup arch-specific data after inferior exit.  */
   virtual void cleanup_windows_arch () = 0;
 
-  /* Reload the thread context.  */
-  virtual void fill_thread_context (windows_thread_info *th) = 0;
-
   /* Prepare the thread context for continuing.  */
   virtual void thread_context_continue (windows_thread_info *th,
 					int killed) = 0;
diff --git a/gdb/x86-windows-nat.c b/gdb/x86-windows-nat.c
index 5c09efce32f..5a4876b7fbe 100644
--- a/gdb/x86-windows-nat.c
+++ b/gdb/x86-windows-nat.c
@@ -48,6 +48,7 @@ struct x86_windows_per_inferior : public windows_per_inferior
      a segment register or not.  */
   segment_register_p_ftype *segment_register_p = nullptr;
 
+  void fill_thread_context (windows_thread_info *th) override;
   void invalidate_thread_context (windows_thread_info *th) override;
 };
 
@@ -56,8 +57,6 @@ struct x86_windows_nat_target final : public x86_nat_target<windows_nat_target>
   void initialize_windows_arch (bool attaching) override;
   void cleanup_windows_arch () override;
 
-  void fill_thread_context (windows_thread_info *th) override;
-
   void thread_context_continue (windows_thread_info *th, int killed) override;
   void thread_context_step (windows_thread_info *th) override;
 
@@ -102,10 +101,10 @@ x86_windows_nat_target::cleanup_windows_arch ()
   x86_cleanup_dregs ();
 }
 
-/* See windows-nat.h.  */
+/* See nat/windows-nat.h.  */
 
 void
-x86_windows_nat_target::fill_thread_context (windows_thread_info *th)
+x86_windows_per_inferior::fill_thread_context (windows_thread_info *th)
 {
   x86_windows_process.with_context (th, [&] (auto *context)
     {
@@ -141,13 +140,56 @@ x86_windows_nat_target::thread_context_continue (windows_thread_info *th,
 
       if (th->debug_registers_changed)
 	{
-	  context->ContextFlags |= WindowsContext<decltype(context)>::debug;
-	  context->Dr0 = state->dr_mirror[0];
-	  context->Dr1 = state->dr_mirror[1];
-	  context->Dr2 = state->dr_mirror[2];
-	  context->Dr3 = state->dr_mirror[3];
-	  context->Dr6 = DR6_CLEAR_VALUE;
-	  context->Dr7 = state->dr_control_mirror;
+	  windows_process->fill_thread_context (th);
+
+	  gdb_assert ((context->ContextFlags & CONTEXT_DEBUG_REGISTERS) != 0);
+
+	  /* Check whether the thread has Dr6 set indicating a
+	     watchpoint hit, and we haven't seen the watchpoint event
+	     yet (reported as
+	     EXCEPTION_SINGLE_STEP/STATUS_WX86_SINGLE_STEP).  In that
+	     case, don't change the debug registers.  Changing debug
+	     registers, even if to the same values, makes the kernel
+	     clear Dr6.  The result would be we would lose the
+	     unreported watchpoint hit.  */
+	  if ((context->Dr6 & ~DR6_CLEAR_VALUE) != 0)
+	    {
+	      if (th->last_event.dwDebugEventCode == EXCEPTION_DEBUG_EVENT
+		  && (th->last_event.u.Exception.ExceptionRecord.ExceptionCode
+		      == EXCEPTION_SINGLE_STEP
+		      || (th->last_event.u.Exception.ExceptionRecord.ExceptionCode
+			  == STATUS_WX86_SINGLE_STEP)))
+		{
+		  DEBUG_EVENTS ("0x%x already reported watchpoint", th->tid);
+		}
+	      else
+		{
+		  DEBUG_EVENTS ("0x%x last reported something else (0x%x)",
+				th->tid,
+				th->last_event.dwDebugEventCode);
+
+		  /* Don't touch debug registers.  Let the pending
+		     watchpoint event be reported instead.  We will
+		     update the debug registers later when the thread
+		     is re-resumed by the core after the watchpoint
+		     event.  */
+		  context->ContextFlags &= ~CONTEXT_DEBUG_REGISTERS;
+		}
+	    }
+	  else
+	    DEBUG_EVENTS ("0x%x has no dr6 set", th->tid);
+
+	  if ((context->ContextFlags & CONTEXT_DEBUG_REGISTERS) != 0)
+	    {
+	      DEBUG_EVENTS ("0x%x changing dregs", th->tid);
+	      context->Dr0 = state->dr_mirror[0];
+	      context->Dr1 = state->dr_mirror[1];
+	      context->Dr2 = state->dr_mirror[2];
+	      context->Dr3 = state->dr_mirror[3];
+	      context->Dr6 = DR6_CLEAR_VALUE;
+	      context->Dr7 = state->dr_control_mirror;
+	    }
+
 	  th->debug_registers_changed = false;
 	}
 
diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc
index e712033898c..826d21867ae 100644
--- a/gdbserver/win32-low.cc
+++ b/gdbserver/win32-low.cc
@@ -118,6 +118,14 @@ win32_require_context (windows_thread_info *th)
 
 /* See nat/windows-nat.h.  */
 
+void
+gdbserver_windows_process::fill_thread_context (windows_thread_info *th)
+{
+  win32_require_context (th);
+}
+
+/* See nat/windows-nat.h.  */
+
 windows_thread_info *
 gdbserver_windows_process::find_thread (ptid_t ptid)
 {
diff --git a/gdbserver/win32-low.h b/gdbserver/win32-low.h
index 01a904043eb..ff680492756 100644
--- a/gdbserver/win32-low.h
+++ b/gdbserver/win32-low.h
@@ -185,6 +185,8 @@ struct gdbserver_windows_process : public windows_nat::windows_process_info
   void handle_unload_dll (const DEBUG_EVENT &current_event) override;
   bool handle_access_violation (const EXCEPTION_RECORD *rec) override;
 
+  void fill_thread_context (windows_nat::windows_thread_info *th) override;
+
   int attaching = 0;
 
   /* A status that hasn't been reported to the core yet, and so