Add AArch64 MASK watchpoint support to debugserver

[jasonmolenda]

Summary

AArch64 supports two types of watchpoints - 1-8 byte watchpoints, which can watch up to 8 contiguous bytes aligned to a doubleword boundary, and mask watchpoints that can watch any power-of-2 range of memory aligned to that boundary (BAS watchpoints and MASK watchpoints). This patch adds support for MASK watchpoints when the user requests larger than 8 bytes be watched.

The major caveat with this is that there remains real work in lldb to support large watchpoints. If a user asks to watch 192 bytes, aligned to a 256 byte boundary, we still need to watch 256 bytes. Writes to the final 64 bytes will trap the watchpoint, and we need to work out how to silently skip these watchpoint hits. Watchpoints have a type: "read", "write", and I expect I'll add a new type "modify" that is the default, and stops when the memory region being watched *changes*.

(this problem of a mask watchpoint watching a larger memory range than the user expected is exacerbated with a memory region not properly aligned to that power-of-2 size. and it can come up easily with smaller objects, e.g. a 24 byte object requires a 32-byte watchpoint, or possibly two watchpoints if it's not aligned to a 32-byte boundary.)

The changes I did https://reviews.llvm.org/D149040 make this patch mostly a simple drop-in of the method for setting the debug control register properly for MASK watchpoints. There are some extra changes to the method which takes a trap address and tries to find the hardware watchpoint register which resulted in that watchpoint hit. (we further refine this later based on the user's watchpoint requests, using a "nearest wp wins" if no watched region correctly includes it, for AArch64 reasons.)

I added a test case with an array of uint32_t's, watch 256 of those unit32_t's, and a loop that modifies every 16th uint32_t. The test confirms that we hit this 256-uint32_t's watchpoint 16 times.

[delcypher]

lldb/test/API/python_api/watchpoint/watchlocation/TestTargetWatchAddress.py

132

@jasonmolenda Maybe this code should be guarded rather than deleted?

Your change looks like it only affects arm64 Apple platforms? For everything else, presumably this restriction still applies? If so maybe we should still be testing it?

[JDevlieghere]

Some small nits but LGTM

lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp

1079

I assume this can be const?

1082

const too?

1260–1262

const bool is_bas_watchpoint = (mask ==0);

[jasonmolenda]

Update patch with Dan and Jonas' feedback.

lldb/test/API/functionalities/watchpoint/large-watchpoint/Makefile

+C_SOURCES := main.c
+
+include Makefile.rules

lldb/test/API/functionalities/watchpoint/large-watchpoint/TestLargeWatchpoint.py

+"""
+Watch larger-than-8-bytes regions of memory, confirm that
+writes to those regions are detected.
+"""
+
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class UnalignedWatchpointTestCase(TestBase):
+
+    def continue_and_report_stop_reason(self, process, iter_str):
+        process.Continue()
+        self.assertIn(process.GetState(), [lldb.eStateStopped, lldb.eStateExited],
+                         iter_str)
+        thread = process.GetSelectedThread()
+        return thread.GetStopReason()
+
+
+    NO_DEBUG_INFO_TESTCASE = True
+    # debugserver on AArch64 has this feature.
+    @skipIf(archs=no_match(['arm64', 'arm64e', 'aarch64']))
+    @skipUnlessDarwin
+
+    # debugserver only gained the ability to watch larger regions
+    # with this patch.
+    @skipIfOutOfTreeDebugserver
+
+    def test_large_watchpoint(self):
+        """Test watchpoint that covers a large region of memory."""
+        self.build()
+        self.main_source_file = lldb.SBFileSpec("main.c")
+        (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(self,
+             "break here", self.main_source_file)
+
+        frame = thread.GetFrameAtIndex(0)
+
+        array_addr = frame.GetValueForVariablePath("array").GetValueAsUnsigned()
+
+        # watch 256 uint32_t elements in the middle of the array, 
+        # don't assume that the heap allocated array is aligned 
+        # to a 1024 byte boundary to begin with, force alignment.
+        wa_256_addr = ((array_addr + 1024) & ~(1024-1))
+        err = lldb.SBError()
+        wp = target.WatchAddress(wa_256_addr, 1024, False, True, err)
+        self.assertTrue(wp.IsValid())
+        self.assertSuccess(err)
+
+        c_count = 0
+        reason = self.continue_and_report_stop_reason(process, "continue #%d" % c_count)
+        while reason == lldb.eStopReasonWatchpoint:
+            c_count = c_count + 1
+            reason = self.continue_and_report_stop_reason(process, "continue #%d" % c_count)
+            self.assertLessEqual(c_count, 16)
+
+        self.assertEqual(c_count, 16)

lldb/test/API/functionalities/watchpoint/large-watchpoint/main.c

+#include <stdint.h>
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+int main() {
+  const int count = 65535;
+  int *array = (int*) malloc(sizeof (int) * count);
+  memset (array, 0, count * sizeof (int));
+
+  puts ("break here");
+
+  for (int i = 0; i < count - 16; i += 16) 
+    array[i] += 10;
+
+  puts ("done, exiting.");
+}

lldb/test/API/python_api/watchpoint/watchlocation/TestTargetWatchAddress.py

def test_watch_address_with_invalid_watch_size(self):
         frame0 = thread.GetFrameAtIndex(0)
 
         value = frame0.FindValue('g_char_ptr',
                                  lldb.eValueTypeVariableGlobal)
         pointee = value.CreateValueFromAddress(
             "pointee",
             value.GetValueAsUnsigned(0),
             value.GetType().GetPointeeType())
+
+        # debugserver on Darwin AArch64 systems can watch large regions
+        # of memory via https://reviews.llvm.org/D149792 , don't run this
+        # test there.
+        if self.getArchitecture() not in ['arm64', 'arm64e', 'arm64_32']:
           # Watch for write to *g_char_ptr.
           error = lldb.SBError()
           watchpoint = target.WatchAddress(
               value.GetValueAsUnsigned(), 365, False, True, error)
           self.assertFalse(watchpoint)
           self.expect(error.GetCString(), exe=False,
                       substrs=['watch size of %d is not supported' % 365])

lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.h

bool DisableHardwareBreakpoint(uint32_t hw_break_index,
                                  bool also_set_on_task) override;
   std::vector<WatchpointSpec>
   AlignRequestedWatchpoint(nub_addr_t requested_addr,
                            nub_size_t requested_size);
   uint32_t EnableHardwareWatchpoint(nub_addr_t addr, nub_size_t size, bool read,
                                     bool write, bool also_set_on_task) override;
   uint32_t SetBASWatchpoint(WatchpointSpec wp, bool read, bool write,
                             bool also_set_on_task);
-  uint32_t SetMASKWatchpoint(WatchpointSpec wp);
+  uint32_t SetMASKWatchpoint(WatchpointSpec wp, bool read, bool write,
+                             bool also_set_on_task);
   bool DisableHardwareWatchpoint(uint32_t hw_break_index,
                                  bool also_set_on_task) override;
   bool DisableHardwareWatchpoint_helper(uint32_t hw_break_index,
                                         bool also_set_on_task);
 
 protected:
   kern_return_t EnableHardwareSingleStep(bool enable);
   static bool FixGenericRegisterNumber(uint32_t &set, uint32_t &reg);

lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp

 #define BCR_ENABLE ((uint32_t)(1u))
 #define WCR_ENABLE ((uint32_t)(1u))
 
 // Watchpoint load/store
 // (LSC bits in the DBGWVRn_EL1 watchpoint control register)
 #define WCR_LOAD ((uint32_t)(1u << 3))
 #define WCR_STORE ((uint32_t)(1u << 4))
 
-// Enable breakpoint, watchpoint, and vector catch debug exceptions.
-// (MDE bit in the MDSCR_EL1 register.  Equivalent to the MDBGen bit in
-// DBGDSCRext in Aarch32)
-#define MDE_ENABLE ((uint32_t)(1u << 15))
-
 // Single instruction step
 // (SS bit in the MDSCR_EL1 register)
 #define SS_ENABLE ((uint32_t)(1u))
 
 static const uint8_t g_arm64_breakpoint_opcode[] = {
     0x00, 0x00, 0x20, 0xD4}; // "brk #0", 0xd4200000 in BE byte order
 
 // If we need to set one logical watchpoint by using
if (i < num_hw_breakpoints) {
 
       DNBLogThreadedIf(LOG_WATCHPOINTS,
                        "DNBArchMachARM64::EnableHardwareBreakpoint() "
                        "adding breakpoint on address 0x%llx with control "
                        "register value 0x%x",
                        (uint64_t)m_state.dbg.__bvr[i],
                        (uint32_t)m_state.dbg.__bcr[i]);
 
-      // The kernel will set the MDE_ENABLE bit in the MDSCR_EL1 for us
-      // automatically, don't need to do it here.
       kret = SetDBGState(also_set_on_task);
 
       DNBLogThreadedIf(LOG_WATCHPOINTS,
                        "DNBArchMachARM64::"
                        "EnableHardwareBreakpoint() "
                        "SetDBGState() => 0x%8.8x.",
                        kret);
 
DNBArchMachARM64::AlignRequestedWatchpoint(nub_addr_t requested_addr,
   nub_size_t aligned_size = std::max(requested_size, min_watchpoint_alignment);
 
   // AArch64 addresses are 8 bytes.
   constexpr int addr_byte_size = 8;
   constexpr int addr_bit_size = addr_byte_size * 8;
 
   /// Round up \a requested_size to the next power-of-2 size, at least 8
   /// bytes
-  /// requested_size == 3  -> aligned_size == 8
-  /// requested_size == 13 -> aligned_size == 16
-  /// requested_size == 16 -> aligned_size == 16
+  /// requested_size == 8   -> aligned_size == 8
+  /// requested_size == 9   -> aligned_size == 16
+  /// requested_size == 15  -> aligned_size == 16
+  /// requested_size == 192 -> aligned_size == 256
   /// Could be `std::bit_ceil(aligned_size)` when we build with C++20?
   aligned_size = 1ULL << (addr_bit_size - __builtin_clzll(aligned_size - 1));
 
   nub_addr_t aligned_start = requested_addr & ~(aligned_size - 1);
   // Does this power-of-2 memory range, aligned to power-of-2, completely
   // encompass the requested watch region.
   if (aligned_start + aligned_size >= requested_addr + requested_size) {
     WatchpointSpec wp;
if (read == false && write == false)
     return INVALID_NUB_HW_INDEX;
 
   // Only one hardware watchpoint needed
   // to implement the user's request.
   if (wps.size() == 1) {
     if (wps[0].aligned_size <= 8)
       return SetBASWatchpoint(wps[0], read, write, also_set_on_task);
     else
-      return INVALID_NUB_HW_INDEX;
+      return SetMASKWatchpoint(wps[0], read, write, also_set_on_task);
   }
 
   // We have multiple WatchpointSpecs
 
   std::vector<uint32_t> wp_slots_used;
   for (size_t i = 0; i < wps.size(); i++) {
     uint32_t idx =
         EnableHardwareWatchpoint(wps[i].requested_start, wps[i].requested_size,
uint32_t DNBArchMachARM64::SetBASWatchpoint(DNBArchMachARM64::WatchpointSpec wp,
 
   DNBLogThreadedIf(LOG_WATCHPOINTS,
                    "DNBArchMachARM64::SetBASWatchpoint() "
                    "adding watchpoint on address 0x%llx with control "
                    "register value 0x%x",
                    (uint64_t)m_state.dbg.__wvr[i],
                    (uint32_t)m_state.dbg.__wcr[i]);
 
-  // The kernel will set the MDE_ENABLE bit in the MDSCR_EL1 for us
-  // automatically, don't need to do it here.
-
   kret = SetDBGState(also_set_on_task);
   // DumpDBGState(m_state.dbg);
 
   DNBLogThreadedIf(LOG_WATCHPOINTS,
                    "DNBArchMachARM64::"
                    "SetBASWatchpoint() "
                    "SetDBGState() => 0x%8.8x.",
                    kret);
 
   if (kret == KERN_SUCCESS)
     return i;
 
   return INVALID_NUB_HW_INDEX;
 }
 
+uint32_t
+DNBArchMachARM64::SetMASKWatchpoint(DNBArchMachARM64::WatchpointSpec wp,
+                                    bool read, bool write,
+                                    bool also_set_on_task) {
+  const uint32_t num_hw_watchpoints = NumSupportedHardwareWatchpoints();
+
+  // Read the debug state
+  kern_return_t kret = GetDBGState(false);
+  if (kret != KERN_SUCCESS)
+    return INVALID_NUB_HW_INDEX;
+
+  // Check to make sure we have the needed hardware support
+  uint32_t i = 0;
+
+  for (i = 0; i < num_hw_watchpoints; ++i) {
+    if ((m_state.dbg.__wcr[i] & WCR_ENABLE) == 0)
+      break; // We found an available hw watchpoint slot
+  }
+  if (i == num_hw_watchpoints) {
+    DNBLogThreadedIf(LOG_WATCHPOINTS,
+                     "DNBArchMachARM64::"
+                     "SetMASKWatchpoint(): All "
+                     "hardware resources (%u) are in use.",
+                     num_hw_watchpoints);
+    return INVALID_NUB_HW_INDEX;
+  }
+
+  DNBLogThreadedIf(LOG_WATCHPOINTS,
+                   "DNBArchMachARM64::"
+                   "SetMASKWatchpoint() "
+                   "set hardware register %d to MASK watchpoint "
+                   "aligned start address 0x%llx, aligned size %zu",
+                   i, wp.aligned_start, wp.aligned_size);
+
+  // Clear any previous LoHi joined-watchpoint that may have been in use
+  LoHi[i] = 0;
+
+  // MASK field is the number of low bits that are masked off
+  // when comparing the address with the DBGWVR<n>_EL1 values.
+  // If aligned size is 16, that means we ignore low 4 bits, 0b1111.
+  // popcount(16 - 1) give us the correct value of 4.
+  // 2GB is max watchable region, which is 31 bits (low bits 0x7fffffff
+  // masked off) -- a MASK value of 31.
+  const uint64_t mask = __builtin_popcountl(wp.aligned_size - 1) << 24;
+  // A '0b11111111' BAS value needed for mask watchpoints plus a
+  // nonzero mask value.
+  const uint64_t not_bas_wp = 0xff << 5;
+
+  m_state.dbg.__wvr[i] = wp.aligned_start;
+  m_state.dbg.__wcr[i] = mask | not_bas_wp | S_USER | // Stop only in user mode
+                         (read ? WCR_LOAD : 0) |      // Stop on read access?
+                         (write ? WCR_STORE : 0) |    // Stop on write access?
+                         WCR_ENABLE;                  // Enable this watchpoint;
+
+  DNBLogThreadedIf(LOG_WATCHPOINTS,
+                   "DNBArchMachARM64::SetMASKWatchpoint() "
+                   "adding watchpoint on address 0x%llx with control "
+                   "register value 0x%llx",
+                   (uint64_t)m_state.dbg.__wvr[i],
+                   (uint64_t)m_state.dbg.__wcr[i]);
+
+  kret = SetDBGState(also_set_on_task);
+
+  DNBLogThreadedIf(LOG_WATCHPOINTS,
+                   "DNBArchMachARM64::"
+                   "SetMASKWatchpoint() "
+                   "SetDBGState() => 0x%8.8x.",
+                   kret);
+
+  if (kret == KERN_SUCCESS)
+    return i;
+
+  return INVALID_NUB_HW_INDEX;
+}
+
 bool DNBArchMachARM64::ReenableHardwareWatchpoint(uint32_t hw_index) {
   // If this logical watchpoint # is actually implemented using
   // two hardware watchpoint registers, re-enable both of them.
 
   if (hw_index < NumSupportedHardwareWatchpoints() && LoHi[hw_index]) {
     return ReenableHardwareWatchpoint_helper(hw_index) &&
            ReenableHardwareWatchpoint_helper(LoHi[hw_index]);
   } else {
bool DNBArchMachARM64::ReenableHardwareWatchpoint_helper(uint32_t hw_index) {
   if (hw_index >= num_hw_points)
     return false;
 
   m_state.dbg.__wvr[hw_index] = m_disabled_watchpoints[hw_index].addr;
   m_state.dbg.__wcr[hw_index] = m_disabled_watchpoints[hw_index].control;
 
   DNBLogThreadedIf(LOG_WATCHPOINTS,
                    "DNBArchMachARM64::"
-                   "SetBASWatchpoint( %u ) - WVR%u = "
+                   "ReenableHardwareWatchpoint_helper( %u ) - WVR%u = "
                    "0x%8.8llx  WCR%u = 0x%8.8llx",
                    hw_index, hw_index, (uint64_t)m_state.dbg.__wvr[hw_index],
                    hw_index, (uint64_t)m_state.dbg.__wcr[hw_index]);
 
-  // The kernel will set the MDE_ENABLE bit in the MDSCR_EL1 for us
-  // automatically, don't need to do it here.
-
   kret = SetDBGState(false);
 
   return (kret == KERN_SUCCESS);
 }
 
 bool DNBArchMachARM64::DisableHardwareWatchpoint(uint32_t hw_index,
                                                  bool also_set_on_task) {
   if (hw_index < NumSupportedHardwareWatchpoints() && LoHi[hw_index]) {
DNBLogThreadedIf(LOG_WATCHPOINTS,
                    "DNBArchMachARM64::GetHardwareWatchpointHit() addr = 0x%llx",
                    (uint64_t)addr);
 
   if (kret == KERN_SUCCESS) {
     DBG &debug_state = m_state.dbg;
     uint32_t i, num = NumSupportedHardwareWatchpoints();
     for (i = 0; i < num; ++i) {
       nub_addr_t wp_addr = GetWatchAddress(debug_state, i);
-      uint32_t byte_mask = bits(debug_state.__wcr[i], 12, 5);
 
       DNBLogThreadedIf(LOG_WATCHPOINTS,
                        "DNBArchImplARM64::"
                        "GetHardwareWatchpointHit() slot: %u "
-                       "(addr = 0x%llx; byte_mask = 0x%x)",
-                       i, static_cast<uint64_t>(wp_addr), byte_mask);
+                       "(addr = 0x%llx, WCR = 0x%llx)",
+                       i, wp_addr, debug_state.__wcr[i]);
 
       if (!IsWatchpointEnabled(debug_state, i))
         continue;
 
+      // DBGWCR<n>EL1.BAS are the bits of the doubleword that are watched
+      // with a BAS watchpoint.
+      uint32_t bas_bits = bits(debug_state.__wcr[i], 12, 5);
+      // DBGWCR<n>EL1.MASK is the number of bits that are masked off the
+      // virtual address when comparing to DBGWVR<n>_EL1.
+      uint32_t mask = bits(debug_state.__wcr[i], 28, 24);
+
+      const bool is_bas_watchpoint = mask == 0;
+
+      DNBLogThreadedIf(
+          LOG_WATCHPOINTS,
+          "DNBArchImplARM64::"
+          "GetHardwareWatchpointHit() slot: %u %s",
+          i, is_bas_watchpoint ? "is BAS watchpoint" : "is MASK watchpoint");
+
+      if (is_bas_watchpoint) {
         if (bits(wp_addr, 48, 3) != bits(addr, 48, 3))
           continue;
+      } else {
+        if (bits(wp_addr, 48, mask) == bits(addr, 48, mask)) {
+          DNBLogThreadedIf(LOG_WATCHPOINTS,
+                           "DNBArchImplARM64::"
+                           "GetHardwareWatchpointHit() slot: %u matched MASK "
+                           "ignoring %u low bits",
+                           i, mask);
+          return i;
+        }
+      }
 
-      // Sanity check the byte_mask
-      uint32_t lsb = LowestBitSet(byte_mask);
+      if (is_bas_watchpoint) {
+        // Sanity check the bas_bits
+        uint32_t lsb = LowestBitSet(bas_bits);
         if (lsb < 0)
           continue;
 
         uint64_t byte_to_match = bits(addr, 2, 0);
 
-      if (byte_mask & (1 << byte_to_match)) {
+        if (bas_bits & (1 << byte_to_match)) {
           addr = wp_addr + lsb;
+          DNBLogThreadedIf(LOG_WATCHPOINTS,
+                           "DNBArchImplARM64::"
+                           "GetHardwareWatchpointHit() slot: %u matched BAS",
+                           i);
           return i;
         }
       }
     }
+  }
   return INVALID_NUB_HW_INDEX;
 }
 
 nub_addr_t DNBArchMachARM64::GetWatchpointAddressByIndex(uint32_t hw_index) {
   kern_return_t kret = GetDBGState(true);
   if (kret != KERN_SUCCESS)
     return INVALID_NUB_ADDRESS;
   const uint32_t num = NumSupportedHardwareWatchpoints();