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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
