[Lldb-commits] [lldb] [lldb][debugserver] Don't report stale hits of disabled watchpoints (PR #238172)

[Raphael Isemann via lldb-commits]

https://github.com/Teemperor created https://github.com/llvm/llvm-project/pull/238172

To step over a watchpoint-triggering instruction, lldb disables the watchpoint and single-steps. Occasionally the thread runs before the hardware debug registers reflect the disabled watchpoint, and the same access traps again. The thread's debug state (thread_get_state) already shows the watchpoint as disabled, so GetHardwareWatchpointHit finds no enabled slot. MachProcess::RefineWatchpointStopInfo then attributes the hit to the nearest enabled watchpoint. For "modify" watchpoints, lldb sees that watchpoint's value unchanged and silently continues, so the real hit is lost.

This is the cause of the flakiness in TestConsecutiveWatchpoints.

This patch doesn't report stops when the watchpoint exception's address matches no enabled watchpoint register. Instead, it makes the thread resume again.

>From dbf6f442d95b37686f6a50b860b3d469ad081004 Mon Sep 17 00:00:00 2001
From: Raphael Isemann <rise at apple.com>
Date: Sun, 1 Nov 2026 11:02:21 -0800
Subject: [PATCH] [lldb][debugserver] Don't report stale hits of disabled
 watchpoints

To step over a watchpoint-triggering instruction, lldb disables the
watchpoint and single-steps. Occasionally the thread runs before the
hardware debug registers reflect the disabled watchpoint, and the same
access traps again. The thread's debug state (thread_get_state) already
shows the watchpoint as disabled, so GetHardwareWatchpointHit finds no
enabled slot. MachProcess::RefineWatchpointStopInfo then attributes the
hit to the nearest enabled watchpoint. For "modify" watchpoints, lldb
sees that watchpoint's value unchanged and silently continues, so the
real hit is lost.

This is the cause of the flakiness in TestConsecutiveWatchpoints.

This patch doesn't report stops when the watchpoint exception's address
matches no enabled watchpoint register. Instead, it makes the thread
resume again.
---
 .../source/MacOSX/arm64/DNBArchImplARM64.cpp  | 53 +++++++++++++++++++
 .../source/MacOSX/arm64/DNBArchImplARM64.h    |  7 +++
 2 files changed, 60 insertions(+)

diff --git a/lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp b/lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp
index 35147c14a55019..4882b6496ebd1e 100644
--- a/lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp
+++ b/lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp
@@ -787,6 +787,27 @@ bool DNBArchMachARM64::NotifyException(MachException::Data &exc) {
       // addr to return the starting address as seen from the debugger side.
       uint32_t hw_index = GetHardwareWatchpointHit(addr);
 
+      // No enabled watchpoint covers the accessed address, but one that was
+      // disabled before the thread resumed does. This is likely caused by
+      // the hardware still having the old debug state. We pretend the hit
+      // never happened as the watchpoint was supposed to be disabled.
+      // We limit the retries to avoid infinite loops in case another bug
+      // causes this to occur deterministically.
+      const uint32_t max_stale_watchpoint_hits = 64;
+      if (hw_index == INVALID_NUB_HW_INDEX &&
+          m_stale_watchpoint_hits < max_stale_watchpoint_hits &&
+          IsDisabledWatchpointHit(exc.exc_data[1])) {
+        ++m_stale_watchpoint_hits;
+        m_resume_after_stale_watchpoint = true;
+        DNBLogThreadedIf(LOG_WATCHPOINTS,
+                         "DNBArchMachARM64::NotifyException "
+                         "watchpoint hit on address 0x%llx of a disabled "
+                         "watchpoint, resuming the thread (attempt %u)",
+                         (uint64_t)exc.exc_data[1], m_stale_watchpoint_hits);
+        return true;
+      }
+      m_stale_watchpoint_hits = 0;
+
       // One logical watchpoint was split into two watchpoint locations because
       // it was too big.  If the watchpoint exception is indicating the 2nd half
       // of the two-parter, find the address of the 1st half and report that --
@@ -821,6 +842,7 @@ bool DNBArchMachARM64::NotifyException(MachException::Data &exc) {
     }
     break;
   }
+  m_stale_watchpoint_hits = 0;
   return false;
 }
 
@@ -828,6 +850,7 @@ bool DNBArchMachARM64::ThreadDidStop() {
   bool success = true;
 
   m_state.InvalidateAllRegisterStates();
+  m_resume_after_stale_watchpoint = false;
 
   if (m_watchpoint_resume_single_step_enabled) {
     // Great!  We now disable the hardware single step as well as re-enable the
@@ -866,6 +889,12 @@ bool DNBArchMachARM64::ThreadDidStop() {
   return success;
 }
 
+bool DNBArchMachARM64::StepNotComplete() {
+  // Resume threads that stopped on a stale watchpoint hit, see
+  // NotifyException.
+  return m_resume_after_stale_watchpoint;
+}
+
 // Set the single step bit in the processor status register.
 kern_return_t DNBArchMachARM64::EnableHardwareSingleStep(bool enable) {
   DNBError err;
@@ -1545,6 +1574,30 @@ uint32_t DNBArchMachARM64::GetHardwareWatchpointHit(nub_addr_t &addr) {
   return INVALID_NUB_HW_INDEX;
 }
 
+// Return true if addr is covered by a watchpoint register that is set up but
+// currently disabled.
+bool DNBArchMachARM64::IsDisabledWatchpointHit(nub_addr_t addr) {
+  if (GetDBGState(false) != KERN_SUCCESS)
+    return false;
+  const DBG &debug_state = m_state.dbg;
+  const uint32_t num = NumSupportedHardwareWatchpoints();
+  for (uint32_t i = 0; i < num; ++i) {
+    if (IsWatchpointEnabled(debug_state, i))
+      continue;
+    nub_addr_t wp_addr = GetWatchAddress(debug_state, i);
+    uint32_t bas_bits = bits(debug_state.__wcr[i], 12, 5);
+    uint32_t mask = bits(debug_state.__wcr[i], 28, 24);
+    if (mask != 0) {
+      if (bits(wp_addr, 48, mask) == bits(addr, 48, mask))
+        return true;
+    } else if (bas_bits != 0 && bits(wp_addr, 48, 3) == bits(addr, 48, 3) &&
+               (bas_bits & (1 << bits(addr, 2, 0)))) {
+      return true;
+    }
+  }
+  return false;
+}
+
 nub_addr_t DNBArchMachARM64::GetWatchpointAddressByIndex(uint32_t hw_index) {
   kern_return_t kret = GetDBGState(true);
   if (kret != KERN_SUCCESS)
diff --git a/lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.h b/lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.h
index 11ad1f40c3ef69..77f16fb3cd2d09 100644
--- a/lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.h
+++ b/lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.h
@@ -71,6 +71,7 @@ class DNBArchMachARM64 : public DNBArchProtocol {
   void ThreadWillResume() override;
   bool ThreadDidStop() override;
   bool NotifyException(MachException::Data &exc) override;
+  bool StepNotComplete() override;
 
   static DNBArchProtocol *Create(MachThread *thread);
   static const uint8_t *SoftwareBreakpointOpcode(nub_size_t byte_size);
@@ -281,6 +282,7 @@ class DNBArchMachARM64 : public DNBArchProtocol {
   virtual bool ReenableHardwareWatchpoint(uint32_t hw_break_index);
   virtual bool ReenableHardwareWatchpoint_helper(uint32_t hw_break_index);
   uint32_t GetHardwareWatchpointHit(nub_addr_t &addr) override;
+  bool IsDisabledWatchpointHit(nub_addr_t addr);
 
   class disabled_watchpoint {
   public:
@@ -317,6 +319,11 @@ class DNBArchMachARM64 : public DNBArchProtocol {
   bool m_watchpoint_did_occur;
   bool m_watchpoint_resume_single_step_enabled;
 
+  // We are resuming after hitting a stale watchpoint.
+  bool m_resume_after_stale_watchpoint = false;
+  // Number of consecutive stale watchpoint hits that were not reported.
+  uint32_t m_stale_watchpoint_hits = 0;
+
   typedef std::map<uint32_t, Context> SaveRegisterStates;
   SaveRegisterStates m_saved_register_states;