Author: Med Ismail Bennani Date: 2026-07-08T00:51:14Z New Revision: cc27a240be812f854801c24aefbea1243c5efde8
URL: https://github.com/llvm/llvm-project/commit/cc27a240be812f854801c24aefbea1243c5efde8 DIFF: https://github.com/llvm/llvm-project/commit/cc27a240be812f854801c24aefbea1243c5efde8.diff LOG: [lldb] Push ExpressionEvaluation policy and remove identity check fallbacks (#195775) Push `PolicyStack::Get().PushPublicStateRunningExpression()` 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 eval. Push `PolicyStack::Get().PushPrivateState()` 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 `PushPrivateStateRunningExpression()` 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. ---- The following PRs are related to the Policy feature: - #195762 - #195771 - #198897 - #195774 - #195775 rdar://176223894 Signed-off-by: Med Ismail Bennani <[email protected]> Added: Modified: lldb/include/lldb/Target/Process.h lldb/include/lldb/Utility/Policy.h lldb/source/Expression/FunctionCaller.cpp lldb/source/Expression/IRInterpreter.cpp lldb/source/Expression/LLVMUserExpression.cpp lldb/source/Target/Process.cpp lldb/source/Target/StackFrameList.cpp lldb/source/Target/StopInfo.cpp lldb/source/Target/Target.cpp lldb/source/Target/Thread.cpp lldb/source/Utility/Policy.cpp lldb/unittests/Utility/PolicyTest.cpp Removed: ################################################################################ diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h index 7b1424e6dfa59..9162158a277d9 100644 --- a/lldb/include/lldb/Target/Process.h +++ b/lldb/include/lldb/Target/Process.h @@ -53,6 +53,7 @@ #include "lldb/Utility/Event.h" #include "lldb/Utility/Listener.h" #include "lldb/Utility/NameMatches.h" +#include "lldb/Utility/Policy.h" #include "lldb/Utility/ProcessInfo.h" #include "lldb/Utility/Status.h" #include "lldb/Utility/StructuredData.h" @@ -2727,10 +2728,6 @@ void PruneThreadPlans(); ProcessRunLock &GetRunLock(); - bool CurrentThreadIsPrivateStateThread(); - - bool CurrentThreadPosesAsPrivateStateThread(); - virtual Status SendEventData(const char *data) { return Status::FromErrorString( "Sending an event is not supported for this process."); @@ -3317,11 +3314,20 @@ void PruneThreadPlans(); /// private state thread that we spin up when we need to run an expression on /// the private state thread. struct PrivateStateThread { + /// Why this PST exists. RunPrivateStateThread reads this directly to + /// decide which Policy to push, rather than re-deriving it from a + /// generic "is this an override PST" flag. This is the same enum + /// Policy::CreatePrivateState()/PolicyStack::PushPrivateState() take, so + /// there's a single purpose value flowing from PST creation through to + /// the policy it pushes. + using Purpose = Policy::PrivateStatePurpose; + PrivateStateThread(Process &process, lldb::StateType public_state, lldb::StateType private_state, - llvm::StringRef thread_name, bool is_override = false) + llvm::StringRef thread_name, + Purpose purpose = Purpose::Default) : m_process(process), m_public_state(public_state), - m_private_state(private_state), m_is_override(is_override), + m_private_state(private_state), m_purpose(purpose), m_thread_name(thread_name) {} // This returns false if we couldn't start up the thread. If that happens, // you won't be doing any debugging today. @@ -3340,7 +3346,7 @@ void PruneThreadPlans(); bool IsRunning() { return m_is_running; } - bool IsOverride() const { return m_is_override; } + bool IsOverride() const { return m_purpose != Purpose::Default; } void SetThreadName(llvm::StringRef new_name) { m_thread_name = new_name; } @@ -3406,7 +3412,7 @@ void PruneThreadPlans(); ProcessRunLock m_public_run_lock; ProcessRunLock m_private_run_lock; bool m_is_running = false; - bool m_is_override = false; + Purpose m_purpose; ///< This will be the thread name given to the Private State HostThread when ///< it gets spun up. std::string m_thread_name; @@ -3665,7 +3671,8 @@ void PruneThreadPlans(); // Starts up the private state thread that will watch for events from the // debugee. - lldb::thread_result_t RunPrivateStateThread(bool is_override); + lldb::thread_result_t + RunPrivateStateThread(PrivateStateThread::Purpose purpose); protected: void HandlePrivateEvent(lldb::EventSP &event_sp); diff --git a/lldb/include/lldb/Utility/Policy.h b/lldb/include/lldb/Utility/Policy.h index 0435bdd4a0e15..c841daf47a97d 100644 --- a/lldb/include/lldb/Utility/Policy.h +++ b/lldb/include/lldb/Utility/Policy.h @@ -52,6 +52,16 @@ struct Policy { bool can_run_frame_recognizers = true; }; + /// Why a private-state policy is being pushed. Distinguishes a PST's + /// ordinary private-state processing from a PST created to service + /// RunThreadPlan expression evaluation, which must not run frame + /// providers or recognizers (see StackFrameList::SelectMostRelevantFrame + /// and Thread::GetStackFrameList). + enum class PrivateStatePurpose { + Default, + RunningExpression, + }; + View view = View::Public; Capabilities capabilities; @@ -62,7 +72,8 @@ struct Policy { /// apply their named change on top. /// @{ static Policy CreatePublicState(); - static Policy CreatePrivateState(); + static Policy CreatePrivateState( + PrivateStatePurpose purpose = PrivateStatePurpose::Default); static Policy CreatePublicStateRunningExpression(); /// @} @@ -120,8 +131,10 @@ class PolicyStack { /// which already inherit from Current(). So the pushed policy preserves /// existing stack state instead of resetting unrelated fields. - [[nodiscard]] Guard PushPrivateState() { - Push(Policy::CreatePrivateState()); + [[nodiscard]] Guard + PushPrivateState(Policy::PrivateStatePurpose purpose = + Policy::PrivateStatePurpose::Default) { + Push(Policy::CreatePrivateState(purpose)); return Guard(); } diff --git a/lldb/source/Expression/FunctionCaller.cpp b/lldb/source/Expression/FunctionCaller.cpp index 6ea3faee98688..0a7dbb5b4ec6e 100644 --- a/lldb/source/Expression/FunctionCaller.cpp +++ b/lldb/source/Expression/FunctionCaller.cpp @@ -25,6 +25,7 @@ #include "lldb/Utility/ErrorMessages.h" #include "lldb/Utility/LLDBLog.h" #include "lldb/Utility/Log.h" +#include "lldb/Utility/Policy.h" #include "lldb/Utility/State.h" #include "lldb/ValueObject/ValueObject.h" #include "lldb/ValueObject/ValueObjectList.h" @@ -384,6 +385,9 @@ lldb::ExpressionResults FunctionCaller::ExecuteFunction( if (exe_ctx.GetProcessPtr()) exe_ctx.GetProcessPtr()->SetRunningUserExpression(true); + PolicyStack::Guard expr_policy_guard = + PolicyStack::Get().PushPublicStateRunningExpression(); + 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..163f9dac384eb 100644 --- a/lldb/source/Expression/IRInterpreter.cpp +++ b/lldb/source/Expression/IRInterpreter.cpp @@ -18,6 +18,7 @@ #include "lldb/Utility/Endian.h" #include "lldb/Utility/LLDBLog.h" #include "lldb/Utility/Log.h" +#include "lldb/Utility/Policy.h" #include "lldb/Utility/Scalar.h" #include "lldb/Utility/Status.h" #include "lldb/Utility/StreamString.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::PolicyStack::Get().PushPublicStateRunningExpression(); + // 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..d2c06cbf3ba72 100644 --- a/lldb/source/Expression/LLVMUserExpression.cpp +++ b/lldb/source/Expression/LLVMUserExpression.cpp @@ -31,6 +31,7 @@ #include "lldb/Utility/ErrorMessages.h" #include "lldb/Utility/LLDBLog.h" #include "lldb/Utility/Log.h" +#include "lldb/Utility/Policy.h" #include "lldb/Utility/StreamString.h" #include "lldb/ValueObject/ValueObjectConstResult.h" @@ -174,6 +175,9 @@ LLVMUserExpression::DoExecute(DiagnosticManager &diagnostic_manager, if (exe_ctx.GetProcessPtr()) exe_ctx.GetProcessPtr()->SetRunningUserExpression(true); + PolicyStack::Guard expr_policy_guard = + PolicyStack::Get().PushPublicStateRunningExpression(); + lldb::ExpressionResults execution_result = exe_ctx.GetProcessRef().RunThreadPlan(exe_ctx, call_plan_sp, options, diagnostic_manager); diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp index 4360250b21475..7be0c46447915 100644 --- a/lldb/source/Target/Process.cpp +++ b/lldb/source/Target/Process.cpp @@ -1287,7 +1287,12 @@ StateType Process::GetState() { if (policy.view == Policy::View::Private) return GetPrivateState(); - if (CurrentThreadPosesAsPrivateStateThread()) + // Once the private state thread has exited, nothing is left to consume the + // public state-changed event and update the public state accordingly (see + // Process::ProcessEventData::DoOnRemoval). The private state is always + // up to date, so fall back to it rather than reporting a stale public + // state indefinitely. + if (!m_current_private_state_thread_sp->IsRunning()) return GetPrivateState(); return GetPublicState(); @@ -4084,7 +4089,7 @@ bool Process::PrivateStateThread::StartupThread() { llvm::Expected<HostThread> private_state_thread = ThreadLauncher::LaunchThread( m_thread_name, - [this] { return m_process.RunPrivateStateThread(m_is_override); }, + [this] { return m_process.RunPrivateStateThread(m_purpose); }, 8 * 1024 * 1024); if (!private_state_thread) { LLDB_LOG_ERROR(GetLog(LLDBLog::Host), private_state_thread.takeError(), @@ -4107,8 +4112,6 @@ ProcessRunLock &Process::PrivateStateThread::GetRunLock() { Policy policy = PolicyStack::Get().Current(); if (policy.view == Policy::View::Private) return m_private_run_lock; - if (IsOnThread(Host::GetCurrentThread())) - return m_private_run_lock; return m_public_run_lock; } @@ -4152,7 +4155,7 @@ bool Process::StartPrivateStateThread( *backup_ptr = m_current_private_state_thread_sp; m_current_private_state_thread_sp.reset(new PrivateStateThread( *this, GetPublicState(), GetPrivateState(), thread_name, - /*is_override=*/true)); + PrivateStateThread::Purpose::RunningExpression)); } else m_current_private_state_thread_sp->SetThreadName(thread_name); @@ -4367,12 +4370,14 @@ Status Process::HaltPrivate() { return error; } -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<PolicyStack::Guard> policy_guard; - if (is_override) - policy_guard = PolicyStack::Get().PushPrivateState(); +thread_result_t +Process::RunPrivateStateThread(PrivateStateThread::Purpose purpose) { + // All PSTs see the private reality (private state, private run lock). + // A PST created to run an expression additionally skips frame providers + // and recognizers, since that's the only reason RunThreadPlan spins up a + // second, temporary PST while the primary one is backed up. + PolicyStack::Guard policy_guard = + PolicyStack::Get().PushPrivateState(purpose); bool control_only = true; @@ -5427,7 +5432,8 @@ Process::RunThreadPlan(ExecutionContext &exe_ctx, // GetStackFrameList returns parent frames during event processing. std::optional<PolicyStack::Guard> policy_guard; if (backup_private_state_thread) - policy_guard = PolicyStack::Get().PushPrivateState(); + policy_guard = PolicyStack::Get().PushPrivateState( + Policy::PrivateStatePurpose::RunningExpression); while (true) { // We usually want to resume the process if we get to the top of the @@ -6151,24 +6157,6 @@ ProcessRunLock &Process::GetRunLock() { return m_current_private_state_thread_sp->GetRunLock(); } -bool Process::CurrentThreadIsPrivateStateThread() -{ - if (!m_current_private_state_thread_sp) - return true; - return m_current_private_state_thread_sp->IsOnThread( - Host::GetCurrentThread()); -} - -bool Process::CurrentThreadPosesAsPrivateStateThread() { - // If we haven't started up the private state thread yet, then whatever thread - // is fetching this event should be temporarily the private state thread. - if (!m_current_private_state_thread_sp || - !m_current_private_state_thread_sp->IsRunning()) - return true; - return m_current_private_state_thread_sp->IsOnThread( - Host::GetCurrentThread()); -} - void Process::Flush() { m_thread_list.Flush(); m_extended_thread_list.Flush(); diff --git a/lldb/source/Target/StackFrameList.cpp b/lldb/source/Target/StackFrameList.cpp index 01ce5870a6edb..465d1af11add0 100644 --- a/lldb/source/Target/StackFrameList.cpp +++ b/lldb/source/Target/StackFrameList.cpp @@ -804,14 +804,12 @@ bool StackFrameList::SetFrameAtIndex(uint32_t idx, StackFrameSP &frame_sp) { } 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. + // Don't call into the frame recognizers while evaluating an expression on + // the private state thread, as they can cause code to run in the inferior + // process, and that can cause deadlocks when fetching stop events for the + // expression. Policy policy = PolicyStack::Get().Current(); - if (policy.view == Policy::View::Private) - return; - - if (m_thread.GetProcess()->CurrentThreadPosesAsPrivateStateThread()) + 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 c20b0ed07ee3c..f2cf1a75851c6 100644 --- a/lldb/source/Target/StopInfo.cpp +++ b/lldb/source/Target/StopInfo.cpp @@ -436,8 +436,7 @@ class StopInfoBreakpoint : public StopInfo { ExecutionContext exe_ctx(thread_sp->GetStackFrameAtIndex(0)); Process *process = exe_ctx.GetProcessPtr(); Policy policy = PolicyStack::Get().Current(); - if (!policy.capabilities.can_run_breakpoint_actions || - process->GetModIDRef().IsRunningExpression()) { + 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 diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp index 23e45f1d4fd76..d1d07b22e382c 100644 --- a/lldb/source/Target/Target.cpp +++ b/lldb/source/Target/Target.cpp @@ -5996,9 +5996,6 @@ std::recursive_mutex &Target::GetAPIMutex() { if (policy.view == Policy::View::Private) return m_private_mutex; - if (GetProcessSP() && GetProcessSP()->CurrentThreadIsPrivateStateThread()) - return m_private_mutex; - return m_mutex; } diff --git a/lldb/source/Target/Thread.cpp b/lldb/source/Target/Thread.cpp index f25a14fbe92e8..e7d1e22953870 100644 --- a/lldb/source/Target/Thread.cpp +++ b/lldb/source/Target/Thread.cpp @@ -1527,18 +1527,22 @@ StackFrameListSP Thread::GetStackFrameList() { // which resumed the process via RunThreadPlan; the private state // thread must process the resulting stop event, but if it tries to // build the synthetic frame list it will re-enter the provider -> - // deadlock. + // deadlock. Detected via the policy capability below rather than + // thread identity, since m_current_private_state_thread_sp may + // already have been reassigned to an override PST by the time this + // runs. // - Any other thread: would run the provider concurrently with the // thread that is already mid-construction. // - // 2. Current thread is a private state thread that should see the - // private reality (Policy::View::Private). + // 2. The current policy disallows loading frame providers (expression + // evaluation on a PST), which requires the private reality. // // For case 1, if a provider is active we return its input (parent) // frames. For case (2), we return/create the unwinder frame list // without caching it in m_curr_frames_sp so that non-private-state // callers still get the public illusion once the process settles. ProcessSP process_sp = GetProcess(); + Policy policy = PolicyStack::Get().Current(); { std::lock_guard<std::mutex> pguard(m_provider_frames_mutex); if (!m_active_frame_providers_by_thread.empty()) { @@ -1549,7 +1553,7 @@ StackFrameListSP Thread::GetStackFrameList() { return it->second.back(); // Case 1b: private state thread while a provider is active elsewhere. - if (process_sp && process_sp->CurrentThreadIsPrivateStateThread()) + if (!policy.capabilities.can_load_frame_providers) return m_active_frame_providers_by_thread.begin()->second.back(); } } @@ -1558,10 +1562,10 @@ StackFrameListSP Thread::GetStackFrameList() { return m_curr_frames_sp; // The private state thread must see the raw unwinder frames, not the - // provider-augmented public view. Policy::CreatePrivateState is pushed by - // RunThreadPlan and RunPrivateStateThread. - Policy policy = PolicyStack::Get().Current(); - if (policy.view == Policy::View::Private) { + // provider-augmented public view, while it is servicing expression + // evaluation. PushPrivateState(RunningExpression) is pushed by + // RunThreadPlan and by RunPrivateStateThread for override PSTs. + if (!policy.capabilities.can_load_frame_providers) { if (!m_unwinder_frames_sp) m_unwinder_frames_sp = std::make_shared<StackFrameList>( *this, m_prev_frames_sp, true, /*provider_id=*/0); diff --git a/lldb/source/Utility/Policy.cpp b/lldb/source/Utility/Policy.cpp index 104df17df7a97..3c2fa8f7fc867 100644 --- a/lldb/source/Utility/Policy.cpp +++ b/lldb/source/Utility/Policy.cpp @@ -31,11 +31,17 @@ Policy PolicyStack::Current() const { // (tests, dump comparisons); it never reads the current stack. Policy Policy::CreatePublicState() { return {}; } -Policy Policy::CreatePrivateState() { +Policy Policy::CreatePrivateState(PrivateStatePurpose purpose) { Policy p = PolicyStack::Get().Current(); p.view = View::Private; - p.capabilities.can_load_frame_providers = false; - p.capabilities.can_run_frame_recognizers = false; + switch (purpose) { + case PrivateStatePurpose::Default: + break; + case PrivateStatePurpose::RunningExpression: + p.capabilities.can_load_frame_providers = false; + p.capabilities.can_run_frame_recognizers = false; + break; + } return p; } diff --git a/lldb/unittests/Utility/PolicyTest.cpp b/lldb/unittests/Utility/PolicyTest.cpp index c582f16a3d7bb..5ad045a03d30b 100644 --- a/lldb/unittests/Utility/PolicyTest.cpp +++ b/lldb/unittests/Utility/PolicyTest.cpp @@ -43,6 +43,18 @@ TEST(PolicyTest, PrivateState) { EXPECT_TRUE(p.capabilities.can_run_all_threads); EXPECT_TRUE(p.capabilities.can_try_all_threads); EXPECT_TRUE(p.capabilities.can_run_breakpoint_actions); + EXPECT_TRUE(p.capabilities.can_load_frame_providers); + EXPECT_TRUE(p.capabilities.can_run_frame_recognizers); +} + +TEST(PolicyTest, PrivateStateRunningExpression) { + Policy p = Policy::CreatePrivateState( + Policy::PrivateStatePurpose::RunningExpression); + EXPECT_EQ(p.view, Policy::View::Private); + EXPECT_TRUE(p.capabilities.can_evaluate_expressions); + EXPECT_TRUE(p.capabilities.can_run_all_threads); + EXPECT_TRUE(p.capabilities.can_try_all_threads); + EXPECT_TRUE(p.capabilities.can_run_breakpoint_actions); EXPECT_FALSE(p.capabilities.can_load_frame_providers); EXPECT_FALSE(p.capabilities.can_run_frame_recognizers); } @@ -67,7 +79,8 @@ TEST(PolicyTest, StackDefaultIsPublicState) { TEST(PolicyTest, StackPushPop) { { - PolicyStack::Guard guard = PolicyStack::Get().PushPrivateState(); + PolicyStack::Guard guard = PolicyStack::Get().PushPrivateState( + Policy::PrivateStatePurpose::RunningExpression); EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private); EXPECT_FALSE( PolicyStack::Get().Current().capabilities.can_load_frame_providers); @@ -92,7 +105,8 @@ TEST(PolicyTest, GuardRAII) { EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Public); { - PolicyStack::Guard guard = PolicyStack::Get().PushPrivateState(); + PolicyStack::Guard guard = PolicyStack::Get().PushPrivateState( + Policy::PrivateStatePurpose::RunningExpression); EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private); EXPECT_FALSE( PolicyStack::Get().Current().capabilities.can_load_frame_providers); @@ -140,7 +154,7 @@ TEST(PolicyTest, DumpPrivateState) { EXPECT_EQ(s.GetString(), "policy: view=private, capabilities={" "eval_expr=true run_all=true try_all=true " - "bp_actions=true frame_providers=false frame_recognizers=false}"); + "bp_actions=true frame_providers=true frame_recognizers=true}"); } TEST(PolicyTest, DumpStack) { @@ -186,8 +200,9 @@ TEST(PolicyStackDeathTest, GuardDestroyedOnDifferentThread) { TEST(PolicyTest, PushInheritsFromCurrent) { // Push* methods inherit from Current() rather than starting from a // default Policy: stacking PushPublicStateRunningExpression on top of - // PushPrivateState must preserve the Private view. - PolicyStack::Guard outer = PolicyStack::Get().PushPrivateState(); + // PushPrivateState(RunningExpression) must preserve the Private view. + PolicyStack::Guard outer = PolicyStack::Get().PushPrivateState( + Policy::PrivateStatePurpose::RunningExpression); EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private); PolicyStack::Guard inner = _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
