[Lldb-commits] [lldb] 186fac3 - Convert the StackFrameList mutex to a shared mutex. (#117252)
via lldb-commits
lldb-commits at lists.llvm.org
Thu Dec 12 12:48:45 PST 2024
Author: jimingham
Date: 2024-12-12T12:48:41-08:00
New Revision: 186fac33d08b34be494caa58fe63972f69c6d6ab
URL: https://github.com/llvm/llvm-project/commit/186fac33d08b34be494caa58fe63972f69c6d6ab
DIFF: https://github.com/llvm/llvm-project/commit/186fac33d08b34be494caa58fe63972f69c6d6ab.diff
LOG: Convert the StackFrameList mutex to a shared mutex. (#117252)
In fact, there's only one public API in StackFrameList that changes
the list explicitly. The rest only change the list if you happen to
ask for more frames than lldb has currently fetched and that
always adds frames "behind the user's back". So we were
much more prone to deadlocking than we needed to be.
This patch uses a shared_mutex instead, and when we have to add more
frames (in GetFramesUpTo) we switches to exclusive long enough to add
the frames, then goes back to shared.
Most of the work here was actually getting the stack frame list locking
to not
require a recursive mutex (shared mutexes aren't recursive).
I also added a test that has 5 threads progressively asking for more
frames simultaneously to make sure we get back valid frames and don't
deadlock.
Added:
lldb/test/API/api/multithreaded/deep_stack.cpp
lldb/test/API/api/multithreaded/test_concurrent_unwind.cpp.template
Modified:
lldb/include/lldb/Target/StackFrameList.h
lldb/source/Target/StackFrameList.cpp
lldb/source/Target/Thread.cpp
lldb/test/API/api/multithreaded/TestMultithreaded.py
Removed:
################################################################################
diff --git a/lldb/include/lldb/Target/StackFrameList.h b/lldb/include/lldb/Target/StackFrameList.h
index 7d0e7a5b9a71b2..8a66296346f2d8 100644
--- a/lldb/include/lldb/Target/StackFrameList.h
+++ b/lldb/include/lldb/Target/StackFrameList.h
@@ -11,6 +11,7 @@
#include <memory>
#include <mutex>
+#include <shared_mutex>
#include <vector>
#include "lldb/Target/StackFrame.h"
@@ -94,24 +95,36 @@ class StackFrameList {
bool show_unique = false, bool show_hidden = false,
const char *frame_marker = nullptr);
+ /// Returns whether we have currently fetched all the frames of a stack.
+ bool WereAllFramesFetched() const;
+
protected:
friend class Thread;
friend class ScriptedThread;
+ /// Use this API to build a stack frame list (used for scripted threads, for
+ /// instance.) This API is not meant for StackFrameLists that have unwinders
+ /// and partake in lazy stack filling (using GetFramesUpTo). Rather if you
+ /// are building StackFrameLists with this API, you should build the entire
+ /// list before making it available for use.
bool SetFrameAtIndex(uint32_t idx, lldb::StackFrameSP &frame_sp);
- /// Realizes frames up to (and including) end_idx (which can be greater than
- /// the actual number of frames.)
+ /// Ensures that frames up to (and including) `end_idx` are realized in the
+ /// StackFrameList. `end_idx` can be larger than the actual number of frames,
+ /// in which case all the frames will be fetched. Acquires the writer end of
+ /// the list mutex.
/// Returns true if the function was interrupted, false otherwise.
- bool GetFramesUpTo(uint32_t end_idx,
- InterruptionControl allow_interrupt = AllowInterruption);
-
- void GetOnlyConcreteFramesUpTo(uint32_t end_idx, Unwind &unwinder);
-
- void SynthesizeTailCallFrames(StackFrame &next_frame);
-
- bool GetAllFramesFetched() { return m_concrete_frames_fetched == UINT32_MAX; }
+ /// Callers should first check (under the shared mutex) whether we need to
+ /// fetch frames or not.
+ bool GetFramesUpTo(uint32_t end_idx, InterruptionControl allow_interrupt);
+
+ // This should be called with either the reader or writer end of the list
+ // mutex held:
+ bool GetAllFramesFetched() const {
+ return m_concrete_frames_fetched == UINT32_MAX;
+ }
+ // This should be called with the writer end of the list mutex held.
void SetAllFramesFetched() { m_concrete_frames_fetched = UINT32_MAX; }
bool DecrementCurrentInlinedDepth();
@@ -122,6 +135,9 @@ class StackFrameList {
void SetCurrentInlinedDepth(uint32_t new_depth);
+ /// Calls into the stack frame recognizers and stop info to set the most
+ /// relevant frame. This can call out to arbitrary user code so it can't
+ /// hold the StackFrameList mutex.
void SelectMostRelevantFrame();
typedef std::vector<lldb::StackFrameSP> collection;
@@ -138,11 +154,16 @@ class StackFrameList {
// source of information.
lldb::StackFrameListSP m_prev_frames_sp;
- /// A mutex for this frame list.
- // TODO: This mutex may not always be held when required. In particular, uses
- // of the StackFrameList APIs in lldb_private::Thread look suspect. Consider
- // passing around a lock_guard reference to enforce proper locking.
- mutable std::recursive_mutex m_mutex;
+ /// A mutex for this frame list. The only public API that requires the
+ /// unique lock is Clear. All other clients take the shared lock, though
+ /// if we need more frames we may swap shared for unique to fulfill that
+ /// requirement.
+ mutable std::shared_mutex m_list_mutex;
+
+ // Setting the inlined depth should be protected against other attempts to
+ // change it, but since it doesn't mutate the list itself, we can limit the
+ // critical regions it produces by having a separate mutex.
+ mutable std::mutex m_inlined_depth_mutex;
/// A cache of frames. This may need to be updated when the program counter
/// changes.
@@ -171,6 +192,21 @@ class StackFrameList {
const bool m_show_inlined_frames;
private:
+ uint32_t SetSelectedFrameNoLock(lldb_private::StackFrame *frame);
+ lldb::StackFrameSP
+ GetFrameAtIndexNoLock(uint32_t idx,
+ std::shared_lock<std::shared_mutex> &guard);
+
+ /// These two Fetch frames APIs and SynthesizeTailCallFrames are called in
+ /// GetFramesUpTo, they are the ones that actually add frames. They must be
+ /// called with the writer end of the list mutex held.
+
+ /// Returns true if fetching frames was interrupted, false otherwise.
+ bool FetchFramesUpTo(uint32_t end_idx, InterruptionControl allow_interrupt);
+ /// Not currently interruptible so returns void.
+ void FetchOnlyConcreteFramesUpTo(uint32_t end_idx);
+ void SynthesizeTailCallFrames(StackFrame &next_frame);
+
StackFrameList(const StackFrameList &) = delete;
const StackFrameList &operator=(const StackFrameList &) = delete;
};
diff --git a/lldb/source/Target/StackFrameList.cpp b/lldb/source/Target/StackFrameList.cpp
index 94a381edd5e202..9c6208e9e0a65a 100644
--- a/lldb/source/Target/StackFrameList.cpp
+++ b/lldb/source/Target/StackFrameList.cpp
@@ -38,7 +38,7 @@ using namespace lldb_private;
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_thread(thread), m_prev_frames_sp(prev_frames_sp), m_frames(),
m_selected_frame_idx(), m_concrete_frames_fetched(0),
m_current_inlined_depth(UINT32_MAX),
m_current_inlined_pc(LLDB_INVALID_ADDRESS),
@@ -63,6 +63,7 @@ void StackFrameList::CalculateCurrentInlinedDepth() {
}
uint32_t StackFrameList::GetCurrentInlinedDepth() {
+ std::lock_guard<std::mutex> guard(m_inlined_depth_mutex);
if (m_show_inlined_frames && m_current_inlined_pc != LLDB_INVALID_ADDRESS) {
lldb::addr_t cur_pc = m_thread.GetRegisterContext()->GetPC();
if (cur_pc != m_current_inlined_pc) {
@@ -84,11 +85,6 @@ void StackFrameList::ResetCurrentInlinedDepth() {
if (!m_show_inlined_frames)
return;
- std::lock_guard<std::recursive_mutex> guard(m_mutex);
-
- m_current_inlined_pc = LLDB_INVALID_ADDRESS;
- m_current_inlined_depth = UINT32_MAX;
-
StopInfoSP stop_info_sp = m_thread.GetStopInfo();
if (!stop_info_sp)
return;
@@ -98,6 +94,7 @@ void StackFrameList::ResetCurrentInlinedDepth() {
// We're only adjusting the inlined stack here.
Log *log = GetLog(LLDBLog::Step);
if (inline_depth) {
+ std::lock_guard<std::mutex> guard(m_inlined_depth_mutex);
m_current_inlined_depth = *inline_depth;
m_current_inlined_pc = m_thread.GetRegisterContext()->GetPC();
@@ -107,6 +104,9 @@ void StackFrameList::ResetCurrentInlinedDepth() {
"depth: %d 0x%" PRIx64 ".\n",
m_current_inlined_depth, m_current_inlined_pc);
} else {
+ std::lock_guard<std::mutex> guard(m_inlined_depth_mutex);
+ m_current_inlined_pc = LLDB_INVALID_ADDRESS;
+ m_current_inlined_depth = UINT32_MAX;
if (log && log->GetVerbose())
LLDB_LOGF(
log,
@@ -119,6 +119,7 @@ bool StackFrameList::DecrementCurrentInlinedDepth() {
uint32_t current_inlined_depth = GetCurrentInlinedDepth();
if (current_inlined_depth != UINT32_MAX) {
if (current_inlined_depth > 0) {
+ std::lock_guard<std::mutex> guard(m_inlined_depth_mutex);
m_current_inlined_depth--;
return true;
}
@@ -128,6 +129,7 @@ bool StackFrameList::DecrementCurrentInlinedDepth() {
}
void StackFrameList::SetCurrentInlinedDepth(uint32_t new_depth) {
+ std::lock_guard<std::mutex> guard(m_inlined_depth_mutex);
m_current_inlined_depth = new_depth;
if (new_depth == UINT32_MAX)
m_current_inlined_pc = LLDB_INVALID_ADDRESS;
@@ -135,23 +137,9 @@ void StackFrameList::SetCurrentInlinedDepth(uint32_t new_depth) {
m_current_inlined_pc = m_thread.GetRegisterContext()->GetPC();
}
-void StackFrameList::GetOnlyConcreteFramesUpTo(uint32_t end_idx,
- Unwind &unwinder) {
- assert(m_thread.IsValid() && "Expected valid thread");
- assert(m_frames.size() <= end_idx && "Expected there to be frames to fill");
-
- if (end_idx < m_concrete_frames_fetched)
- return;
-
- uint32_t num_frames = unwinder.GetFramesUpTo(end_idx);
- if (num_frames <= end_idx + 1) {
- // Done unwinding.
- m_concrete_frames_fetched = UINT32_MAX;
- }
-
- // Don't create the frames eagerly. Defer this work to GetFrameAtIndex,
- // which can lazily query the unwinder to create frames.
- m_frames.resize(num_frames);
+bool StackFrameList::WereAllFramesFetched() const {
+ std::shared_lock<std::shared_mutex> guard(m_list_mutex);
+ return GetAllFramesFetched();
}
/// A sequence of calls that comprise some portion of a backtrace. Each frame
@@ -167,6 +155,8 @@ using CallSequence = std::vector<CallDescriptor>;
/// 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
/// on failure \p path is unchanged.
+/// This function doesn't currently access StackFrameLists at all, it only looks
+/// at the frame set in the ExecutionContext it passes around.
static void FindInterveningFrames(Function &begin, Function &end,
ExecutionContext &exe_ctx, Target &target,
addr_t return_pc, CallSequence &path,
@@ -350,23 +340,65 @@ void StackFrameList::SynthesizeTailCallFrames(StackFrame &next_frame) {
bool StackFrameList::GetFramesUpTo(uint32_t end_idx,
InterruptionControl allow_interrupt) {
+ // GetFramesUpTo is always called with the intent to add frames, so get the
+ // writer lock:
+ std::unique_lock<std::shared_mutex> guard(m_list_mutex);
+ // Now that we have the lock, check to make sure someone didn't get there
+ // ahead of us:
+ if (m_frames.size() > end_idx || GetAllFramesFetched())
+ return false;
+
// Do not fetch frames for an invalid thread.
bool was_interrupted = false;
if (!m_thread.IsValid())
return false;
- // We've already gotten more frames than asked for, or we've already finished
- // unwinding, return.
- if (m_frames.size() > end_idx || GetAllFramesFetched())
+ // lock the writer side of m_list_mutex as we're going to add frames here:
+ if (!m_show_inlined_frames) {
+ if (end_idx < m_concrete_frames_fetched)
+ return false;
+ // We're adding concrete frames now:
+ // FIXME: This should also be interruptible:
+ FetchOnlyConcreteFramesUpTo(end_idx);
return false;
+ }
+
+ // We're adding concrete and inlined frames now:
+ was_interrupted = FetchFramesUpTo(end_idx, allow_interrupt);
+
+#if defined(DEBUG_STACK_FRAMES)
+ s.PutCString("\n\nNew frames:\n");
+ Dump(&s);
+ s.EOL();
+#endif
+ return was_interrupted;
+}
+
+void StackFrameList::FetchOnlyConcreteFramesUpTo(uint32_t end_idx) {
+ assert(m_thread.IsValid() && "Expected valid thread");
+ assert(m_frames.size() <= end_idx && "Expected there to be frames to fill");
Unwind &unwinder = m_thread.GetUnwinder();
- if (!m_show_inlined_frames) {
- GetOnlyConcreteFramesUpTo(end_idx, unwinder);
- return false;
+ if (end_idx < m_concrete_frames_fetched)
+ return;
+
+ uint32_t num_frames = unwinder.GetFramesUpTo(end_idx);
+ if (num_frames <= end_idx + 1) {
+ // Done unwinding.
+ m_concrete_frames_fetched = UINT32_MAX;
}
+ // Don't create the frames eagerly. Defer this work to GetFrameAtIndex,
+ // which can lazily query the unwinder to create frames.
+ m_frames.resize(num_frames);
+}
+
+bool StackFrameList::FetchFramesUpTo(uint32_t end_idx,
+ InterruptionControl allow_interrupt) {
+ Unwind &unwinder = m_thread.GetUnwinder();
+ bool was_interrupted = false;
+
#if defined(DEBUG_STACK_FRAMES)
StreamFile s(stdout, false);
#endif
@@ -421,11 +453,11 @@ bool StackFrameList::GetFramesUpTo(uint32_t end_idx,
} else {
// Check for interruption when building the frames.
// Do the check in idx > 0 so that we'll always create a 0th frame.
- if (allow_interrupt
- && INTERRUPT_REQUESTED(dbg, "Interrupted having fetched {0} frames",
- m_frames.size())) {
- was_interrupted = true;
- break;
+ if (allow_interrupt &&
+ INTERRUPT_REQUESTED(dbg, "Interrupted having fetched {0} frames",
+ m_frames.size())) {
+ was_interrupted = true;
+ break;
}
const bool success =
@@ -534,12 +566,6 @@ bool StackFrameList::GetFramesUpTo(uint32_t end_idx,
// We are done with the old stack frame list, we can release it now.
m_prev_frames_sp.reset();
}
-
-#if defined(DEBUG_STACK_FRAMES)
- s.PutCString("\n\nNew frames:\n");
- Dump(&s);
- s.EOL();
-#endif
// Don't report interrupted if we happen to have gotten all the frames:
if (!GetAllFramesFetched())
return was_interrupted;
@@ -547,20 +573,23 @@ bool StackFrameList::GetFramesUpTo(uint32_t end_idx,
}
uint32_t StackFrameList::GetNumFrames(bool can_create) {
- std::lock_guard<std::recursive_mutex> guard(m_mutex);
-
- if (can_create) {
+ if (!WereAllFramesFetched() && can_create) {
// Don't allow interrupt or we might not return the correct count
- GetFramesUpTo(UINT32_MAX, DoNotAllowInterruption);
+ GetFramesUpTo(UINT32_MAX, DoNotAllowInterruption);
+ }
+ uint32_t frame_idx;
+ {
+ std::shared_lock<std::shared_mutex> guard(m_list_mutex);
+ frame_idx = GetVisibleStackFrameIndex(m_frames.size());
}
- return GetVisibleStackFrameIndex(m_frames.size());
+ return frame_idx;
}
void StackFrameList::Dump(Stream *s) {
if (s == nullptr)
return;
- std::lock_guard<std::recursive_mutex> guard(m_mutex);
+ std::shared_lock<std::shared_mutex> guard(m_list_mutex);
const_iterator pos, begin = m_frames.begin(), end = m_frames.end();
for (pos = begin; pos != end; ++pos) {
@@ -578,72 +607,53 @@ void StackFrameList::Dump(Stream *s) {
StackFrameSP StackFrameList::GetFrameAtIndex(uint32_t idx) {
StackFrameSP frame_sp;
- std::lock_guard<std::recursive_mutex> guard(m_mutex);
uint32_t original_idx = idx;
- uint32_t inlined_depth = GetCurrentInlinedDepth();
- if (inlined_depth != UINT32_MAX)
- idx += inlined_depth;
+ // We're going to consult the m_frames.size, but if there are already
+ // enough frames for our request we don't want to block other readers, so
+ // first acquire the shared lock:
+ { // Scope for shared lock:
+ std::shared_lock<std::shared_mutex> guard(m_list_mutex);
- if (idx < m_frames.size())
- frame_sp = m_frames[idx];
+ uint32_t inlined_depth = GetCurrentInlinedDepth();
+ if (inlined_depth != UINT32_MAX)
+ idx += inlined_depth;
- if (frame_sp)
- return frame_sp;
+ if (idx < m_frames.size())
+ frame_sp = m_frames[idx];
+
+ if (frame_sp)
+ return frame_sp;
+ } // End of reader lock scope
// GetFramesUpTo will fill m_frames with as many frames as you asked for, if
// there are that many. If there weren't then you asked for too many frames.
// GetFramesUpTo returns true if interrupted:
- if (GetFramesUpTo(idx)) {
+ if (GetFramesUpTo(idx, AllowInterruption)) {
Log *log = GetLog(LLDBLog::Thread);
LLDB_LOG(log, "GetFrameAtIndex was interrupted");
return {};
}
- if (idx < m_frames.size()) {
- if (m_show_inlined_frames) {
- // When inline frames are enabled we actually create all the frames in
- // GetFramesUpTo.
+ { // Now we're accessing m_frames as a reader, so acquire the reader lock.
+ std::shared_lock<std::shared_mutex> guard(m_list_mutex);
+ if (idx < m_frames.size()) {
frame_sp = m_frames[idx];
- } else {
- addr_t pc, cfa;
- bool behaves_like_zeroth_frame = (idx == 0);
- if (m_thread.GetUnwinder().GetFrameInfoAtIndex(
- idx, cfa, pc, behaves_like_zeroth_frame)) {
- const bool cfa_is_valid = true;
- frame_sp = std::make_shared<StackFrame>(
- m_thread.shared_from_this(), idx, idx, cfa, cfa_is_valid, pc,
- StackFrame::Kind::Regular, behaves_like_zeroth_frame, nullptr);
-
- Function *function =
- frame_sp->GetSymbolContext(eSymbolContextFunction).function;
- if (function) {
- // When we aren't showing inline functions we always use the top
- // most function block as the scope.
- frame_sp->SetSymbolContextScope(&function->GetBlock(false));
- } else {
- // Set the symbol scope from the symbol regardless if it is nullptr
- // or valid.
- frame_sp->SetSymbolContextScope(
- frame_sp->GetSymbolContext(eSymbolContextSymbol).symbol);
- }
- SetFrameAtIndex(idx, frame_sp);
+ } else if (original_idx == 0) {
+ // There should ALWAYS be a frame at index 0. If something went wrong
+ // with the CurrentInlinedDepth such that there weren't as many frames as
+ // we thought taking that into account, then reset the current inlined
+ // depth and return the real zeroth frame.
+ if (m_frames.empty()) {
+ // Why do we have a thread with zero frames, that should not ever
+ // happen...
+ assert(!m_thread.IsValid() && "A valid thread has no frames.");
+ } else {
+ ResetCurrentInlinedDepth();
+ frame_sp = m_frames[original_idx];
}
}
- } else if (original_idx == 0) {
- // There should ALWAYS be a frame at index 0. If something went wrong with
- // the CurrentInlinedDepth such that there weren't as many frames as we
- // thought taking that into account, then reset the current inlined depth
- // and return the real zeroth frame.
- if (m_frames.empty()) {
- // Why do we have a thread with zero frames, that should not ever
- // happen...
- assert(!m_thread.IsValid() && "A valid thread has no frames.");
- } else {
- ResetCurrentInlinedDepth();
- frame_sp = m_frames[original_idx];
- }
- }
+ } // End of reader lock scope
return frame_sp;
}
@@ -675,19 +685,18 @@ StackFrameSP StackFrameList::GetFrameWithStackID(const StackID &stack_id) {
StackFrameSP frame_sp;
if (stack_id.IsValid()) {
- std::lock_guard<std::recursive_mutex> guard(m_mutex);
uint32_t frame_idx = 0;
- // Do a binary search in case the stack frame is already in our cache
- collection::const_iterator begin = m_frames.begin();
- collection::const_iterator end = m_frames.end();
- if (begin != end) {
+ {
+ // First see if the frame is already realized. This is the scope for
+ // the shared mutex:
+ std::shared_lock<std::shared_mutex> guard(m_list_mutex);
+ // Do a binary search in case the stack frame is already in our cache
collection::const_iterator pos =
- std::lower_bound(begin, end, stack_id, CompareStackID);
- if (pos != end) {
- if ((*pos)->GetStackID() == stack_id)
- return *pos;
- }
+ llvm::lower_bound(m_frames, stack_id, CompareStackID);
+ if (pos != m_frames.end() && (*pos)->GetStackID() == stack_id)
+ return *pos;
}
+ // If we needed to add more frames, we would get to here.
do {
frame_sp = GetFrameAtIndex(frame_idx);
if (frame_sp && frame_sp->GetStackID() == stack_id)
@@ -699,6 +708,7 @@ StackFrameSP StackFrameList::GetFrameWithStackID(const StackID &stack_id) {
}
bool StackFrameList::SetFrameAtIndex(uint32_t idx, StackFrameSP &frame_sp) {
+ std::unique_lock<std::shared_mutex> guard(m_list_mutex);
if (idx >= m_frames.size())
m_frames.resize(idx + 1);
// Make sure allocation succeeded by checking bounds again
@@ -738,7 +748,7 @@ void StackFrameList::SelectMostRelevantFrame() {
}
LLDB_LOG(log, "Frame #0 not recognized");
- // If this thread has a non-trivial StopInof, then let it suggest
+ // If this thread has a non-trivial StopInfo, then let it suggest
// a most relevant frame:
StopInfoSP stop_info_sp = m_thread.GetStopInfo();
uint32_t stack_idx = 0;
@@ -771,9 +781,8 @@ void StackFrameList::SelectMostRelevantFrame() {
LLDB_LOG(log, "No relevant frame!");
}
-uint32_t StackFrameList::GetSelectedFrameIndex(
- SelectMostRelevant select_most_relevant) {
- std::lock_guard<std::recursive_mutex> guard(m_mutex);
+uint32_t
+StackFrameList::GetSelectedFrameIndex(SelectMostRelevant select_most_relevant) {
if (!m_selected_frame_idx && select_most_relevant)
SelectMostRelevantFrame();
if (!m_selected_frame_idx) {
@@ -788,7 +797,8 @@ uint32_t StackFrameList::GetSelectedFrameIndex(
}
uint32_t StackFrameList::SetSelectedFrame(lldb_private::StackFrame *frame) {
- std::lock_guard<std::recursive_mutex> guard(m_mutex);
+ std::shared_lock<std::shared_mutex> guard(m_list_mutex);
+
const_iterator pos;
const_iterator begin = m_frames.begin();
const_iterator end = m_frames.end();
@@ -803,13 +813,11 @@ uint32_t StackFrameList::SetSelectedFrame(lldb_private::StackFrame *frame) {
break;
}
}
-
SetDefaultFileAndLineToSelectedFrame();
return *m_selected_frame_idx;
}
bool StackFrameList::SetSelectedFrameByIndex(uint32_t idx) {
- std::lock_guard<std::recursive_mutex> guard(m_mutex);
StackFrameSP frame_sp(GetFrameAtIndex(idx));
if (frame_sp) {
SetSelectedFrame(frame_sp.get());
@@ -840,7 +848,7 @@ void StackFrameList::SetDefaultFileAndLineToSelectedFrame() {
// 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);
+ std::unique_lock<std::shared_mutex> guard(m_list_mutex);
m_frames.clear();
m_concrete_frames_fetched = 0;
m_selected_frame_idx.reset();
@@ -848,6 +856,7 @@ void StackFrameList::Clear() {
lldb::StackFrameSP
StackFrameList::GetStackFrameSPForStackFramePtr(StackFrame *stack_frame_ptr) {
+ std::shared_lock<std::shared_mutex> guard(m_list_mutex);
const_iterator pos;
const_iterator begin = m_frames.begin();
const_iterator end = m_frames.end();
diff --git a/lldb/source/Target/Thread.cpp b/lldb/source/Target/Thread.cpp
index fb17276051909c..a6130f6b925bbf 100644
--- a/lldb/source/Target/Thread.cpp
+++ b/lldb/source/Target/Thread.cpp
@@ -1449,7 +1449,7 @@ void Thread::ClearStackFrames() {
// frames:
// FIXME: At some point we can try to splice in the frames we have fetched
// into the new frame as we make it, but let's not try that now.
- if (m_curr_frames_sp && m_curr_frames_sp->GetAllFramesFetched())
+ if (m_curr_frames_sp && m_curr_frames_sp->WereAllFramesFetched())
m_prev_frames_sp.swap(m_curr_frames_sp);
m_curr_frames_sp.reset();
diff --git a/lldb/test/API/api/multithreaded/TestMultithreaded.py b/lldb/test/API/api/multithreaded/TestMultithreaded.py
index 07c9f5b9bbcca3..d5b29ec7af1818 100644
--- a/lldb/test/API/api/multithreaded/TestMultithreaded.py
+++ b/lldb/test/API/api/multithreaded/TestMultithreaded.py
@@ -22,6 +22,7 @@ def setUp(self):
self.generateSource("test_listener_event_process_state.cpp")
self.generateSource("test_listener_resume.cpp")
self.generateSource("test_stop-hook.cpp")
+ self.generateSource("test_concurrent_unwind.cpp")
@skipIfRemote
# clang-cl does not support throw or catch (llvm.org/pr24538)
@@ -91,7 +92,19 @@ def test_sb_api_listener_resume(self):
"test_listener_resume",
)
- def build_and_test(self, sources, test_name, args=None):
+ @skipIfRemote
+ # clang-cl does not support throw or catch (llvm.org/pr24538)
+ @skipIfWindows
+ @skipIfHostIncompatibleWithTarget
+ def test_concurrent_unwind(self):
+ """Test that you can run a python command in a stop-hook when stdin is File based."""
+ self.build_and_test(
+ "driver.cpp test_concurrent_unwind.cpp",
+ "test_concurrent_unwind",
+ inferior_source="deep_stack.cpp",
+ )
+
+ def build_and_test(self, sources, test_name, inferior_source="inferior.cpp"):
"""Build LLDB test from sources, and run expecting 0 exit code"""
# These tests link against host lldb API.
@@ -104,7 +117,7 @@ def build_and_test(self, sources, test_name, args=None):
)
self.inferior = "inferior_program"
- self.buildProgram("inferior.cpp", self.inferior)
+ self.buildProgram(inferior_source, self.inferior)
self.addTearDownHook(lambda: os.remove(self.getBuildArtifact(self.inferior)))
self.buildDriver(sources, test_name)
diff --git a/lldb/test/API/api/multithreaded/deep_stack.cpp b/lldb/test/API/api/multithreaded/deep_stack.cpp
new file mode 100644
index 00000000000000..da89228766e425
--- /dev/null
+++ b/lldb/test/API/api/multithreaded/deep_stack.cpp
@@ -0,0 +1,17 @@
+// This is a test program that makes a deep stack
+// so we can test unwinding from multiple threads.
+
+void call_me(int input) {
+ if (input > 1000) {
+ input += 1; // Set a breakpoint here
+ if (input > 1001)
+ input += 1;
+ return;
+ } else
+ call_me(++input);
+}
+
+int main() {
+ call_me(0);
+ return 0;
+}
diff --git a/lldb/test/API/api/multithreaded/test_concurrent_unwind.cpp.template b/lldb/test/API/api/multithreaded/test_concurrent_unwind.cpp.template
new file mode 100644
index 00000000000000..e5101dde79619a
--- /dev/null
+++ b/lldb/test/API/api/multithreaded/test_concurrent_unwind.cpp.template
@@ -0,0 +1,91 @@
+#include "pseudo_barrier.h"
+
+#include <atomic>
+#include <thread>
+
+%include_SB_APIs%
+
+#include "common.h"
+
+using namespace lldb;
+
+void test (SBDebugger &dbg, std::vector<std::string> args) {
+
+SBError error;
+ dbg.SetAsync(false);
+ SBTarget target = dbg.CreateTarget(args.at(0).c_str());
+ if (!target.IsValid())
+ throw Exception("Invalid target");
+
+ // Now set our breakpoint and launch:
+ SBFileSpec main_sourcefile("deep_stack.cpp");
+ SBBreakpoint bkpt = target.BreakpointCreateBySourceRegex("Set a breakpoint here",
+ main_sourcefile);
+ if (bkpt.GetNumLocations() == 0)
+ throw Exception("Main breakpoint got no locations");
+
+ SBLaunchInfo launch_info = target.GetLaunchInfo();
+ SBProcess process = target.Launch(launch_info, error);
+ if (error.Fail())
+ throw Exception("Failed to launch process");
+ if (!process.IsValid())
+ throw Exception("Process is not valid");
+ if (process.GetState() != lldb::eStateStopped)
+ throw Exception("Process was not stopped");
+
+ size_t num_threads = process.GetNumThreads();
+ if (num_threads != 1)
+ throw Exception("Unexpected number of threads.");
+ SBThread cur_thread = process.GetThreadAtIndex(0);
+ if (!cur_thread.IsValid())
+ throw Exception("Didn't get a valid thread");
+
+ // Record the number of frames at the point where we stopped:
+ const size_t num_frames = cur_thread.GetNumFrames();
+ // Now step once to clear the frame cache:
+ cur_thread.StepOver();
+
+ // Create three threads and set them to getting frames simultaneously,
+ // and make sure we don't deadlock.
+ pseudo_barrier_t rendevous;
+ pseudo_barrier_init(rendevous, 5);
+ std::atomic_size_t success(true);
+ std::atomic_size_t largest(0);
+
+ auto lambda = [&](size_t stride){
+ pseudo_barrier_wait(rendevous);
+ bool younger = true;
+ while (1) {
+ size_t cursor = largest;
+ if (cursor > stride && !younger) {
+ cursor -= stride;
+ younger = true;
+ } else {
+ cursor += stride;
+ largest += stride;
+ younger = false;
+ }
+ SBFrame frame = cur_thread.GetFrameAtIndex(cursor);
+ if (!frame.IsValid()) {
+ if (cursor < num_frames)
+ success = false;
+ break;
+ }
+ }
+
+ };
+
+ std::thread thread1(lambda, 1);
+ std::thread thread2(lambda, 3);
+ std::thread thread3(lambda, 5);
+ std::thread thread4(lambda, 7);
+ std::thread thread5(lambda, 11);
+ thread1.join();
+ thread2.join();
+ thread3.join();
+ thread4.join();
+ thread5.join();
+
+ if (!success)
+ throw Exception("One thread stopped before 1000");
+}
More information about the lldb-commits
mailing list