[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
Thu Jun 4 14:48:26 PDT 2026
https://github.com/medismailben updated https://github.com/llvm/llvm-project/pull/195774
>From 02844ae048bc04f245942ee4f6f2d6e7f742550a Mon Sep 17 00:00:00 2001
From: Med Ismail Bennani <ismail at bennani.ma>
Date: Thu, 4 Jun 2026 13:05:50 -0700
Subject: [PATCH] [lldb] Harden PolicyStack: thread-binding, factory
consistency, ODR-safe formatting
Three 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 assert (and log via
LLDBLog::Process) when they happen on a different thread, since the
PolicyStack is thread_local: popping from the wrong thread silently
corrupts that thread's stack. The diagnostic includes both the
current and creator thread IDs.
A format_provider<std::thread::id> specialization lives in Policy.h
(not Policy.cpp) so any TU that wants to log a std::thread::id sees
the same definition, avoiding ODR hazards.
Push/Pop on PolicyStack are now private. Callers go through named
factories (PushPrivateState, PushPublicStateRunningExpression) that
return RAII Guards. The transition factories on Policy (PrivateState,
PublicStateRunningExpression) inherit from PolicyStack::Get().Current()
and apply their named change on top, so pushed policies preserve
existing stack state rather than resetting unrelated fields. PublicState
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 | 96 ++++++++++++++++++++-------
lldb/source/Target/Process.cpp | 12 ++--
lldb/source/Target/StopInfo.cpp | 4 +-
lldb/source/Utility/Policy.cpp | 77 +++++++++++++++++++++
lldb/unittests/Utility/PolicyTest.cpp | 74 +++++++++++++++------
5 files changed, 211 insertions(+), 52 deletions(-)
diff --git a/lldb/include/lldb/Utility/Policy.h b/lldb/include/lldb/Utility/Policy.h
index e785f13ab9287..8dd61809e737c 100644
--- a/lldb/include/lldb/Utility/Policy.h
+++ b/lldb/include/lldb/Utility/Policy.h
@@ -10,8 +10,30 @@
#define LLDB_UTILITY_POLICY_H
#include "llvm/ADT/SmallVector.h"
+#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/raw_ostream.h"
#include <cassert>
+#include <sstream>
+#include <thread>
+
+namespace llvm {
+/// Allow std::thread::id to be passed directly to formatv-based logging
+/// macros (LLDB_LOG, etc.). std::thread::id has no raw_ostream operator<<,
+/// so we route through std::stringstream which it does support.
+///
+/// Defined in this header (rather than a .cpp) so that any TU that wants
+/// to log a std::thread::id sees the same specialization, avoiding ODR
+/// hazards.
+template <> struct format_provider<std::thread::id> {
+ static void format(const std::thread::id &id, raw_ostream &os,
+ StringRef /*style*/) {
+ std::stringstream ss;
+ ss << id;
+ os << ss.str();
+ }
+};
+} // namespace llvm
namespace lldb_private {
@@ -54,21 +76,12 @@ 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;
- }
+ /// Static factories. PublicState is the baseline (returns default
+ /// Policy{}). The transition factories below start from
+ /// PolicyStack::Get().Current() and apply their named change on top.
+ static Policy PublicState();
+ static Policy PrivateState();
+ static Policy PublicStateRunningExpression();
void Dump(Stream &s) const;
};
@@ -79,6 +92,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 +108,55 @@ 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 (this is asserted in debug builds; release builds log
+ /// the violation and then perform the underlying Pop, which will likely
+ /// corrupt state).
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::PrivateState());
+ return Guard();
+ }
+
+ [[nodiscard]] Guard PushPublicStateRunningExpression() {
+ Push(Policy::PublicStateRunningExpression());
+ 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 ac1357f7d00a1..591e7c72c17d3 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -4362,7 +4362,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;
@@ -5562,7 +5562,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
@@ -5634,10 +5634,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/Utility/Policy.cpp b/lldb/source/Utility/Policy.cpp
index 2abc0d6281161..063dae7dba795 100644
--- a/lldb/source/Utility/Policy.cpp
+++ b/lldb/source/Utility/Policy.cpp
@@ -24,6 +24,83 @@ Policy PolicyStack::Current() const {
return p;
}
+// PublicState 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::PublicState() { return {}; }
+
+Policy Policy::PrivateState() {
+ 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::PublicStateRunningExpression() {
+ Policy p = PolicyStack::Get().Current();
+ p.capabilities.can_run_breakpoint_actions = false;
+ return p;
+}
+
+PolicyStack::Guard::~Guard() {
+ if (!m_active)
+ return;
+ std::thread::id current = std::this_thread::get_id();
+ if (m_thread_id != current)
+ LLDB_LOG(GetLog(LLDBLog::Process),
+ "PolicyStack::Guard destroyed on thread {0} but was created on "
+ "thread {1}; this would corrupt thread {0}'s policy stack",
+ current, m_thread_id);
+ assert(m_thread_id == current &&
+ "PolicyStack::Guard destroyed on a different thread than the one "
+ "that created it (see preceding log line for thread ids)");
+ Get().Pop();
+}
+
+PolicyStack::Guard::Guard(Guard &&other)
+ : m_thread_id(other.m_thread_id), m_active(other.m_active) {
+ std::thread::id current = std::this_thread::get_id();
+ if (m_thread_id != current)
+ LLDB_LOG(GetLog(LLDBLog::Process),
+ "PolicyStack::Guard move-constructed on thread {0} but was "
+ "created on thread {1}",
+ current, m_thread_id);
+ assert(m_thread_id == current && "PolicyStack::Guard moved across threads "
+ "(see preceding log line for thread ids)");
+ other.m_active = false;
+}
+
+PolicyStack::Guard &PolicyStack::Guard::operator=(Guard &&other) {
+ if (this != &other) {
+ std::thread::id current = std::this_thread::get_id();
+ if (other.m_thread_id != current)
+ LLDB_LOG(GetLog(LLDBLog::Process),
+ "PolicyStack::Guard move-assigned on thread {0} from a guard "
+ "created on thread {1}",
+ current, other.m_thread_id);
+ assert(other.m_thread_id == current &&
+ "PolicyStack::Guard move-assigned across threads "
+ "(see preceding log line for thread ids)");
+ if (m_active) {
+ if (m_thread_id != current)
+ LLDB_LOG(GetLog(LLDBLog::Process),
+ "PolicyStack::Guard destroyed during move-assign on thread "
+ "{0} but was created on thread {1}",
+ current, m_thread_id);
+ assert(m_thread_id == current &&
+ "PolicyStack::Guard destroyed on a different thread "
+ "(see preceding log line for thread ids)");
+ 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..1586b27ca9ed2 100644
--- a/lldb/unittests/Utility/PolicyTest.cpp
+++ b/lldb/unittests/Utility/PolicyTest.cpp
@@ -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,8 +123,6 @@ 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) {
@@ -139,13 +144,44 @@ 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);
+}
+
+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::Get().Pop();
+ PolicyStack::Guard moved = std::move(outer);
+ EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Private);
+ }
+ EXPECT_EQ(PolicyStack::Get().Current().view, Policy::View::Public);
+}
+
+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