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