[lldb] Use Address to setup breakpoint
#94794
Conversation
|
@llvm/pr-subscribers-lldb Author: Julian Lettner (yln) ChangesUse rdar://124399066 Full diff: https://github.com/llvm/llvm-project/pull/94794.diff 2 Files Affected:
diff --git a/lldb/source/Plugins/InstrumentationRuntime/ASanLibsanitizers/InstrumentationRuntimeASanLibsanitizers.cpp b/lldb/source/Plugins/InstrumentationRuntime/ASanLibsanitizers/InstrumentationRuntimeASanLibsanitizers.cpp
index cd91f4a6ff1bc..b1151febb7cc4 100644
--- a/lldb/source/Plugins/InstrumentationRuntime/ASanLibsanitizers/InstrumentationRuntimeASanLibsanitizers.cpp
+++ b/lldb/source/Plugins/InstrumentationRuntime/ASanLibsanitizers/InstrumentationRuntimeASanLibsanitizers.cpp
@@ -90,17 +90,9 @@ void InstrumentationRuntimeASanLibsanitizers::Activate() {
if (!process_sp)
return;
- lldb::ModuleSP module_sp = GetRuntimeModuleSP();
-
Breakpoint *breakpoint = ReportRetriever::SetupBreakpoint(
- module_sp, process_sp, ConstString("sanitizers_address_on_report"));
-
- if (!breakpoint) {
- breakpoint = ReportRetriever::SetupBreakpoint(
- module_sp, process_sp,
- ConstString("_Z22raise_sanitizers_error23sanitizer_error_context"));
- }
-
+ GetRuntimeModuleSP(), process_sp,
+ ConstString("sanitizers_address_on_report"));
if (!breakpoint)
return;
diff --git a/lldb/source/Plugins/InstrumentationRuntime/Utility/ReportRetriever.cpp b/lldb/source/Plugins/InstrumentationRuntime/Utility/ReportRetriever.cpp
index 298b63bc716fc..4e382005f8c2b 100644
--- a/lldb/source/Plugins/InstrumentationRuntime/Utility/ReportRetriever.cpp
+++ b/lldb/source/Plugins/InstrumentationRuntime/Utility/ReportRetriever.cpp
@@ -219,7 +219,6 @@ bool ReportRetriever::NotifyBreakpointHit(ProcessSP process_sp,
return true; // Return true to stop the target
}
-// FIXME: Setup the breakpoint using a less fragile SPI. rdar://124399066
Breakpoint *ReportRetriever::SetupBreakpoint(ModuleSP module_sp,
ProcessSP process_sp,
ConstString symbol_name) {
@@ -235,18 +234,13 @@ Breakpoint *ReportRetriever::SetupBreakpoint(ModuleSP module_sp,
if (!symbol->ValueIsAddress() || !symbol->GetAddressRef().IsValid())
return nullptr;
- Target &target = process_sp->GetTarget();
- addr_t symbol_address = symbol->GetAddressRef().GetOpcodeLoadAddress(&target);
-
- if (symbol_address == LLDB_INVALID_ADDRESS)
- return nullptr;
-
+ const Address &address = symbol->GetAddressRef();
const bool internal = true;
const bool hardware = false;
Breakpoint *breakpoint =
process_sp->GetTarget()
- .CreateBreakpoint(symbol_address, internal, hardware)
+ .CreateBreakpoint(address, internal, hardware)
.get();
return breakpoint;
|
| const bool internal = true; | ||
| const bool hardware = false; | ||
|
|
||
| Breakpoint *breakpoint = | ||
| process_sp->GetTarget() | ||
| .CreateBreakpoint(symbol_address, internal, hardware) | ||
| .CreateBreakpoint(address, internal, hardware) |
There was a problem hiding this comment.
Cleanup suggested by @jimingham here: #84583 (comment)
you could set it by Address, in which case it would survive re-running, but instead it converts the Address to an addr_t - the most fragile way to set a breakpoint
So we are using the CreateBreakpoint overload that takes Address instead of addr_t now and Jim mentioned that this survives re-running. Are we concerned at all about this change in behavior? For example, could we get into a situation where we keep adding superfluous breakpoints because the old ones survive?
@jimingham @usama54321
| if (!breakpoint) { | ||
| breakpoint = ReportRetriever::SetupBreakpoint( | ||
| module_sp, process_sp, | ||
| ConstString("_Z22raise_sanitizers_error23sanitizer_error_context")); |
There was a problem hiding this comment.
@usama54321 it's now safe to ignore the legacy symbol, right?
Use `Address` (instead of `addr_t`) to setup breakpoint in `ReportRetriever::SetupBreakpoint`. This is cleaner and the breakpoint should now survive re-running of the binary. rdar://124399066
yln
force-pushed
the
users/yln/lldb-ReportRetriever_SetupBreakpoint-cleanup
branch
from
dd02ca5 to
484f913
Compare
|
I'm assuming that Can this be tested? Not sure what coverage we have of these plugins right now. |
| GetRuntimeModuleSP(), process_sp, | ||
| ConstString("sanitizers_address_on_report")); |
There was a problem hiding this comment.
This doesn't look right. Is this left over from the older code? Remove this?
There was a problem hiding this comment.
Hi @clayborg, I don't follow. Can you elaborate?
There was a problem hiding this comment.
@clayborg The old code had a fallback to _Z22raise_sanitizers_error23sanitizer_error_context but this breakpoint is still the right one.
| GetRuntimeModuleSP(), process_sp, | ||
| ConstString("sanitizers_address_on_report")); |
There was a problem hiding this comment.
@clayborg The old code had a fallback to _Z22raise_sanitizers_error23sanitizer_error_context but this breakpoint is still the right one.
There was a problem hiding this comment.
LGTM. It was silly to remove the section information from the Address only to have the Breakpoint have to find it again.
The only potentially concerning change here is the removal of a check for when you go to set the breakpoint and the Address you got for the Symbol was not yet loaded into the process you are debugging (at removed lines 241-2).
You could easily put that check back in but that was not necessary in the original code, since you only setup the breakpoint after you see the RuntimeModule get loaded; and it's doubly unnecessary now since you switched to setting the breakpoint with an Address gotten from a module, so it will track the correct load address even if set before the load.
Use
Address(instead ofaddr_t) to setupbreakpoint in
ReportRetriever::SetupBreakpoint.This is cleaner and the breakpoint should now
survive re-running of the binary.
rdar://124399066