[Lldb-commits] [lldb] [lldb] Harden PolicyStack: thread-binding, factory consistency, ODR-safe formatting (PR #195774)
Med Ismail Bennani via lldb-commits
lldb-commits at lists.llvm.org
Wed Jul 1 00:18:18 PDT 2026
https://github.com/medismailben updated https://github.com/llvm/llvm-project/pull/195774
>From 480680e80c7d9a520b51765a04ffeb20d604d038 Mon Sep 17 00:00:00 2001
From: Med Ismail Bennani <ismail at bennani.ma>
Date: Fri, 5 Jun 2026 17:38:50 -0700
Subject: [PATCH] [lldb] Harden PolicyStack against cross-thread Guard misuse
Two related correctness/clarity improvements to the Policy
infrastructure introduced by 504a11278112:
PolicyStack::Guard now stores the std::thread::id of the thread that
created it. Destruction and move operations call
llvm::report_fatal_error when they happen on a different thread, since
the PolicyStack is thread_local: popping from the wrong thread would
silently corrupt that thread's stack.
Push/Pop on PolicyStack are now private. Callers go through named
factories (PushPrivateState, PushPublicStateRunningExpression) that
return RAII Guards. The transition factories on Policy
(CreatePrivateState, CreatePublicStateRunningExpression) inherit from
PolicyStack::Get().Current() and apply their named change on top, so
pushed policies preserve existing stack state rather than resetting
unrelated fields. CreatePublicState remains the baseline reference
value (returns a default Policy{}); the stack returns to public state
by popping the private guards, not by pushing a "public" policy on
top.
rdar://176223894
Signed-off-by: Med Ismail Bennani <ismail at bennani.ma>
---
lldb/include/lldb/Utility/Policy.h | 79 +++++++++++++-------
lldb/source/Target/Process.cpp | 12 ++--
lldb/source/Target/StopInfo.cpp | 4 +-
lldb/source/Target/Thread.cpp | 2 +-
lldb/source/Utility/Policy.cpp | 56 +++++++++++++++
lldb/unittests/Utility/PolicyTest.cpp | 100 +++++++++++++++++++-------
6 files changed, 194 insertions(+), 59 deletions(-)
diff --git a/lldb/include/lldb/Utility/Policy.h b/lldb/include/lldb/Utility/Policy.h
index e785f13ab9287..0435bdd4a0e15 100644
--- a/lldb/include/lldb/Utility/Policy.h
+++ b/lldb/include/lldb/Utility/Policy.h
@@ -12,6 +12,7 @@
#include "llvm/ADT/SmallVector.h"
#include <cassert>
+#include <thread>
namespace lldb_private {
@@ -28,7 +29,7 @@ class Stream;
/// top of the private unwinder stack. The private state thread must see the
/// raw unwinder frames, while public clients see the augmented view. Rather
/// than checking thread identity at every callsite, the private state thread
-/// pushes Policy::PrivateState() and the rest follows from the policy.
+/// pushes Policy::CreatePrivateState() and the rest follows from the policy.
struct Policy {
/// What view of the process this thread sees.
enum class View {
@@ -54,21 +55,16 @@ struct Policy {
View view = View::Public;
Capabilities capabilities;
- static Policy PublicState() { return {}; }
-
- static Policy PrivateState() {
- Policy p;
- p.view = View::Private;
- p.capabilities.can_load_frame_providers = false;
- p.capabilities.can_run_frame_recognizers = false;
- return p;
- }
-
- static Policy PublicStateRunningExpression() {
- Policy p;
- p.capabilities.can_run_breakpoint_actions = false;
- return p;
- }
+ /// @name Factories
+ ///
+ /// CreatePublicState is the baseline (returns default Policy{}). The
+ /// transition factories below start from PolicyStack::Get().Current() and
+ /// apply their named change on top.
+ /// @{
+ static Policy CreatePublicState();
+ static Policy CreatePrivateState();
+ static Policy CreatePublicStateRunningExpression();
+ /// @}
void Dump(Stream &s) const;
};
@@ -79,6 +75,10 @@ struct Policy {
/// initialized with a default-constructed base entry that is never popped.
/// RAII guards (Guard) push and pop policies.
///
+/// Policies are pushed via named factory methods (PushPrivateState, etc.)
+/// that return an RAII Guard. Direct Push is private to prevent callers
+/// from assembling arbitrary capability combinations.
+///
/// 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.
@@ -91,26 +91,53 @@ class PolicyStack {
Policy Current() const;
- void Push(Policy policy) { m_stack.push_back(std::move(policy)); }
-
- void Pop() {
- assert(!m_stack.empty() && "can't pop the base policy");
- m_stack.pop_back();
- }
-
void Dump(Stream &s) const;
- /// RAII guard that pushes a policy on construction and pops on destruction.
+ /// RAII guard that pops a policy on destruction.
+ ///
+ /// A Guard is bound to the thread that created it: the policy stack lives
+ /// in thread_local storage, so popping from a different thread would
+ /// corrupt that thread's stack. Guards may be moved, but only on the
+ /// owning thread; a cross-thread move or destruction is a fatal error.
class Guard {
+ friend class PolicyStack;
+
public:
- explicit Guard(Policy policy) { Get().Push(std::move(policy)); }
- ~Guard() { Get().Pop(); }
+ ~Guard();
+ Guard(Guard &&other);
+ Guard &operator=(Guard &&other);
Guard(const Guard &) = delete;
Guard &operator=(const Guard &) = delete;
+
+ private:
+ Guard() : m_thread_id(std::this_thread::get_id()), m_active(true) {}
+ std::thread::id m_thread_id;
+ bool m_active = false;
};
+ /// All Push* methods delegate to the named static factories on Policy,
+ /// which already inherit from Current(). So the pushed policy preserves
+ /// existing stack state instead of resetting unrelated fields.
+
+ [[nodiscard]] Guard PushPrivateState() {
+ Push(Policy::CreatePrivateState());
+ return Guard();
+ }
+
+ [[nodiscard]] Guard PushPublicStateRunningExpression() {
+ Push(Policy::CreatePublicStateRunningExpression());
+ return Guard();
+ }
+
private:
+ void Push(Policy policy) { m_stack.push_back(std::move(policy)); }
+
+ void Pop() {
+ assert(!m_stack.empty() && "can't pop the base policy");
+ m_stack.pop_back();
+ }
+
llvm::SmallVector<Policy> m_stack = {Policy{}};
};
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index 1661ae5734b0a..4360250b21475 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -4372,7 +4372,7 @@ thread_result_t Process::RunPrivateStateThread(bool is_override) {
// They must see parent frames, not provider-augmented frames.
std::optional<PolicyStack::Guard> policy_guard;
if (is_override)
- policy_guard.emplace(Policy::PrivateState());
+ policy_guard = 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.emplace(Policy::PrivateState());
+ policy_guard = PolicyStack::Get().PushPrivateState();
while (true) {
// We usually want to resume the process if we get to the top of the
@@ -5499,10 +5499,10 @@ Process::RunThreadPlan(ExecutionContext &exe_ctx,
Halt(clear_thread_plans, use_run_lock);
}
- diagnostic_manager.Printf(
- lldb::eSeverityError,
- "didn't get running event after initial resume, got %s instead.",
- StateAsCString(stop_state));
+ diagnostic_manager.Printf(lldb::eSeverityError,
+ "didn't get running event after initial "
+ "resume, got %s instead.",
+ StateAsCString(stop_state));
return_value = eExpressionSetupError;
break;
}
diff --git a/lldb/source/Target/StopInfo.cpp b/lldb/source/Target/StopInfo.cpp
index f438d368c9ba8..c20b0ed07ee3c 100644
--- a/lldb/source/Target/StopInfo.cpp
+++ b/lldb/source/Target/StopInfo.cpp
@@ -193,7 +193,7 @@ class StopInfoBreakpoint : public StopInfo {
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());
+ PolicyStack::Guard policy_guard = PolicyStack::Get().PushPrivateState();
ThreadSP thread_sp(m_thread_wp.lock());
if (thread_sp) {
@@ -903,7 +903,7 @@ 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());
+ PolicyStack::Guard policy_guard = PolicyStack::Get().PushPrivateState();
// If we are running our step-over the watchpoint plan, stop if it's done
// and continue if it's not:
diff --git a/lldb/source/Target/Thread.cpp b/lldb/source/Target/Thread.cpp
index 87ec77452f2cd..f25a14fbe92e8 100644
--- a/lldb/source/Target/Thread.cpp
+++ b/lldb/source/Target/Thread.cpp
@@ -1558,7 +1558,7 @@ 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::PrivateState is pushed by
+ // provider-augmented public view. Policy::CreatePrivateState is pushed by
// RunThreadPlan and RunPrivateStateThread.
Policy policy = PolicyStack::Get().Current();
if (policy.view == Policy::View::Private) {
diff --git a/lldb/source/Utility/Policy.cpp b/lldb/source/Utility/Policy.cpp
index 2abc0d6281161..104df17df7a97 100644
--- a/lldb/source/Utility/Policy.cpp
+++ b/lldb/source/Utility/Policy.cpp
@@ -11,6 +11,7 @@
#include "lldb/Utility/Log.h"
#include "lldb/Utility/Stream.h"
#include "lldb/Utility/StreamString.h"
+#include "llvm/Support/ErrorHandling.h"
using namespace lldb_private;
@@ -24,6 +25,61 @@ Policy PolicyStack::Current() const {
return p;
}
+// CreatePublicState is the baseline, not a transition. The stack returns to
+// public state by popping the private-state guards, not by pushing a
+// "public" policy on top. This factory exists only as a reference value
+// (tests, dump comparisons); it never reads the current stack.
+Policy Policy::CreatePublicState() { return {}; }
+
+Policy Policy::CreatePrivateState() {
+ Policy p = PolicyStack::Get().Current();
+ p.view = View::Private;
+ p.capabilities.can_load_frame_providers = false;
+ p.capabilities.can_run_frame_recognizers = false;
+ return p;
+}
+
+Policy Policy::CreatePublicStateRunningExpression() {
+ Policy p = PolicyStack::Get().Current();
+ p.capabilities.can_run_breakpoint_actions = false;
+ return p;
+}
+
+PolicyStack::Guard::~Guard() {
+ if (!m_active)
+ return;
+ if (m_thread_id != std::this_thread::get_id())
+ llvm::report_fatal_error(
+ "PolicyStack::Guard destroyed on a different thread than the one "
+ "that created it");
+ Get().Pop();
+}
+
+PolicyStack::Guard::Guard(Guard &&other)
+ : m_thread_id(other.m_thread_id), m_active(other.m_active) {
+ if (m_active && m_thread_id != std::this_thread::get_id())
+ llvm::report_fatal_error("PolicyStack::Guard moved across threads");
+ other.m_active = false;
+}
+
+PolicyStack::Guard &PolicyStack::Guard::operator=(Guard &&other) {
+ if (this != &other) {
+ if (other.m_active && other.m_thread_id != std::this_thread::get_id())
+ llvm::report_fatal_error("PolicyStack::Guard moved across threads");
+ if (m_active) {
+ if (m_thread_id != std::this_thread::get_id())
+ llvm::report_fatal_error(
+ "PolicyStack::Guard destroyed on a different thread than the "
+ "one that created it");
+ Get().Pop();
+ }
+ m_thread_id = other.m_thread_id;
+ m_active = other.m_active;
+ other.m_active = false;
+ }
+ return *this;
+}
+
void Policy::Dump(Stream &s) const {
s << "policy: view=" << (view == View::Public ? "public" : "private");
s << ", capabilities={";
diff --git a/lldb/unittests/Utility/PolicyTest.cpp b/lldb/unittests/Utility/PolicyTest.cpp
index b7771e52b46e4..c582f16a3d7bb 100644
--- a/lldb/unittests/Utility/PolicyTest.cpp
+++ b/lldb/unittests/Utility/PolicyTest.cpp
@@ -26,7 +26,7 @@ TEST(PolicyTest, DefaultIsPublicWithAllCapabilities) {
}
TEST(PolicyTest, PublicState) {
- Policy p = Policy::PublicState();
+ Policy p = Policy::CreatePublicState();
EXPECT_EQ(p.view, Policy::View::Public);
EXPECT_TRUE(p.capabilities.can_evaluate_expressions);
EXPECT_TRUE(p.capabilities.can_run_all_threads);
@@ -37,7 +37,7 @@ TEST(PolicyTest, PublicState) {
}
TEST(PolicyTest, PrivateState) {
- Policy p = Policy::PrivateState();
+ Policy p = Policy::CreatePrivateState();
EXPECT_EQ(p.view, Policy::View::Private);
EXPECT_TRUE(p.capabilities.can_evaluate_expressions);
EXPECT_TRUE(p.capabilities.can_run_all_threads);
@@ -48,7 +48,7 @@ TEST(PolicyTest, PrivateState) {
}
TEST(PolicyTest, PublicStateRunningExpression) {
- Policy p = Policy::PublicStateRunningExpression();
+ Policy p = Policy::CreatePublicStateRunningExpression();
EXPECT_EQ(p.view, Policy::View::Public);
EXPECT_TRUE(p.capabilities.can_evaluate_expressions);
EXPECT_TRUE(p.capabilities.can_run_all_threads);
@@ -66,20 +66,25 @@ TEST(PolicyTest, StackDefaultIsPublicState) {
}
TEST(PolicyTest, StackPushPop) {
- PolicyStack::Get().Push(Policy::PrivateState());
- EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private);
- EXPECT_FALSE(
- PolicyStack::Get().Current().capabilities.can_load_frame_providers);
+ {
+ PolicyStack::Guard guard = PolicyStack::Get().PushPrivateState();
+ EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private);
+ EXPECT_FALSE(
+ PolicyStack::Get().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);
+ {
+ PolicyStack::Guard inner =
+ PolicyStack::Get().PushPublicStateRunningExpression();
+ // PushPublicStateRunningExpression inherits from Current() and only
+ // toggles bp_actions; view stays Private.
+ EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private);
+ EXPECT_FALSE(
+ PolicyStack::Get().Current().capabilities.can_run_breakpoint_actions);
+ }
- PolicyStack::Get().Pop();
- EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private);
+ EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private);
+ }
- PolicyStack::Get().Pop();
EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Public);
}
@@ -87,14 +92,16 @@ TEST(PolicyTest, GuardRAII) {
EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Public);
{
- PolicyStack::Guard guard(Policy::PrivateState());
+ PolicyStack::Guard guard = PolicyStack::Get().PushPrivateState();
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(PolicyStack::Get().Current().view, Policy::View::Public);
+ PolicyStack::Guard inner =
+ PolicyStack::Get().PushPublicStateRunningExpression();
+ // Inherits Private view from outer guard.
+ EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private);
EXPECT_FALSE(
PolicyStack::Get().Current().capabilities.can_run_breakpoint_actions);
}
@@ -106,7 +113,7 @@ TEST(PolicyTest, GuardRAII) {
}
TEST(PolicyTest, StackIsPerThread) {
- PolicyStack::Get().Push(Policy::PrivateState());
+ PolicyStack::Guard guard = PolicyStack::Get().PushPrivateState();
Policy::View other_thread_view;
std::thread t([&other_thread_view]() {
@@ -116,13 +123,11 @@ TEST(PolicyTest, StackIsPerThread) {
EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private);
EXPECT_EQ(other_thread_view, Policy::View::Public);
-
- PolicyStack::Get().Pop();
}
TEST(PolicyTest, DumpPublicState) {
StreamString s;
- Policy::PublicState().Dump(s);
+ Policy::CreatePublicState().Dump(s);
EXPECT_EQ(s.GetString(),
"policy: view=public, capabilities={"
"eval_expr=true run_all=true try_all=true "
@@ -131,7 +136,7 @@ TEST(PolicyTest, DumpPublicState) {
TEST(PolicyTest, DumpPrivateState) {
StreamString s;
- Policy::PrivateState().Dump(s);
+ Policy::CreatePrivateState().Dump(s);
EXPECT_EQ(s.GetString(),
"policy: view=private, capabilities={"
"eval_expr=true run_all=true try_all=true "
@@ -139,13 +144,60 @@ TEST(PolicyTest, DumpPrivateState) {
}
TEST(PolicyTest, DumpStack) {
- PolicyStack::Get().Push(Policy::PrivateState());
+ PolicyStack::Guard guard = PolicyStack::Get().PushPrivateState();
StreamString s;
PolicyStack::Get().Dump(s);
EXPECT_NE(s.GetString().find("depth=2"), 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);
+}
- PolicyStack::Get().Pop();
+TEST(PolicyTest, GuardSameThreadMove) {
+ // Move on the same thread is fine; the moved-into Guard still pops on
+ // destruction and the moved-from Guard becomes a no-op.
+ EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Public);
+ {
+ PolicyStack::Guard outer = PolicyStack::Get().PushPrivateState();
+ EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private);
+
+ PolicyStack::Guard moved = std::move(outer);
+ EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private);
+ }
+ EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Public);
+}
+
+#if GTEST_HAS_DEATH_TEST
+TEST(PolicyStackDeathTest, GuardDestroyedOnDifferentThread) {
+ PolicyStack::Guard outer = PolicyStack::Get().PushPrivateState();
+ // The move into the closure happens here, on the constructing thread, so
+ // it doesn't trip the thread-affinity check. The closure (and thus the
+ // Guard) is destroyed on the worker thread once it returns, which is
+ // where the violation is detected.
+ EXPECT_DEATH(
+ {
+ std::thread t([guard = std::move(outer)]() mutable { (void)guard; });
+ t.join();
+ },
+ "PolicyStack::Guard");
+}
+#endif
+
+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();
+ EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private);
+
+ PolicyStack::Guard inner =
+ PolicyStack::Get().PushPublicStateRunningExpression();
+ // Capability from inner push.
+ EXPECT_FALSE(
+ PolicyStack::Get().Current().capabilities.can_run_breakpoint_actions);
+ // View inherited from outer push (would be Public if Push reset state).
+ EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private);
+ // Capabilities from outer push also inherited.
+ EXPECT_FALSE(
+ PolicyStack::Get().Current().capabilities.can_load_frame_providers);
}
More information about the lldb-commits
mailing list