https://github.com/medismailben updated https://github.com/llvm/llvm-project/pull/195775
>From 4e52db9cd6f92dcc595d950f75c4df24e95adf28 Mon Sep 17 00:00:00 2001 From: Med Ismail Bennani <[email protected]> Date: Tue, 7 Jul 2026 15:20:50 -0700 Subject: [PATCH] [lldb] Push ExpressionEvaluation policy and remove identity check fallbacks 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. rdar://176223894 Signed-off-by: Med Ismail Bennani <[email protected]> --- lldb/include/lldb/Target/Process.h | 25 ++++++---- lldb/include/lldb/Utility/Policy.h | 19 ++++++-- lldb/source/Expression/FunctionCaller.cpp | 4 ++ lldb/source/Expression/IRInterpreter.cpp | 4 ++ lldb/source/Expression/LLVMUserExpression.cpp | 4 ++ lldb/source/Target/Process.cpp | 48 +++++++------------ lldb/source/Target/StackFrameList.cpp | 12 ++--- lldb/source/Target/StopInfo.cpp | 3 +- lldb/source/Target/Target.cpp | 3 -- lldb/source/Target/Thread.cpp | 20 ++++---- lldb/source/Utility/Policy.cpp | 14 ++++-- lldb/unittests/Utility/PolicyTest.cpp | 25 ++++++++-- 12 files changed, 111 insertions(+), 70 deletions(-) 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..505f007a7a3ae 100644 --- a/lldb/source/Utility/Policy.cpp +++ b/lldb/source/Utility/Policy.cpp @@ -31,11 +31,19 @@ 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; + default: + llvm_unreachable("unknown PrivateStatePurpose"); + } 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
