Fix ARM/AArch64 Step-Over watchpoint issue remove provision for duplicate watchpoints
authorOmair Javaid <omair.javaid@linaro.org>
Thu, 20 Oct 2016 09:07:26 +0000 (09:07 +0000)
committerOmair Javaid <omair.javaid@linaro.org>
Thu, 20 Oct 2016 09:07:26 +0000 (09:07 +0000)
This patch fixes ARM/AArch64 watchpoint bug which was taking inferior out of control while stepping over watchpoints.
Also adds a test case that tests above problem.

Differential revision: https://reviews.llvm.org/D25057

llvm-svn: 284706

lldb/packages/Python/lldbsuite/test/functionalities/watchpoint/multi_watchpoint_slots/Makefile [new file with mode: 0644]
lldb/packages/Python/lldbsuite/test/functionalities/watchpoint/multi_watchpoint_slots/TestWatchpointMultipleSlots.py [new file with mode: 0644]
lldb/packages/Python/lldbsuite/test/functionalities/watchpoint/multi_watchpoint_slots/main.c [new file with mode: 0644]
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp

diff --git a/lldb/packages/Python/lldbsuite/test/functionalities/watchpoint/multi_watchpoint_slots/Makefile b/lldb/packages/Python/lldbsuite/test/functionalities/watchpoint/multi_watchpoint_slots/Makefile
new file mode 100644 (file)
index 0000000..b09a579
--- /dev/null
@@ -0,0 +1,5 @@
+LEVEL = ../../../make
+
+C_SOURCES := main.c
+
+include $(LEVEL)/Makefile.rules
diff --git a/lldb/packages/Python/lldbsuite/test/functionalities/watchpoint/multi_watchpoint_slots/TestWatchpointMultipleSlots.py b/lldb/packages/Python/lldbsuite/test/functionalities/watchpoint/multi_watchpoint_slots/TestWatchpointMultipleSlots.py
new file mode 100644 (file)
index 0000000..2e58cb8
--- /dev/null
@@ -0,0 +1,97 @@
+"""
+Test watchpoint slots we should not be able to install multiple watchpoints
+within same word boundary. We should be able to install individual watchpoints
+on any of the bytes, half-word, or word. This is only for ARM/AArch64 targets.
+"""
+
+from __future__ import print_function
+
+import os
+import time
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class WatchpointSlotsTestCase(TestBase):
+    NO_DEBUG_INFO_TESTCASE = True
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def setUp(self):
+        # Call super's setUp().
+        TestBase.setUp(self)
+
+        # Source filename.
+        self.source = 'main.c'
+
+        # Output filename.
+        self.exe_name = 'a.out'
+        self.d = {'C_SOURCES': self.source, 'EXE': self.exe_name}
+
+    # Watchpoints not supported
+    @expectedFailureAndroid(archs=['arm', 'aarch64'])
+    # This is a arm and aarch64 specific test case. No other architectures tested.
+    @skipIf(archs=no_match(['arm', 'aarch64']))
+    def test_multiple_watchpoints_on_same_word(self):
+
+        self.build(dictionary=self.d)
+        self.setTearDownCleanup(dictionary=self.d)
+
+        exe = os.path.join(os.getcwd(), self.exe_name)
+        self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+        # Detect line number after which we are going to increment arrayName.
+        loc_line = line_number('main.c', '// About to write byteArray')
+
+        # Set a breakpoint on the line detected above.
+        lldbutil.run_break_set_by_file_and_line(
+            self, "main.c", loc_line, num_expected_locations=1, loc_exact=True)
+
+        # Run the program.
+        self.runCmd("run", RUN_SUCCEEDED)
+
+        # The stop reason of the thread should be breakpoint.
+        self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+                     substrs=['stopped', 'stop reason = breakpoint'])
+
+        # Delete breakpoint we just hit.
+        self.expect("breakpoint delete 1", substrs=['1 breakpoints deleted'])
+
+        # Set a watchpoint at byteArray[0]
+        self.expect("watchpoint set variable byteArray[0]", WATCHPOINT_CREATED,
+                    substrs=['Watchpoint created','size = 1'])
+
+        # Use the '-v' option to do verbose listing of the watchpoint.
+        # The hit count should be 0 initially.
+        self.expect("watchpoint list -v 1", substrs=['hit_count = 0'])
+
+        # Try setting a watchpoint at byteArray[1]
+        self.expect("watchpoint set variable byteArray[1]", error=True,
+                    substrs=['Watchpoint creation failed'])
+
+        self.runCmd("process continue")
+
+        # We should be stopped due to the watchpoint.
+        # The stop reason of the thread should be watchpoint.
+        self.expect("thread list", STOPPED_DUE_TO_WATCHPOINT,
+                    substrs=['stopped', 'stop reason = watchpoint 1'])
+
+        # Delete the watchpoint we hit above successfully.
+        self.expect("watchpoint delete 1", substrs=['1 watchpoints deleted'])
+
+        # Set a watchpoint at byteArray[3]
+        self.expect("watchpoint set variable byteArray[3]", WATCHPOINT_CREATED,
+                    substrs=['Watchpoint created','size = 1'])
+   
+        # Resume inferior.
+        self.runCmd("process continue")
+
+        # We should be stopped due to the watchpoint.
+        # The stop reason of the thread should be watchpoint.
+        self.expect("thread list -v", STOPPED_DUE_TO_WATCHPOINT,
+                    substrs=['stopped', 'stop reason = watchpoint 3'])
+   
+        # Resume inferior.
+        self.runCmd("process continue")
diff --git a/lldb/packages/Python/lldbsuite/test/functionalities/watchpoint/multi_watchpoint_slots/main.c b/lldb/packages/Python/lldbsuite/test/functionalities/watchpoint/multi_watchpoint_slots/main.c
new file mode 100644 (file)
index 0000000..fd14a9b
--- /dev/null
@@ -0,0 +1,29 @@
+//===-- main.c --------------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+#include <stdio.h>
+#include <stdint.h>
+
+uint64_t pad0 = 0;
+uint8_t byteArray[4] = {0};
+uint64_t pad1 = 0;
+
+int main(int argc, char** argv) {
+
+    int i;
+
+    for (i = 0; i < 4; i++)
+    {
+        printf("About to write byteArray[%d] ...\n", i); // About to write byteArray
+        pad0++;
+        byteArray[i] = 7;
+        pad1++;
+    }
+
+    return 0;
+}
index 758d5e7..9e85713 100644 (file)
@@ -570,42 +570,33 @@ uint32_t NativeRegisterContextLinux_arm::SetHardwareWatchpoint(
   // Make sure bits 1:0 are clear in our address
   addr &= ~((lldb::addr_t)3);
 
-  // Iterate over stored watchpoints
-  // Find a free wp_index or update reference count if duplicate.
+  // Iterate over stored watchpoints and find a free wp_index
   wp_index = LLDB_INVALID_INDEX32;
   for (uint32_t i = 0; i < m_max_hwp_supported; i++) {
     if ((m_hwp_regs[i].control & 1) == 0) {
       wp_index = i; // Mark last free slot
-    } else if (m_hwp_regs[i].address == addr &&
-               m_hwp_regs[i].control == control_value) {
-      wp_index = i; // Mark duplicate index
-      break;        // Stop searching here
+    } else if (m_hwp_regs[i].address == addr) {
+      return LLDB_INVALID_INDEX32; // We do not support duplicate watchpoints.
     }
   }
 
   if (wp_index == LLDB_INVALID_INDEX32)
     return LLDB_INVALID_INDEX32;
 
-  // Add new or update existing watchpoint
-  if ((m_hwp_regs[wp_index].control & 1) == 0) {
-    // Update watchpoint in local cache
-    m_hwp_regs[wp_index].real_addr = real_addr;
-    m_hwp_regs[wp_index].address = addr;
-    m_hwp_regs[wp_index].control = control_value;
-    m_hwp_regs[wp_index].refcount = 1;
+  // Update watchpoint in local cache
+  m_hwp_regs[wp_index].real_addr = real_addr;
+  m_hwp_regs[wp_index].address = addr;
+  m_hwp_regs[wp_index].control = control_value;
 
-    // PTRACE call to set corresponding watchpoint register.
-    error = WriteHardwareDebugRegs(eDREGTypeWATCH, wp_index);
+  // PTRACE call to set corresponding watchpoint register.
+  error = WriteHardwareDebugRegs(eDREGTypeWATCH, wp_index);
 
-    if (error.Fail()) {
-      m_hwp_regs[wp_index].address = 0;
-      m_hwp_regs[wp_index].control &= ~1;
-      m_hwp_regs[wp_index].refcount = 0;
+  if (error.Fail()) {
+    m_hwp_regs[wp_index].address = 0;
+    m_hwp_regs[wp_index].control &= ~1;
 
-      return LLDB_INVALID_INDEX32;
-    }
-  } else
-    m_hwp_regs[wp_index].refcount++;
+    return LLDB_INVALID_INDEX32;
+  }
 
   return wp_index;
 }
@@ -628,36 +619,25 @@ bool NativeRegisterContextLinux_arm::ClearHardwareWatchpoint(
   if (wp_index >= m_max_hwp_supported)
     return false;
 
-  // Update reference count if multiple references.
-  if (m_hwp_regs[wp_index].refcount > 1) {
-    m_hwp_regs[wp_index].refcount--;
-    return true;
-  } else if (m_hwp_regs[wp_index].refcount == 1) {
-    // Create a backup we can revert to in case of failure.
-    lldb::addr_t tempAddr = m_hwp_regs[wp_index].address;
-    uint32_t tempControl = m_hwp_regs[wp_index].control;
-    uint32_t tempRefCount = m_hwp_regs[wp_index].refcount;
+  // Create a backup we can revert to in case of failure.
+  lldb::addr_t tempAddr = m_hwp_regs[wp_index].address;
+  uint32_t tempControl = m_hwp_regs[wp_index].control;
 
-    // Update watchpoint in local cache
-    m_hwp_regs[wp_index].control &= ~1;
-    m_hwp_regs[wp_index].address = 0;
-    m_hwp_regs[wp_index].refcount = 0;
+  // Update watchpoint in local cache
+  m_hwp_regs[wp_index].control &= ~1;
+  m_hwp_regs[wp_index].address = 0;
 
-    // Ptrace call to update hardware debug registers
-    error = WriteHardwareDebugRegs(eDREGTypeWATCH, wp_index);
-
-    if (error.Fail()) {
-      m_hwp_regs[wp_index].control = tempControl;
-      m_hwp_regs[wp_index].address = tempAddr;
-      m_hwp_regs[wp_index].refcount = tempRefCount;
+  // Ptrace call to update hardware debug registers
+  error = WriteHardwareDebugRegs(eDREGTypeWATCH, wp_index);
 
-      return false;
-    }
+  if (error.Fail()) {
+    m_hwp_regs[wp_index].control = tempControl;
+    m_hwp_regs[wp_index].address = tempAddr;
 
-    return true;
+    return false;
   }
 
-  return false;
+  return true;
 }
 
 Error NativeRegisterContextLinux_arm::ClearAllHardwareWatchpoints() {
@@ -675,19 +655,17 @@ Error NativeRegisterContextLinux_arm::ClearAllHardwareWatchpoints() {
     return error;
 
   lldb::addr_t tempAddr = 0;
-  uint32_t tempControl = 0, tempRefCount = 0;
+  uint32_t tempControl = 0;
 
   for (uint32_t i = 0; i < m_max_hwp_supported; i++) {
     if (m_hwp_regs[i].control & 0x01) {
       // Create a backup we can revert to in case of failure.
       tempAddr = m_hwp_regs[i].address;
       tempControl = m_hwp_regs[i].control;
-      tempRefCount = m_hwp_regs[i].refcount;
 
       // Clear watchpoints in local cache
       m_hwp_regs[i].control &= ~1;
       m_hwp_regs[i].address = 0;
-      m_hwp_regs[i].refcount = 0;
 
       // Ptrace call to update hardware debug registers
       error = WriteHardwareDebugRegs(eDREGTypeWATCH, i);
@@ -695,7 +673,6 @@ Error NativeRegisterContextLinux_arm::ClearAllHardwareWatchpoints() {
       if (error.Fail()) {
         m_hwp_regs[i].control = tempControl;
         m_hwp_regs[i].address = tempAddr;
-        m_hwp_regs[i].refcount = tempRefCount;
 
         return error;
       }
@@ -750,8 +727,8 @@ Error NativeRegisterContextLinux_arm::GetWatchpointHitIndex(
     watch_size = GetWatchpointSize(wp_index);
     watch_addr = m_hwp_regs[wp_index].address;
 
-    if (m_hwp_regs[wp_index].refcount >= 1 && WatchpointIsEnabled(wp_index) &&
-        trap_addr >= watch_addr && trap_addr < watch_addr + watch_size) {
+    if (WatchpointIsEnabled(wp_index) && trap_addr >= watch_addr &&
+        trap_addr < watch_addr + watch_size) {
       m_hwp_regs[wp_index].hit_addr = trap_addr;
       return Error();
     }
index 83cd179..9171029 100644 (file)
@@ -538,42 +538,33 @@ uint32_t NativeRegisterContextLinux_arm64::SetHardwareWatchpoint(
   control_value |= ((1 << size) - 1) << 5;
   control_value |= (2 << 1) | 1;
 
-  // Iterate over stored watchpoints
-  // Find a free wp_index or update reference count if duplicate.
+  // Iterate over stored watchpoints and find a free wp_index
   wp_index = LLDB_INVALID_INDEX32;
   for (uint32_t i = 0; i < m_max_hwp_supported; i++) {
     if ((m_hwp_regs[i].control & 1) == 0) {
       wp_index = i; // Mark last free slot
-    } else if (m_hwp_regs[i].address == addr &&
-               m_hwp_regs[i].control == control_value) {
-      wp_index = i; // Mark duplicate index
-      break;        // Stop searching here
+    } else if (m_hwp_regs[i].address == addr) {
+      return LLDB_INVALID_INDEX32; // We do not support duplicate watchpoints.
     }
   }
 
   if (wp_index == LLDB_INVALID_INDEX32)
     return LLDB_INVALID_INDEX32;
 
-  // Add new or update existing watchpoint
-  if ((m_hwp_regs[wp_index].control & 1) == 0) {
-    // Update watchpoint in local cache
-    m_hwp_regs[wp_index].real_addr = real_addr;
-    m_hwp_regs[wp_index].address = addr;
-    m_hwp_regs[wp_index].control = control_value;
-    m_hwp_regs[wp_index].refcount = 1;
+  // Update watchpoint in local cache
+  m_hwp_regs[wp_index].real_addr = real_addr;
+  m_hwp_regs[wp_index].address = addr;
+  m_hwp_regs[wp_index].control = control_value;
 
-    // PTRACE call to set corresponding watchpoint register.
-    error = WriteHardwareDebugRegs(eDREGTypeWATCH);
+  // PTRACE call to set corresponding watchpoint register.
+  error = WriteHardwareDebugRegs(eDREGTypeWATCH);
 
-    if (error.Fail()) {
-      m_hwp_regs[wp_index].address = 0;
-      m_hwp_regs[wp_index].control &= ~1;
-      m_hwp_regs[wp_index].refcount = 0;
+  if (error.Fail()) {
+    m_hwp_regs[wp_index].address = 0;
+    m_hwp_regs[wp_index].control &= ~1;
 
-      return LLDB_INVALID_INDEX32;
-    }
-  } else
-    m_hwp_regs[wp_index].refcount++;
+    return LLDB_INVALID_INDEX32;
+  }
 
   return wp_index;
 }
@@ -596,36 +587,25 @@ bool NativeRegisterContextLinux_arm64::ClearHardwareWatchpoint(
   if (wp_index >= m_max_hwp_supported)
     return false;
 
-  // Update reference count if multiple references.
-  if (m_hwp_regs[wp_index].refcount > 1) {
-    m_hwp_regs[wp_index].refcount--;
-    return true;
-  } else if (m_hwp_regs[wp_index].refcount == 1) {
-    // Create a backup we can revert to in case of failure.
-    lldb::addr_t tempAddr = m_hwp_regs[wp_index].address;
-    uint32_t tempControl = m_hwp_regs[wp_index].control;
-    uint32_t tempRefCount = m_hwp_regs[wp_index].refcount;
+  // Create a backup we can revert to in case of failure.
+  lldb::addr_t tempAddr = m_hwp_regs[wp_index].address;
+  uint32_t tempControl = m_hwp_regs[wp_index].control;
 
-    // Update watchpoint in local cache
-    m_hwp_regs[wp_index].control &= ~1;
-    m_hwp_regs[wp_index].address = 0;
-    m_hwp_regs[wp_index].refcount = 0;
+  // Update watchpoint in local cache
+  m_hwp_regs[wp_index].control &= ~1;
+  m_hwp_regs[wp_index].address = 0;
 
-    // Ptrace call to update hardware debug registers
-    error = WriteHardwareDebugRegs(eDREGTypeWATCH);
-
-    if (error.Fail()) {
-      m_hwp_regs[wp_index].control = tempControl;
-      m_hwp_regs[wp_index].address = tempAddr;
-      m_hwp_regs[wp_index].refcount = tempRefCount;
+  // Ptrace call to update hardware debug registers
+  error = WriteHardwareDebugRegs(eDREGTypeWATCH);
 
-      return false;
-    }
+  if (error.Fail()) {
+    m_hwp_regs[wp_index].control = tempControl;
+    m_hwp_regs[wp_index].address = tempAddr;
 
-    return true;
+    return false;
   }
 
-  return false;
+  return true;
 }
 
 Error NativeRegisterContextLinux_arm64::ClearAllHardwareWatchpoints() {
@@ -650,12 +630,10 @@ Error NativeRegisterContextLinux_arm64::ClearAllHardwareWatchpoints() {
       // Create a backup we can revert to in case of failure.
       tempAddr = m_hwp_regs[i].address;
       tempControl = m_hwp_regs[i].control;
-      tempRefCount = m_hwp_regs[i].refcount;
 
       // Clear watchpoints in local cache
       m_hwp_regs[i].control &= ~1;
       m_hwp_regs[i].address = 0;
-      m_hwp_regs[i].refcount = 0;
 
       // Ptrace call to update hardware debug registers
       error = WriteHardwareDebugRegs(eDREGTypeWATCH);
@@ -663,7 +641,6 @@ Error NativeRegisterContextLinux_arm64::ClearAllHardwareWatchpoints() {
       if (error.Fail()) {
         m_hwp_regs[i].control = tempControl;
         m_hwp_regs[i].address = tempAddr;
-        m_hwp_regs[i].refcount = tempRefCount;
 
         return error;
       }
@@ -718,8 +695,8 @@ Error NativeRegisterContextLinux_arm64::GetWatchpointHitIndex(
     watch_size = GetWatchpointSize(wp_index);
     watch_addr = m_hwp_regs[wp_index].address;
 
-    if (m_hwp_regs[wp_index].refcount >= 1 && WatchpointIsEnabled(wp_index) &&
-        trap_addr >= watch_addr && trap_addr < watch_addr + watch_size) {
+    if (WatchpointIsEnabled(wp_index) && trap_addr >= watch_addr &&
+        trap_addr < watch_addr + watch_size) {
       m_hwp_regs[wp_index].hit_addr = trap_addr;
       return Error();
     }