Fix ARM/AArch64 Step-Over watchpoint issue remove provision for duplicate watchpoints

[omjavaid]

Summary

On ARM Linux targets watchpoints are reported by PTrace before the instruction causing watchpoint to trigger is executed.
This means we cannot step-over watchpoint causing instruction as long as there is watchpoint installed on that location.
For this reason we cannot have multiple watchpoint slots watching same region word/byte/halfword etc as we have to disable watchpoint for stepping over and only watchpoint index reported by lldb-server is disabled.
Similarly we cannot keep a single hardware watchpoint slot referring to multiple watchpoints in LLDB. This actually means that hware watchpoint indexes are exclusively assigned to a watchpoint and cannot be shared by other watchpoint. Also no other watchpoint should watch same address already watched by any other index.

More discussion on this can be found here:
https://reviews.llvm.org/D24610

This patch fixes issue with stepping over watchpoint by removing the provision to enable multiple watchpoints referring to same hardware watchpoint slot (same HW index). Also we restrict one watchpoint per word aligned address so duplication of address will not be allowed to avoid any step-over failures.

I have also attached a test-case that tests this scenario.

[omjavaid]

@labath Referring to your email on the mailing list.

Thanks for helping out with this work.

I think we should push this fix, as you suggested this does not fix everything in a holistic way but it corrects the functionality that is currently available right now with limitations ofcourse.

So here is functionality we are currently lacking:

• Ability to put multiple watchpoints with same address range.

This is more concerning because we cannot put a watch on say byte 0 and byte 7 in case of aarch64.

• Ability to use single slot for multiple watchpoints.

This is more like a nice to have and I think if we fix the above functionality this may well be fixed automatically.

This is what I think LLDB client or server has to do:

• Keep a cache of watchpoint registers available and modify registers in cache when a set/remove request is made.
• As pre-req for set/remove is to have the target in stopped state this will mean that when we set/remove we make changes to actual registers before we resume for continue or stepping.
• I dont think keeping the cache and then updating on resume actually means we are lying to client. Cache will also remain limited and will behave like an actual write to the registers. It will serve us well to support the functionality we intend to support.

In case of GDB this functionality is handled by gdbserver and gdb client is not aware of the fact that watchpoint registers are not actually written until we resume.

To implement this in LLDB client or server is a design decision and I just see that it should be easier to achieve on LLDB server side though it may require changes to the way we approach watchpoint for all targets but it will then remain restricted to the concerning target specific area.

I am OOO till 16th If you are OK with this change I will push it whenever it gets a LGTM.

[labath]

In D25057#560325, @omjavaid wrote:

@labath Referring to your email on the mailing list.

Thanks for helping out with this work.

I think we should push this fix, as you suggested this does not fix everything in a holistic way but it corrects the functionality that is currently available right now with limitations ofcourse.

So here is functionality we are currently lacking:

• Ability to put multiple watchpoints with same address range.

This is more concerning because we cannot put a watch on say byte 0 and byte 7 in case of aarch64.

Agreed. However, I'd rephrase this as "ability to correctly handle a single instruction triggering multiple watchpoints. If done properly, you will get the previous item for free.

Also, apparently lldb has a bug, where it mishandles the case where you do a (user-level) single step over an instruction triggering a watchpoint. That would be pretty important to fix as well.

• Ability to use single slot for multiple watchpoints.

This is more like a nice to have and I think if we fix the above functionality this may well be fixed automatically.

I am not sure I agree with this, but I'd have to see the implementation to tell. One of the issues I have with reusing same slot for multiple watchpoints is that it does not work at all for the "read" case, and even the "write" case is extremely dodgy. Normally a "write" watchpoint should trigger whenever the memory is written to, but here we are treating it more like a "modify" watchpoint, where we stop only if the write actually modifies the memory value. Reusing a slot for "modify" watchpoints is easy, doing it correctly for the "write" case is quite hard.

This is what I think LLDB client or server has to do:

• Keep a cache of watchpoint registers available and modify registers in cache when a set/remove request is made.

Sure, why not.

• As pre-req for set/remove is to have the target in stopped state this will mean that when we set/remove we make changes to actual registers before we resume for continue or stepping.

OK, you cannot set watchpoint set/clear packets while the target is running anyway.

• I dont think keeping the cache and then updating on resume actually means we are lying to client. Cache will also remain limited and will behave like an actual write to the registers. It will serve us well to support the functionality we intend to support.

It depends, on whether the fact that we don't write to the registers immediately has any observable effects for the client. In your previous patch, it did. This happened because you were only syncing the registers on a resume, so if the client did a single-step instead, the watch would not trigger. This is what I consider "lying", and violating the gdb-remote protocol. If the client cannot tell the difference, you are free to do whatever you want.

In case of GDB this functionality is handled by gdbserver and gdb client is not aware of the fact that watchpoint registers are not actually written until we resume.

This is interesting. Can you tell me what is the packet sequence in the case where the client has watchpoints set at 0x1000 and 0x1001 and an instruction trips both of them?

To implement this in LLDB client or server is a design decision and I just see that it should be easier to achieve on LLDB server side though it may require changes to the way we approach watchpoint for all targets but it will then remain restricted to the concerning target specific area.

I don't see how you can handle the case when an instruction trips two watchpoints in different slots with server-side changes only. If it can be done, then it is a design decision, but until then, I believe it is a decision between a correct and a partial solution, and I'd go with the fully correct one.

I am OOO till 16th If you are OK with this change I will push it whenever it gets a LGTM.

Some small nits on the patch you should fix before committing. Otherwise, looks good.

packages/Python/lldbsuite/test/functionalities/watchpoint/multi_watchpoint_slots/TestWatchpointMultipleSlots.py

35 ↗

(On Diff #72932)

this is unnecesary, as the test is already skipped.

39 ↗

(On Diff #72932)

this is unnecesary, as the test is already skipped.

source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp

579 ↗

(On Diff #72932)

The formatting here is incorrect. Please run clang-format on the patch.

source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp

547 ↗

(On Diff #72932)

ditto

[omjavaid]

Sorry I was on Holiday so couldnt get back to this earlier.

I ll get back to your comments about GDB packet sequence in a separate thread after collecting further information.

I have made the suggested corrections in above patch.

[labath]

LGTM, thanks.

BTW, it looks like the linux kernel is about to grow support for proper byte-range watchpoints http://lists.infradead.org/pipermail/linux-arm-kernel/2016-October/461036.html.

lldb/trunk/packages/Python/lldbsuite/test/functionalities/watchpoint/multi_watchpoint_slots/Makefile

+LEVEL = ../../../make
+
+C_SOURCES := main.c
+
+include $(LEVEL)/Makefile.rules

lldb/trunk/packages/Python/lldbsuite/test/functionalities/watchpoint/multi_watchpoint_slots/TestWatchpointMultipleSlots.py

+"""
+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")

lldb/trunk/packages/Python/lldbsuite/test/functionalities/watchpoint/multi_watchpoint_slots/main.c

+//===-- 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;
+}

lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp

uint32_t NativeRegisterContextLinux_arm::SetHardwareWatchpoint(
   control_value |= (watch_flags << 3);
 
   // Enable this watchpoint and make it stop in privileged or user mode;
   control_value |= 7;
 
   // 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;
 
-    // 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;
 
-      return LLDB_INVALID_INDEX32;
-    }
-  } else
-    m_hwp_regs[wp_index].refcount++;
 
   return wp_index;
 }
 
 bool NativeRegisterContextLinux_arm::ClearHardwareWatchpoint(
     uint32_t wp_index) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_WATCHPOINTS));
 
   if (log)
     log->Printf("NativeRegisterContextLinux_arm::%s()", __FUNCTION__);
 
   Error error;
 
   // Read hardware breakpoint and watchpoint information.
   error = ReadHardwareDebugInfo();
 
   if (error.Fail())
     return false;
 
   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;
 
-    // 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;
 
-    // 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;
 
-      return false;
-    }
 
-    return true;
-  }
 
-  return false;
-}
-
 Error NativeRegisterContextLinux_arm::ClearAllHardwareWatchpoints() {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_WATCHPOINTS));
 
   if (log)
     log->Printf("NativeRegisterContextLinux_arm::%s()", __FUNCTION__);
 
   Error error;
 
   // Read hardware breakpoint and watchpoint information.
   error = ReadHardwareDebugInfo();
 
   if (error.Fail())
     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);
 
       if (error.Fail()) {
         m_hwp_regs[i].control = tempControl;
         m_hwp_regs[i].address = tempAddr;
-        m_hwp_regs[i].refcount = tempRefCount;
 
         return error;
       }
     }
   }
 
   return Error();
 }
Error NativeRegisterContextLinux_arm::GetWatchpointHitIndex(
 
   uint32_t watch_size;
   lldb::addr_t watch_addr;
 
   for (wp_index = 0; wp_index < m_max_hwp_supported; ++wp_index) {
     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();
     }
   }
 
   wp_index = LLDB_INVALID_INDEX32;
   return Error();
 }

lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp

if (addr & 0x07) {
     addr = addr & (~0x07);
   }
 
   // Setup control value
   control_value = watch_flags << 3;
   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;
 
-    // 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;
 
-      return LLDB_INVALID_INDEX32;
-    }
-  } else
-    m_hwp_regs[wp_index].refcount++;
 
   return wp_index;
 }
 
 bool NativeRegisterContextLinux_arm64::ClearHardwareWatchpoint(
     uint32_t wp_index) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_WATCHPOINTS));
 
   if (log)
     log->Printf("NativeRegisterContextLinux_arm64::%s()", __FUNCTION__);
 
   Error error;
 
   // Read hardware breakpoint and watchpoint information.
   error = ReadHardwareDebugInfo();
 
   if (error.Fail())
     return false;
 
   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;
 
-    // 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;
 
-    // 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;
 
-      return false;
-    }
 
-    return true;
-  }
 
-  return false;
-}
-
 Error NativeRegisterContextLinux_arm64::ClearAllHardwareWatchpoints() {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_WATCHPOINTS));
 
   if (log)
     log->Printf("NativeRegisterContextLinux_arm64::%s()", __FUNCTION__);
 
   Error error;
 
   // Read hardware breakpoint and watchpoint information.
   error = ReadHardwareDebugInfo();
 
   if (error.Fail())
     return error;
 
   lldb::addr_t tempAddr = 0;
   uint32_t tempControl = 0, tempRefCount = 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);
 
       if (error.Fail()) {
         m_hwp_regs[i].control = tempControl;
         m_hwp_regs[i].address = tempAddr;
-        m_hwp_regs[i].refcount = tempRefCount;
 
         return error;
       }
     }
   }
 
   return Error();
 }
Error NativeRegisterContextLinux_arm64::GetWatchpointHitIndex(
 
   uint32_t watch_size;
   lldb::addr_t watch_addr;
 
   for (wp_index = 0; wp_index < m_max_hwp_supported; ++wp_index) {
     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();
     }
   }
 
   wp_index = LLDB_INVALID_INDEX32;
   return Error();
 }