[PATCH] [debugserver/ARM64] Make sure watchpoints hit are attributed correctly.

[davide]

Summary

From e330facaea0c3780734a6a061134551662fb9d74 Mon Sep 17 00:00:00 2001
From: Davide Italiano <[e-mail]>
Date: Tue, 31 Mar 2020 13:55:36 -0700
Subject: [PATCH] [debugserver/ARM64] Make sure watchpoints hit are attributed
correctly.

This didn't happen for arm64 if you have watches for variables
that are contigous in memory.

rdar://problem/55135006

.../watchpoints/watchpoint_count/Makefile | 3 ++
.../watchpoint_count/TestWatchpointCount.py | 43 +++++++++++++++++++
.../watchpoints/watchpoint_count/main.c | 13 ++++++
.../source/MacOSX/arm64/DNBArchImplARM64.cpp | 43 ++++++++++---------
4 files changed, 82 insertions(+), 20 deletions(-)
create mode 100644 lldb/test/API/commands/watchpoints/watchpoint_count/Makefile
create mode 100644 lldb/test/API/commands/watchpoints/watchpoint_count/TestWatchpointCount.py
create mode 100644 lldb/test/API/commands/watchpoints/watchpoint_count/main.c

[jasonmolenda]

I was a little worried about the case where we have an 8-byte region being watched that is 4-byte aligned (so it's spanning two 8-byte watchpoints), but a quick test of that with this patch shows it works correctly. This is fine with me.

lldb/test/API/commands/watchpoints/watchpoint_count/Makefile

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

lldb/test/API/commands/watchpoints/watchpoint_count/TestWatchpointCount.py

+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestWatchpointCount(TestBase):
+    mydir = TestBase.compute_mydir(__file__)
+    NO_DEBUG_INFO_TESTCASE = True
+
+    def setUp(self):
+        TestBase.setUp(self)
+
+    def test_watchpoint_count(self):
+        self.build()
+        (_, process, thread, _) = lldbutil.run_to_source_breakpoint(self, "patatino", lldb.SBFileSpec("main.c"))
+        frame = thread.GetFrameAtIndex(0)
+        first_var = frame.FindVariable("x1")
+        second_var = frame.FindVariable("x2")
+
+        error = lldb.SBError()
+        first_watch = first_var.Watch(True, False, True, error)
+        if not error.Success():
+            self.fail(
+                "Failed to make watchpoint for x1: %s" %
+                (error.GetCString()))
+
+        second_watch = second_var.Watch(True, False, True, error)
+        if not error.Success():
+            self.fail(
+                "Failed to make watchpoint for x2: %s" %
+                (error.GetCString()))
+        process.Continue()
+
+        stop_reason = thread.GetStopReason()
+        self.assertEqual(stop_reason, lldb.eStopReasonWatchpoint, "watchpoint for x1 not hit")
+        stop_reason_descr = thread.GetStopDescription(256)
+        self.assertEqual(stop_reason_descr, "watchpoint 1")
+
+        process.Continue()
+        stop_reason = thread.GetStopReason()
+        self.assertEqual(stop_reason, lldb.eStopReasonWatchpoint, "watchpoint for x2 not hit")
+        stop_reason_descr = thread.GetStopDescription(256)
+        self.assertEqual(stop_reason_descr, "watchpoint 2")

lldb/test/API/commands/watchpoints/watchpoint_count/main.c

+#include <stdint.h>
+#include <stdio.h>
+
+int main() {
+  uint8_t x1 = 0;
+  uint16_t x2 = 0;
+
+  printf("patatino\n");
+
+  x1 += 1;
+  x2 += 2;
+  return 0;
+}

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

                    "DNBArchMachARM64::GetHardwareWatchpointHit() addr = 0x%llx",
                    (uint64_t)addr);
 
-  // This is the watchpoint value to match against, i.e., word address.
-  nub_addr_t wp_val = addr & ~((nub_addr_t)3);
   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);
-      DNBLogThreadedIf(LOG_WATCHPOINTS, "DNBArchMachARM64::"
-                                        "GetHardwareWatchpointHit() slot: %u "
-                                        "(addr = 0x%llx).",
-                       i, (uint64_t)wp_addr);
-      if (wp_val == wp_addr) {
-        uint32_t byte_mask = bits(debug_state.__wcr[i], 12, 5);
-
-        // Sanity check the byte_mask, first.
-        if (LowestBitSet(byte_mask) < 0)
-          continue;
-
-        // Check that the watchpoint is enabled.
-        if (!IsWatchpointEnabled(debug_state, i))
-          continue;
-
-        // Compute the starting address (from the point of view of the
-        // debugger).
-        addr = wp_addr + LowestBitSet(byte_mask);
+      uint32_t byte_mask = bits(debug_state.__wcr[i], 12, 5);
+
+      DNBLogThreadedIf(LOG_WATCHPOINTS, "DNBArchImplX86_64::"
+                       "GetHardwareWatchpointHit() slot: %u "
+                       "(addr = 0x%llx; byte_mask = 0x%x)",
+                       i, static_cast<uint64_t>(wp_addr),
+                       byte_mask);
+
+      if (!IsWatchpointEnabled(debug_state, i))
+
+      if (bits(wp_addr, 48, 3) != bits(addr, 48, 3))
+        continue;
+
+      // Sanity check the byte_mask
+      uint32_t lsb = LowestBitSet(byte_mask);
+      if (lsb < 0)
+        continue;
+
+      uint64_t byte_to_match = bits(addr, 2, 0);
+
+      if (byte_mask & (1 << byte_to_match)) {
+        addr = wp_addr + lsb;
         return i;
       }
     }