[Lldb-commits] [PATCH] D24610: LLDB Arm Watchpoints: Use single hardware watchpoint slot to watch multiple bytes where possible

[Muhammad Omair Javaid via lldb-commits]

omjavaid added a comment.

comments inline.


================
Comment at: packages/Python/lldbsuite/test/functionalities/watchpoint/watchpoint_size/TestWatchpointSizes.py:43
@@ -42,2 +42,3 @@
         """Test to selectively watch different bytes in a 8-byte array."""
-        self.run_watchpoint_size_test('byteArray', 8, '1')
+        if self.getArchitecture() in ['arm']:
+            self.run_watchpoint_size_test('byteArray', 8, '1', 1)
----------------
labath wrote:
> It's not clear to me why you need to modify the existing test for this change. You are adding functionality, so all existing tests should pass as-is (which will also validate that your change did not introduce regressions).
As we keep on adding these tests its increasing our overall testing time. I just thought using the same test with some changes will cover the cases we want to test.

Anyways we can write separate tests as well.

================
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp:556
@@ +555,3 @@
+    uint32_t current_size = GetWatchpointSize(wp_index);
+    if ((current_size == 4) || (current_size == 2 && watch_mask <= 2) ||
+        (current_size == 1 && watch_mask == 1))
----------------
labath wrote:
> The logic here is extremely convoluted. Doesn't this code basically boil down to:
> ```
> current_size = m_hwp_regs[wp_index].control & 1 ? GetWatchpointSize(wp_index) : 0;
> new_size = llvm::NextPowerOf2(std::max(current_size, watch_mask));
> // update the control value, write the debug registers...
> ```
> 
> Also `watch_mask` should probably be renamed to `watch_size`, as it doesn't appear to be a mask.
Seems legit. I ll update this in next patch.

================
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp:602
@@ -612,3 +601,3 @@
 
 bool NativeRegisterContextLinux_arm::ClearHardwareWatchpoint(
     uint32_t wp_index) {
----------------
labath wrote:
> This looks a bit worrying.
> What will happen after the following sequence of events:
> - client tells us to set a watchpoint at 0x1000
> - we set the watchpoint
> - client tells us to set a watchpoint at 0x1001
> - we extend the previous watchpoint to watch this address as well
> - client tells us to delete the watchpoint at 0x1000
> - ???
> 
> Will we remain watching the address 0x1001? I don't see how will you be able to do that without maintaining a some info about the original watchpoints the client requested (and I have not seen that code). Please add a test for this.
I just realized that I missed a crucial change in my last patch that we need to make all this work.

Let me get back with correction and desired test cases.


https://reviews.llvm.org/D24610