From 730c8e160c9dead94ba534d5ad7cc8e47ce892db Mon Sep 17 00:00:00 2001 From: Jim Ingham Date: Fri, 7 Apr 2023 12:17:57 -0700 Subject: [PATCH] [lldb] Move "SelectMostRelevantFrame" from Thread::WillStop 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 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. rdar://107643231 Differential revision: https://reviews.llvm.org/D147753 --- lldb/include/lldb/Target/StackFrameList.h | 10 ++++-- lldb/include/lldb/Target/Thread.h | 2 -- lldb/source/Target/StackFrameList.cpp | 58 +++++++++++++++++++++++++++---- lldb/source/Target/Thread.cpp | 27 -------------- 4 files changed, 59 insertions(+), 38 deletions(-) diff --git a/lldb/include/lldb/Target/StackFrameList.h b/lldb/include/lldb/Target/StackFrameList.h index e05a398..ae99853 100644 --- a/lldb/include/lldb/Target/StackFrameList.h +++ b/lldb/include/lldb/Target/StackFrameList.h @@ -46,7 +46,7 @@ public: 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. @@ -110,6 +110,8 @@ protected: void SetCurrentInlinedDepth(uint32_t new_depth); + void SelectMostRelevantFrame(); + typedef std::vector collection; typedef collection::iterator iterator; typedef collection::const_iterator const_iterator; @@ -134,8 +136,10 @@ protected: /// changes. collection m_frames; - /// The currently selected frame. - uint32_t m_selected_frame_idx; + /// The currently selected frame. An optional is used to record 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. + std::optional m_selected_frame_idx; /// The number of concrete frames fetched while filling the frame list. This /// is only used when synthetic frames are enabled. diff --git a/lldb/include/lldb/Target/Thread.h b/lldb/include/lldb/Target/Thread.h index efe8272..882f001e 100644 --- a/lldb/include/lldb/Target/Thread.h +++ b/lldb/include/lldb/Target/Thread.h @@ -217,8 +217,6 @@ public: virtual void RefreshStateAfterStop() = 0; - void SelectMostRelevantFrame(); - std::string GetStopDescription(); std::string GetStopDescriptionRaw(); diff --git a/lldb/source/Target/StackFrameList.cpp b/lldb/source/Target/StackFrameList.cpp index 2610b1c..3b0c66a 100644 --- a/lldb/source/Target/StackFrameList.cpp +++ b/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" @@ -38,7 +39,7 @@ StackFrameList::StackFrameList(Thread &thread, const lldb::StackFrameListSP &prev_frames_sp, bool show_inline_frames) : m_thread(thread), m_prev_frames_sp(prev_frames_sp), m_mutex(), m_frames(), - m_selected_frame_idx(0), m_concrete_frames_fetched(0), + m_selected_frame_idx(), m_concrete_frames_fetched(0), m_current_inlined_depth(UINT32_MAX), m_current_inlined_pc(LLDB_INVALID_ADDRESS), m_show_inlined_frames(show_inline_frames) { @@ -252,7 +253,7 @@ struct CallDescriptor { using CallSequence = std::vector; /// Find the unique path through the call graph from \p begin (with return PC -/// \p return_pc) to \p end. On success this path is stored into \p path, and +/// \p return_pc) to \p end. On success this path is stored into \p path, and /// on failure \p path is unchanged. static void FindInterveningFrames(Function &begin, Function &end, ExecutionContext &exe_ctx, Target &target, @@ -781,9 +782,46 @@ bool StackFrameList::SetFrameAtIndex(uint32_t idx, StackFrameSP &frame_sp) { 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 guard(m_mutex); - return m_selected_frame_idx; + if (!m_selected_frame_idx) + SelectMostRelevantFrame(); + if (!m_selected_frame_idx) + m_selected_frame_idx = 0; + return *m_selected_frame_idx; } uint32_t StackFrameList::SetSelectedFrame(lldb_private::StackFrame *frame) { @@ -792,17 +830,19 @@ uint32_t StackFrameList::SetSelectedFrame(lldb_private::StackFrame *frame) { const_iterator begin = m_frames.begin(); const_iterator end = m_frames.end(); m_selected_frame_idx = 0; + for (pos = begin; pos != end; ++pos) { if (pos->get() == frame) { m_selected_frame_idx = std::distance(begin, pos); uint32_t inlined_depth = GetCurrentInlinedDepth(); if (inlined_depth != UINT32_MAX) - m_selected_frame_idx -= inlined_depth; + m_selected_frame_idx = *m_selected_frame_idx - inlined_depth; break; } } + SetDefaultFileAndLineToSelectedFrame(); - return m_selected_frame_idx; + return *m_selected_frame_idx; } bool StackFrameList::SetSelectedFrameByIndex(uint32_t idx) { @@ -830,10 +870,16 @@ void StackFrameList::SetDefaultFileAndLineToSelectedFrame() { // 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 guard(m_mutex); m_frames.clear(); m_concrete_frames_fetched = 0; + m_selected_frame_idx.reset(); } lldb::StackFrameSP diff --git a/lldb/source/Target/Thread.cpp b/lldb/source/Target/Thread.cpp index d620f74..ab8c115 100644 --- a/lldb/source/Target/Thread.cpp +++ b/lldb/source/Target/Thread.cpp @@ -588,36 +588,9 @@ std::string Thread::GetStopDescriptionRaw() { 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. -- 2.7.4