llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Med Ismail Bennani (medismailben) <details> <summary>Changes</summary> Push `Policy::PublicStateRunningExpression` at all three expression evaluation entry points (`LLVMUserExpression::DoExecute`, `FunctionCaller::ExecuteFunction`, `IRInterpreter`). This policy sets `can_run_breakpoint_actions=false`, preventing recursive breakpoint callback execution during expression evaluation. `Push Policy::PrivateState` unconditionally for all PSTs in `RunPrivateStateThread` (not just overrides), giving every PST the private view while keeping frame providers and recognizers enabled for normal stop processing. Override PSTs use `Policy::PrivateStateRunningExpression` which additionally disables providers and recognizers. With all PSTs and expression eval sites now covered by the policy, remove all host thread identity check fallbacks: - `CurrentThreadPosesAsPrivateStateThread` in `Process::GetState` - `CurrentThreadIsPrivateStateThread` in `Target::GetAPIMutex` - `IsOnThread` in `PrivateStateThread::GetRunLock` - `CurrentThreadPosesAsPrivateStateThread` in `SelectMostRelevantFrame` - `IsRunningExpression` in `StopInfoBreakpoint::PerformAction` `SelectMostRelevantFrame` now checks `!can_run_frame_recognizers` instead of `View::Private`, so recognizers run during normal PST stop processing but are skipped during expression evaluation. rdar://176223894 Signed-off-by: Med Ismail Bennani <ismail@<!-- -->bennani.ma> --- Patch is 32.91 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/195775.diff 18 Files Affected: - (modified) lldb/include/lldb/Host/ProcessRunLock.h (+4-21) - (added) lldb/include/lldb/Target/Policy.h (+86) - (modified) lldb/include/lldb/Target/Process.h (+4-25) - (added) lldb/include/lldb/Utility/ThreadLocalPolicyStack.h (+57) - (modified) lldb/source/Expression/FunctionCaller.cpp (+3) - (modified) lldb/source/Expression/IRInterpreter.cpp (+4) - (modified) lldb/source/Expression/LLVMUserExpression.cpp (+4) - (modified) lldb/source/Host/common/ProcessRunLock.cpp (+38) - (modified) lldb/source/Target/Process.cpp (+14-13) - (modified) lldb/source/Target/StackFrameList.cpp (+3-1) - (modified) lldb/source/Target/StopInfo.cpp (+19-1) - (modified) lldb/source/Target/Target.cpp (+5-3) - (modified) lldb/source/Target/Thread.cpp (+9-7) - (added) lldb/test/API/functionalities/scripted_frame_provider/runlock_reentrant_deadlock/Makefile (+3) - (added) lldb/test/API/functionalities/scripted_frame_provider/runlock_reentrant_deadlock/TestRunLockReentrantDeadlock.py (+125) - (added) lldb/test/API/functionalities/scripted_frame_provider/runlock_reentrant_deadlock/bkpt_resolver.py (+47) - (added) lldb/test/API/functionalities/scripted_frame_provider/runlock_reentrant_deadlock/frame_provider.py (+38) - (added) lldb/test/API/functionalities/scripted_frame_provider/runlock_reentrant_deadlock/main.c (+18) ``````````diff 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 new file mode 100644 index 0000000000000..c16d1aa83cd6d --- /dev/null +++ b/lldb/include/lldb/Target/Policy.h @@ -0,0 +1,86 @@ +//===----------------------------------------------------------------------===// +// +// 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; + bool holds_run_lock : 1; + }; + + View view = View::Public; + Capabilities capabilities = { + /*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, + /*holds_run_lock=*/false, + }; + + static Policy PublicState() { return {}; } + + static Policy PrivateStateRunningExpression() { + Policy p; + p.view = View::Private; + p.capabilities.can_load_frame_providers = false; + p.capabilities.can_run_frame_recognizers = false; + return p; + } + + static Policy PrivateState() { + Policy p; + p.view = View::Private; + 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/Target/Process.h b/lldb/include/lldb/Target/Process.h index 67a9a883cd36c..145d31e60b52e 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,10 @@ void PruneThreadPlans(); } ProcessRunLock &GetRunLock() { - if (IsOnThread(Host::GetCurrentThread())) + auto &policy = PolicyStack::GetForCurrentThread().Current(); + if (policy.view == Policy::View::Private) return m_private_run_lock; - else - return m_public_run_lock; + return m_public_run_lock; } Process &m_process; @@ -3719,28 +3720,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/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 diff --git a/lldb/source/Expression/FunctionCaller.cpp b/lldb/source/Expression/FunctionCaller.cpp index 6ea3faee98688..522261f63b631 100644 --- a/lldb/source/Expression/FunctionCaller.cpp +++ b/lldb/source/Expression/FunctionCaller.cpp @@ -15,6 +15,7 @@ #include "lldb/Symbol/Function.h" #include "lldb/Symbol/Type.h" #include "lldb/Target/ExecutionContext.h" +#include "lldb/Target/Policy.h" #include "lldb/Target/Process.h" #include "lldb/Target/RegisterContext.h" #include "lldb/Target/Target.h" @@ -384,6 +385,8 @@ lldb::ExpressionResults FunctionCaller::ExecuteFunction( if (exe_ctx.GetProcessPtr()) exe_ctx.GetProcessPtr()->SetRunningUserExpression(true); + PolicyStack::Guard expr_policy_guard(Policy::PublicStateRunningExpression()); + return_value = exe_ctx.GetProcessRef().RunThreadPlan( exe_ctx, call_plan_sp, real_options, diagnostic_manager); diff --git a/lldb/source/Expression/IRInterpreter.cpp b/lldb/source/Expression/IRInterpreter.cpp index 69e7d0b327803..7c0ab05c2bfa6 100644 --- a/lldb/source/Expression/IRInterpreter.cpp +++ b/lldb/source/Expression/IRInterpreter.cpp @@ -25,6 +25,7 @@ #include "lldb/Target/ABI.h" #include "lldb/Target/ExecutionContext.h" +#include "lldb/Target/Policy.h" #include "lldb/Target/Target.h" #include "lldb/Target/Thread.h" #include "lldb/Target/ThreadPlan.h" @@ -1581,6 +1582,9 @@ bool IRInterpreter::Interpret(llvm::Module &module, llvm::Function &function, process->SetRunningUserExpression(true); + lldb_private::PolicyStack::Guard expr_policy_guard( + lldb_private::Policy::PublicStateRunningExpression()); + // Execute the actual function call thread plan lldb::ExpressionResults res = process->RunThreadPlan(exe_ctx, call_plan_sp, options, diagnostics); diff --git a/lldb/source/Expression/LLVMUserExpression.cpp b/lldb/source/Expression/LLVMUserExpression.cpp index 2d59194027b57..5f5a5f9d66f52 100644 --- a/lldb/source/Expression/LLVMUserExpression.cpp +++ b/lldb/source/Expression/LLVMUserExpression.cpp @@ -22,6 +22,7 @@ #include "lldb/Symbol/VariableList.h" #include "lldb/Target/ABI.h" #include "lldb/Target/ExecutionContext.h" +#include "lldb/Target/Policy.h" #include "lldb/Target/Process.h" #include "lldb/Target/StackFrame.h" #include "lldb/Target/Target.h" @@ -174,6 +175,9 @@ LLVMUserExpression::DoExecute(DiagnosticManager &diagnostic_manager, if (exe_ctx.GetProcessPtr()) exe_ctx.GetProcessPtr()->SetRunningUserExpression(true); + PolicyStack::Guard expr_policy_guard( + Policy::PublicStateRunningExpression()); + lldb::ExpressionResults execution_result = exe_ctx.GetProcessRef().RunThreadPlan(exe_ctx, call_plan_sp, options, diagnostic_manager); 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/source/Target/Process.cpp b/lldb/source/Target/Process.cpp index 114bbd7355f0c..7b35b544537f2 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,11 @@ StateType Process::GetState() { if (!m_current_private_state_thread_sp) return eStateUnloaded; - if (CurrentThreadPosesAsPrivateStateThread()) + auto &policy = PolicyStack::GetForCurrentThread().Current(); + if (policy.view == Policy::View::Private) return GetPrivateState(); - else - return GetPublicState(); + + return GetPublicState(); } void Process::SetPublicState(StateType new_state, bool restarted) { @@ -4320,14 +4322,13 @@ 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; - if (is_override) - pst_guard.emplace(); + // All PSTs see the private reality (private state, private run lock). + // Override PSTs additionally skip frame providers and recognizers since + // they exist solely to service RunThreadPlan expression evaluation. + PolicyStack::Guard policy_guard(is_override + ? Policy::PrivateStateRunningExpression() + : Policy::PrivateState()); bool control_only = true; @@ -5525,9 +5526,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::PrivateStateRunningExpression()); while (true) { // We usually want to resume the process if we get to the top of the @@ -5929,7 +5930,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..453961caaa774 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,7 +807,8 @@ void StackFrameList::SelectMostRelevantFrame() { // Don't call into the frame recognizers on the private state thread as // they can cause code to run in the target, and that can cause deadlocks // when fetching stop events for the expression. - if (m_thread.GetProcess()->CurrentThreadPosesAsPrivateStateThread()) + auto &policy = PolicyStack::GetForCurrentThread().Current(); + if (!policy.capabilities.can_run_frame_recognizers) return; Log *log = GetLog(LLDBLog::Thread); diff --git a/lldb/source/Target/StopInfo.cpp b/lldb/source/Target/StopInfo.cpp index 2a7ea2808d87f..103ea24004dc7 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,8 @@ 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) { // 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 +867,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 +978,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..2006c3564b5d3 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,11 @@ Target::TargetEventData::GetModuleListFromEvent(const Event ... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/195775 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
