Convert the StackFrameList mutex to a shared mutex. #117252
Open
+389
−196
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
|
@llvm/pr-subscribers-lldb Author: None (jimingham) ChangesIn fact, there's only one public API in StackFrameList that changes This patch uses a shared_mutex instead, and when we have to add more Most of the work here was actually getting the stack frame list locking to not I also added a test that has 5 threads progressively asking for more Patch is 35.23 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/117252.diff 5 Files Affected:
diff --git a/lldb/include/lldb/Target/StackFrameList.h b/lldb/include/lldb/Target/StackFrameList.h
index 7d0e7a5b9a71b2..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,11 +104,19 @@ 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.
bool GetFramesUpTo(uint32_t end_idx,
- InterruptionControl allow_interrupt = AllowInterruption);
+ InterruptionControl allow_interrupt,
+ std::shared_lock<std::shared_mutex> &guard);
- 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.
void SynthesizeTailCallFrames(StackFrame &next_frame);
bool GetAllFramesFetched() { return m_concrete_frames_fetched == UINT32_MAX; }
@@ -122,6 +131,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 +150,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 +188,10 @@ 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);
+
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..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>
@@ -38,7 +39,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 +64,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 +86,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 +95,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 +105,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 +120,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 +130,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;
@@ -136,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
@@ -167,6 +181,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,
@@ -349,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())
@@ -363,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 def...
[truncated]
|
|
✅ With the latest revision this PR passed the Python code formatter. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
JDevlieghere
approved these changes
Nov 22, 2024
labath
reviewed
Nov 22, 2024
Comment on lines
+833
to
+834
| std::shared_lock<std::shared_mutex> guard(m_list_mutex); | ||
| result = SetSelectedFrameNoLock(frame); |
There was a problem hiding this comment.
A shared lock around a function called "Set" is very suspicious. What is this trying to protect?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.