[Lldb-commits] [lldb] Convert the StackFrameList mutex to a shared mutex. (PR #117252)
via lldb-commits
lldb-commits at lists.llvm.org
Mon Dec 9 17:47:17 PST 2024
https://github.com/jimingham updated https://github.com/llvm/llvm-project/pull/117252
>From b0d2179cf01cdd0b07bc43cef2a8c14d282e7ee7 Mon Sep 17 00:00:00 2001
From: Jim Ingham <jingham at apple.com>
Date: Tue, 19 Nov 2024 17:38:02 -0800
Subject: [PATCH 1/7] Convert the recursive StackFrameList mutex to a
non-recursive one.
---
lldb/include/lldb/Target/StackFrameList.h | 19 +++-
lldb/source/Target/StackFrameList.cpp | 104 +++++++++++++---------
2 files changed, 81 insertions(+), 42 deletions(-)
diff --git a/lldb/include/lldb/Target/StackFrameList.h b/lldb/include/lldb/Target/StackFrameList.h
index 7d0e7a5b9a71b2..1c59d2e97937da 100644
--- a/lldb/include/lldb/Target/StackFrameList.h
+++ b/lldb/include/lldb/Target/StackFrameList.h
@@ -103,11 +103,15 @@ class StackFrameList {
/// Realizes frames up to (and including) end_idx (which can be greater than
/// the actual number of frames.)
/// Returns true if the function was interrupted, false otherwise.
+ /// Does not hold the StackFrameList mutex.
bool GetFramesUpTo(uint32_t end_idx,
InterruptionControl allow_interrupt = AllowInterruption);
+ /// Does not hold the StackFrameList mutex.
void GetOnlyConcreteFramesUpTo(uint32_t end_idx, Unwind &unwinder);
+ // This gets called without the StackFrameList lock held, callers should
+ // hold the lock.
void SynthesizeTailCallFrames(StackFrame &next_frame);
bool GetAllFramesFetched() { return m_concrete_frames_fetched == UINT32_MAX; }
@@ -122,6 +126,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;
@@ -142,7 +149,14 @@ class StackFrameList {
// 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;
+ mutable std::mutex m_mutex;
+
+ // llvm::sys::RWMutex m_stack_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 +185,9 @@ class StackFrameList {
const bool m_show_inlined_frames;
private:
+ uint32_t SetSelectedFrameNoLock(lldb_private::StackFrame *frame);
+ lldb::StackFrameSP GetFrameAtIndexNoLock(uint32_t idx);
+
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..dddd8dc358e492 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;
@@ -167,6 +169,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,
@@ -547,7 +551,7 @@ bool StackFrameList::GetFramesUpTo(uint32_t end_idx,
}
uint32_t StackFrameList::GetNumFrames(bool can_create) {
- std::lock_guard<std::recursive_mutex> guard(m_mutex);
+ std::lock_guard<std::mutex> guard(m_mutex);
if (can_create) {
// Don't allow interrupt or we might not return the correct count
@@ -560,7 +564,7 @@ void StackFrameList::Dump(Stream *s) {
if (s == nullptr)
return;
- std::lock_guard<std::recursive_mutex> guard(m_mutex);
+ std::lock_guard<std::mutex> guard(m_mutex);
const_iterator pos, begin = m_frames.begin(), end = m_frames.end();
for (pos = begin; pos != end; ++pos) {
@@ -577,8 +581,12 @@ void StackFrameList::Dump(Stream *s) {
}
StackFrameSP StackFrameList::GetFrameAtIndex(uint32_t idx) {
+ std::lock_guard<std::mutex> guard(m_mutex);
+ return GetFrameAtIndexNoLock(idx);
+}
+
+StackFrameSP StackFrameList::GetFrameAtIndexNoLock(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();
@@ -657,11 +665,12 @@ StackFrameList::GetFrameWithConcreteFrameIndex(uint32_t unwind_idx) {
// after we make all the inlined frames. Most of the time the unwind frame
// index (or the concrete frame index) is the same as the frame index.
uint32_t frame_idx = unwind_idx;
- StackFrameSP frame_sp(GetFrameAtIndex(frame_idx));
+ std::lock_guard<std::mutex> guard(m_mutex);
+ StackFrameSP frame_sp(GetFrameAtIndexNoLock(frame_idx));
while (frame_sp) {
if (frame_sp->GetFrameIndex() == unwind_idx)
break;
- frame_sp = GetFrameAtIndex(++frame_idx);
+ frame_sp = GetFrameAtIndexNoLock(++frame_idx);
}
return frame_sp;
}
@@ -675,7 +684,7 @@ StackFrameSP StackFrameList::GetFrameWithStackID(const StackID &stack_id) {
StackFrameSP frame_sp;
if (stack_id.IsValid()) {
- std::lock_guard<std::recursive_mutex> guard(m_mutex);
+ std::lock_guard<std::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();
@@ -689,7 +698,7 @@ StackFrameSP StackFrameList::GetFrameWithStackID(const StackID &stack_id) {
}
}
do {
- frame_sp = GetFrameAtIndex(frame_idx);
+ frame_sp = GetFrameAtIndexNoLock(frame_idx);
if (frame_sp && frame_sp->GetStackID() == stack_id)
break;
frame_idx++;
@@ -738,7 +747,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;
@@ -773,43 +782,55 @@ void StackFrameList::SelectMostRelevantFrame() {
uint32_t StackFrameList::GetSelectedFrameIndex(
SelectMostRelevant select_most_relevant) {
- std::lock_guard<std::recursive_mutex> guard(m_mutex);
if (!m_selected_frame_idx && select_most_relevant)
SelectMostRelevantFrame();
- if (!m_selected_frame_idx) {
- // If we aren't selecting the most relevant frame, and the selected frame
- // isn't set, then don't force a selection here, just return 0.
- if (!select_most_relevant)
- return 0;
- // If the inlined stack frame is set, then use that:
- m_selected_frame_idx = 0;
+ { // Scope for lock guard
+ std::lock_guard<std::mutex> guard(m_mutex);
+ if (!m_selected_frame_idx) {
+ // If we aren't selecting the most relevant frame, and the selected frame
+ // isn't set, then don't force a selection here, just return 0.
+ if (!select_most_relevant)
+ return 0;
+ // If the inlined stack frame is set, then use that:
+ m_selected_frame_idx = 0;
+ }
+ return *m_selected_frame_idx;
}
- return *m_selected_frame_idx;
}
uint32_t StackFrameList::SetSelectedFrame(lldb_private::StackFrame *frame) {
- std::lock_guard<std::recursive_mutex> guard(m_mutex);
- const_iterator pos;
- 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 = *m_selected_frame_idx - inlined_depth;
- break;
- }
+ uint32_t result = 0;
+ {
+ std::lock_guard<std::mutex> guard(m_mutex);
+ result = SetSelectedFrameNoLock(frame);
}
-
SetDefaultFileAndLineToSelectedFrame();
- return *m_selected_frame_idx;
+ return result;
+}
+
+uint32_t StackFrameList::SetSelectedFrameNoLock(lldb_private::StackFrame *frame)
+{
+ uint32_t result = 0;
+
+ const_iterator pos;
+ 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 = *m_selected_frame_idx - inlined_depth;
+ break;
+ }
+ }
+ result = *m_selected_frame_idx;
+ return result;
}
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 +861,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::lock_guard<std::mutex> guard(m_mutex);
m_frames.clear();
m_concrete_frames_fetched = 0;
m_selected_frame_idx.reset();
@@ -848,6 +869,7 @@ void StackFrameList::Clear() {
lldb::StackFrameSP
StackFrameList::GetStackFrameSPForStackFramePtr(StackFrame *stack_frame_ptr) {
+ std::lock_guard<std::mutex> guard(m_mutex);
const_iterator pos;
const_iterator begin = m_frames.begin();
const_iterator end = m_frames.end();
>From a7d9dc919b46e1024d89cbe4ccb77dba6f5b9ee3 Mon Sep 17 00:00:00 2001
From: Jim Ingham <jingham at apple.com>
Date: Thu, 21 Nov 2024 14:30:39 -0800
Subject: [PATCH 2/7] Convert the StackFrameList mutex to a shared mutex.
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. 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.
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.
---
lldb/include/lldb/Target/StackFrameList.h | 28 +-
lldb/source/Target/StackFrameList.cpp | 388 ++++++++++--------
.../api/multithreaded/TestMultithreaded.py | 15 +-
.../test/API/api/multithreaded/deep_stack.cpp | 17 +
.../test_concurrent_unwind.cpp.template | 91 ++++
5 files changed, 345 insertions(+), 194 deletions(-)
create mode 100644 lldb/test/API/api/multithreaded/deep_stack.cpp
create mode 100644 lldb/test/API/api/multithreaded/test_concurrent_unwind.cpp.template
diff --git a/lldb/include/lldb/Target/StackFrameList.h b/lldb/include/lldb/Target/StackFrameList.h
index 1c59d2e97937da..8f6f6500f1ef96 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"
@@ -103,12 +104,16 @@ class StackFrameList {
/// Realizes frames up to (and including) end_idx (which can be greater than
/// the actual number of frames.)
/// Returns true if the function was interrupted, false otherwise.
- /// Does not hold the StackFrameList mutex.
+ /// Must be called with a shared_locked mutex. This might unlock the
+ /// shared side if it needs to fetch more frames, but will reacquire the
+ /// shared lock on the way out.
bool GetFramesUpTo(uint32_t end_idx,
- InterruptionControl allow_interrupt = AllowInterruption);
+ InterruptionControl allow_interrupt,
+ std::shared_lock<std::shared_mutex> &guard);
- /// Does not hold the StackFrameList mutex.
- void GetOnlyConcreteFramesUpTo(uint32_t end_idx, Unwind &unwinder);
+ /// Does not hold the StackFrameList mutex. Same comment as GetFramesUpTo.
+ void GetOnlyConcreteFramesUpTo(uint32_t end_idx, Unwind &unwinder,
+ std::shared_lock<std::shared_mutex> &guard);
// This gets called without the StackFrameList lock held, callers should
// hold the lock.
@@ -145,13 +150,11 @@ 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::mutex m_mutex;
-
- // llvm::sys::RWMutex m_stack_list_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
@@ -186,7 +189,8 @@ class StackFrameList {
private:
uint32_t SetSelectedFrameNoLock(lldb_private::StackFrame *frame);
- lldb::StackFrameSP GetFrameAtIndexNoLock(uint32_t idx);
+ lldb::StackFrameSP GetFrameAtIndexNoLock(uint32_t idx,
+ std::shared_lock<std::shared_mutex> &guard);
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 dddd8dc358e492..0dc4fa7797c58b 100644
--- a/lldb/source/Target/StackFrameList.cpp
+++ b/lldb/source/Target/StackFrameList.cpp
@@ -25,6 +25,7 @@
#include "lldb/Target/Unwind.h"
#include "lldb/Utility/LLDBLog.h"
#include "lldb/Utility/Log.h"
+#include "llvm/ADT/ScopeExit.h"
#include "llvm/ADT/SmallPtrSet.h"
#include <memory>
@@ -138,22 +139,33 @@ void StackFrameList::SetCurrentInlinedDepth(uint32_t new_depth) {
}
void StackFrameList::GetOnlyConcreteFramesUpTo(uint32_t end_idx,
- Unwind &unwinder) {
+ Unwind &unwinder,
+ std::shared_lock<std::shared_mutex> &guard) {
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;
+ { // Scope for swapping reader and writer locks
+ m_list_mutex.lock();
+ auto on_exit = llvm::make_scope_exit(
+ [&]() {
+ m_list_mutex.unlock();
+ guard.lock();
+ });
+ 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;
- }
+ 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);
+ // 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);
+ }
}
/// A sequence of calls that comprise some portion of a backtrace. Each frame
@@ -353,7 +365,8 @@ void StackFrameList::SynthesizeTailCallFrames(StackFrame &next_frame) {
}
bool StackFrameList::GetFramesUpTo(uint32_t end_idx,
- InterruptionControl allow_interrupt) {
+ InterruptionControl allow_interrupt,
+ std::shared_lock<std::shared_mutex> &guard) {
// Do not fetch frames for an invalid thread.
bool was_interrupted = false;
if (!m_thread.IsValid())
@@ -367,177 +380,193 @@ bool StackFrameList::GetFramesUpTo(uint32_t end_idx,
Unwind &unwinder = m_thread.GetUnwinder();
if (!m_show_inlined_frames) {
- GetOnlyConcreteFramesUpTo(end_idx, unwinder);
+ GetOnlyConcreteFramesUpTo(end_idx, unwinder, guard);
return false;
}
-
-#if defined(DEBUG_STACK_FRAMES)
- StreamFile s(stdout, false);
-#endif
- // If we are hiding some frames from the outside world, we need to add
- // those onto the total count of frames to fetch. However, we don't need
- // to do that if end_idx is 0 since in that case we always get the first
- // concrete frame and all the inlined frames below it... And of course, if
- // end_idx is UINT32_MAX that means get all, so just do that...
-
- uint32_t inlined_depth = 0;
- if (end_idx > 0 && end_idx != UINT32_MAX) {
- inlined_depth = GetCurrentInlinedDepth();
- if (inlined_depth != UINT32_MAX) {
- if (end_idx > 0)
- end_idx += inlined_depth;
+
+ // We're going to have to add frames, so get the writer side of the lock,
+ // and then when we're done, relock the reader side.
+ guard.unlock();
+ { // Scope for switching the writer -> reader and back
+ m_list_mutex.lock();
+ auto on_exit = llvm::make_scope_exit(
+ [&]() {
+ m_list_mutex.unlock();
+ guard.lock();
+ });
+
+ if (m_frames.size() > end_idx || GetAllFramesFetched()) {
+ return false;
+ }
+
+ #if defined(DEBUG_STACK_FRAMES)
+ StreamFile s(stdout, false);
+ #endif
+ // If we are hiding some frames from the outside world, we need to add
+ // those onto the total count of frames to fetch. However, we don't need
+ // to do that if end_idx is 0 since in that case we always get the first
+ // concrete frame and all the inlined frames below it... And of course, if
+ // end_idx is UINT32_MAX that means get all, so just do that...
+
+ uint32_t inlined_depth = 0;
+ if (end_idx > 0 && end_idx != UINT32_MAX) {
+ inlined_depth = GetCurrentInlinedDepth();
+ if (inlined_depth != UINT32_MAX) {
+ if (end_idx > 0)
+ end_idx += inlined_depth;
+ }
}
- }
- StackFrameSP unwind_frame_sp;
- Debugger &dbg = m_thread.GetProcess()->GetTarget().GetDebugger();
- do {
- uint32_t idx = m_concrete_frames_fetched++;
- lldb::addr_t pc = LLDB_INVALID_ADDRESS;
- lldb::addr_t cfa = LLDB_INVALID_ADDRESS;
- bool behaves_like_zeroth_frame = (idx == 0);
- if (idx == 0) {
- // We might have already created frame zero, only create it if we need
- // to.
- if (m_frames.empty()) {
- RegisterContextSP reg_ctx_sp(m_thread.GetRegisterContext());
-
- if (reg_ctx_sp) {
- const bool success = unwinder.GetFrameInfoAtIndex(
- idx, cfa, pc, behaves_like_zeroth_frame);
- // There shouldn't be any way not to get the frame info for frame
- // 0. But if the unwinder can't make one, lets make one by hand
- // with the SP as the CFA and see if that gets any further.
- if (!success) {
- cfa = reg_ctx_sp->GetSP();
- pc = reg_ctx_sp->GetPC();
+ StackFrameSP unwind_frame_sp;
+ Debugger &dbg = m_thread.GetProcess()->GetTarget().GetDebugger();
+ do {
+ uint32_t idx = m_concrete_frames_fetched++;
+ lldb::addr_t pc = LLDB_INVALID_ADDRESS;
+ lldb::addr_t cfa = LLDB_INVALID_ADDRESS;
+ bool behaves_like_zeroth_frame = (idx == 0);
+ if (idx == 0) {
+ // We might have already created frame zero, only create it if we need
+ // to.
+ if (m_frames.empty()) {
+ RegisterContextSP reg_ctx_sp(m_thread.GetRegisterContext());
+
+ if (reg_ctx_sp) {
+ const bool success = unwinder.GetFrameInfoAtIndex(
+ idx, cfa, pc, behaves_like_zeroth_frame);
+ // There shouldn't be any way not to get the frame info for frame
+ // 0. But if the unwinder can't make one, lets make one by hand
+ // with the SP as the CFA and see if that gets any further.
+ if (!success) {
+ cfa = reg_ctx_sp->GetSP();
+ pc = reg_ctx_sp->GetPC();
+ }
+
+ unwind_frame_sp = std::make_shared<StackFrame>(
+ m_thread.shared_from_this(), m_frames.size(), idx, reg_ctx_sp,
+ cfa, pc, behaves_like_zeroth_frame, nullptr);
+ m_frames.push_back(unwind_frame_sp);
}
-
- unwind_frame_sp = std::make_shared<StackFrame>(
- m_thread.shared_from_this(), m_frames.size(), idx, reg_ctx_sp,
- cfa, pc, behaves_like_zeroth_frame, nullptr);
- m_frames.push_back(unwind_frame_sp);
+ } else {
+ unwind_frame_sp = m_frames.front();
+ cfa = unwind_frame_sp->m_id.GetCallFrameAddress();
}
} else {
- unwind_frame_sp = m_frames.front();
- cfa = unwind_frame_sp->m_id.GetCallFrameAddress();
- }
- } 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;
- }
-
- const bool success =
- unwinder.GetFrameInfoAtIndex(idx, cfa, pc, behaves_like_zeroth_frame);
- if (!success) {
- // We've gotten to the end of the stack.
- SetAllFramesFetched();
- break;
- }
- const bool cfa_is_valid = true;
- unwind_frame_sp = std::make_shared<StackFrame>(
- m_thread.shared_from_this(), m_frames.size(), idx, cfa, cfa_is_valid,
- pc, StackFrame::Kind::Regular, behaves_like_zeroth_frame, nullptr);
+ // 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;
+ }
- // Create synthetic tail call frames between the previous frame and the
- // newly-found frame. The new frame's index may change after this call,
- // although its concrete index will stay the same.
- SynthesizeTailCallFrames(*unwind_frame_sp.get());
+ const bool success =
+ unwinder.GetFrameInfoAtIndex(idx, cfa, pc, behaves_like_zeroth_frame);
+ if (!success) {
+ // We've gotten to the end of the stack.
+ SetAllFramesFetched();
+ break;
+ }
+ const bool cfa_is_valid = true;
+ unwind_frame_sp = std::make_shared<StackFrame>(
+ m_thread.shared_from_this(), m_frames.size(), idx, cfa, cfa_is_valid,
+ pc, StackFrame::Kind::Regular, behaves_like_zeroth_frame, nullptr);
- m_frames.push_back(unwind_frame_sp);
- }
+ // Create synthetic tail call frames between the previous frame and the
+ // newly-found frame. The new frame's index may change after this call,
+ // although its concrete index will stay the same.
+ SynthesizeTailCallFrames(*unwind_frame_sp.get());
- assert(unwind_frame_sp);
- SymbolContext unwind_sc = unwind_frame_sp->GetSymbolContext(
- eSymbolContextBlock | eSymbolContextFunction);
- Block *unwind_block = unwind_sc.block;
- TargetSP target_sp = m_thread.CalculateTarget();
- if (unwind_block) {
- Address curr_frame_address(
- unwind_frame_sp->GetFrameCodeAddressForSymbolication());
-
- SymbolContext next_frame_sc;
- Address next_frame_address;
-
- while (unwind_sc.GetParentOfInlinedScope(
- curr_frame_address, next_frame_sc, next_frame_address)) {
- next_frame_sc.line_entry.ApplyFileMappings(target_sp);
- behaves_like_zeroth_frame = false;
- StackFrameSP frame_sp(new StackFrame(
- m_thread.shared_from_this(), m_frames.size(), idx,
- unwind_frame_sp->GetRegisterContextSP(), cfa, next_frame_address,
- behaves_like_zeroth_frame, &next_frame_sc));
-
- m_frames.push_back(frame_sp);
- unwind_sc = next_frame_sc;
- curr_frame_address = next_frame_address;
+ m_frames.push_back(unwind_frame_sp);
}
- }
- } while (m_frames.size() - 1 < end_idx);
- // Don't try to merge till you've calculated all the frames in this stack.
- if (GetAllFramesFetched() && m_prev_frames_sp) {
- StackFrameList *prev_frames = m_prev_frames_sp.get();
- StackFrameList *curr_frames = this;
-
-#if defined(DEBUG_STACK_FRAMES)
- s.PutCString("\nprev_frames:\n");
- prev_frames->Dump(&s);
- s.PutCString("\ncurr_frames:\n");
- curr_frames->Dump(&s);
- s.EOL();
-#endif
- size_t curr_frame_num, prev_frame_num;
-
- for (curr_frame_num = curr_frames->m_frames.size(),
- prev_frame_num = prev_frames->m_frames.size();
- curr_frame_num > 0 && prev_frame_num > 0;
- --curr_frame_num, --prev_frame_num) {
- const size_t curr_frame_idx = curr_frame_num - 1;
- const size_t prev_frame_idx = prev_frame_num - 1;
- StackFrameSP curr_frame_sp(curr_frames->m_frames[curr_frame_idx]);
- StackFrameSP prev_frame_sp(prev_frames->m_frames[prev_frame_idx]);
-
-#if defined(DEBUG_STACK_FRAMES)
- s.Printf("\n\nCurr frame #%u ", curr_frame_idx);
- if (curr_frame_sp)
- curr_frame_sp->Dump(&s, true, false);
- else
- s.PutCString("NULL");
- s.Printf("\nPrev frame #%u ", prev_frame_idx);
- if (prev_frame_sp)
- prev_frame_sp->Dump(&s, true, false);
- else
- s.PutCString("NULL");
-#endif
+ assert(unwind_frame_sp);
+ SymbolContext unwind_sc = unwind_frame_sp->GetSymbolContext(
+ eSymbolContextBlock | eSymbolContextFunction);
+ Block *unwind_block = unwind_sc.block;
+ TargetSP target_sp = m_thread.CalculateTarget();
+ if (unwind_block) {
+ Address curr_frame_address(
+ unwind_frame_sp->GetFrameCodeAddressForSymbolication());
+
+ SymbolContext next_frame_sc;
+ Address next_frame_address;
+
+ while (unwind_sc.GetParentOfInlinedScope(
+ curr_frame_address, next_frame_sc, next_frame_address)) {
+ next_frame_sc.line_entry.ApplyFileMappings(target_sp);
+ behaves_like_zeroth_frame = false;
+ StackFrameSP frame_sp(new StackFrame(
+ m_thread.shared_from_this(), m_frames.size(), idx,
+ unwind_frame_sp->GetRegisterContextSP(), cfa, next_frame_address,
+ behaves_like_zeroth_frame, &next_frame_sc));
+
+ m_frames.push_back(frame_sp);
+ unwind_sc = next_frame_sc;
+ curr_frame_address = next_frame_address;
+ }
+ }
+ } while (m_frames.size() - 1 < end_idx);
+
+ // Don't try to merge till you've calculated all the frames in this stack.
+ if (GetAllFramesFetched() && m_prev_frames_sp) {
+ StackFrameList *prev_frames = m_prev_frames_sp.get();
+ StackFrameList *curr_frames = this;
+
+ #if defined(DEBUG_STACK_FRAMES)
+ s.PutCString("\nprev_frames:\n");
+ prev_frames->Dump(&s);
+ s.PutCString("\ncurr_frames:\n");
+ curr_frames->Dump(&s);
+ s.EOL();
+ #endif
+ size_t curr_frame_num, prev_frame_num;
+
+ for (curr_frame_num = curr_frames->m_frames.size(),
+ prev_frame_num = prev_frames->m_frames.size();
+ curr_frame_num > 0 && prev_frame_num > 0;
+ --curr_frame_num, --prev_frame_num) {
+ const size_t curr_frame_idx = curr_frame_num - 1;
+ const size_t prev_frame_idx = prev_frame_num - 1;
+ StackFrameSP curr_frame_sp(curr_frames->m_frames[curr_frame_idx]);
+ StackFrameSP prev_frame_sp(prev_frames->m_frames[prev_frame_idx]);
+
+ #if defined(DEBUG_STACK_FRAMES)
+ s.Printf("\n\nCurr frame #%u ", curr_frame_idx);
+ if (curr_frame_sp)
+ curr_frame_sp->Dump(&s, true, false);
+ else
+ s.PutCString("NULL");
+ s.Printf("\nPrev frame #%u ", prev_frame_idx);
+ if (prev_frame_sp)
+ prev_frame_sp->Dump(&s, true, false);
+ else
+ s.PutCString("NULL");
+ #endif
- StackFrame *curr_frame = curr_frame_sp.get();
- StackFrame *prev_frame = prev_frame_sp.get();
+ StackFrame *curr_frame = curr_frame_sp.get();
+ StackFrame *prev_frame = prev_frame_sp.get();
- if (curr_frame == nullptr || prev_frame == nullptr)
- break;
+ if (curr_frame == nullptr || prev_frame == nullptr)
+ break;
- // Check the stack ID to make sure they are equal.
- if (curr_frame->GetStackID() != prev_frame->GetStackID())
- break;
+ // Check the stack ID to make sure they are equal.
+ if (curr_frame->GetStackID() != prev_frame->GetStackID())
+ break;
- prev_frame->UpdatePreviousFrameFromCurrentFrame(*curr_frame);
- // Now copy the fixed up previous frame into the current frames so the
- // pointer doesn't change.
- m_frames[curr_frame_idx] = prev_frame_sp;
+ prev_frame->UpdatePreviousFrameFromCurrentFrame(*curr_frame);
+ // Now copy the fixed up previous frame into the current frames so the
+ // pointer doesn't change.
+ m_frames[curr_frame_idx] = prev_frame_sp;
-#if defined(DEBUG_STACK_FRAMES)
- s.Printf("\n Copying previous frame to current frame");
-#endif
+ #if defined(DEBUG_STACK_FRAMES)
+ s.Printf("\n Copying previous frame to current frame");
+ #endif
+ }
+ // We are done with the old stack frame list, we can release it now.
+ m_prev_frames_sp.reset();
}
- // We are done with the old stack frame list, we can release it now.
- m_prev_frames_sp.reset();
- }
+ } // End scope for writer lock
#if defined(DEBUG_STACK_FRAMES)
s.PutCString("\n\nNew frames:\n");
@@ -551,11 +580,11 @@ bool StackFrameList::GetFramesUpTo(uint32_t end_idx,
}
uint32_t StackFrameList::GetNumFrames(bool can_create) {
- std::lock_guard<std::mutex> guard(m_mutex);
+ std::shared_lock<std::shared_mutex> guard(m_list_mutex);
if (can_create) {
// Don't allow interrupt or we might not return the correct count
- GetFramesUpTo(UINT32_MAX, DoNotAllowInterruption);
+ GetFramesUpTo(UINT32_MAX, DoNotAllowInterruption, guard);
}
return GetVisibleStackFrameIndex(m_frames.size());
}
@@ -564,7 +593,7 @@ void StackFrameList::Dump(Stream *s) {
if (s == nullptr)
return;
- std::lock_guard<std::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) {
@@ -581,11 +610,12 @@ void StackFrameList::Dump(Stream *s) {
}
StackFrameSP StackFrameList::GetFrameAtIndex(uint32_t idx) {
- std::lock_guard<std::mutex> guard(m_mutex);
- return GetFrameAtIndexNoLock(idx);
+ std::shared_lock<std::shared_mutex> guard(m_list_mutex);
+ return GetFrameAtIndexNoLock(idx, guard);
}
-StackFrameSP StackFrameList::GetFrameAtIndexNoLock(uint32_t idx) {
+StackFrameSP StackFrameList::GetFrameAtIndexNoLock(uint32_t idx,
+ std::shared_lock<std::shared_mutex> &guard) {
StackFrameSP frame_sp;
uint32_t original_idx = idx;
@@ -602,7 +632,7 @@ StackFrameSP StackFrameList::GetFrameAtIndexNoLock(uint32_t idx) {
// 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, guard)) {
Log *log = GetLog(LLDBLog::Thread);
LLDB_LOG(log, "GetFrameAtIndex was interrupted");
return {};
@@ -665,12 +695,12 @@ StackFrameList::GetFrameWithConcreteFrameIndex(uint32_t unwind_idx) {
// after we make all the inlined frames. Most of the time the unwind frame
// index (or the concrete frame index) is the same as the frame index.
uint32_t frame_idx = unwind_idx;
- std::lock_guard<std::mutex> guard(m_mutex);
- StackFrameSP frame_sp(GetFrameAtIndexNoLock(frame_idx));
+ std::shared_lock<std::shared_mutex> guard(m_list_mutex);
+ StackFrameSP frame_sp(GetFrameAtIndexNoLock(frame_idx, guard));
while (frame_sp) {
if (frame_sp->GetFrameIndex() == unwind_idx)
break;
- frame_sp = GetFrameAtIndexNoLock(++frame_idx);
+ frame_sp = GetFrameAtIndexNoLock(++frame_idx, guard);
}
return frame_sp;
}
@@ -684,7 +714,7 @@ StackFrameSP StackFrameList::GetFrameWithStackID(const StackID &stack_id) {
StackFrameSP frame_sp;
if (stack_id.IsValid()) {
- std::lock_guard<std::mutex> guard(m_mutex);
+ std::shared_lock<std::shared_mutex> guard(m_list_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();
@@ -698,7 +728,7 @@ StackFrameSP StackFrameList::GetFrameWithStackID(const StackID &stack_id) {
}
}
do {
- frame_sp = GetFrameAtIndexNoLock(frame_idx);
+ frame_sp = GetFrameAtIndexNoLock(frame_idx, guard);
if (frame_sp && frame_sp->GetStackID() == stack_id)
break;
frame_idx++;
@@ -785,7 +815,7 @@ uint32_t StackFrameList::GetSelectedFrameIndex(
if (!m_selected_frame_idx && select_most_relevant)
SelectMostRelevantFrame();
{ // Scope for lock guard
- std::lock_guard<std::mutex> guard(m_mutex);
+ std::shared_lock<std::shared_mutex> guard(m_list_mutex);
if (!m_selected_frame_idx) {
// If we aren't selecting the most relevant frame, and the selected frame
// isn't set, then don't force a selection here, just return 0.
@@ -801,7 +831,7 @@ uint32_t StackFrameList::GetSelectedFrameIndex(
uint32_t StackFrameList::SetSelectedFrame(lldb_private::StackFrame *frame) {
uint32_t result = 0;
{
- std::lock_guard<std::mutex> guard(m_mutex);
+ std::shared_lock<std::shared_mutex> guard(m_list_mutex);
result = SetSelectedFrameNoLock(frame);
}
SetDefaultFileAndLineToSelectedFrame();
@@ -861,7 +891,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::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();
@@ -869,7 +899,7 @@ void StackFrameList::Clear() {
lldb::StackFrameSP
StackFrameList::GetStackFrameSPForStackFramePtr(StackFrame *stack_frame_ptr) {
- std::lock_guard<std::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();
diff --git a/lldb/test/API/api/multithreaded/TestMultithreaded.py b/lldb/test/API/api/multithreaded/TestMultithreaded.py
index 07c9f5b9bbcca3..3c0c6547ead467 100644
--- a/lldb/test/API/api/multithreaded/TestMultithreaded.py
+++ b/lldb/test/API/api/multithreaded/TestMultithreaded.py
@@ -22,7 +22,8 @@ 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)
@skipIfWindows
@@ -91,7 +92,15 @@ 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 +113,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");
+}
>From b3042320b79e2d1666778798103a0e1493cc8cf2 Mon Sep 17 00:00:00 2001
From: Jim Ingham <jingham at apple.com>
Date: Thu, 21 Nov 2024 15:05:02 -0800
Subject: [PATCH 3/7] Formatting.
---
lldb/include/lldb/Target/StackFrameList.h | 22 ++--
lldb/source/Target/StackFrameList.cpp | 111 +++++++++---------
.../api/multithreaded/TestMultithreaded.py | 8 +-
3 files changed, 72 insertions(+), 69 deletions(-)
diff --git a/lldb/include/lldb/Target/StackFrameList.h b/lldb/include/lldb/Target/StackFrameList.h
index 8f6f6500f1ef96..1a05b799813f40 100644
--- a/lldb/include/lldb/Target/StackFrameList.h
+++ b/lldb/include/lldb/Target/StackFrameList.h
@@ -101,21 +101,20 @@ class StackFrameList {
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.)
+ /// Realizes frames up to (and including) end_idx (which can be greater than
+ /// the actual number of frames.)
/// Returns true if the function was interrupted, false otherwise.
/// Must be called with a shared_locked mutex. This might unlock the
- /// shared side if it needs to fetch more frames, but will reacquire the
+ /// shared side if it needs to fetch more frames, but will reacquire the
/// shared lock on the way out.
- bool GetFramesUpTo(uint32_t end_idx,
- InterruptionControl allow_interrupt,
- std::shared_lock<std::shared_mutex> &guard);
+ bool GetFramesUpTo(uint32_t end_idx, InterruptionControl allow_interrupt,
+ std::shared_lock<std::shared_mutex> &guard);
/// Does not hold the StackFrameList mutex. Same comment as GetFramesUpTo.
void GetOnlyConcreteFramesUpTo(uint32_t end_idx, Unwind &unwinder,
- std::shared_lock<std::shared_mutex> &guard);
+ std::shared_lock<std::shared_mutex> &guard);
- // This gets called without the StackFrameList lock held, callers should
+ // This gets called without the StackFrameList lock held, callers should
// hold the lock.
void SynthesizeTailCallFrames(StackFrame &next_frame);
@@ -155,7 +154,7 @@ class StackFrameList {
/// 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.
@@ -189,8 +188,9 @@ class StackFrameList {
private:
uint32_t SetSelectedFrameNoLock(lldb_private::StackFrame *frame);
- lldb::StackFrameSP GetFrameAtIndexNoLock(uint32_t idx,
- std::shared_lock<std::shared_mutex> &guard);
+ lldb::StackFrameSP
+ GetFrameAtIndexNoLock(uint32_t idx,
+ std::shared_lock<std::shared_mutex> &guard);
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 0dc4fa7797c58b..a35b41a4486008 100644
--- a/lldb/source/Target/StackFrameList.cpp
+++ b/lldb/source/Target/StackFrameList.cpp
@@ -138,9 +138,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,
- std::shared_lock<std::shared_mutex> &guard) {
+void StackFrameList::GetOnlyConcreteFramesUpTo(
+ uint32_t end_idx, Unwind &unwinder,
+ std::shared_lock<std::shared_mutex> &guard) {
assert(m_thread.IsValid() && "Expected valid thread");
assert(m_frames.size() <= end_idx && "Expected there to be frames to fill");
@@ -148,11 +148,10 @@ void StackFrameList::GetOnlyConcreteFramesUpTo(uint32_t end_idx,
return;
{ // Scope for swapping reader and writer locks
m_list_mutex.lock();
- auto on_exit = llvm::make_scope_exit(
- [&]() {
- m_list_mutex.unlock();
- guard.lock();
- });
+ auto on_exit = llvm::make_scope_exit([&]() {
+ m_list_mutex.unlock();
+ guard.lock();
+ });
if (end_idx < m_concrete_frames_fetched)
return;
@@ -181,7 +180,7 @@ 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
+/// 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,
@@ -366,7 +365,7 @@ void StackFrameList::SynthesizeTailCallFrames(StackFrame &next_frame) {
bool StackFrameList::GetFramesUpTo(uint32_t end_idx,
InterruptionControl allow_interrupt,
- std::shared_lock<std::shared_mutex> &guard) {
+ std::shared_lock<std::shared_mutex> &guard) {
// Do not fetch frames for an invalid thread.
bool was_interrupted = false;
if (!m_thread.IsValid())
@@ -383,25 +382,24 @@ bool StackFrameList::GetFramesUpTo(uint32_t end_idx,
GetOnlyConcreteFramesUpTo(end_idx, unwinder, guard);
return false;
}
-
+
// We're going to have to add frames, so get the writer side of the lock,
// and then when we're done, relock the reader side.
guard.unlock();
{ // Scope for switching the writer -> reader and back
m_list_mutex.lock();
- auto on_exit = llvm::make_scope_exit(
- [&]() {
- m_list_mutex.unlock();
- guard.lock();
- });
+ auto on_exit = llvm::make_scope_exit([&]() {
+ m_list_mutex.unlock();
+ guard.lock();
+ });
if (m_frames.size() > end_idx || GetAllFramesFetched()) {
return false;
}
-
- #if defined(DEBUG_STACK_FRAMES)
+
+#if defined(DEBUG_STACK_FRAMES)
StreamFile s(stdout, false);
- #endif
+#endif
// If we are hiding some frames from the outside world, we need to add
// those onto the total count of frames to fetch. However, we don't need
// to do that if end_idx is 0 since in that case we always get the first
@@ -453,15 +451,15 @@ 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 =
- unwinder.GetFrameInfoAtIndex(idx, cfa, pc, behaves_like_zeroth_frame);
+ const bool success = unwinder.GetFrameInfoAtIndex(
+ idx, cfa, pc, behaves_like_zeroth_frame);
if (!success) {
// We've gotten to the end of the stack.
SetAllFramesFetched();
@@ -469,8 +467,9 @@ bool StackFrameList::GetFramesUpTo(uint32_t end_idx,
}
const bool cfa_is_valid = true;
unwind_frame_sp = std::make_shared<StackFrame>(
- m_thread.shared_from_this(), m_frames.size(), idx, cfa, cfa_is_valid,
- pc, StackFrame::Kind::Regular, behaves_like_zeroth_frame, nullptr);
+ m_thread.shared_from_this(), m_frames.size(), idx, cfa,
+ cfa_is_valid, pc, StackFrame::Kind::Regular,
+ behaves_like_zeroth_frame, nullptr);
// Create synthetic tail call frames between the previous frame and the
// newly-found frame. The new frame's index may change after this call,
@@ -513,13 +512,13 @@ bool StackFrameList::GetFramesUpTo(uint32_t end_idx,
StackFrameList *prev_frames = m_prev_frames_sp.get();
StackFrameList *curr_frames = this;
- #if defined(DEBUG_STACK_FRAMES)
+#if defined(DEBUG_STACK_FRAMES)
s.PutCString("\nprev_frames:\n");
prev_frames->Dump(&s);
s.PutCString("\ncurr_frames:\n");
curr_frames->Dump(&s);
s.EOL();
- #endif
+#endif
size_t curr_frame_num, prev_frame_num;
for (curr_frame_num = curr_frames->m_frames.size(),
@@ -531,7 +530,7 @@ bool StackFrameList::GetFramesUpTo(uint32_t end_idx,
StackFrameSP curr_frame_sp(curr_frames->m_frames[curr_frame_idx]);
StackFrameSP prev_frame_sp(prev_frames->m_frames[prev_frame_idx]);
- #if defined(DEBUG_STACK_FRAMES)
+#if defined(DEBUG_STACK_FRAMES)
s.Printf("\n\nCurr frame #%u ", curr_frame_idx);
if (curr_frame_sp)
curr_frame_sp->Dump(&s, true, false);
@@ -542,7 +541,7 @@ bool StackFrameList::GetFramesUpTo(uint32_t end_idx,
prev_frame_sp->Dump(&s, true, false);
else
s.PutCString("NULL");
- #endif
+#endif
StackFrame *curr_frame = curr_frame_sp.get();
StackFrame *prev_frame = prev_frame_sp.get();
@@ -559,9 +558,9 @@ bool StackFrameList::GetFramesUpTo(uint32_t end_idx,
// pointer doesn't change.
m_frames[curr_frame_idx] = prev_frame_sp;
- #if defined(DEBUG_STACK_FRAMES)
+#if defined(DEBUG_STACK_FRAMES)
s.Printf("\n Copying previous frame to current frame");
- #endif
+#endif
}
// We are done with the old stack frame list, we can release it now.
m_prev_frames_sp.reset();
@@ -584,7 +583,7 @@ uint32_t StackFrameList::GetNumFrames(bool can_create) {
if (can_create) {
// Don't allow interrupt or we might not return the correct count
- GetFramesUpTo(UINT32_MAX, DoNotAllowInterruption, guard);
+ GetFramesUpTo(UINT32_MAX, DoNotAllowInterruption, guard);
}
return GetVisibleStackFrameIndex(m_frames.size());
}
@@ -614,8 +613,8 @@ StackFrameSP StackFrameList::GetFrameAtIndex(uint32_t idx) {
return GetFrameAtIndexNoLock(idx, guard);
}
-StackFrameSP StackFrameList::GetFrameAtIndexNoLock(uint32_t idx,
- std::shared_lock<std::shared_mutex> &guard) {
+StackFrameSP StackFrameList::GetFrameAtIndexNoLock(
+ uint32_t idx, std::shared_lock<std::shared_mutex> &guard) {
StackFrameSP frame_sp;
uint32_t original_idx = idx;
@@ -810,8 +809,8 @@ void StackFrameList::SelectMostRelevantFrame() {
LLDB_LOG(log, "No relevant frame!");
}
-uint32_t StackFrameList::GetSelectedFrameIndex(
- SelectMostRelevant select_most_relevant) {
+uint32_t
+StackFrameList::GetSelectedFrameIndex(SelectMostRelevant select_most_relevant) {
if (!m_selected_frame_idx && select_most_relevant)
SelectMostRelevantFrame();
{ // Scope for lock guard
@@ -838,25 +837,25 @@ uint32_t StackFrameList::SetSelectedFrame(lldb_private::StackFrame *frame) {
return result;
}
-uint32_t StackFrameList::SetSelectedFrameNoLock(lldb_private::StackFrame *frame)
-{
+uint32_t
+StackFrameList::SetSelectedFrameNoLock(lldb_private::StackFrame *frame) {
uint32_t result = 0;
-
- const_iterator pos;
- 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 = *m_selected_frame_idx - inlined_depth;
- break;
- }
+
+ const_iterator pos;
+ 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 = *m_selected_frame_idx - inlined_depth;
+ break;
}
- result = *m_selected_frame_idx;
+ }
+ result = *m_selected_frame_idx;
return result;
}
diff --git a/lldb/test/API/api/multithreaded/TestMultithreaded.py b/lldb/test/API/api/multithreaded/TestMultithreaded.py
index 3c0c6547ead467..d5b29ec7af1818 100644
--- a/lldb/test/API/api/multithreaded/TestMultithreaded.py
+++ b/lldb/test/API/api/multithreaded/TestMultithreaded.py
@@ -23,7 +23,7 @@ def setUp(self):
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)
@skipIfWindows
@@ -98,7 +98,11 @@ def test_sb_api_listener_resume(self):
@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")
+ 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"""
>From a6f21a171ab55d3bfe0c171a8c04865a2af3989c Mon Sep 17 00:00:00 2001
From: Jim Ingham <jingham at apple.com>
Date: Fri, 6 Dec 2024 17:41:40 -0800
Subject: [PATCH 4/7] Refactor the second half of GetFramesUpTo (the part that
required the exclusive mutex) into a separate function for clarity.
---
lldb/include/lldb/Target/StackFrameList.h | 17 +-
lldb/source/Target/StackFrameList.cpp | 404 +++++++++++-----------
2 files changed, 219 insertions(+), 202 deletions(-)
diff --git a/lldb/include/lldb/Target/StackFrameList.h b/lldb/include/lldb/Target/StackFrameList.h
index 1a05b799813f40..f7905c7f732622 100644
--- a/lldb/include/lldb/Target/StackFrameList.h
+++ b/lldb/include/lldb/Target/StackFrameList.h
@@ -104,15 +104,20 @@ class StackFrameList {
/// Realizes frames up to (and including) end_idx (which can be greater than
/// the actual number of frames.)
/// Returns true if the function was interrupted, false otherwise.
- /// Must be called with a shared_locked mutex. This might unlock the
- /// shared side if it needs to fetch more frames, but will reacquire the
- /// shared lock on the way out.
+ /// Must be called with a shared_locked mutex locked.
bool GetFramesUpTo(uint32_t end_idx, InterruptionControl allow_interrupt,
std::shared_lock<std::shared_mutex> &guard);
- /// Does not hold the StackFrameList mutex. Same comment as GetFramesUpTo.
- void GetOnlyConcreteFramesUpTo(uint32_t end_idx, Unwind &unwinder,
- std::shared_lock<std::shared_mutex> &guard);
+ /// These two Fetch frames APIs must be called with the stack mutex shared
+ /// lock acquired. They are the only places where we acquire the writer
+ /// end of the stack list mutex to add frames, but they will always exit with
+ /// the shared side reacquired.
+ /// Returns true if fetching frames was interrupted, false otherwise
+ bool FetchFramesUpTo(uint32_t end_idx, InterruptionControl allow_interrupt,
+ std::shared_lock<std::shared_mutex> &guard);
+
+ void FetchOnlyConcreteFramesUpTo(uint32_t end_idx,
+ std::shared_lock<std::shared_mutex> &guard);
// This gets called without the StackFrameList lock held, callers should
// hold the lock.
diff --git a/lldb/source/Target/StackFrameList.cpp b/lldb/source/Target/StackFrameList.cpp
index a35b41a4486008..ba7220e273af6c 100644
--- a/lldb/source/Target/StackFrameList.cpp
+++ b/lldb/source/Target/StackFrameList.cpp
@@ -138,35 +138,6 @@ void StackFrameList::SetCurrentInlinedDepth(uint32_t new_depth) {
m_current_inlined_pc = m_thread.GetRegisterContext()->GetPC();
}
-void StackFrameList::GetOnlyConcreteFramesUpTo(
- uint32_t end_idx, Unwind &unwinder,
- std::shared_lock<std::shared_mutex> &guard) {
- 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;
- { // Scope for swapping reader and writer locks
- m_list_mutex.lock();
- auto on_exit = llvm::make_scope_exit([&]() {
- m_list_mutex.unlock();
- guard.lock();
- });
- 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);
- }
-}
-
/// A sequence of calls that comprise some portion of a backtrace. Each frame
/// is represented as a pair of a callee (Function *) and an address within the
/// callee.
@@ -376,206 +347,247 @@ bool StackFrameList::GetFramesUpTo(uint32_t end_idx,
if (m_frames.size() > end_idx || GetAllFramesFetched())
return false;
- Unwind &unwinder = m_thread.GetUnwinder();
-
if (!m_show_inlined_frames) {
- GetOnlyConcreteFramesUpTo(end_idx, unwinder, guard);
+ if (end_idx < m_concrete_frames_fetched)
+ return false;
+ // We're adding concrete frames now:
+ // FIXME: This should also be interruptible:
+ FetchOnlyConcreteFramesUpTo(end_idx, guard);
return false;
}
- // We're going to have to add frames, so get the writer side of the lock,
- // and then when we're done, relock the reader side.
+ // We're adding concrete and inlined frames now:
+ was_interrupted = FetchFramesUpTo(end_idx, allow_interrupt, guard);
+
+#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;
+ return false;
+
+}
+
+void StackFrameList::FetchOnlyConcreteFramesUpTo(
+ uint32_t end_idx, std::shared_lock<std::shared_mutex> &guard) {
+ assert(m_thread.IsValid() && "Expected valid thread");
+ assert(m_frames.size() <= end_idx && "Expected there to be frames to fill");
+ assert(guard.owns_lock() && "Must be called with the shared lock acquired");
+
+ Unwind &unwinder = m_thread.GetUnwinder();
+
guard.unlock();
- { // Scope for switching the writer -> reader and back
- m_list_mutex.lock();
- auto on_exit = llvm::make_scope_exit([&]() {
- m_list_mutex.unlock();
- guard.lock();
- });
-
- if (m_frames.size() > end_idx || GetAllFramesFetched()) {
- return false;
- }
+ m_list_mutex.lock();
+ auto on_exit = llvm::make_scope_exit([&]() {
+ m_list_mutex.unlock();
+ guard.lock();
+ });
+ 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,
+ std::shared_lock<std::shared_mutex> &guard) {
+ assert(guard.owns_lock() && "Must be called with the shared lock acquired");
+
+ Unwind &unwinder = m_thread.GetUnwinder();
+ bool was_interrupted = false;
+
+ guard.unlock();
+ m_list_mutex.lock();
+ auto on_exit = llvm::make_scope_exit([&]() {
+ m_list_mutex.unlock();
+ guard.lock();
+ });
+
+ if (m_frames.size() > end_idx || GetAllFramesFetched()) {
+ return false;
+ }
#if defined(DEBUG_STACK_FRAMES)
- StreamFile s(stdout, false);
+ StreamFile s(stdout, false);
#endif
- // If we are hiding some frames from the outside world, we need to add
- // those onto the total count of frames to fetch. However, we don't need
- // to do that if end_idx is 0 since in that case we always get the first
- // concrete frame and all the inlined frames below it... And of course, if
- // end_idx is UINT32_MAX that means get all, so just do that...
-
- uint32_t inlined_depth = 0;
- if (end_idx > 0 && end_idx != UINT32_MAX) {
- inlined_depth = GetCurrentInlinedDepth();
- if (inlined_depth != UINT32_MAX) {
- if (end_idx > 0)
- end_idx += inlined_depth;
- }
+ // If we are hiding some frames from the outside world, we need to add
+ // those onto the total count of frames to fetch. However, we don't need
+ // to do that if end_idx is 0 since in that case we always get the first
+ // concrete frame and all the inlined frames below it... And of course, if
+ // end_idx is UINT32_MAX that means get all, so just do that...
+
+ uint32_t inlined_depth = 0;
+ if (end_idx > 0 && end_idx != UINT32_MAX) {
+ inlined_depth = GetCurrentInlinedDepth();
+ if (inlined_depth != UINT32_MAX) {
+ if (end_idx > 0)
+ end_idx += inlined_depth;
}
+ }
- StackFrameSP unwind_frame_sp;
- Debugger &dbg = m_thread.GetProcess()->GetTarget().GetDebugger();
- do {
- uint32_t idx = m_concrete_frames_fetched++;
- lldb::addr_t pc = LLDB_INVALID_ADDRESS;
- lldb::addr_t cfa = LLDB_INVALID_ADDRESS;
- bool behaves_like_zeroth_frame = (idx == 0);
- if (idx == 0) {
- // We might have already created frame zero, only create it if we need
- // to.
- if (m_frames.empty()) {
- RegisterContextSP reg_ctx_sp(m_thread.GetRegisterContext());
-
- if (reg_ctx_sp) {
- const bool success = unwinder.GetFrameInfoAtIndex(
- idx, cfa, pc, behaves_like_zeroth_frame);
- // There shouldn't be any way not to get the frame info for frame
- // 0. But if the unwinder can't make one, lets make one by hand
- // with the SP as the CFA and see if that gets any further.
- if (!success) {
- cfa = reg_ctx_sp->GetSP();
- pc = reg_ctx_sp->GetPC();
- }
-
- unwind_frame_sp = std::make_shared<StackFrame>(
- m_thread.shared_from_this(), m_frames.size(), idx, reg_ctx_sp,
- cfa, pc, behaves_like_zeroth_frame, nullptr);
- m_frames.push_back(unwind_frame_sp);
+ StackFrameSP unwind_frame_sp;
+ Debugger &dbg = m_thread.GetProcess()->GetTarget().GetDebugger();
+ do {
+ uint32_t idx = m_concrete_frames_fetched++;
+ lldb::addr_t pc = LLDB_INVALID_ADDRESS;
+ lldb::addr_t cfa = LLDB_INVALID_ADDRESS;
+ bool behaves_like_zeroth_frame = (idx == 0);
+ if (idx == 0) {
+ // We might have already created frame zero, only create it if we need
+ // to.
+ if (m_frames.empty()) {
+ RegisterContextSP reg_ctx_sp(m_thread.GetRegisterContext());
+
+ if (reg_ctx_sp) {
+ const bool success = unwinder.GetFrameInfoAtIndex(
+ idx, cfa, pc, behaves_like_zeroth_frame);
+ // There shouldn't be any way not to get the frame info for frame
+ // 0. But if the unwinder can't make one, lets make one by hand
+ // with the SP as the CFA and see if that gets any further.
+ if (!success) {
+ cfa = reg_ctx_sp->GetSP();
+ pc = reg_ctx_sp->GetPC();
}
- } else {
- unwind_frame_sp = m_frames.front();
- cfa = unwind_frame_sp->m_id.GetCallFrameAddress();
- }
- } 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;
- }
- const bool success = unwinder.GetFrameInfoAtIndex(
- idx, cfa, pc, behaves_like_zeroth_frame);
- if (!success) {
- // We've gotten to the end of the stack.
- SetAllFramesFetched();
- break;
+ unwind_frame_sp = std::make_shared<StackFrame>(
+ m_thread.shared_from_this(), m_frames.size(), idx, reg_ctx_sp,
+ cfa, pc, behaves_like_zeroth_frame, nullptr);
+ m_frames.push_back(unwind_frame_sp);
}
- const bool cfa_is_valid = true;
- unwind_frame_sp = std::make_shared<StackFrame>(
- m_thread.shared_from_this(), m_frames.size(), idx, cfa,
- cfa_is_valid, pc, StackFrame::Kind::Regular,
- behaves_like_zeroth_frame, nullptr);
-
- // Create synthetic tail call frames between the previous frame and the
- // newly-found frame. The new frame's index may change after this call,
- // although its concrete index will stay the same.
- SynthesizeTailCallFrames(*unwind_frame_sp.get());
+ } else {
+ unwind_frame_sp = m_frames.front();
+ cfa = unwind_frame_sp->m_id.GetCallFrameAddress();
+ }
+ } 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;
+ }
- m_frames.push_back(unwind_frame_sp);
+ const bool success = unwinder.GetFrameInfoAtIndex(
+ idx, cfa, pc, behaves_like_zeroth_frame);
+ if (!success) {
+ // We've gotten to the end of the stack.
+ SetAllFramesFetched();
+ break;
}
+ const bool cfa_is_valid = true;
+ unwind_frame_sp = std::make_shared<StackFrame>(
+ m_thread.shared_from_this(), m_frames.size(), idx, cfa,
+ cfa_is_valid, pc, StackFrame::Kind::Regular,
+ behaves_like_zeroth_frame, nullptr);
+
+ // Create synthetic tail call frames between the previous frame and the
+ // newly-found frame. The new frame's index may change after this call,
+ // although its concrete index will stay the same.
+ SynthesizeTailCallFrames(*unwind_frame_sp.get());
+
+ m_frames.push_back(unwind_frame_sp);
+ }
- assert(unwind_frame_sp);
- SymbolContext unwind_sc = unwind_frame_sp->GetSymbolContext(
- eSymbolContextBlock | eSymbolContextFunction);
- Block *unwind_block = unwind_sc.block;
- TargetSP target_sp = m_thread.CalculateTarget();
- if (unwind_block) {
- Address curr_frame_address(
- unwind_frame_sp->GetFrameCodeAddressForSymbolication());
-
- SymbolContext next_frame_sc;
- Address next_frame_address;
-
- while (unwind_sc.GetParentOfInlinedScope(
- curr_frame_address, next_frame_sc, next_frame_address)) {
- next_frame_sc.line_entry.ApplyFileMappings(target_sp);
- behaves_like_zeroth_frame = false;
- StackFrameSP frame_sp(new StackFrame(
- m_thread.shared_from_this(), m_frames.size(), idx,
- unwind_frame_sp->GetRegisterContextSP(), cfa, next_frame_address,
- behaves_like_zeroth_frame, &next_frame_sc));
-
- m_frames.push_back(frame_sp);
- unwind_sc = next_frame_sc;
- curr_frame_address = next_frame_address;
- }
+ assert(unwind_frame_sp);
+ SymbolContext unwind_sc = unwind_frame_sp->GetSymbolContext(
+ eSymbolContextBlock | eSymbolContextFunction);
+ Block *unwind_block = unwind_sc.block;
+ TargetSP target_sp = m_thread.CalculateTarget();
+ if (unwind_block) {
+ Address curr_frame_address(
+ unwind_frame_sp->GetFrameCodeAddressForSymbolication());
+
+ SymbolContext next_frame_sc;
+ Address next_frame_address;
+
+ while (unwind_sc.GetParentOfInlinedScope(
+ curr_frame_address, next_frame_sc, next_frame_address)) {
+ next_frame_sc.line_entry.ApplyFileMappings(target_sp);
+ behaves_like_zeroth_frame = false;
+ StackFrameSP frame_sp(new StackFrame(
+ m_thread.shared_from_this(), m_frames.size(), idx,
+ unwind_frame_sp->GetRegisterContextSP(), cfa, next_frame_address,
+ behaves_like_zeroth_frame, &next_frame_sc));
+
+ m_frames.push_back(frame_sp);
+ unwind_sc = next_frame_sc;
+ curr_frame_address = next_frame_address;
}
- } while (m_frames.size() - 1 < end_idx);
+ }
+ } while (m_frames.size() - 1 < end_idx);
- // Don't try to merge till you've calculated all the frames in this stack.
- if (GetAllFramesFetched() && m_prev_frames_sp) {
- StackFrameList *prev_frames = m_prev_frames_sp.get();
- StackFrameList *curr_frames = this;
+ // Don't try to merge till you've calculated all the frames in this stack.
+ if (GetAllFramesFetched() && m_prev_frames_sp) {
+ StackFrameList *prev_frames = m_prev_frames_sp.get();
+ StackFrameList *curr_frames = this;
#if defined(DEBUG_STACK_FRAMES)
- s.PutCString("\nprev_frames:\n");
- prev_frames->Dump(&s);
- s.PutCString("\ncurr_frames:\n");
- curr_frames->Dump(&s);
- s.EOL();
+ s.PutCString("\nprev_frames:\n");
+ prev_frames->Dump(&s);
+ s.PutCString("\ncurr_frames:\n");
+ curr_frames->Dump(&s);
+ s.EOL();
#endif
- size_t curr_frame_num, prev_frame_num;
+ size_t curr_frame_num, prev_frame_num;
- for (curr_frame_num = curr_frames->m_frames.size(),
- prev_frame_num = prev_frames->m_frames.size();
- curr_frame_num > 0 && prev_frame_num > 0;
- --curr_frame_num, --prev_frame_num) {
- const size_t curr_frame_idx = curr_frame_num - 1;
- const size_t prev_frame_idx = prev_frame_num - 1;
- StackFrameSP curr_frame_sp(curr_frames->m_frames[curr_frame_idx]);
- StackFrameSP prev_frame_sp(prev_frames->m_frames[prev_frame_idx]);
+ for (curr_frame_num = curr_frames->m_frames.size(),
+ prev_frame_num = prev_frames->m_frames.size();
+ curr_frame_num > 0 && prev_frame_num > 0;
+ --curr_frame_num, --prev_frame_num) {
+ const size_t curr_frame_idx = curr_frame_num - 1;
+ const size_t prev_frame_idx = prev_frame_num - 1;
+ StackFrameSP curr_frame_sp(curr_frames->m_frames[curr_frame_idx]);
+ StackFrameSP prev_frame_sp(prev_frames->m_frames[prev_frame_idx]);
#if defined(DEBUG_STACK_FRAMES)
- s.Printf("\n\nCurr frame #%u ", curr_frame_idx);
- if (curr_frame_sp)
- curr_frame_sp->Dump(&s, true, false);
- else
- s.PutCString("NULL");
- s.Printf("\nPrev frame #%u ", prev_frame_idx);
- if (prev_frame_sp)
- prev_frame_sp->Dump(&s, true, false);
- else
- s.PutCString("NULL");
+ s.Printf("\n\nCurr frame #%u ", curr_frame_idx);
+ if (curr_frame_sp)
+ curr_frame_sp->Dump(&s, true, false);
+ else
+ s.PutCString("NULL");
+ s.Printf("\nPrev frame #%u ", prev_frame_idx);
+ if (prev_frame_sp)
+ prev_frame_sp->Dump(&s, true, false);
+ else
+ s.PutCString("NULL");
#endif
- StackFrame *curr_frame = curr_frame_sp.get();
- StackFrame *prev_frame = prev_frame_sp.get();
+ StackFrame *curr_frame = curr_frame_sp.get();
+ StackFrame *prev_frame = prev_frame_sp.get();
- if (curr_frame == nullptr || prev_frame == nullptr)
- break;
+ if (curr_frame == nullptr || prev_frame == nullptr)
+ break;
- // Check the stack ID to make sure they are equal.
- if (curr_frame->GetStackID() != prev_frame->GetStackID())
- break;
+ // Check the stack ID to make sure they are equal.
+ if (curr_frame->GetStackID() != prev_frame->GetStackID())
+ break;
- prev_frame->UpdatePreviousFrameFromCurrentFrame(*curr_frame);
- // Now copy the fixed up previous frame into the current frames so the
- // pointer doesn't change.
- m_frames[curr_frame_idx] = prev_frame_sp;
+ prev_frame->UpdatePreviousFrameFromCurrentFrame(*curr_frame);
+ // Now copy the fixed up previous frame into the current frames so the
+ // pointer doesn't change.
+ m_frames[curr_frame_idx] = prev_frame_sp;
#if defined(DEBUG_STACK_FRAMES)
- s.Printf("\n Copying previous frame to current frame");
+ s.Printf("\n Copying previous frame to current frame");
#endif
- }
- // We are done with the old stack frame list, we can release it now.
- m_prev_frames_sp.reset();
}
- } // End scope for writer lock
-
-#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;
- return false;
+ // We are done with the old stack frame list, we can release it now.
+ m_prev_frames_sp.reset();
+ }
+ return was_interrupted;
}
uint32_t StackFrameList::GetNumFrames(bool can_create) {
>From daa9968af442e0c23c07f7fa7a0d05e34bf92829 Mon Sep 17 00:00:00 2001
From: Jim Ingham <jingham at apple.com>
Date: Mon, 9 Dec 2024 10:45:58 -0800
Subject: [PATCH 5/7] Remove code that tried to set concrete frames after
GetFramesUpTo.
That code was wrong, it was prefaced by the comment that only when
we get inlined frames does GetFramesUpTo fetch the needed frames, but
that's not true.
---
lldb/source/Target/StackFrameList.cpp | 28 ---------------------------
1 file changed, 28 deletions(-)
diff --git a/lldb/source/Target/StackFrameList.cpp b/lldb/source/Target/StackFrameList.cpp
index ba7220e273af6c..9466cf080823b3 100644
--- a/lldb/source/Target/StackFrameList.cpp
+++ b/lldb/source/Target/StackFrameList.cpp
@@ -650,35 +650,7 @@ StackFrameSP StackFrameList::GetFrameAtIndexNoLock(
}
if (idx < m_frames.size()) {
- if (m_show_inlined_frames) {
- // When inline frames are enabled we actually create all the frames in
- // GetFramesUpTo.
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
>From 3236978b2b7dd035b0556ee9ac83bdcd62184559 Mon Sep 17 00:00:00 2001
From: Jim Ingham <jingham at apple.com>
Date: Mon, 9 Dec 2024 15:47:18 -0800
Subject: [PATCH 6/7] Move some functions to private where appropriate, and add
a few comments explaining how this is to be used.
---
lldb/include/lldb/Target/StackFrameList.h | 44 +++++++++++++----------
lldb/source/Target/StackFrameList.cpp | 5 +--
2 files changed, 27 insertions(+), 22 deletions(-)
diff --git a/lldb/include/lldb/Target/StackFrameList.h b/lldb/include/lldb/Target/StackFrameList.h
index f7905c7f732622..a0c4506b8ba6eb 100644
--- a/lldb/include/lldb/Target/StackFrameList.h
+++ b/lldb/include/lldb/Target/StackFrameList.h
@@ -99,30 +99,21 @@ class StackFrameList {
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.
/// Returns true if the function was interrupted, false otherwise.
- /// Must be called with a shared_locked mutex locked.
+ /// Must be called with a shared mutex locked in `guard`.
bool GetFramesUpTo(uint32_t end_idx, InterruptionControl allow_interrupt,
std::shared_lock<std::shared_mutex> &guard);
- /// These two Fetch frames APIs must be called with the stack mutex shared
- /// lock acquired. They are the only places where we acquire the writer
- /// end of the stack list mutex to add frames, but they will always exit with
- /// the shared side reacquired.
- /// Returns true if fetching frames was interrupted, false otherwise
- bool FetchFramesUpTo(uint32_t end_idx, InterruptionControl allow_interrupt,
- std::shared_lock<std::shared_mutex> &guard);
-
- void FetchOnlyConcreteFramesUpTo(uint32_t end_idx,
- std::shared_lock<std::shared_mutex> &guard);
-
- // This gets called without the StackFrameList lock held, callers should
- // hold the lock.
- void SynthesizeTailCallFrames(StackFrame &next_frame);
-
bool GetAllFramesFetched() { return m_concrete_frames_fetched == UINT32_MAX; }
void SetAllFramesFetched() { m_concrete_frames_fetched = UINT32_MAX; }
@@ -197,6 +188,23 @@ class StackFrameList {
GetFrameAtIndexNoLock(uint32_t idx,
std::shared_lock<std::shared_mutex> &guard);
+ /// These two Fetch frames APIs are called in GetFramesUpTo, they are the ones
+ /// that actually add frames. They must be called with the stack list shared
+ /// lock acquired. They will acquire the writer end of the stack list mutex
+ /// to add frames, but they will always exit with the shared side reacquired.
+ /// Returns true if fetching frames was interrupted, false otherwise.
+ bool FetchFramesUpTo(uint32_t end_idx, InterruptionControl allow_interrupt,
+ std::shared_lock<std::shared_mutex> &guard);
+
+ /// This is the same as FetchFramesUpTo, but only fetches concrete frames.
+ /// It is not currently interruptible - so it returns nothing.
+ void FetchOnlyConcreteFramesUpTo(uint32_t end_idx,
+ std::shared_lock<std::shared_mutex> &guard);
+
+ // This is a utility function, called by FetchFramesUpTo. It must be called
+ // with the writer end of the stack list mutex held.
+ 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 9466cf080823b3..fbf5f81bb10c92 100644
--- a/lldb/source/Target/StackFrameList.cpp
+++ b/lldb/source/Target/StackFrameList.cpp
@@ -415,10 +415,6 @@ bool StackFrameList::FetchFramesUpTo(uint32_t end_idx,
guard.lock();
});
- if (m_frames.size() > end_idx || GetAllFramesFetched()) {
- return false;
- }
-
#if defined(DEBUG_STACK_FRAMES)
StreamFile s(stdout, false);
#endif
@@ -721,6 +717,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
>From 7df3d995e45f67b083d3e1ad1baffbcbdab4eaeb Mon Sep 17 00:00:00 2001
From: Jim Ingham <jingham at apple.com>
Date: Mon, 9 Dec 2024 17:46:32 -0800
Subject: [PATCH 7/7] Reduce the use of m_list_mutex, try to separate reader
and writer lock uses.
---
lldb/include/lldb/Target/StackFrameList.h | 42 +++---
lldb/source/Target/StackFrameList.cpp | 166 ++++++++++------------
lldb/source/Target/Thread.cpp | 2 +-
3 files changed, 98 insertions(+), 112 deletions(-)
diff --git a/lldb/include/lldb/Target/StackFrameList.h b/lldb/include/lldb/Target/StackFrameList.h
index a0c4506b8ba6eb..044bdef015b424 100644
--- a/lldb/include/lldb/Target/StackFrameList.h
+++ b/lldb/include/lldb/Target/StackFrameList.h
@@ -94,6 +94,9 @@ class StackFrameList {
bool show_frame_info, uint32_t num_frames_with_source,
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;
@@ -108,14 +111,20 @@ class StackFrameList {
/// 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.
+ /// 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.
- /// Must be called with a shared mutex locked in `guard`.
- bool GetFramesUpTo(uint32_t end_idx, InterruptionControl allow_interrupt,
- std::shared_lock<std::shared_mutex> &guard);
-
- 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();
@@ -188,21 +197,14 @@ class StackFrameList {
GetFrameAtIndexNoLock(uint32_t idx,
std::shared_lock<std::shared_mutex> &guard);
- /// These two Fetch frames APIs are called in GetFramesUpTo, they are the ones
- /// that actually add frames. They must be called with the stack list shared
- /// lock acquired. They will acquire the writer end of the stack list mutex
- /// to add frames, but they will always exit with the shared side reacquired.
- /// Returns true if fetching frames was interrupted, false otherwise.
- bool FetchFramesUpTo(uint32_t end_idx, InterruptionControl allow_interrupt,
- 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.
- /// This is the same as FetchFramesUpTo, but only fetches concrete frames.
- /// It is not currently interruptible - so it returns nothing.
- void FetchOnlyConcreteFramesUpTo(uint32_t end_idx,
- std::shared_lock<std::shared_mutex> &guard);
-
- // This is a utility function, called by FetchFramesUpTo. It must be called
- // with the writer end of the stack 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;
diff --git a/lldb/source/Target/StackFrameList.cpp b/lldb/source/Target/StackFrameList.cpp
index fbf5f81bb10c92..f3c3d8170a4bc1 100644
--- a/lldb/source/Target/StackFrameList.cpp
+++ b/lldb/source/Target/StackFrameList.cpp
@@ -138,6 +138,11 @@ void StackFrameList::SetCurrentInlinedDepth(uint32_t new_depth) {
m_current_inlined_pc = m_thread.GetRegisterContext()->GetPC();
}
+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
/// is represented as a pair of a callee (Function *) and an address within the
/// callee.
@@ -335,56 +340,47 @@ void StackFrameList::SynthesizeTailCallFrames(StackFrame &next_frame) {
}
bool StackFrameList::GetFramesUpTo(uint32_t end_idx,
- InterruptionControl allow_interrupt,
- std::shared_lock<std::shared_mutex> &guard) {
- // Do not fetch frames for an invalid thread.
- bool was_interrupted = false;
- if (!m_thread.IsValid())
+ 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;
- // We've already gotten more frames than asked for, or we've already finished
- // unwinding, return.
- if (m_frames.size() > end_idx || GetAllFramesFetched())
+ // Do not fetch frames for an invalid thread.
+ bool was_interrupted = false;
+ if (!m_thread.IsValid())
return false;
+ // 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, guard);
+ FetchOnlyConcreteFramesUpTo(end_idx);
return false;
}
// We're adding concrete and inlined frames now:
- was_interrupted = FetchFramesUpTo(end_idx, allow_interrupt, guard);
+ was_interrupted = FetchFramesUpTo(end_idx, allow_interrupt);
#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;
- return false;
-
+ return was_interrupted;
}
-void StackFrameList::FetchOnlyConcreteFramesUpTo(
- uint32_t end_idx, std::shared_lock<std::shared_mutex> &guard) {
+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");
- assert(guard.owns_lock() && "Must be called with the shared lock acquired");
Unwind &unwinder = m_thread.GetUnwinder();
- guard.unlock();
- m_list_mutex.lock();
- auto on_exit = llvm::make_scope_exit([&]() {
- m_list_mutex.unlock();
- guard.lock();
- });
if (end_idx < m_concrete_frames_fetched)
return;
@@ -401,20 +397,10 @@ void StackFrameList::FetchOnlyConcreteFramesUpTo(
bool StackFrameList::FetchFramesUpTo(uint32_t end_idx,
- InterruptionControl allow_interrupt,
- std::shared_lock<std::shared_mutex> &guard) {
- assert(guard.owns_lock() && "Must be called with the shared lock acquired");
-
+ InterruptionControl allow_interrupt) {
Unwind &unwinder = m_thread.GetUnwinder();
bool was_interrupted = false;
- guard.unlock();
- m_list_mutex.lock();
- auto on_exit = llvm::make_scope_exit([&]() {
- m_list_mutex.unlock();
- guard.lock();
- });
-
#if defined(DEBUG_STACK_FRAMES)
StreamFile s(stdout, false);
#endif
@@ -583,16 +569,19 @@ bool StackFrameList::FetchFramesUpTo(uint32_t end_idx,
// We are done with the old stack frame list, we can release it now.
m_prev_frames_sp.reset();
}
- return was_interrupted;
+ // Don't report interrupted if we happen to have gotten all the frames:
+ if (!GetAllFramesFetched())
+ return was_interrupted;
+ return false;
}
uint32_t StackFrameList::GetNumFrames(bool can_create) {
- std::shared_lock<std::shared_mutex> guard(m_list_mutex);
-
- if (can_create) {
+ if (!GetAllFramesFetched() && can_create) {
// Don't allow interrupt or we might not return the correct count
- GetFramesUpTo(UINT32_MAX, DoNotAllowInterruption, guard);
+ GetFramesUpTo(UINT32_MAX, DoNotAllowInterruption);
}
+ // Formally we should acquire the shared lock here, but since we've forced
+ // fetching all the frames, it can't change anymore so that's not required.
return GetVisibleStackFrameIndex(m_frames.size());
}
@@ -617,29 +606,30 @@ void StackFrameList::Dump(Stream *s) {
}
StackFrameSP StackFrameList::GetFrameAtIndex(uint32_t idx) {
- std::shared_lock<std::shared_mutex> guard(m_list_mutex);
- return GetFrameAtIndexNoLock(idx, guard);
-}
-
-StackFrameSP StackFrameList::GetFrameAtIndexNoLock(
- uint32_t idx, std::shared_lock<std::shared_mutex> &guard) {
StackFrameSP frame_sp;
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);
+
+ uint32_t inlined_depth = GetCurrentInlinedDepth();
+ if (inlined_depth != UINT32_MAX)
+ idx += inlined_depth;
- if (idx < m_frames.size())
- frame_sp = m_frames[idx];
+ if (idx < m_frames.size())
+ frame_sp = m_frames[idx];
- if (frame_sp)
- return frame_sp;
+ if (frame_sp)
+ return frame_sp;
+ }
// 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, AllowInterruption, guard)) {
+ if (GetFramesUpTo(idx, AllowInterruption)) {
Log *log = GetLog(LLDBLog::Thread);
LLDB_LOG(log, "GetFrameAtIndex was interrupted");
return {};
@@ -674,12 +664,11 @@ StackFrameList::GetFrameWithConcreteFrameIndex(uint32_t unwind_idx) {
// after we make all the inlined frames. Most of the time the unwind frame
// index (or the concrete frame index) is the same as the frame index.
uint32_t frame_idx = unwind_idx;
- std::shared_lock<std::shared_mutex> guard(m_list_mutex);
- StackFrameSP frame_sp(GetFrameAtIndexNoLock(frame_idx, guard));
+ StackFrameSP frame_sp(GetFrameAtIndex(frame_idx));
while (frame_sp) {
if (frame_sp->GetFrameIndex() == unwind_idx)
break;
- frame_sp = GetFrameAtIndexNoLock(++frame_idx, guard);
+ frame_sp = GetFrameAtIndex(++frame_idx);
}
return frame_sp;
}
@@ -693,21 +682,26 @@ StackFrameSP StackFrameList::GetFrameWithStackID(const StackID &stack_id) {
StackFrameSP frame_sp;
if (stack_id.IsValid()) {
- std::shared_lock<std::shared_mutex> guard(m_list_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) {
- collection::const_iterator pos =
- std::lower_bound(begin, end, stack_id, CompareStackID);
- if (pos != end) {
- if ((*pos)->GetStackID() == stack_id)
- return *pos;
+ {
+ // 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 begin = m_frames.begin();
+ collection::const_iterator end = m_frames.end();
+ if (begin != end) {
+ collection::const_iterator pos =
+ std::lower_bound(begin, end, stack_id, CompareStackID);
+ if (pos != end) {
+ if ((*pos)->GetStackID() == stack_id)
+ return *pos;
+ }
}
}
+ // If we needed to add more frames, we would get to here.
do {
- frame_sp = GetFrameAtIndexNoLock(frame_idx, guard);
+ frame_sp = GetFrameAtIndex(frame_idx);
if (frame_sp && frame_sp->GetStackID() == stack_id)
break;
frame_idx++;
@@ -794,34 +788,23 @@ uint32_t
StackFrameList::GetSelectedFrameIndex(SelectMostRelevant select_most_relevant) {
if (!m_selected_frame_idx && select_most_relevant)
SelectMostRelevantFrame();
- { // Scope for lock guard
- std::shared_lock<std::shared_mutex> guard(m_list_mutex);
- if (!m_selected_frame_idx) {
- // If we aren't selecting the most relevant frame, and the selected frame
- // isn't set, then don't force a selection here, just return 0.
- if (!select_most_relevant)
- return 0;
- // If the inlined stack frame is set, then use that:
- m_selected_frame_idx = 0;
- }
- return *m_selected_frame_idx;
- }
-}
-
-uint32_t StackFrameList::SetSelectedFrame(lldb_private::StackFrame *frame) {
- uint32_t result = 0;
- {
- std::shared_lock<std::shared_mutex> guard(m_list_mutex);
- result = SetSelectedFrameNoLock(frame);
- }
- SetDefaultFileAndLineToSelectedFrame();
- return result;
+ if (!m_selected_frame_idx) {
+ // If we aren't selecting the most relevant frame, and the selected frame
+ // isn't set, then don't force a selection here, just return 0.
+ if (!select_most_relevant)
+ return 0;
+ // If the inlined stack frame is set, then use that:
+ m_selected_frame_idx = 0;
+ }
+ return *m_selected_frame_idx;
}
uint32_t
-StackFrameList::SetSelectedFrameNoLock(lldb_private::StackFrame *frame) {
+StackFrameList::SetSelectedFrame(lldb_private::StackFrame *frame) {
uint32_t result = 0;
+ 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();
@@ -836,6 +819,7 @@ StackFrameList::SetSelectedFrameNoLock(lldb_private::StackFrame *frame) {
break;
}
}
+ SetDefaultFileAndLineToSelectedFrame();
result = *m_selected_frame_idx;
return result;
}
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();
More information about the lldb-commits
mailing list