JDevlieghere added inline comments.
================
Comment at:
lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp:833-835
+std::vector<DNBArchMachARM64::watchpoint_spec>
+DNBArchMachARM64::AlignRequestedWatchpoint(nub_addr_t user_addr,
+ nub_size_t user_size) {
----------------
Do you ever expect this to return more than two watchpoints? Seems like this
could be a `struct` that holds two optional `WatchpointSpec`s. I don't feel
strongly about it, but it looks like you never iterate over the list and the
way you have to check after the recursive call is a bit awkward.
edit: nvm, it looks like you do actually iterate over them in
`EnableHardwareWatchpoint`
================
Comment at: lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp:841
+ if (user_size == 0)
+ return wps;
+
----------------
I would get rid of `wps` and return an empty list here (`return {}`). It's
pretty obvious here, but on line 893 I was forced to go back and make sure
nothing had been added to the list in the meantime.
================
Comment at: lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp:844
+ // Smallest size we can watch on AArch64 is 8 bytes
+ const nub_size_t min_watchpoint_alignment = 8;
+ nub_size_t aligned_size = std::max(user_size, min_watchpoint_alignment);
----------------
Could be `constexpr`. Same for `addr_byte_size` and `addr_bit_size`
================
Comment at: lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp:851
+
+ /// Round up \a user_size to the nearest power-of-2 size, at least 8 bytes
+ // user_size == 3 -> aligned_size == 8
----------------
`s/nearest/next/`
================
Comment at: lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp:856
+ // user_size == 16 -> aligned_size == 16
+ aligned_size = 1ULL << (addr_bit_size - __builtin_clzll(aligned_size - 1));
+
----------------
Beautiful. Once we have C++20 we can use `std::bit_ceil`
(https://en.cppreference.com/w/cpp/numeric/bit_ceil)
================
Comment at: lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp:897
+ wps.push_back(second_wp[0]);
+ return wps;
+}
----------------
If you get rid of `wps` you can use `return {{first_wp[0], second_wp[0]}}`
instead
================
Comment at: lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp:923
- // Otherwise, can't watch more than 8 bytes per WVR/WCR pair
- if (size > 8)
- return INVALID_NUB_HW_INDEX;
-
- // Aarch64 watchpoints are in one of two forms: (1) 1-8 bytes, aligned to
- // an 8 byte address, or (2) a power-of-two size region of memory; minimum
- // 8 bytes, maximum 2GB; the starting address must be aligned to that power
- // of two.
- //
- // For (1), 1-8 byte watchpoints, using the Byte Address Selector field in
- // DBGWCR<n>.BAS. Any of the bytes may be watched, but if multiple bytes
- // are watched, the bytes selected must be contiguous. The start address
- // watched must be doubleword (8-byte) aligned; if the start address is
- // word (4-byte) aligned, only 4 bytes can be watched.
- //
- // For (2), the MASK field in DBGWCR<n>.MASK is used.
- //
- // See the ARM ARM, section "Watchpoint exceptions", and more specifically,
- // "Watchpoint data address comparisons".
- //
- // debugserver today only supports (1) - the Byte Address Selector 1-8 byte
- // watchpoints that are 8-byte aligned. To support larger watchpoints,
- // debugserver would need to interpret the mach exception when the watched
- // region was hit, see if the address accessed lies within the subset
- // of the power-of-two region that lldb asked us to watch (v. ARM ARM,
- // "Determining the memory location that caused a Watchpoint exception"),
- // and silently resume the inferior (disable watchpoint, stepi, re-enable
- // watchpoint) if the address lies outside the region that lldb asked us
- // to watch.
- //
- // Alternatively, lldb would need to be prepared for a larger region
- // being watched than it requested, and silently resume the inferior if
- // the accessed address is outside the region lldb wants to watch.
-
- nub_addr_t aligned_wp_address = addr & ~0x7;
- uint32_t addr_dword_offset = addr & 0x7;
-
- // Do we need to split up this logical watchpoint into two hardware
watchpoint
- // registers?
- // e.g. a watchpoint of length 4 on address 6. We need do this with
- // one watchpoint on address 0 with bytes 6 & 7 being monitored
- // one watchpoint on address 8 with bytes 0, 1, 2, 3 being monitored
-
- if (addr_dword_offset + size > 8) {
- DNBLogThreadedIf(LOG_WATCHPOINTS, "DNBArchMachARM64::"
- "EnableHardwareWatchpoint(addr = "
- "0x%8.8llx, size = %zu) needs two "
- "hardware watchpoints slots to monitor",
- (uint64_t)addr, size);
- int low_watchpoint_size = 8 - addr_dword_offset;
- int high_watchpoint_size = addr_dword_offset + size - 8;
-
- uint32_t lo = EnableHardwareWatchpoint(addr, low_watchpoint_size, read,
- write, also_set_on_task);
- if (lo == INVALID_NUB_HW_INDEX)
- return INVALID_NUB_HW_INDEX;
- uint32_t hi =
- EnableHardwareWatchpoint(aligned_wp_address + 8, high_watchpoint_size,
- read, write, also_set_on_task);
- if (hi == INVALID_NUB_HW_INDEX) {
- DisableHardwareWatchpoint(lo, also_set_on_task);
+ if (wps.size() > 1) {
+ std::vector<uint32_t> wp_slots_used;
----------------
Why is this `> 1` and not `>= 1` (i.e. no 0 which we checked on line 916). TBH
I don't really understand what this function is doing.
================
Comment at: lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.h:39
+ struct watchpoint_spec {
+ nub_addr_t aligned_start;
----------------
All of debugserver's structs and classes use CamelCase.
================
Comment at: lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.h:82-83
bool also_set_on_task) override;
+ std::vector<watchpoint_spec> AlignRequestedWatchpoint(nub_addr_t user_addr,
+ nub_size_t user_size);
uint32_t EnableHardwareWatchpoint(nub_addr_t addr, nub_size_t size, bool
read,
----------------
Nit: I know what `user_` means in the context of this patch, but I'd go with
either `requested_addr` and `requested_size` or even just `addr` and `size`.
Another option would be `user_specified_addr` but that seems pretty verbose,
although it would be consistent with the members of the struct.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D149040/new/
https://reviews.llvm.org/D149040
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
