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

[Pavel Labath via lldb-commits]

labath requested changes to this revision.
labath added a comment.
This revision now requires changes to proceed.

I have some doubts about the validity of this patch. We should make sure those are cleared before putting this in.


================
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)
----------------
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).

================
Comment at: packages/Python/lldbsuite/test/functionalities/watchpoint/watchpoint_size/TestWatchpointSizes.py:154
@@ +153,3 @@
+            # Continue after hitting watchpoint twice (Read/Write)
+            if slots != 0:
+                self.runCmd("process continue")
----------------
It looks like this test will test something completely different on arm than on other architectures. You would probably be better off writing a new test for this.

================
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))
----------------
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.

================
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp:602
@@ -612,3 +601,3 @@
 
 bool NativeRegisterContextLinux_arm::ClearHardwareWatchpoint(
     uint32_t wp_index) {
----------------
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.


https://reviews.llvm.org/D24610