jingham created this revision.
jingham added reviewers: JDevlieghere, mib.
Herald added a project: All.
jingham requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
SelectMostRelevantFrame triggers the StackFrameRecognizer construction, which
can run arbitrary Python code, call expressions etc. WillStop gets called on
every private stop while the recognizers are a user-facing feature, so first
off doing this work on every stop is inefficient. But more importantly, you
can get in to situations where the recognizer causes an expression to get run,
then when we fetch the stop event at the end of the expression evaluation, we
call WillStop again on the expression handling thread, which will do the same
StackFrameRecognizer work again. If anyone is locking along that path, you
will end up with a deadlock between the two threads.
The example that brought this to my attention was the objc_exception_throw
recognizer which can cause the objc runtime introspection functions to get run,
and those take a lock in
lldb_private::AppleObjCRuntimeV2::DynamicClassInfoExtractor::UpdateISAToDescriptorMap
along this path, so the second thread servicing the expression deadlocks
against the first thread waiting for the expression to complete.
It makes more sense to have the frame recognizers run on demand, either when
someone asks for the variables for the frame, or when someone does
GetSelectedFrame. The former already worked that way, the only reason this was
being done in WillStop was because the StackFrameRecognizers can change the
SelectedFrame, so you needed to run them before the anyone requested the
SelectedFrame.
This patch moves SelectMostRelevantFrame to StackFrameList, and runs it when
GetSelectedFrame is called for the first time on a given stop. If you call
SetSelectedFrame before GetSelectedFrame, then you should NOT run the
recognizer & change the frame out from under you. This patch also makes that
work. There were already tests for this behavior, and for the feature that
caused the hang, but the hang is racy, and it doesn't trigger all the time, so
I don't have a way to test that explicitly.
One more detail: it's actually pretty easy to end up calling GetSelectedFrame,
for instance if you ask for the best ExecutionContext from an
ExecutionContextRef it will fill the StackFrame with the result of
GetSelectedFrame and that would still have the same problems if this happens on
the Private State Thread. So this patch also short-circuits
SelectMostRelevantFrame if run on the that thread. I can't think of any reason
the computations that go on on the Private State Thread would actually want the
SelectedFrame - that's a user-facing concept, so avoiding that complication is
the best way to go.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D147753
Files:
lldb/include/lldb/Target/StackFrameList.h
lldb/include/lldb/Target/Thread.h
lldb/source/Target/StackFrameList.cpp
lldb/source/Target/Thread.cpp
Index: lldb/source/Target/Thread.cpp
===================================================================
--- lldb/source/Target/Thread.cpp
+++ lldb/source/Target/Thread.cpp
@@ -588,36 +588,9 @@
return raw_stop_description;
}
-void Thread::SelectMostRelevantFrame() {
- Log *log = GetLog(LLDBLog::Thread);
-
- auto frames_list_sp = GetStackFrameList();
-
- // Only the top frame should be recognized.
- auto frame_sp = frames_list_sp->GetFrameAtIndex(0);
-
- auto recognized_frame_sp = frame_sp->GetRecognizedFrame();
-
- if (!recognized_frame_sp) {
- LLDB_LOG(log, "Frame #0 not recognized");
- return;
- }
-
- if (StackFrameSP most_relevant_frame_sp =
- recognized_frame_sp->GetMostRelevantFrame()) {
- LLDB_LOG(log, "Found most relevant frame at index {0}",
- most_relevant_frame_sp->GetFrameIndex());
- SetSelectedFrame(most_relevant_frame_sp.get());
- } else {
- LLDB_LOG(log, "No relevant frame!");
- }
-}
-
void Thread::WillStop() {
ThreadPlan *current_plan = GetCurrentPlan();
- SelectMostRelevantFrame();
-
// FIXME: I may decide to disallow threads with no plans. In which
// case this should go to an assert.
Index: lldb/source/Target/StackFrameList.cpp
===================================================================
--- lldb/source/Target/StackFrameList.cpp
+++ lldb/source/Target/StackFrameList.cpp
@@ -18,6 +18,7 @@
#include "lldb/Target/Process.h"
#include "lldb/Target/RegisterContext.h"
#include "lldb/Target/StackFrame.h"
+#include "lldb/Target/StackFrameRecognizer.h"
#include "lldb/Target/StopInfo.h"
#include "lldb/Target/Target.h"
#include "lldb/Target/Thread.h"
@@ -781,8 +782,46 @@
return false; // resize failed, out of memory?
}
-uint32_t StackFrameList::GetSelectedFrameIndex() const {
+void StackFrameList::SelectMostRelevantFrame() {
+ // Don't call into the frame recognizers on the private state thread as
+ // they can cause code to run in the target, and that can cause deadlocks
+ // when fetching stop events for the expression.
+ if (m_thread.GetProcess()->CurrentThreadIsPrivateStateThread())
+ return;
+
+ Log *log = GetLog(LLDBLog::Thread);
+
+
+ // Only the top frame should be recognized.
+ StackFrameSP frame_sp = GetFrameAtIndex(0);
+ if (!frame_sp) {
+ LLDB_LOG(log, "Failed to construct Frame #0");
+ return;
+ }
+
+ RecognizedStackFrameSP recognized_frame_sp = frame_sp->GetRecognizedFrame();
+
+ if (!recognized_frame_sp) {
+ LLDB_LOG(log, "Frame #0 not recognized");
+ return;
+ }
+
+ if (StackFrameSP most_relevant_frame_sp =
+ recognized_frame_sp->GetMostRelevantFrame()) {
+ LLDB_LOG(log, "Found most relevant frame at index {0}",
+ most_relevant_frame_sp->GetFrameIndex());
+ SetSelectedFrame(most_relevant_frame_sp.get());
+ } else {
+ LLDB_LOG(log, "No relevant frame!");
+ }
+}
+
+uint32_t StackFrameList::GetSelectedFrameIndex() {
std::lock_guard<std::recursive_mutex> guard(m_mutex);
+ if (m_selected_frame_set == false) {
+ SelectMostRelevantFrame();
+ m_selected_frame_set = true;
+ }
return m_selected_frame_idx;
}
@@ -792,6 +831,8 @@
const_iterator begin = m_frames.begin();
const_iterator end = m_frames.end();
m_selected_frame_idx = 0;
+ m_selected_frame_set = true;
+
for (pos = begin; pos != end; ++pos) {
if (pos->get() == frame) {
m_selected_frame_idx = std::distance(begin, pos);
@@ -830,10 +871,16 @@
// The thread has been run, reset the number stack frames to zero so we can
// determine how many frames we have lazily.
+// Note, we don't actually re-use StackFrameLists, we always make a new
+// StackFrameList every time we stop, and then copy frame information frame
+// by frame from the old to the new StackFrameList. So the comment above,
+// does not describe how StackFrameLists are currently used.
+// Clear is currently only used to clear the list in the destructor.
void StackFrameList::Clear() {
std::lock_guard<std::recursive_mutex> guard(m_mutex);
m_frames.clear();
m_concrete_frames_fetched = 0;
+ m_selected_frame_set = false;
}
lldb::StackFrameSP
Index: lldb/include/lldb/Target/Thread.h
===================================================================
--- lldb/include/lldb/Target/Thread.h
+++ lldb/include/lldb/Target/Thread.h
@@ -217,8 +217,6 @@
virtual void RefreshStateAfterStop() = 0;
- void SelectMostRelevantFrame();
-
std::string GetStopDescription();
std::string GetStopDescriptionRaw();
Index: lldb/include/lldb/Target/StackFrameList.h
===================================================================
--- lldb/include/lldb/Target/StackFrameList.h
+++ lldb/include/lldb/Target/StackFrameList.h
@@ -46,7 +46,7 @@
uint32_t SetSelectedFrame(lldb_private::StackFrame *frame);
/// Get the currently selected frame index.
- uint32_t GetSelectedFrameIndex() const;
+ uint32_t GetSelectedFrameIndex();
/// Mark a stack frame as the currently selected frame using the frame index
/// \p idx. Like \ref GetFrameAtIndex, invisible frames cannot be selected.
@@ -109,6 +109,8 @@
uint32_t GetCurrentInlinedDepth();
void SetCurrentInlinedDepth(uint32_t new_depth);
+
+ void SelectMostRelevantFrame();
typedef std::vector<lldb::StackFrameSP> collection;
typedef collection::iterator iterator;
@@ -136,6 +138,10 @@
/// The currently selected frame.
uint32_t m_selected_frame_idx;
+ // Records whether anyone has set the selected frame on this stack yet.
+ // We only let recognizers change the frame if this is the first time
+ // GetSelectedFrame is called.
+ bool m_selected_frame_set = false;
/// The number of concrete frames fetched while filling the frame list. This
/// is only used when synthetic frames are enabled.
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
