[LLDB] ASanLibsanitizers Use sanitizers_address_on_report breakpoint

[usama54321]

symbol

This patch puts the default breakpoint on the
sanitizers_address_on_report symbol, and uses the old symbol as a backup if the default case is not found

rdar://123911522

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Usama Hameed (usama54321)

Changes

symbol

This patch puts the default breakpoint on the
sanitizers_address_on_report symbol, and uses the old symbol as a backup if the default case is not found

rdar://123911522

---

Full diff: https://github.com/llvm/llvm-project/pull/84583.diff

1 Files Affected:

• (modified) lldb/source/Plugins/InstrumentationRuntime/ASanLibsanitizers/InstrumentationRuntimeASanLibsanitizers.cpp (+10-2)

diff --git a/lldb/source/Plugins/InstrumentationRuntime/ASanLibsanitizers/InstrumentationRuntimeASanLibsanitizers.cpp b/lldb/source/Plugins/InstrumentationRuntime/ASanLibsanitizers/InstrumentationRuntimeASanLibsanitizers.cpp
index d84cd36d7ce17b..e095d3e6b2e339 100644
--- a/lldb/source/Plugins/InstrumentationRuntime/ASanLibsanitizers/InstrumentationRuntimeASanLibsanitizers.cpp
+++ b/lldb/source/Plugins/InstrumentationRuntime/ASanLibsanitizers/InstrumentationRuntimeASanLibsanitizers.cpp
@@ -90,9 +90,17 @@ void InstrumentationRuntimeASanLibsanitizers::Activate() {
   if (!process_sp)
     return;
 
+  lldb::ModuleSP module_sp = GetRuntimeModuleSP();
+
   Breakpoint *breakpoint = ReportRetriever::SetupBreakpoint(
-      GetRuntimeModuleSP(), process_sp,
-      ConstString("_Z22raise_sanitizers_error23sanitizer_error_context"));
+      module_sp, process_sp,
+      ConstString("_sanitizers_address_on_report"));
+
+  if (!breakpoint) {
+    breakpoint = ReportRetriever::SetupBreakpoint(
+        module_sp, process_sp,
+        ConstString("_Z22raise_sanitizers_error23sanitizer_error_context"));
+  }
 
   if (!breakpoint)
     return;

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[yln]

LGTM, thanks!

[jimingham]

ReportRetriever::SetupBreakpoint is a weird way to set a breakpoint. This is somewhat off topic, but do you know why it was done this way? You could set it by symbol & module and then you'd have something that survives rebuilds of the module, or 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 - and uses that...

This came up because you could (if this weren't done so oddly) handle this by having a list of candidate functions, and make a single breakpoint with all those names...

[yln]

@jimingham, thanks for the review and feedback on how to improve the code. Are you okay with us addressing this in a follow-up?

do you know why it was done this way?

I don't think we do. I think we just copied what was done in the old ASan plugin. My guess for the original code is that there was no specific reason other than "it worked". I definitely want to follow your guidance and improve this if it has benefits (and it seems easy to do).

survives rebuilds

I think we are fine for rebuilds. The module we are setting the breakpoint in is the ASan runtime, which doesn't get rebuilt.

This came up because you could (if this weren't done so oddly) handle this by having a list of candidate functions, and make a single breakpoint with all those names...

@usama54321 please add a FIXME/TODO in the code and file a radar to improve this (you can assign it to me)

[jimingham]

I have no problem with doing this in stages, what needs doing right now and cleanups to follow.

[jimingham]

@jimingham, thanks for the review and feedback on how to improve the code. Are you okay with us addressing this in a follow-up?

do you know why it was done this way?

I don't think we do. I think we just copied what was done in the old ASan plugin. My guess for the original code is that there was no specific reason other than "it worked". I definitely want to follow your guidance and improve this if it has benefits (and it seems easy to do).

survives rebuilds

I think we are fine for rebuilds. The module we are setting the breakpoint in is the ASan runtime, which doesn't get rebuilt.

Unless you happen to be working on the asan runtime...

This came up because you could (if this weren't done so oddly) handle this by having a list of candidate functions, and make a single breakpoint with all those names...

@usama54321 please add a FIXME/TODO in the code and file a radar to improve this (you can assign it to me)

[jimingham]

LGTM