[Lldb-commits] [PATCH] D149040: Refactor and generalize debugserver code for setting hardware watchpoints on AArch64

[Jonas Devlieghere via Phabricator via lldb-commits]

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