https://github.com/medismailben updated https://github.com/llvm/llvm-project/pull/195774
>From eef23e2c2095572fd20ff4ddb88ddea99bba0878 Mon Sep 17 00:00:00 2001 From: Med Ismail Bennani <[email protected]> Date: Mon, 4 May 2026 18:19:57 -0700 Subject: [PATCH 1/3] [lldb] Add Policy infrastructure Add a generic thread-local policy stack and a Policy struct that describes what view of the process a thread should see (private reality vs public illusion) and what operations it is allowed to perform. This is the infrastructure for replacing ad-hoc host thread identity checks (CurrentThreadIsPrivateStateThread, IsOnThread, etc.) with a unified, composable mechanism. No behavioral changes yet -- adoption will follow in subsequent patches. rdar://176223894 Signed-off-by: Med Ismail Bennani <[email protected]> --- lldb/include/lldb/Target/Policy.h | 78 +++++++++++++++++++ .../lldb/Utility/ThreadLocalPolicyStack.h | 57 ++++++++++++++ 2 files changed, 135 insertions(+) create mode 100644 lldb/include/lldb/Target/Policy.h create mode 100644 lldb/include/lldb/Utility/ThreadLocalPolicyStack.h diff --git a/lldb/include/lldb/Target/Policy.h b/lldb/include/lldb/Target/Policy.h new file mode 100644 index 0000000000000..93bd5c39380f3 --- /dev/null +++ b/lldb/include/lldb/Target/Policy.h @@ -0,0 +1,78 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLDB_TARGET_POLICY_H +#define LLDB_TARGET_POLICY_H + +#include "lldb/Utility/ThreadLocalPolicyStack.h" + +namespace lldb_private { + +/// Describes what view of the process a thread should see and what +/// operations it is allowed to perform. +/// +/// Frame providers are a public illusion layered on top of the private +/// reality (the unwinder stack). The private state thread manages the +/// stop of that private reality, so the correct view for its logic IS the +/// private reality. The public illusion is only applied once the process +/// has settled and clients query the stopped state. +/// +/// This struct is a pure data store -- no business logic. It is pushed +/// onto a per-thread stack (PolicyStack) so that code +/// can consult the current policy instead of comparing host thread +/// identities. +struct Policy { + /// What view of the process this thread sees. + enum class View { + Public, // Provider-augmented frames, public state, public run lock. + Private, // Parent (unwinder) frames, private state, private run lock. + }; + + /// What operations this thread is allowed to perform. + /// Enforced at specific callsites, not by the policy itself. + struct Capabilities { + bool can_evaluate_expressions : 1; + bool stop_others_only : 1; + bool can_try_all_threads : 1; + bool can_run_breakpoint_actions : 1; + bool can_load_frame_providers : 1; + bool can_run_frame_recognizers : 1; + }; + + View view = View::Public; + Capabilities capabilities = { + true, // can_evaluate_expressions + false, // stop_others_only + true, // can_try_all_threads + true, // can_run_breakpoint_actions + true, // can_load_frame_providers + true, // can_run_frame_recognizers + }; + + static Policy PublicState() { return {}; } + + static Policy PrivateState() { + Policy p; + p.view = View::Private; + p.capabilities.can_load_frame_providers = false; + p.capabilities.can_run_frame_recognizers = false; + return p; + } + + static Policy PublicStateRunningExpression() { + Policy p; + p.capabilities.can_run_breakpoint_actions = false; + return p; + } +}; + +using PolicyStack = ThreadLocalPolicyStack<Policy>; + +} // namespace lldb_private + +#endif // LLDB_TARGET_POLICY_H diff --git a/lldb/include/lldb/Utility/ThreadLocalPolicyStack.h b/lldb/include/lldb/Utility/ThreadLocalPolicyStack.h new file mode 100644 index 0000000000000..d24b23a34a583 --- /dev/null +++ b/lldb/include/lldb/Utility/ThreadLocalPolicyStack.h @@ -0,0 +1,57 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLDB_UTILITY_THREADLOCALPOLICYSTACK_H +#define LLDB_UTILITY_THREADLOCALPOLICYSTACK_H + +#include <vector> + +namespace lldb_private { + +/// Generic per-thread policy stack. +/// +/// The stack lives in thread_local storage. Each thread has its own stack, +/// initialized with a default-constructed base entry that is never popped. +/// RAII guards (Guard) push and pop policies. +/// +/// For thread pool workers that don't inherit thread_local storage, the +/// policy must be passed into the lambda and pushed onto the worker +/// thread's stack when the task starts. +template <typename Policy> class ThreadLocalPolicyStack { +public: + static ThreadLocalPolicyStack &GetForCurrentThread() { + static thread_local ThreadLocalPolicyStack s_stack; + return s_stack; + } + + const Policy &Current() const { return m_stack.back(); } + + void Push(Policy policy) { m_stack.push_back(policy); } + + void Pop() { + if (m_stack.size() > 1) + m_stack.pop_back(); + } + + /// RAII guard that pushes a policy on construction and pops on destruction. + class Guard { + public: + Guard(Policy policy) { GetForCurrentThread().Push(policy); } + ~Guard() { GetForCurrentThread().Pop(); } + + Guard(const Guard &) = delete; + Guard &operator=(const Guard &) = delete; + }; + +private: + std::vector<Policy> m_stack = {Policy{}}; +}; + +} // namespace lldb_private + +#endif // LLDB_UTILITY_THREADLOCALPOLICYSTACK_H >From 1984dd1e22798915427760758fcbfa3de176dd76 Mon Sep 17 00:00:00 2001 From: Med Ismail Bennani <[email protected]> Date: Mon, 4 May 2026 18:41:00 -0700 Subject: [PATCH 2/3] [lldb] Adopt Policy for private/public view and capability decisions Push Policy::PrivateState() at all sites where the private state thread needs to see the private reality instead of the public illusion: - StopInfoBreakpoint::ShouldStopSynchronous (sync callbacks, WasHit) - StopInfoBreakpoint::PerformAction (async callbacks, conditions) - StopInfoWatchpoint::ShouldStopSynchronous - StopInfoWatchpoint::PerformAction - RunThreadPlan (original PST during expression evaluation) - RunPrivateStateThread (override PSTs) Consult the policy at all view decision points: - Process::GetState() -- private vs public state - Target::GetAPIMutex() -- private vs public mutex - PrivateStateThread::GetRunLock() -- private vs public run lock - Thread::GetStackFrameList() -- parent vs provider-augmented frames - StackFrameList::SelectMostRelevantFrame() -- skip frame recognizers - StopInfoBreakpoint::PerformAction() -- skip breakpoint actions The existing host thread identity checks remain as fallbacks until Policy is pushed for all PSTs unconditionally. rdar://176223894 Signed-off-by: Med Ismail Bennani <[email protected]> --- lldb/include/lldb/Target/Policy.h | 12 +++++------ lldb/include/lldb/Target/Process.h | 29 +++++---------------------- lldb/source/Target/Process.cpp | 21 ++++++++++--------- lldb/source/Target/StackFrameList.cpp | 5 +++++ lldb/source/Target/StopInfo.cpp | 21 ++++++++++++++++++- lldb/source/Target/Target.cpp | 9 +++++++-- lldb/source/Target/Thread.cpp | 16 ++++++++------- 7 files changed, 64 insertions(+), 49 deletions(-) diff --git a/lldb/include/lldb/Target/Policy.h b/lldb/include/lldb/Target/Policy.h index 93bd5c39380f3..6a9664fa5b085 100644 --- a/lldb/include/lldb/Target/Policy.h +++ b/lldb/include/lldb/Target/Policy.h @@ -46,12 +46,12 @@ struct Policy { View view = View::Public; Capabilities capabilities = { - true, // can_evaluate_expressions - false, // stop_others_only - true, // can_try_all_threads - true, // can_run_breakpoint_actions - true, // can_load_frame_providers - true, // can_run_frame_recognizers + /*can_evaluate_expressions=*/true, + /*stop_others_only=*/false, + /*can_try_all_threads=*/true, + /*can_run_breakpoint_actions=*/true, + /*can_load_frame_providers=*/true, + /*can_run_frame_recognizers=*/true, }; static Policy PublicState() { return {}; } diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h index 67a9a883cd36c..e6669701971fc 100644 --- a/lldb/include/lldb/Target/Process.h +++ b/lldb/include/lldb/Target/Process.h @@ -41,6 +41,7 @@ #include "lldb/Target/InstrumentationRuntime.h" #include "lldb/Target/Memory.h" #include "lldb/Target/MemoryTagManager.h" +#include "lldb/Target/Policy.h" #include "lldb/Target/QueueList.h" #include "lldb/Target/ThreadList.h" #include "lldb/Target/ThreadPlanStack.h" @@ -3334,10 +3335,12 @@ void PruneThreadPlans(); } ProcessRunLock &GetRunLock() { + auto &policy = PolicyStack::GetForCurrentThread().Current(); + if (policy.view == Policy::View::Private) + return m_private_run_lock; if (IsOnThread(Host::GetCurrentThread())) return m_private_run_lock; - else - return m_public_run_lock; + return m_public_run_lock; } Process &m_process; @@ -3719,28 +3722,6 @@ class UtilityFunctionScope { } }; -/// RAII guard that marks the current thread as a private state thread. -/// -/// When RunThreadPlan detects it is running on the private state thread, it -/// spins up an override thread and reassigns m_current_private_state_thread_sp -/// to it. The original PST continues processing events via DoOnRemoval -/// callbacks, but CurrentThreadIsPrivateStateThread() no longer recognizes it. -/// This guard sets a thread_local flag so that GetStackFrameList can identify -/// the original PST and return parent frames instead of provider-augmented -/// frames. -struct PrivateStateThreadGuard { - PrivateStateThreadGuard() { g_is_private_state_thread = true; } - ~PrivateStateThreadGuard() { g_is_private_state_thread = false; } - static bool IsPrivateStateThread() { return g_is_private_state_thread; } - - // Non-copyable, non-movable. - PrivateStateThreadGuard(const PrivateStateThreadGuard &) = delete; - PrivateStateThreadGuard &operator=(const PrivateStateThreadGuard &) = delete; - -private: - static thread_local bool g_is_private_state_thread; -}; - } // namespace lldb_private #endif // LLDB_TARGET_PROCESS_H diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp index 114bbd7355f0c..6f34de08c2683 100644 --- a/lldb/source/Target/Process.cpp +++ b/lldb/source/Target/Process.cpp @@ -52,6 +52,7 @@ #include "lldb/Target/MemoryRegionInfo.h" #include "lldb/Target/OperatingSystem.h" #include "lldb/Target/Platform.h" +#include "lldb/Target/Policy.h" #include "lldb/Target/Process.h" #include "lldb/Target/RegisterContext.h" #include "lldb/Target/StopInfo.h" @@ -1277,10 +1278,14 @@ StateType Process::GetState() { if (!m_current_private_state_thread_sp) return eStateUnloaded; + auto &policy = PolicyStack::GetForCurrentThread().Current(); + if (policy.view == Policy::View::Private) + return GetPrivateState(); + if (CurrentThreadPosesAsPrivateStateThread()) return GetPrivateState(); - else - return GetPublicState(); + + return GetPublicState(); } void Process::SetPublicState(StateType new_state, bool restarted) { @@ -4320,14 +4325,12 @@ Status Process::HaltPrivate() { return error; } -thread_local bool PrivateStateThreadGuard::g_is_private_state_thread = false; - thread_result_t Process::RunPrivateStateThread(bool is_override) { // Override PSTs exist solely to service RunThreadPlan expression evaluation. // They must see parent frames, not provider-augmented frames. - std::optional<PrivateStateThreadGuard> pst_guard; + std::optional<PolicyStack::Guard> policy_guard; if (is_override) - pst_guard.emplace(); + policy_guard.emplace(Policy::PrivateState()); bool control_only = true; @@ -5525,9 +5528,9 @@ Process::RunThreadPlan(ExecutionContext &exe_ctx, // If we spawned an override PST, mark the current (original) PST so // GetStackFrameList returns parent frames during event processing. - std::optional<PrivateStateThreadGuard> private_state_thread_guard; + std::optional<PolicyStack::Guard> policy_guard; if (backup_private_state_thread) - private_state_thread_guard.emplace(); + policy_guard.emplace(Policy::PrivateState()); while (true) { // We usually want to resume the process if we get to the top of the @@ -5929,7 +5932,7 @@ Process::RunThreadPlan(ExecutionContext &exe_ctx, } } // END WAIT LOOP - private_state_thread_guard.reset(); + policy_guard.reset(); // If we had to start up a temporary private state thread to run this // thread plan, shut it down now. diff --git a/lldb/source/Target/StackFrameList.cpp b/lldb/source/Target/StackFrameList.cpp index 5423785c2fd51..d8558c78262d8 100644 --- a/lldb/source/Target/StackFrameList.cpp +++ b/lldb/source/Target/StackFrameList.cpp @@ -15,6 +15,7 @@ #include "lldb/Symbol/Block.h" #include "lldb/Symbol/Function.h" #include "lldb/Symbol/Symbol.h" +#include "lldb/Target/Policy.h" #include "lldb/Target/Process.h" #include "lldb/Target/RegisterContext.h" #include "lldb/Target/StackFrame.h" @@ -806,6 +807,10 @@ void StackFrameList::SelectMostRelevantFrame() { // Don't call into the frame recognizers on the private state thread as // they can cause code to run in the target, and that can cause deadlocks // when fetching stop events for the expression. + auto &policy = PolicyStack::GetForCurrentThread().Current(); + if (policy.view == Policy::View::Private) + return; + if (m_thread.GetProcess()->CurrentThreadPosesAsPrivateStateThread()) return; diff --git a/lldb/source/Target/StopInfo.cpp b/lldb/source/Target/StopInfo.cpp index 2a7ea2808d87f..88f81832c18c5 100644 --- a/lldb/source/Target/StopInfo.cpp +++ b/lldb/source/Target/StopInfo.cpp @@ -16,6 +16,7 @@ #include "lldb/Core/Debugger.h" #include "lldb/Expression/UserExpression.h" #include "lldb/Symbol/Block.h" +#include "lldb/Target/Policy.h" #include "lldb/Target/Process.h" #include "lldb/Target/StopInfo.h" #include "lldb/Target/Target.h" @@ -153,6 +154,10 @@ class StopInfoBreakpoint : public StopInfo { StopReason GetStopReason() const override { return eStopReasonBreakpoint; } bool ShouldStopSynchronous(Event *event_ptr) override { + // Breakpoint callbacks run on the PST during stop processing. Push + // private state context so callback code sees the private reality. + PolicyStack::Guard policy_guard(Policy::PrivateState()); + ThreadSP thread_sp(m_thread_wp.lock()); if (thread_sp) { if (!m_should_stop_is_valid) { @@ -322,6 +327,10 @@ class StopInfoBreakpoint : public StopInfo { m_should_perform_action = false; bool all_stopping_locs_internal = true; + // Breakpoint callbacks run on the PST during stop processing. Push + // private state context so callback code sees the private reality. + PolicyStack::Guard policy_guard(Policy::PrivateState()); + ThreadSP thread_sp(m_thread_wp.lock()); if (thread_sp) { @@ -393,7 +402,9 @@ class StopInfoBreakpoint : public StopInfo { ExecutionContext exe_ctx(thread_sp->GetStackFrameAtIndex(0)); Process *process = exe_ctx.GetProcessPtr(); - if (process->GetModIDRef().IsRunningExpression()) { + auto &policy = PolicyStack::GetForCurrentThread().Current(); + if (!policy.capabilities.can_run_breakpoint_actions || + process->GetModIDRef().IsRunningExpression()) { // If we are in the middle of evaluating an expression, don't run // asynchronous breakpoint commands or expressions. That could // lead to infinite recursion if the command or condition re-calls @@ -857,6 +868,10 @@ class StopInfoWatchpoint : public StopInfo { }; bool ShouldStopSynchronous(Event *event_ptr) override { + // Watchpoint callbacks run on the PST during stop processing. Push + // private state context so callback code sees the private reality. + PolicyStack::Guard policy_guard(Policy::PrivateState()); + // If we are running our step-over the watchpoint plan, stop if it's done // and continue if it's not: if (m_should_stop_is_valid) @@ -964,6 +979,10 @@ class StopInfoWatchpoint : public StopInfo { } void PerformAction(Event *event_ptr) override { + // Watchpoint callbacks run on the PST during stop processing. Push + // private state context so callback code sees the private reality. + PolicyStack::Guard policy_guard(Policy::PrivateState()); + Log *log = GetLog(LLDBLog::Watchpoints); // We're going to calculate if we should stop or not in some way during the // course of this code. Also by default we're going to stop, so set that diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp index e2bae8fae1a26..d41bdbd9328c7 100644 --- a/lldb/source/Target/Target.cpp +++ b/lldb/source/Target/Target.cpp @@ -51,6 +51,7 @@ #include "lldb/Target/ExecutionContext.h" #include "lldb/Target/Language.h" #include "lldb/Target/LanguageRuntime.h" +#include "lldb/Target/Policy.h" #include "lldb/Target/Process.h" #include "lldb/Target/RegisterTypeBuilder.h" #include "lldb/Target/SectionLoadList.h" @@ -5886,10 +5887,14 @@ Target::TargetEventData::GetModuleListFromEvent(const Event *event_ptr) { } std::recursive_mutex &Target::GetAPIMutex() { + auto &policy = PolicyStack::GetForCurrentThread().Current(); + if (policy.view == Policy::View::Private) + return m_private_mutex; + if (GetProcessSP() && GetProcessSP()->CurrentThreadIsPrivateStateThread()) return m_private_mutex; - else - return m_mutex; + + return m_mutex; } /// Get metrics associated with this target in JSON format. diff --git a/lldb/source/Target/Thread.cpp b/lldb/source/Target/Thread.cpp index 70053b8822bdc..b37c5c9313ef2 100644 --- a/lldb/source/Target/Thread.cpp +++ b/lldb/source/Target/Thread.cpp @@ -24,6 +24,7 @@ #include "lldb/Target/DynamicLoader.h" #include "lldb/Target/ExecutionContext.h" #include "lldb/Target/LanguageRuntime.h" +#include "lldb/Target/Policy.h" #include "lldb/Target/Process.h" #include "lldb/Target/RegisterContext.h" #include "lldb/Target/ScriptedThreadPlan.h" @@ -1533,7 +1534,7 @@ StackFrameListSP Thread::GetStackFrameList() { // thread that is already mid-construction. // // 2. Current thread is a private state thread that should see the - // private reality (PrivateStateThreadGuard::IsPrivateStateThread). + // private reality (Policy::View::Private). // // For case 1, if a provider is active we return its input (parent) // frames. For case (2), we return/create the unwinder frame list @@ -1559,12 +1560,13 @@ StackFrameListSP Thread::GetStackFrameList() { return m_curr_frames_sp; // For case 2, PST must see the private reality, not the public illusion. - // PrivateStateThreadGuard is a thread_local flag set by RunThreadPlan - // (for the original PST) and RunPrivateStateThread (for override PSTs). - // We cannot use CurrentThreadIsPrivateStateThread() here because - // RunThreadPlan reassigns m_current_private_state_thread_sp to the - // override, so the original PST is no longer recognized. - if (PrivateStateThreadGuard::IsPrivateStateThread()) { + // Policy is pushed by RunThreadPlan (for the original PST) + // and RunPrivateStateThread (for override PSTs). We cannot use + // CurrentThreadIsPrivateStateThread() here because RunThreadPlan reassigns + // m_current_private_state_thread_sp to the override, so the original PST + // is no longer recognized. + auto &policy = PolicyStack::GetForCurrentThread().Current(); + if (policy.view == Policy::View::Private) { if (!m_unwinder_frames_sp) m_unwinder_frames_sp = std::make_shared<StackFrameList>( *this, m_prev_frames_sp, true, /*provider_id=*/0); >From 55ab4c68618ed4c348bca998ca593cbe707c3a87 Mon Sep 17 00:00:00 2001 From: Med Ismail Bennani <[email protected]> Date: Mon, 4 May 2026 18:52:49 -0700 Subject: [PATCH 3/3] [lldb] Use Policy for ProcessRunLock re-entrancy When SB API code enters a frame provider's Python callback and the callback calls back into the SB API, the inner call tries to re-acquire the same ProcessRunLock read lock. On a write-preferring rwlock, a pending writer (e.g. the override PST calling SetStopped) blocks the re-entrant reader, causing a deadlock. Fix this by having ProcessRunLocker::TryLock push a policy with holds_run_lock=true after acquiring the real lock. Re-entrant ReadTryLock calls check this flag and skip the rwlock entirely. ProcessRunLocker::Unlock pops the policy before releasing the lock. This replaces the previous thread_local unordered_map workaround with the Policy framework. rdar://176223894 Signed-off-by: Med Ismail Bennani <[email protected]> --- lldb/include/lldb/Host/ProcessRunLock.h | 25 +--- lldb/include/lldb/Target/Policy.h | 2 + lldb/source/Host/common/ProcessRunLock.cpp | 38 ++++++ .../runlock_reentrant_deadlock/Makefile | 3 + .../TestRunLockReentrantDeadlock.py | 125 ++++++++++++++++++ .../bkpt_resolver.py | 47 +++++++ .../frame_provider.py | 38 ++++++ .../runlock_reentrant_deadlock/main.c | 18 +++ 8 files changed, 275 insertions(+), 21 deletions(-) create mode 100644 lldb/test/API/functionalities/scripted_frame_provider/runlock_reentrant_deadlock/Makefile create mode 100644 lldb/test/API/functionalities/scripted_frame_provider/runlock_reentrant_deadlock/TestRunLockReentrantDeadlock.py create mode 100644 lldb/test/API/functionalities/scripted_frame_provider/runlock_reentrant_deadlock/bkpt_resolver.py create mode 100644 lldb/test/API/functionalities/scripted_frame_provider/runlock_reentrant_deadlock/frame_provider.py create mode 100644 lldb/test/API/functionalities/scripted_frame_provider/runlock_reentrant_deadlock/main.c diff --git a/lldb/include/lldb/Host/ProcessRunLock.h b/lldb/include/lldb/Host/ProcessRunLock.h index 27d10942ddb4c..049eb36deea27 100644 --- a/lldb/include/lldb/Host/ProcessRunLock.h +++ b/lldb/include/lldb/Host/ProcessRunLock.h @@ -58,29 +58,12 @@ class ProcessRunLock { bool IsLocked() const { return m_lock; } // Try to lock the read lock, but only do so if there are no writers. - bool TryLock(ProcessRunLock *lock) { - if (m_lock) { - if (m_lock == lock) - return true; // We already have this lock locked - else - Unlock(); - } - if (lock) { - if (lock->ReadTryLock()) { - m_lock = lock; - return true; - } - } - return false; - } + // Pushes a policy with holds_run_lock=true so that re-entrant + // ReadTryLock calls from the same thread skip the real lock. + bool TryLock(ProcessRunLock *lock); protected: - void Unlock() { - if (m_lock) { - m_lock->ReadUnlock(); - m_lock = nullptr; - } - } + void Unlock(); ProcessRunLock *m_lock = nullptr; diff --git a/lldb/include/lldb/Target/Policy.h b/lldb/include/lldb/Target/Policy.h index 6a9664fa5b085..d239aaff1757a 100644 --- a/lldb/include/lldb/Target/Policy.h +++ b/lldb/include/lldb/Target/Policy.h @@ -42,6 +42,7 @@ struct Policy { bool can_run_breakpoint_actions : 1; bool can_load_frame_providers : 1; bool can_run_frame_recognizers : 1; + bool holds_run_lock : 1; }; View view = View::Public; @@ -52,6 +53,7 @@ struct Policy { /*can_run_breakpoint_actions=*/true, /*can_load_frame_providers=*/true, /*can_run_frame_recognizers=*/true, + /*holds_run_lock=*/false, }; static Policy PublicState() { return {}; } diff --git a/lldb/source/Host/common/ProcessRunLock.cpp b/lldb/source/Host/common/ProcessRunLock.cpp index 8e7ef45e1e350..a7078f335c957 100644 --- a/lldb/source/Host/common/ProcessRunLock.cpp +++ b/lldb/source/Host/common/ProcessRunLock.cpp @@ -8,6 +8,7 @@ #ifndef _WIN32 #include "lldb/Host/ProcessRunLock.h" +#include "lldb/Target/Policy.h" namespace lldb_private { @@ -22,6 +23,10 @@ ProcessRunLock::~ProcessRunLock() { } bool ProcessRunLock::ReadTryLock() { + auto &policy = PolicyStack::GetForCurrentThread().Current(); + if (policy.capabilities.holds_run_lock) + return !m_running; + ::pthread_rwlock_rdlock(&m_rwlock); if (!m_running) { // coverity[missing_unlock] @@ -32,9 +37,42 @@ bool ProcessRunLock::ReadTryLock() { } bool ProcessRunLock::ReadUnlock() { + auto &policy = PolicyStack::GetForCurrentThread().Current(); + if (policy.capabilities.holds_run_lock) + return true; + return ::pthread_rwlock_unlock(&m_rwlock) == 0; } +bool ProcessRunLock::ProcessRunLocker::TryLock(ProcessRunLock *lock) { + if (m_lock) { + if (m_lock == lock) + return true; + Unlock(); + } + if (lock) { + if (lock->ReadTryLock()) { + m_lock = lock; + // Push a policy so re-entrant ReadTryLock calls from the same + // thread (e.g. provider Python calling back into SB API) skip + // the real lock and avoid deadlocking with a pending writer. + auto policy = PolicyStack::GetForCurrentThread().Current(); + policy.capabilities.holds_run_lock = true; + PolicyStack::GetForCurrentThread().Push(policy); + return true; + } + } + return false; +} + +void ProcessRunLock::ProcessRunLocker::Unlock() { + if (m_lock) { + PolicyStack::GetForCurrentThread().Pop(); + m_lock->ReadUnlock(); + m_lock = nullptr; + } +} + bool ProcessRunLock::SetRunning() { ::pthread_rwlock_wrlock(&m_rwlock); bool was_stopped = !m_running; diff --git a/lldb/test/API/functionalities/scripted_frame_provider/runlock_reentrant_deadlock/Makefile b/lldb/test/API/functionalities/scripted_frame_provider/runlock_reentrant_deadlock/Makefile new file mode 100644 index 0000000000000..0b710c6e298ae --- /dev/null +++ b/lldb/test/API/functionalities/scripted_frame_provider/runlock_reentrant_deadlock/Makefile @@ -0,0 +1,3 @@ +C_SOURCES := main.c +CFLAGS_EXTRAS := -std=c99 +include Makefile.rules diff --git a/lldb/test/API/functionalities/scripted_frame_provider/runlock_reentrant_deadlock/TestRunLockReentrantDeadlock.py b/lldb/test/API/functionalities/scripted_frame_provider/runlock_reentrant_deadlock/TestRunLockReentrantDeadlock.py new file mode 100644 index 0000000000000..2933c073ce8c9 --- /dev/null +++ b/lldb/test/API/functionalities/scripted_frame_provider/runlock_reentrant_deadlock/TestRunLockReentrantDeadlock.py @@ -0,0 +1,125 @@ +""" +Test that ProcessRunLock re-entrant read locks don't deadlock when a frame +provider's get_frame_at_index calls SB API methods. + +This reproduces the deadlock seen in lldb-rpc-server where: + + - An RPC client thread holds a ProcessRunLock read lock (from the outer + SB API call) and enters a provider's get_frame_at_index, which calls + SBFrame.IsValid -> GetStoppedExecutionContext -> ReadTryLock (re-entrant). + + - The override PST is exiting RunPrivateStateThread and calls SetStopped + -> pthread_rwlock_wrlock (blocked by client thread's read lock). + + - The client thread's re-entrant ReadTryLock blocks because the pending + writer prevents new readers on a write-preferring rwlock. + + - The original PST is blocked joining the override thread. +""" + +import os +import threading +import lldb +import lldbsuite.test.lldbutil as lldbutil +from lldbsuite.test.decorators import * +from lldbsuite.test.lldbtest import * + + +class TestRunLockReentrantDeadlock(TestBase): + NO_DEBUG_INFO_TESTCASE = True + + @expectedFailureAll(oslist=["windows"], bugnumber="llvm.org/pr24528") + def test_runlock_reentrant_no_deadlock(self): + """ + Test that a frame provider calling SBFrame.IsValid from + get_frame_at_index does not deadlock with the override PST's + SetStopped when another thread triggers EvaluateExpression. + """ + self.build() + + target, process, thread, _ = lldbutil.run_to_name_breakpoint(self, "main") + + resolver_path = os.path.join(self.getSourceDir(), "bkpt_resolver.py") + provider_path = os.path.join(self.getSourceDir(), "frame_provider.py") + self.runCmd("command script import " + resolver_path) + self.runCmd("command script import " + provider_path) + + error = lldb.SBError() + provider_id = target.RegisterScriptedFrameProvider( + "frame_provider.SBAPIAccessInGetFrameProvider", + lldb.SBStructuredData(), + error, + ) + self.assertTrue( + error.Success(), + f"Should register frame provider: {error}", + ) + self.assertNotEqual(provider_id, 0, "Provider ID should be non-zero") + + extra_args = lldb.SBStructuredData() + stream = lldb.SBStream() + stream.Print('{"symbol": "target_func"}') + extra_args.SetFromJSON(stream) + + bkpt = target.BreakpointCreateFromScript( + "bkpt_resolver.ExprEvalResolver", + extra_args, + lldb.SBFileSpecList(), + lldb.SBFileSpecList(), + ) + self.assertTrue(bkpt.IsValid(), "Scripted breakpoint should be valid") + self.assertGreater( + bkpt.GetNumLocations(), 0, "Breakpoint should have locations" + ) + + # Spawn a thread that repeatedly accesses frames through the provider. + # This simulates an RPC client thread that holds the ProcessRunLock + # read lock and enters get_frame_at_index -> SBFrame.IsValid. + stop_frame_access = threading.Event() + frame_access_error = [None] + + def access_frames(): + try: + while not stop_frame_access.is_set(): + t = process.GetSelectedThread() + if t.IsValid(): + for i in range(t.GetNumFrames()): + f = t.GetFrameAtIndex(i) + f.IsValid() + except Exception as e: + frame_access_error[0] = str(e) + + frame_thread = threading.Thread(target=access_frames, daemon=True) + frame_thread.start() + + # Continue into the breakpoint. was_hit calls EvaluateExpression on + # the PST, which triggers RunThreadPlan -> override PST. + # The frame access thread is concurrently calling GetFrameAtIndex -> + # provider get_frame_at_index -> SBFrame.IsValid. + # Without the fix, this deadlocks. + process.Continue() + self.assertState(process.GetState(), lldb.eStateStopped) + + stop_frame_access.set() + frame_thread.join(timeout=5) + self.assertFalse(frame_thread.is_alive(), "Frame access thread should exit") + self.assertIsNone( + frame_access_error[0], + f"Frame access thread hit an error: {frame_access_error[0]}", + ) + + thread = process.GetSelectedThread() + self.assertTrue(thread.IsValid(), "Thread should be valid") + self.assertEqual( + thread.GetStopReason(), + lldb.eStopReasonBreakpoint, + "Should stop at breakpoint", + ) + + g_value = target.FindFirstGlobalVariable("g_value") + self.assertTrue(g_value.IsValid(), "Should find g_value") + self.assertGreater( + g_value.GetValueAsUnsigned(), + 0, + "g_value should have been incremented by the was_hit callback", + ) diff --git a/lldb/test/API/functionalities/scripted_frame_provider/runlock_reentrant_deadlock/bkpt_resolver.py b/lldb/test/API/functionalities/scripted_frame_provider/runlock_reentrant_deadlock/bkpt_resolver.py new file mode 100644 index 0000000000000..7fb7f8675f2a6 --- /dev/null +++ b/lldb/test/API/functionalities/scripted_frame_provider/runlock_reentrant_deadlock/bkpt_resolver.py @@ -0,0 +1,47 @@ +""" +Scripted breakpoint resolver whose was_hit callback calls EvaluateExpression. + +This reproduces the deadlock seen in the sample where: +1. BreakpointResolverScripted::WasHit runs on the private state thread +2. was_hit calls SBFrame.EvaluateExpression -> RunThreadPlan -> Halt -> + WaitForProcessToStop (holds a mutex, waits for state event) +3. The override private state thread handles the stop and loads a scripted + frame provider +4. The provider's __init__ calls SBThread.__bool__ -> GetStoppedExecutionContext + -> tries to acquire the same mutex -> DEADLOCK +""" + +import lldb + + +class ExprEvalResolver: + """Scripted breakpoint resolver that evaluates an expression in was_hit.""" + + def __init__(self, bkpt, extra_args, dict): + self.bkpt = bkpt + sym_name = extra_args.GetValueForKey("symbol").GetStringValue(100) + self.sym_name = sym_name + self.facade_loc = None + + def __callback__(self, sym_ctx): + sym = sym_ctx.module.FindSymbol(self.sym_name, lldb.eSymbolTypeCode) + if sym.IsValid(): + self.bkpt.AddLocation(sym.GetStartAddress()) + self.facade_loc = self.bkpt.AddFacadeLocation() + + def get_short_help(self): + return f"ExprEvalResolver for {self.sym_name}" + + def was_hit(self, frame, bp_loc): + # This runs on the private state thread. Calling EvaluateExpression + # here triggers RunThreadPlan -> Halt -> WaitForProcessToStop, which + # holds a mutex and waits for a state change event. + options = lldb.SBExpressionOptions() + options.SetStopOthers(True) + options.SetTryAllThreads(False) + + result = frame.EvaluateExpression("increment()", options) + if not result.error.success: + return lldb.LLDB_INVALID_BREAK_ID + + return self.facade_loc diff --git a/lldb/test/API/functionalities/scripted_frame_provider/runlock_reentrant_deadlock/frame_provider.py b/lldb/test/API/functionalities/scripted_frame_provider/runlock_reentrant_deadlock/frame_provider.py new file mode 100644 index 0000000000000..413484c264513 --- /dev/null +++ b/lldb/test/API/functionalities/scripted_frame_provider/runlock_reentrant_deadlock/frame_provider.py @@ -0,0 +1,38 @@ +""" +Frame provider whose get_frame_at_index calls SBFrame::IsValid on input frames. + +When a client thread accesses frames through this provider while the PST is +mid-expression-eval (RunThreadPlan), the following deadlock occurs: + + - Client thread: holds ProcessRunLock read lock (from outer SB API call), + enters get_frame_at_index, calls SBFrame.IsValid -> + GetStoppedExecutionContext -> ReadTryLock (re-entrant, blocked by + pending writer) + + - Override PST: finishing RunPrivateStateThread, calls SetStopped -> + pthread_rwlock_wrlock (blocked by client thread's read lock) + +The re-entrant read lock blocks because macOS uses a write-preferring rwlock. +""" + +import lldb +from lldb.plugins.scripted_frame_provider import ScriptedFrameProvider + + +class SBAPIAccessInGetFrameProvider(ScriptedFrameProvider): + """Provider that calls SBFrame.IsValid from get_frame_at_index.""" + + @staticmethod + def get_description(): + return "Provider that accesses SB API in get_frame_at_index" + + def get_frame_at_index(self, idx): + if idx < len(self.input_frames): + frame = self.input_frames.GetFrameAtIndex(idx) + # This call triggers GetStoppedExecutionContext -> + # ProcessRunLock::ReadTryLock. If the current thread already + # holds the read lock (from the outer SB API entry point), + # and a writer is pending, this re-entrant read lock blocks. + frame.IsValid() + return idx + return None diff --git a/lldb/test/API/functionalities/scripted_frame_provider/runlock_reentrant_deadlock/main.c b/lldb/test/API/functionalities/scripted_frame_provider/runlock_reentrant_deadlock/main.c new file mode 100644 index 0000000000000..9ebeaf3380ed9 --- /dev/null +++ b/lldb/test/API/functionalities/scripted_frame_provider/runlock_reentrant_deadlock/main.c @@ -0,0 +1,18 @@ +#include <stdio.h> + +int g_value = 0; + +int increment() { return ++g_value; } + +int target_func() { + printf("target_func: %d\n", g_value); + return g_value; +} + +int main() { + for (int i = 0; i < 10; i++) { + target_func(); + } + increment(); + return 0; +} _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
