-
Notifications
You must be signed in to change notification settings - Fork 15k
[lldb][Instrumentation] Set selected frame to outside sanitizer libraries #133079
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) ChangesWhen hitting a sanitizer breakpoint, LLDB currently displays the frame in the sanitizer dylib (which we usually don't have debug-info for), which isn't very helpful to the user. A more helpful frame to display would be the first frame not in the sanitizer library (we have a similar heuristic when we trap inside libc++). This patch does just that, by implementing the Depends on #133078 Full diff: https://github.com/llvm/llvm-project/pull/133079.diff 8 Files Affected:
diff --git a/lldb/include/lldb/Target/InstrumentationRuntimeStopInfo.h b/lldb/include/lldb/Target/InstrumentationRuntimeStopInfo.h
index 5345160850914..dafa41c11327a 100644
--- a/lldb/include/lldb/Target/InstrumentationRuntimeStopInfo.h
+++ b/lldb/include/lldb/Target/InstrumentationRuntimeStopInfo.h
@@ -24,6 +24,9 @@ class InstrumentationRuntimeStopInfo : public StopInfo {
return lldb::eStopReasonInstrumentation;
}
+ std::optional<uint32_t>
+ GetSuggestedStackFrameIndex(bool inlined_stack) override;
+
const char *GetDescription() override;
bool DoShouldNotify(Event *event_ptr) override { return true; }
diff --git a/lldb/include/lldb/Target/StackFrameList.h b/lldb/include/lldb/Target/StackFrameList.h
index 8a66296346f2d..be6ec3b09d8aa 100644
--- a/lldb/include/lldb/Target/StackFrameList.h
+++ b/lldb/include/lldb/Target/StackFrameList.h
@@ -36,6 +36,8 @@ class StackFrameList {
/// Get the frame at index \p idx. Invisible frames cannot be indexed.
lldb::StackFrameSP GetFrameAtIndex(uint32_t idx);
+ void ResetSuggestedStackFrameIndex() { m_selected_frame_idx.reset(); }
+
/// Get the first concrete frame with index greater than or equal to \p idx.
/// Unlike \ref GetFrameAtIndex, this cannot return a synthetic frame.
lldb::StackFrameSP GetFrameWithConcreteFrameIndex(uint32_t unwind_idx);
diff --git a/lldb/include/lldb/Target/Thread.h b/lldb/include/lldb/Target/Thread.h
index 1d1e3dcfc1dc6..747d7299025f8 100644
--- a/lldb/include/lldb/Target/Thread.h
+++ b/lldb/include/lldb/Target/Thread.h
@@ -433,6 +433,10 @@ class Thread : public std::enable_shared_from_this<Thread>,
return GetStackFrameList()->GetFrameAtIndex(idx);
}
+ virtual void ResetSuggestedStackFrameIndex() {
+ return GetStackFrameList()->ResetSuggestedStackFrameIndex();
+ }
+
virtual lldb::StackFrameSP
GetFrameWithConcreteFrameIndex(uint32_t unwind_idx);
diff --git a/lldb/source/Target/InstrumentationRuntimeStopInfo.cpp b/lldb/source/Target/InstrumentationRuntimeStopInfo.cpp
index 7f82581cc601e..1daeebdbaf9c7 100644
--- a/lldb/source/Target/InstrumentationRuntimeStopInfo.cpp
+++ b/lldb/source/Target/InstrumentationRuntimeStopInfo.cpp
@@ -8,13 +8,20 @@
#include "lldb/Target/InstrumentationRuntimeStopInfo.h"
+#include "lldb/Core/Module.h"
#include "lldb/Target/InstrumentationRuntime.h"
#include "lldb/Target/Process.h"
+#include "lldb/lldb-enumerations.h"
#include "lldb/lldb-private.h"
using namespace lldb;
using namespace lldb_private;
+static bool IsStoppedInDarwinSanitizer(Thread &thread, Module &module) {
+ return module.GetFileSpec().GetFilename().GetStringRef().starts_with(
+ "libclang_rt.");
+}
+
InstrumentationRuntimeStopInfo::InstrumentationRuntimeStopInfo(
Thread &thread, std::string description,
StructuredData::ObjectSP additional_data)
@@ -34,3 +41,38 @@ InstrumentationRuntimeStopInfo::CreateStopReasonWithInstrumentationData(
return StopInfoSP(
new InstrumentationRuntimeStopInfo(thread, description, additionalData));
}
+
+std::optional<uint32_t>
+InstrumentationRuntimeStopInfo::GetSuggestedStackFrameIndex(
+ bool inlined_stack) {
+ auto thread_sp = GetThread();
+ if (!thread_sp)
+ return std::nullopt;
+
+ // Defensive upper-bound of when we stop walking up the frames in
+ // case we somehow ended up looking at an infinite recursion.
+ const size_t max_stack_depth = 128;
+
+ // Start at parent frame.
+ size_t stack_idx = 1;
+ StackFrameSP most_relevant_frame_sp =
+ thread_sp->GetStackFrameAtIndex(stack_idx);
+
+ while (most_relevant_frame_sp && stack_idx <= max_stack_depth) {
+ auto const &sc =
+ most_relevant_frame_sp->GetSymbolContext(lldb::eSymbolContextModule);
+
+ if (!sc.module_sp)
+ return std::nullopt;
+
+ // Found a frame outside of the sanitizer runtime libraries.
+ // That's the one we want to display.
+ if (!IsStoppedInDarwinSanitizer(*thread_sp, *sc.module_sp))
+ return stack_idx;
+
+ ++stack_idx;
+ most_relevant_frame_sp = thread_sp->GetStackFrameAtIndex(stack_idx);
+ }
+
+ return stack_idx;
+}
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index f2f5598f0ab53..f82cea2d668ed 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -4257,6 +4257,8 @@ bool Process::ProcessEventData::ShouldStop(Event *event_ptr,
// appropriately. We also need to stop processing actions, since they
// aren't expecting the target to be running.
+ thread_sp->ResetSuggestedStackFrameIndex();
+
// FIXME: we might have run.
if (stop_info_sp->HasTargetRunSinceMe()) {
SetRestarted(true);
diff --git a/lldb/test/API/functionalities/asan/TestMemoryHistory.py b/lldb/test/API/functionalities/asan/TestMemoryHistory.py
index b04182a543719..03003f1e4112a 100644
--- a/lldb/test/API/functionalities/asan/TestMemoryHistory.py
+++ b/lldb/test/API/functionalities/asan/TestMemoryHistory.py
@@ -52,6 +52,10 @@ def libsanitizer_tests(self):
substrs=["stopped", "stop reason = Use of deallocated memory"],
)
+ # Make sure we're not stopped in the sanitizer library but instead at the
+ # point of failure in the user-code.
+ self.assertEqual(self.frame().GetFunctionName(), "main")
+
# test the 'memory history' command
self.expect(
"memory history 'pointer'",
diff --git a/lldb/test/API/functionalities/asan/TestReportData.py b/lldb/test/API/functionalities/asan/TestReportData.py
index fabc985d0ed44..496a2db67c038 100644
--- a/lldb/test/API/functionalities/asan/TestReportData.py
+++ b/lldb/test/API/functionalities/asan/TestReportData.py
@@ -69,6 +69,10 @@ def asan_tests(self, libsanitizers=False):
lldb.eStopReasonInstrumentation,
)
+ # Make sure we're not stopped in the sanitizer library but instead at the
+ # point of failure in the user-code.
+ self.assertEqual(self.frame().GetFunctionName(), "main")
+
self.expect(
"bt",
"The backtrace should show the crashing line",
diff --git a/lldb/test/API/functionalities/ubsan/basic/TestUbsanBasic.py b/lldb/test/API/functionalities/ubsan/basic/TestUbsanBasic.py
index 868a2864d2b5e..f46d167d910ea 100644
--- a/lldb/test/API/functionalities/ubsan/basic/TestUbsanBasic.py
+++ b/lldb/test/API/functionalities/ubsan/basic/TestUbsanBasic.py
@@ -52,8 +52,8 @@ def ubsan_tests(self):
substrs=["1 match found"],
)
- # We should be stopped in __ubsan_on_report
- self.assertIn("__ubsan_on_report", frame.GetFunctionName())
+ # We should not be stopped in the sanitizer library.
+ self.assertIn("main", frame.GetFunctionName())
# The stopped thread backtrace should contain either 'align line'
found = False
|
befd014
to
85f9488
Compare
…tion (#133078) The motivation for this patch is that `StopInfo::GetSuggestedStackFrameIndex` would not take effect for `InstrumentationRuntimeStopInfo` (which we plan to implement in #133079). This was happening because the instrumentation runtime plugins would run utility expressions as part of the stop that would set the `m_selected_frame_idx`. This means `SelectMostRelevantFrame` was never called, and we would not be able to report the selected frame via the `StopInfo` object. This patch makes sure we clear the `m_selected_frame_idx` to allow `GetSuggestedStackFrameIndex` to take effect, regardless of what the frame recognizers choose to do.
…::PerformAction (#133078) The motivation for this patch is that `StopInfo::GetSuggestedStackFrameIndex` would not take effect for `InstrumentationRuntimeStopInfo` (which we plan to implement in llvm/llvm-project#133079). This was happening because the instrumentation runtime plugins would run utility expressions as part of the stop that would set the `m_selected_frame_idx`. This means `SelectMostRelevantFrame` was never called, and we would not be able to report the selected frame via the `StopInfo` object. This patch makes sure we clear the `m_selected_frame_idx` to allow `GetSuggestedStackFrameIndex` to take effect, regardless of what the frame recognizers choose to do.
85f9488
to
8f3f629
Compare
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/181/builds/27444 Here is the relevant piece of the build log for the reference
|
Since #133079 we no longer stop in the stanitizer library when hitting a sanitizer breakpoint. Adjust the test accordingly.
Since llvm/llvm-project#133079 we no longer stop in the stanitizer library when hitting a sanitizer breakpoint. Adjust the test accordingly.
@@ -218,6 +222,10 @@ def compiler_rt_asan_tests(self): | |||
|
|||
self.check_traces() | |||
|
|||
# Make sure we're not stopped in the sanitizer library but instead at the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Michael137 Is this correct? This test compiler_rt_asan_tests
runs on all platforms, but your heuristics in IsStoppedInDarwinSanitizer
is for Darwin only?
In most CI this test is somehow disabled (maybe this is an issue as well?), but in my (totally not related) PR #157529 it keeps failing because the frame is __sanitizer::Die()
not main
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yea that should only apply on Darwin. Will correct that
Would be nice if we made it work on non-Darwin too but that's for later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing, the two tests in question are:
lldb-api :: functionalities/asan/TestMemoryHistory.py
lldb-api :: functionalities/asan/TestReportData.py
Randomly grabbed some jobs, seems like other PRs (e.g. #157534, cc @Michael137) are also affected by these two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fixed now. Let me know if you're still seeing issues
…forms The frame recognizer for the instrumentation runtimes (added in #133079) only triggers on Darwin for `libclang_rt`. Adjust the tests accordingly.
…Darwin platforms The frame recognizer for the instrumentation runtimes (added in llvm/llvm-project#133079) only triggers on Darwin for `libclang_rt`. Adjust the tests accordingly.
We started to seeing test failure (TestMemoryHistory.py) after this change:
Update: |
/pull-request #157568 |
When hitting a sanitizer breakpoint, LLDB currently displays the frame in the sanitizer dylib (which we usually don't have debug-info for), which isn't very helpful to the user. A more helpful frame to display would be the first frame not in the sanitizer library (we have a similar heuristic when we trap inside libc++). This patch does just that, by implementing the
GetSuggestedStackFrameIndex
APIDepends on #133078