https://github.com/medismailben updated https://github.com/llvm/llvm-project/pull/195775
>From da7565d8733704276b1127e1c7f61d5cc1b62d3a Mon Sep 17 00:00:00 2001 From: Med Ismail Bennani <[email protected]> Date: Thu, 2 Jul 2026 19:31:41 -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/Utility/Policy.h | 6 +++++ lldb/source/Expression/FunctionCaller.cpp | 4 ++++ lldb/source/Expression/IRInterpreter.cpp | 4 ++++ lldb/source/Expression/LLVMUserExpression.cpp | 4 ++++ lldb/source/Target/Process.cpp | 22 ++++++++--------- lldb/source/Target/StackFrameList.cpp | 5 +--- lldb/source/Target/StopInfo.cpp | 3 +-- lldb/source/Target/Target.cpp | 3 --- lldb/source/Target/Thread.cpp | 11 +++++---- lldb/source/Utility/Policy.cpp | 5 ++++ lldb/unittests/Utility/PolicyTest.cpp | 24 +++++++++++++++---- 11 files changed, 61 insertions(+), 30 deletions(-) diff --git a/lldb/include/lldb/Utility/Policy.h b/lldb/include/lldb/Utility/Policy.h index 0435bdd4a0e15..3ff73a7e81725 100644 --- a/lldb/include/lldb/Utility/Policy.h +++ b/lldb/include/lldb/Utility/Policy.h @@ -63,6 +63,7 @@ struct Policy { /// @{ static Policy CreatePublicState(); static Policy CreatePrivateState(); + static Policy CreatePrivateStateRunningExpression(); static Policy CreatePublicStateRunningExpression(); /// @} @@ -125,6 +126,11 @@ class PolicyStack { return Guard(); } + [[nodiscard]] Guard PushPrivateStateRunningExpression() { + Push(Policy::CreatePrivateStateRunningExpression()); + return Guard(); + } + [[nodiscard]] Guard PushPublicStateRunningExpression() { Push(Policy::CreatePublicStateRunningExpression()); 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..5c041307491f0 100644 --- a/lldb/source/Target/Process.cpp +++ b/lldb/source/Target/Process.cpp @@ -1287,9 +1287,6 @@ StateType Process::GetState() { if (policy.view == Policy::View::Private) return GetPrivateState(); - if (CurrentThreadPosesAsPrivateStateThread()) - return GetPrivateState(); - return GetPublicState(); } @@ -4107,8 +4104,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; } @@ -4368,11 +4363,16 @@ Status Process::HaltPrivate() { } 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(); + // All PSTs see the private reality (private state, private run lock). + // Override PSTs additionally skip frame providers and recognizers. At + // present, expression evaluation (RunThreadPlan) is the only reason we + // ever create an override PST, so is_override is treated as synonymous + // with "is running an expression" here. If another modal interaction + // starts spinning up an override PST for a different reason, this + // assumption breaks and we'll need a way to distinguish the two cases. + PolicyStack::Guard policy_guard = + is_override ? PolicyStack::Get().PushPrivateStateRunningExpression() + : PolicyStack::Get().PushPrivateState(); bool control_only = true; @@ -5427,7 +5427,7 @@ 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().PushPrivateStateRunningExpression(); while (true) { // We usually want to resume the process if we get to the top of the diff --git a/lldb/source/Target/StackFrameList.cpp b/lldb/source/Target/StackFrameList.cpp index 01ce5870a6edb..60d5585777084 100644 --- a/lldb/source/Target/StackFrameList.cpp +++ b/lldb/source/Target/StackFrameList.cpp @@ -808,10 +808,7 @@ void StackFrameList::SelectMostRelevantFrame() { // they can cause code to run in the target, 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..45aebe267e301 100644 --- a/lldb/source/Target/Thread.cpp +++ b/lldb/source/Target/Thread.cpp @@ -1531,8 +1531,8 @@ StackFrameListSP Thread::GetStackFrameList() { // - 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 @@ -1558,10 +1558,11 @@ 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. + // provider-augmented public view, while it is servicing expression + // evaluation. PushPrivateStateRunningExpression is pushed by + // RunThreadPlan and by RunPrivateStateThread for override PSTs. Policy policy = PolicyStack::Get().Current(); - if (policy.view == Policy::View::Private) { + 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..2284985903e6f 100644 --- a/lldb/source/Utility/Policy.cpp +++ b/lldb/source/Utility/Policy.cpp @@ -34,6 +34,11 @@ Policy Policy::CreatePublicState() { return {}; } Policy Policy::CreatePrivateState() { Policy p = PolicyStack::Get().Current(); p.view = View::Private; + return p; +} + +Policy Policy::CreatePrivateStateRunningExpression() { + Policy p = CreatePrivateState(); p.capabilities.can_load_frame_providers = false; p.capabilities.can_run_frame_recognizers = false; return p; diff --git a/lldb/unittests/Utility/PolicyTest.cpp b/lldb/unittests/Utility/PolicyTest.cpp index c582f16a3d7bb..bef3920e03e24 100644 --- a/lldb/unittests/Utility/PolicyTest.cpp +++ b/lldb/unittests/Utility/PolicyTest.cpp @@ -43,6 +43,17 @@ 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::CreatePrivateStateRunningExpression(); + 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 +78,8 @@ TEST(PolicyTest, StackDefaultIsPublicState) { TEST(PolicyTest, StackPushPop) { { - PolicyStack::Guard guard = PolicyStack::Get().PushPrivateState(); + PolicyStack::Guard guard = + PolicyStack::Get().PushPrivateStateRunningExpression(); EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private); EXPECT_FALSE( PolicyStack::Get().Current().capabilities.can_load_frame_providers); @@ -92,7 +104,8 @@ TEST(PolicyTest, GuardRAII) { EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Public); { - PolicyStack::Guard guard = PolicyStack::Get().PushPrivateState(); + PolicyStack::Guard guard = + PolicyStack::Get().PushPrivateStateRunningExpression(); EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private); EXPECT_FALSE( PolicyStack::Get().Current().capabilities.can_load_frame_providers); @@ -140,7 +153,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 +199,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(); + // PushPrivateStateRunningExpression must preserve the Private view. + PolicyStack::Guard outer = + PolicyStack::Get().PushPrivateStateRunningExpression(); 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
