[Lldb-commits] [PATCH] D29669: Hardware breakpoints implementation for AArch64 targets

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Don't be intimidated by the number of comments :) - I think the change looks reasonable as a whole - most of them are just about style and come from the fact you were probably working on the patch while I was refactoring this code. As for tests, what I'd definitely want to see is a lldb-server-style test, which tests just the server changes without the client part (maybe you we're already talking about those - I just want to make sure we're on the same page).

As for the stepping speed, do you have any data which shows that this is actually a bottleneck in those operations? (I'm not against doing that, I'm just curious).



================
Comment at: include/lldb/Host/common/HardwareBreakpointList.h:24
+
+class HardwareBreakpointList {
+public:
----------------
What's the benefit of this class over using `std::map<lldb::addr_t, HardwareBreakpoint>` directly?


================
Comment at: source/Host/common/NativeProcessProtocol.cpp:142
 
-uint32_t NativeProcessProtocol::GetMaxWatchpoints() const {
+bool NativeProcessProtocol::GetHardwareDebugSupportInfo(
+    uint32_t &breakpoints, uint32_t &watchpoints) const {
----------------
`llvm::Optional<std::pair<uint32_t, uint32_t>> GetHardwareDebugSupportInfo()`


================
Comment at: source/Host/common/NativeProcessProtocol.cpp:145
   // This default implementation will return the number of
   // *hardware* breakpoints available.  MacOSX and other OS
   // implementations that support software breakpoints will want to
----------------
This comment needs update


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:875
+    error = thread.GetRegisterContext()->GetHardwareBreakHitIndex(
+        wp_index, (uintptr_t)info.si_addr);
+    if (error.Fail())
----------------
Please rename the wp_index variable to something more generic if you're going to use it here.


================
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:367
+
+  if (log)
+    log->Printf("NativeRegisterContextLinux_arm64::%s()", __FUNCTION__);
----------------
Please use LLDB_LOG here.


================
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:384
                                                         size_t size) {
-  Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(POSIX_LOG_WATCHPOINTS));
+  Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(LIBLLDB_LOG_BREAKPOINTS));
   LLDB_LOG(log, "addr: {0:x}, size: {1:x}", addr, size);
----------------
Please use POSIX_LOG_BREAKPOINTS here (the code in the NativeProcessLinux plugin was using a combination of posix and lldb log channels, so i've standardized it on posix).


================
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:414
+    } else if (m_hbr_regs[i].address == addr) {
+      return LLDB_INVALID_INDEX32; // We do not support duplicate watchpoints.
     }
----------------
s/watchpoints/breakpoints/ ?


================
Comment at: source/Plugins/Process/Linux/NativeThreadLinux.cpp:246
 
+  // Set all active hardware breakpoint on all threads.
+  if (m_hw_break_index_map.empty()) {
----------------
This doesn't appear to be setting the breakpoint on *all* threads (and it probably shouldnt)


https://reviews.llvm.org/D29669