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