[Lldb-commits] [PATCH] D24610: LLDB Arm Watchpoints: Use single hardware watchpoint slot to watch multiple bytes where possible
Pavel Labath via lldb-commits
lldb-commits at lists.llvm.org
Sun Sep 18 13:14:39 PDT 2016
labath added inline comments.
Comment at: packages/Python/lldbsuite/test/functionalities/watchpoint/multi_watchpoint_slots/main.c:23
@@ +22,3 @@
+ printf("About to write byteArray[%d] ...\n", i); // About to write byteArray
> What's up with all the double spaced source code? Is this intentional?
Indeed. The spaces seem superfluous.
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp:513-521
@@ -513,1 +512,11 @@
+ // Find out how many bytes we need to watch after 4-byte alignment boundary.
+ uint8_t watch_size = (addr & 0x03) + size;
+ // We cannot watch zero or more than 4 bytes after 4-byte alignment boundary.
+ if (size == 0 || watch_size > 4)
+ return LLDB_INVALID_INDEX32;
+ // Strip away last two bits of address for byte/half-word/word selection.
+ addr &= ~((lldb::addr_t)3);
> This block of code is a bit confusing to me. Is this equivalent to:
> lldb::addr_t start = llvm::alignDown(addr, 4);
> lldb::addr_t end = addr + size;
> if (start == end || (end-start)>4)
> return LLDB_INVALID_INDEX32;
I am not sure this is much clearer, especially, as we will later need a separate varaible for `end-start` anyway.
+1 for `llvm::alignDown` though.
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp:552
@@ -559,1 +551,3 @@
+ uint8_t current_watch_size, new_watch_size;
+ // Calculate overall size width to be watched by current hardware watchpoint slot.
Looks much better. Any reason for not using `NextPowerOf2` ? Among other things, it is self-documenting, so you do not need the comment above that.
Comment at: source/Plugins/Process/Linux/NativeThreadLinux.cpp:202
@@ +201,3 @@
+ // Invalidate watchpoint index map to re-sync watchpoint registers.
If you add this, then the comment below becomes obsolete.
Seems like a pretty elegant solution to the incremental watchpoint update problem. I am wondering whether we need to do it on every resume though. I think it should be enough to do it when a watchpoint gets deleted (`NTL::RemoveWatchpoint`). Also, we should throw out the implementation of `NativeRegisterContextLinux_arm::ClearHardwareWatchpoint` -- it's no longer necessary, and it's not even correct anymore.
More information about the lldb-commits