https://github.com/medismailben updated https://github.com/llvm/llvm-project/pull/195771
>From 83e26275ba34ab4f3dcf034faa3b693746b12950 Mon Sep 17 00:00:00 2001 From: Med Ismail Bennani <[email protected]> Date: Wed, 6 May 2026 16:50:36 -0700 Subject: [PATCH] [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 | 10 ++-- lldb/include/lldb/Target/Process.h | 29 ++--------- lldb/source/Target/Policy.cpp | 21 ++++++-- lldb/source/Target/Process.cpp | 22 +++++---- lldb/source/Target/StackFrameList.cpp | 5 ++ lldb/source/Target/StopInfo.cpp | 24 +++++++++- lldb/source/Target/Target.cpp | 9 +++- lldb/source/Target/Thread.cpp | 15 +++--- lldb/unittests/Target/PolicyTest.cpp | 69 +++++++++++++-------------- 9 files changed, 115 insertions(+), 89 deletions(-) diff --git a/lldb/include/lldb/Target/Policy.h b/lldb/include/lldb/Target/Policy.h index 4f3d6f4bee7ff..b398b6bfd650a 100644 --- a/lldb/include/lldb/Target/Policy.h +++ b/lldb/include/lldb/Target/Policy.h @@ -84,17 +84,17 @@ struct Policy { /// thread's stack when the task starts. class PolicyStack { public: - static PolicyStack &GetForCurrentThread() { + static PolicyStack &Get() { static thread_local PolicyStack s_stack; return s_stack; } - Policy Current() const { return m_stack.back(); } + Policy Current() const; void Push(Policy policy) { m_stack.push_back(std::move(policy)); } void Pop() { - assert(!m_stack.empty() && "can't pop the last policy"); + assert(!m_stack.empty() && "can't pop the base policy"); m_stack.pop_back(); } @@ -103,8 +103,8 @@ class PolicyStack { /// RAII guard that pushes a policy on construction and pops on destruction. class Guard { public: - explicit Guard(Policy policy) { GetForCurrentThread().Push(policy); } - ~Guard() { GetForCurrentThread().Pop(); } + explicit Guard(Policy policy) { Get().Push(std::move(policy)); } + ~Guard() { Get().Pop(); } Guard(const Guard &) = delete; Guard &operator=(const Guard &) = delete; diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h index 67a9a883cd36c..27d374ee5dc63 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() { + 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; - 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/Policy.cpp b/lldb/source/Target/Policy.cpp index b21ac3e074ca2..0757bcc1bf397 100644 --- a/lldb/source/Target/Policy.cpp +++ b/lldb/source/Target/Policy.cpp @@ -7,20 +7,33 @@ //===----------------------------------------------------------------------===// #include "lldb/Target/Policy.h" +#include "lldb/Utility/LLDBLog.h" +#include "lldb/Utility/Log.h" #include "lldb/Utility/Stream.h" +#include "lldb/Utility/StreamString.h" using namespace lldb_private; +Policy PolicyStack::Current() const { + Policy p = m_stack.back(); + if (Log *log = GetLog(LLDBLog::Process)) { + StreamString s; + p.Dump(s); + LLDB_LOG(log, "{0}", s.GetData()); + } + return p; +} + void Policy::Dump(Stream &s) const { - s.Printf("view=%s", view == View::Public ? "public" : "private"); - s.PutCString(", capabilities={"); + s << "policy: view=" << (view == View::Public ? "public" : "private") + << ", capabilities={"; s.Printf("eval_expr=%d", capabilities.can_evaluate_expressions); s.Printf(" run_all=%d", capabilities.can_run_all_threads); s.Printf(" try_all=%d", capabilities.can_try_all_threads); s.Printf(" bp_actions=%d", capabilities.can_run_breakpoint_actions); s.Printf(" frame_providers=%d", capabilities.can_load_frame_providers); s.Printf(" frame_recognizers=%d", capabilities.can_run_frame_recognizers); - s.PutChar('}'); + s << '}'; } void PolicyStack::Dump(Stream &s) const { @@ -28,6 +41,6 @@ void PolicyStack::Dump(Stream &s) const { for (size_t i = 0; i < m_stack.size(); i++) { s.Printf(" [%zu] ", i); m_stack[i].Dump(s); - s.PutChar('\n'); + s << '\n'; } } diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp index 9fa6d5b26e9de..b846acf2f4482 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" @@ -73,6 +74,7 @@ #include "lldb/Utility/ProcessInfo.h" #include "lldb/Utility/SelectHelper.h" #include "lldb/Utility/State.h" +#include "lldb/Utility/StreamString.h" #include "lldb/Utility/Timer.h" using namespace lldb; @@ -1277,10 +1279,14 @@ StateType Process::GetState() { if (!m_current_private_state_thread_sp) return eStateUnloaded; + Policy policy = PolicyStack::Get().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) { @@ -4328,14 +4334,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; @@ -5533,9 +5537,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 @@ -5937,7 +5941,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..97b54403bec8b 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. + Policy policy = PolicyStack::Get().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..768d2e8fc0dae 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) { @@ -393,7 +398,9 @@ class StopInfoBreakpoint : public StopInfo { ExecutionContext exe_ctx(thread_sp->GetStackFrameAtIndex(0)); Process *process = exe_ctx.GetProcessPtr(); - if (process->GetModIDRef().IsRunningExpression()) { + Policy policy = PolicyStack::Get().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 +864,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) @@ -965,6 +976,17 @@ class StopInfoWatchpoint : public StopInfo { void PerformAction(Event *event_ptr) override { Log *log = GetLog(LLDBLog::Watchpoints); + + Policy policy = PolicyStack::Get().Current(); + if (!policy.capabilities.can_run_breakpoint_actions) { + m_should_stop = false; + m_should_stop_is_valid = true; + LLDB_LOGF(log, "StopInfoWatchpoint::PerformAction - Hit a " + "watchpoint while running an expression," + " not running commands to avoid recursion."); + return; + } + // 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 // here. diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp index d7611f470b9b6..1c2e96ab59517 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" @@ -5889,10 +5890,14 @@ Target::TargetEventData::GetModuleListFromEvent(const Event *event_ptr) { } std::recursive_mutex &Target::GetAPIMutex() { + Policy policy = PolicyStack::Get().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..89e8bb468de7c 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 @@ -1558,13 +1559,11 @@ StackFrameListSP Thread::GetStackFrameList() { if (m_curr_frames_sp) 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()) { + // The private state thread must see the raw unwinder frames, not the + // provider-augmented public view. Policy::PrivateState is pushed by + // RunThreadPlan and RunPrivateStateThread. + Policy policy = PolicyStack::Get().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); diff --git a/lldb/unittests/Target/PolicyTest.cpp b/lldb/unittests/Target/PolicyTest.cpp index 691ca52b34c31..0b6a34df4f1fe 100644 --- a/lldb/unittests/Target/PolicyTest.cpp +++ b/lldb/unittests/Target/PolicyTest.cpp @@ -59,74 +59,72 @@ TEST(PolicyTest, PublicStateRunningExpression) { } TEST(PolicyTest, StackDefaultIsPublicState) { - PolicyStack &stack = PolicyStack::GetForCurrentThread(); - Policy current = stack.Current(); + Policy current = PolicyStack::Get().Current(); EXPECT_EQ(current.view, Policy::View::Public); EXPECT_TRUE(current.capabilities.can_evaluate_expressions); EXPECT_TRUE(current.capabilities.can_load_frame_providers); } TEST(PolicyTest, StackPushPop) { - PolicyStack &stack = PolicyStack::GetForCurrentThread(); + PolicyStack::Get().Push(Policy::PrivateState()); + EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private); + EXPECT_FALSE( + PolicyStack::Get().Current().capabilities.can_load_frame_providers); - stack.Push(Policy::PrivateState()); - EXPECT_EQ(stack.Current().view, Policy::View::Private); - EXPECT_FALSE(stack.Current().capabilities.can_load_frame_providers); + PolicyStack::Get().Push(Policy::PublicStateRunningExpression()); + EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Public); + EXPECT_FALSE( + PolicyStack::Get().Current().capabilities.can_run_breakpoint_actions); - stack.Push(Policy::PublicStateRunningExpression()); - EXPECT_EQ(stack.Current().view, Policy::View::Public); - EXPECT_FALSE(stack.Current().capabilities.can_run_breakpoint_actions); + PolicyStack::Get().Pop(); + EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private); - stack.Pop(); - EXPECT_EQ(stack.Current().view, Policy::View::Private); - - stack.Pop(); - EXPECT_EQ(stack.Current().view, Policy::View::Public); + PolicyStack::Get().Pop(); + EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Public); } TEST(PolicyTest, GuardRAII) { - PolicyStack &stack = PolicyStack::GetForCurrentThread(); - EXPECT_EQ(stack.Current().view, Policy::View::Public); + EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Public); { PolicyStack::Guard guard(Policy::PrivateState()); - EXPECT_EQ(stack.Current().view, Policy::View::Private); - EXPECT_FALSE(stack.Current().capabilities.can_load_frame_providers); + EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private); + EXPECT_FALSE( + PolicyStack::Get().Current().capabilities.can_load_frame_providers); { PolicyStack::Guard inner(Policy::PublicStateRunningExpression()); - EXPECT_EQ(stack.Current().view, Policy::View::Public); - EXPECT_FALSE(stack.Current().capabilities.can_run_breakpoint_actions); + EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Public); + EXPECT_FALSE( + PolicyStack::Get().Current().capabilities.can_run_breakpoint_actions); } - EXPECT_EQ(stack.Current().view, Policy::View::Private); + EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private); } - EXPECT_EQ(stack.Current().view, Policy::View::Public); + EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Public); } TEST(PolicyTest, StackIsPerThread) { - PolicyStack &main_stack = PolicyStack::GetForCurrentThread(); - main_stack.Push(Policy::PrivateState()); + PolicyStack::Get().Push(Policy::PrivateState()); Policy::View other_thread_view; std::thread t([&other_thread_view]() { - PolicyStack &stack = PolicyStack::GetForCurrentThread(); - other_thread_view = stack.Current().view; + other_thread_view = PolicyStack::Get().Current().view; }); t.join(); - EXPECT_EQ(main_stack.Current().view, Policy::View::Private); + EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private); EXPECT_EQ(other_thread_view, Policy::View::Public); - main_stack.Pop(); + PolicyStack::Get().Pop(); } TEST(PolicyTest, DumpPublicState) { StreamString s; Policy::PublicState().Dump(s); EXPECT_EQ(s.GetString(), - "view=public, capabilities={" + "policy: view=public, capabilities={" "eval_expr=1 run_all=1 try_all=1 " "bp_actions=1 frame_providers=1 frame_recognizers=1}"); } @@ -135,20 +133,19 @@ TEST(PolicyTest, DumpPrivateState) { StreamString s; Policy::PrivateState().Dump(s); EXPECT_EQ(s.GetString(), - "view=private, capabilities={" + "policy: view=private, capabilities={" "eval_expr=1 run_all=1 try_all=1 " "bp_actions=1 frame_providers=0 frame_recognizers=0}"); } TEST(PolicyTest, DumpStack) { - PolicyStack &stack = PolicyStack::GetForCurrentThread(); - stack.Push(Policy::PrivateState()); + PolicyStack::Get().Push(Policy::PrivateState()); StreamString s; - stack.Dump(s); + PolicyStack::Get().Dump(s); EXPECT_NE(s.GetString().find("depth=2"), std::string::npos); - EXPECT_NE(s.GetString().find("[0] view=public"), std::string::npos); - EXPECT_NE(s.GetString().find("[1] view=private"), std::string::npos); + EXPECT_NE(s.GetString().find("[0] policy: view=public"), std::string::npos); + EXPECT_NE(s.GetString().find("[1] policy: view=private"), std::string::npos); - stack.Pop(); + PolicyStack::Get().Pop(); } _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
