https://github.com/medismailben updated https://github.com/llvm/llvm-project/pull/208242
>From 75f0569eb0d23eafdff79b28134d04af326eba94 Mon Sep 17 00:00:00 2001 From: Med Ismail Bennani <[email protected]> Date: Fri, 10 Jul 2026 15:32:20 -0700 Subject: [PATCH] [lldb] Fix scripted frame provider cross-thread re-entrant deadlock `GetStoppedExecutionContext` unconditionally blocked acquiring the target's API mutex. A thread already holding that mutex (for example a `bt` command thread, through `CommandObjectParsed`'s `eCommandTryTargetAPILock`) can end up waiting on a `StackFrameList` lock held by another thread (for example the debugger's event-handler thread) that is itself blocked re-acquiring the API mutex from inside a scripted frame provider's Python code that touches the SB API. This is a classic AB-BA deadlock. This patch introduces `Policy::Capabilities::can_bypass_target_api_mutex`, pushed around every scripted-extension callback in `ScriptedPythonInterface::Dispatch` and `CallStaticMethod`. A thread running one of these callbacks isn't servicing a client-facing SB API entry point; it doesn't need the same locking guarantees a top-level SB API call does for any of the calls it makes during that window, not just the one that happens to deadlock. It encodes the check inside `Target::GetAPIMutex()` itself: when the current thread's policy says it can bypass, `GetAPIMutex()` hands out a thread-local mutex instead of the real one. Every existing caller keeps its own locking code unchanged and becomes deadlock-safe automatically. `SBTarget::GetAPIMutex()` hands scripts and tools such as `lldb-dap` an `SBMutex` that can be locked from any thread and held indefinitely, so it can't just alias whatever mutex was current when it was constructed. `SBMutex` now wraps a new `Target::APIMutexHandle`, which holds a `TargetSP` and re-resolves `GetAPIMutex()` fresh on every `lock()`, `unlock()`, and `try_lock()` call. That keeps ordinary cross-thread use correct (it resolves to the real mutex) while also making it safe to call from inside a scripted-extension callback (it resolves to the bypass mutex there instead). This patch adds regression tests for both the original deadlock and for a blocking `SBMutex.lock()` call made from inside a callback. Signed-off-by: Med Ismail Bennani <[email protected]> --- lldb/include/lldb/API/SBMutex.h | 4 +- lldb/include/lldb/Target/ExecutionContext.h | 4 +- lldb/include/lldb/Target/Target.h | 27 ++++ lldb/include/lldb/Utility/Policy.h | 11 ++ lldb/include/lldb/lldb-forward.h | 1 + lldb/source/API/SBMutex.cpp | 6 +- .../Interfaces/ScriptedPythonInterface.h | 7 + lldb/source/Target/ExecutionContext.cpp | 3 +- lldb/source/Target/Target.cpp | 35 +++++ lldb/source/Utility/Policy.cpp | 7 + .../Makefile | 2 + ...ProviderRegisterCommandAPIMutexDeadlock.py | 89 +++++++++++++ .../frame_provider.py | 28 ++++ .../main.c | 7 + .../sbmutex_reflects_target_mutex/Makefile | 2 + .../TestHoldMutexNoDeadlock.py | 88 ++++++++++++ .../TestSBMutexReflectsTargetMutex.py | 125 ++++++++++++++++++ .../hold_mutex_frame_provider.py | 46 +++++++ .../sbmutex_reflects_target_mutex/main.c | 12 ++ .../sbmutex_frame_provider.py | 56 ++++++++ lldb/tools/lldb-dap/InstructionBreakpoint.cpp | 1 + lldb/tools/lldb-dap/ProtocolUtils.cpp | 1 + lldb/unittests/Utility/PolicyTest.cpp | 6 +- 23 files changed, 556 insertions(+), 12 deletions(-) create mode 100644 lldb/test/API/functionalities/scripted_frame_provider/register_command_api_mutex_deadlock/Makefile create mode 100644 lldb/test/API/functionalities/scripted_frame_provider/register_command_api_mutex_deadlock/TestFrameProviderRegisterCommandAPIMutexDeadlock.py create mode 100644 lldb/test/API/functionalities/scripted_frame_provider/register_command_api_mutex_deadlock/frame_provider.py create mode 100644 lldb/test/API/functionalities/scripted_frame_provider/register_command_api_mutex_deadlock/main.c create mode 100644 lldb/test/API/functionalities/scripted_frame_provider/sbmutex_reflects_target_mutex/Makefile create mode 100644 lldb/test/API/functionalities/scripted_frame_provider/sbmutex_reflects_target_mutex/TestHoldMutexNoDeadlock.py create mode 100644 lldb/test/API/functionalities/scripted_frame_provider/sbmutex_reflects_target_mutex/TestSBMutexReflectsTargetMutex.py create mode 100644 lldb/test/API/functionalities/scripted_frame_provider/sbmutex_reflects_target_mutex/hold_mutex_frame_provider.py create mode 100644 lldb/test/API/functionalities/scripted_frame_provider/sbmutex_reflects_target_mutex/main.c create mode 100644 lldb/test/API/functionalities/scripted_frame_provider/sbmutex_reflects_target_mutex/sbmutex_frame_provider.py diff --git a/lldb/include/lldb/API/SBMutex.h b/lldb/include/lldb/API/SBMutex.h index 826ad077f159f..2c7f859abdd71 100644 --- a/lldb/include/lldb/API/SBMutex.h +++ b/lldb/include/lldb/API/SBMutex.h @@ -11,7 +11,7 @@ #include "lldb/API/SBDefines.h" #include "lldb/lldb-forward.h" -#include <mutex> +#include <memory> namespace lldb { @@ -41,7 +41,7 @@ class LLDB_API SBMutex { SBMutex(lldb::TargetSP target_sp); friend class SBTarget; - std::shared_ptr<std::recursive_mutex> m_opaque_sp; + std::shared_ptr<lldb_private::APIMutexHandle> m_opaque_sp; }; } // namespace lldb diff --git a/lldb/include/lldb/Target/ExecutionContext.h b/lldb/include/lldb/Target/ExecutionContext.h index bf976f4db8c87..706fd6f64ba53 100644 --- a/lldb/include/lldb/Target/ExecutionContext.h +++ b/lldb/include/lldb/Target/ExecutionContext.h @@ -561,8 +561,8 @@ class ExecutionContext { }; /// A wrapper class representing an execution context with non-null Target -/// and Process pointers, a locked API mutex and a locked ProcessRunLock. -/// The locks are private by design: to unlock them, destroy the +/// and Process pointers, a locked ProcessRunLock, and a locked API mutex. +/// The locks are private by design; to unlock them, destroy the /// StoppedExecutionContext. struct StoppedExecutionContext : ExecutionContext { StoppedExecutionContext(lldb::TargetSP &target_sp, diff --git a/lldb/include/lldb/Target/Target.h b/lldb/include/lldb/Target/Target.h index fb43f432a08da..1ecfb99627371 100644 --- a/lldb/include/lldb/Target/Target.h +++ b/lldb/include/lldb/Target/Target.h @@ -572,6 +572,28 @@ class EvaluateExpressionOptions { SymbolContextList m_preferred_lookup_contexts; }; +/// A movable, sharable handle to a Target's API mutex, safe to lock from any +/// thread and to hold past the point where the constructing thread's policy +/// still applies. Every call re-resolves Target::GetAPIMutex() fresh, rather +/// than binding to whichever mutex was current at construction time, so it +/// always reflects the calling thread's own, current bypass status. +/// +/// A handle constructed without a target owns an independent mutex instead, +/// for callers that just want a general-purpose lockable object. +class APIMutexHandle { +public: + APIMutexHandle(); + explicit APIMutexHandle(lldb::TargetSP target_sp); + + void lock(); + void unlock(); + bool try_lock(); + +private: + lldb::TargetSP m_target_sp; + std::recursive_mutex m_standalone_mutex; +}; + // Target class Target : public std::enable_shared_from_this<Target>, public TargetProperties, @@ -761,6 +783,11 @@ class Target : public std::enable_shared_from_this<Target>, static TargetProperties &GetGlobalProperties(); + /// Returns the mutex a caller should serialize on before touching the + /// target through the SB API. When the current thread's policy says it + /// can bypass it, this returns a mutex private to that thread instead, so + /// callers can lock it unconditionally without ever contending with + /// whichever thread holds the real one. std::recursive_mutex &GetAPIMutex(); void DeleteCurrentProcess(); diff --git a/lldb/include/lldb/Utility/Policy.h b/lldb/include/lldb/Utility/Policy.h index afeeab19c2ed0..83248cf61d0c6 100644 --- a/lldb/include/lldb/Utility/Policy.h +++ b/lldb/include/lldb/Utility/Policy.h @@ -50,6 +50,11 @@ struct Policy { bool can_run_breakpoint_actions = true; bool can_load_frame_providers = true; bool can_run_frame_recognizers = true; + /// Whether the current thread may bypass the target's API mutex + /// entirely when it re-enters it, because the thread is already + /// running under whatever protections its caller set up rather than + /// servicing a top-level SB API entry point itself. + bool can_bypass_target_api_mutex = false; }; /// Why a private-state policy is being pushed. Distinguishes a PST's @@ -75,6 +80,7 @@ struct Policy { static Policy CreatePrivateState( PrivateStatePurpose purpose = PrivateStatePurpose::Default); static Policy CreatePublicStateRunningExpression(); + static Policy CreateScriptedExtensionCall(); /// @} void Dump(Stream &s) const; @@ -140,6 +146,11 @@ class PolicyStack { return Guard(); } + [[nodiscard]] Guard PushScriptedExtensionCall() { + Push(Policy::CreateScriptedExtensionCall()); + return Guard(); + } + private: void Push(Policy policy) { m_stack.push_back(std::move(policy)); } diff --git a/lldb/include/lldb/lldb-forward.h b/lldb/include/lldb/lldb-forward.h index 157aa5743f016..8eb3695d706a9 100644 --- a/lldb/include/lldb/lldb-forward.h +++ b/lldb/include/lldb/lldb-forward.h @@ -15,6 +15,7 @@ namespace lldb_private { class ABI; +class APIMutexHandle; class ASTResultSynthesizer; class ASTStructExtractor; class Address; diff --git a/lldb/source/API/SBMutex.cpp b/lldb/source/API/SBMutex.cpp index c7844dec658cc..1be8d69021554 100644 --- a/lldb/source/API/SBMutex.cpp +++ b/lldb/source/API/SBMutex.cpp @@ -11,12 +11,11 @@ #include "lldb/Utility/Instrumentation.h" #include "lldb/lldb-forward.h" #include <memory> -#include <mutex> using namespace lldb; using namespace lldb_private; -SBMutex::SBMutex() : m_opaque_sp(std::make_shared<std::recursive_mutex>()) { +SBMutex::SBMutex() : m_opaque_sp(std::make_shared<APIMutexHandle>()) { LLDB_INSTRUMENT_VA(this); } @@ -32,8 +31,7 @@ const SBMutex &SBMutex::operator=(const SBMutex &rhs) { } SBMutex::SBMutex(lldb::TargetSP target_sp) - : m_opaque_sp(std::shared_ptr<std::recursive_mutex>( - target_sp, &target_sp->GetAPIMutex())) { + : m_opaque_sp(std::make_shared<APIMutexHandle>(target_sp)) { LLDB_INSTRUMENT_VA(this, target_sp); } diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h index aaa0b6a0f7a59..ec115081f647f 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h +++ b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h @@ -17,6 +17,7 @@ #include "lldb/Interpreter/Interfaces/ScriptedInterface.h" #include "lldb/Utility/DataBufferHeap.h" +#include "lldb/Utility/Policy.h" #include "../PythonDataObjects.h" #include "../SWIGPythonBridge.h" @@ -420,6 +421,9 @@ class ScriptedPythonInterface : virtual public ScriptedInterface { return ErrorWithMessage<T>(caller_signature, "missing script class name", error); + PolicyStack::Guard policy_guard = + PolicyStack::Get().PushScriptedExtensionCall(); + Locker py_lock(&m_interpreter, Locker::AcquireLock | Locker::NoSTDIN, Locker::FreeLock); @@ -515,6 +519,9 @@ class ScriptedPythonInterface : virtual public ScriptedInterface { return ErrorWithMessage<T>(caller_signature, "python object ill-formed", error); + PolicyStack::Guard policy_guard = + PolicyStack::Get().PushScriptedExtensionCall(); + Locker py_lock(&m_interpreter, Locker::AcquireLock | Locker::NoSTDIN, Locker::FreeLock); diff --git a/lldb/source/Target/ExecutionContext.cpp b/lldb/source/Target/ExecutionContext.cpp index e4b2f07d8d8d1..f2ac6a0bc8e24 100644 --- a/lldb/source/Target/ExecutionContext.cpp +++ b/lldb/source/Target/ExecutionContext.cpp @@ -145,8 +145,7 @@ lldb_private::GetStoppedExecutionContext( return llvm::createStringError( "StoppedExecutionContext created with a null target"); - auto api_lock = - std::unique_lock<std::recursive_mutex>(target_sp->GetAPIMutex()); + std::unique_lock<std::recursive_mutex> api_lock(target_sp->GetAPIMutex()); auto process_sp = exe_ctx_ref_ptr->GetProcessSP(); if (!process_sp) diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp index c98b30bedaaa6..1fec6ec4d6157 100644 --- a/lldb/source/Target/Target.cpp +++ b/lldb/source/Target/Target.cpp @@ -5992,8 +5992,43 @@ Target::TargetEventData::GetModuleListFromEvent(const Event *event_ptr) { return module_list; } +APIMutexHandle::APIMutexHandle() = default; + +APIMutexHandle::APIMutexHandle(lldb::TargetSP target_sp) + : m_target_sp(std::move(target_sp)) {} + +void APIMutexHandle::lock() { + if (m_target_sp) + m_target_sp->GetAPIMutex().lock(); + else + m_standalone_mutex.lock(); +} + +void APIMutexHandle::unlock() { + if (m_target_sp) + m_target_sp->GetAPIMutex().unlock(); + else + m_standalone_mutex.unlock(); +} + +bool APIMutexHandle::try_lock() { + if (m_target_sp) + return m_target_sp->GetAPIMutex().try_lock(); + return m_standalone_mutex.try_lock(); +} + std::recursive_mutex &Target::GetAPIMutex() { Policy policy = PolicyStack::Get().Current(); + + // A thread whose policy says it can bypass the API mutex gets a mutex of + // its own instead of a no-op: every caller still locks *something*, but + // since it's thread-local, that lock never contends with whatever other + // thread holds the real mutex. + if (policy.capabilities.can_bypass_target_api_mutex) { + static thread_local std::recursive_mutex s_bypass_mutex; + return s_bypass_mutex; + } + if (policy.view == Policy::View::Private) return m_private_mutex; diff --git a/lldb/source/Utility/Policy.cpp b/lldb/source/Utility/Policy.cpp index 4d1999aaf7b92..04293d7a03f85 100644 --- a/lldb/source/Utility/Policy.cpp +++ b/lldb/source/Utility/Policy.cpp @@ -64,6 +64,12 @@ Policy Policy::CreatePublicStateRunningExpression() { return p; } +Policy Policy::CreateScriptedExtensionCall() { + Policy p = PolicyStack::Get().Current(); + p.capabilities.can_bypass_target_api_mutex = true; + return p; +} + PolicyStack::Guard::~Guard() { if (!m_active) return; @@ -108,6 +114,7 @@ void Policy::Dump(Stream &s) const { s << " bp_actions=" << capabilities.can_run_breakpoint_actions; s << " frame_providers=" << capabilities.can_load_frame_providers; s << " frame_recognizers=" << capabilities.can_run_frame_recognizers; + s << " bypass_api_mutex=" << capabilities.can_bypass_target_api_mutex; s << '}'; } diff --git a/lldb/test/API/functionalities/scripted_frame_provider/register_command_api_mutex_deadlock/Makefile b/lldb/test/API/functionalities/scripted_frame_provider/register_command_api_mutex_deadlock/Makefile new file mode 100644 index 0000000000000..c9319d6e6888a --- /dev/null +++ b/lldb/test/API/functionalities/scripted_frame_provider/register_command_api_mutex_deadlock/Makefile @@ -0,0 +1,2 @@ +C_SOURCES := main.c +include Makefile.rules diff --git a/lldb/test/API/functionalities/scripted_frame_provider/register_command_api_mutex_deadlock/TestFrameProviderRegisterCommandAPIMutexDeadlock.py b/lldb/test/API/functionalities/scripted_frame_provider/register_command_api_mutex_deadlock/TestFrameProviderRegisterCommandAPIMutexDeadlock.py new file mode 100644 index 0000000000000..09345b11cd84c --- /dev/null +++ b/lldb/test/API/functionalities/scripted_frame_provider/register_command_api_mutex_deadlock/TestFrameProviderRegisterCommandAPIMutexDeadlock.py @@ -0,0 +1,89 @@ +""" +Test that a scripted frame provider whose get_frame_at_index touches SB +API (self.input_frames) does not deadlock when running `bt` from the +command interpreter. + +GetStoppedExecutionContext (used by SBFrame::IsValid, among others) +unconditionally blocked acquiring the target's API mutex. The command +thread running `bt` already holds that mutex (CommandObjectParsed's +eCommandTryTargetAPILock) and can end up waiting on a StackFrameList +lock held by the debugger's event-handler thread, which is itself +blocked re-acquiring the API mutex from inside this provider's Python +code -- an AB-BA deadlock between the command thread and the +event-handler thread. + +The event-handler thread only runs when commands are driven through +SBDebugger.RunCommandInterpreter (what the lldb driver itself uses), +not through plain HandleCommand, so this test drives commands that way. + +Note: this is a genuine cross-thread race (the command thread vs. the +debugger's event-handler thread), not a deterministic sequential +deadlock, so this test is best-effort -- like the sibling +runlock_reentrant_deadlock/was_hit_deadlock tests, it raises the odds of +hitting the race within a single invocation but cannot guarantee it. +""" + +import os +import lldb +import lldbsuite.test.lldbutil as lldbutil +from lldbsuite.test.decorators import * +from lldbsuite.test.lldbtest import * + + +class TestFrameProviderRegisterCommandAPIMutexDeadlock(TestBase): + NO_DEBUG_INFO_TESTCASE = True + + def test_register_command_then_bt_no_deadlock(self): + """ + Register a scripted frame provider whose get_frame_at_index + touches SB API, then repeatedly run `bt` through + RunCommandInterpreter. Should complete without deadlocking. + """ + self.build() + + lldbutil.run_to_name_breakpoint(self, "frame3") + + provider_path = os.path.join(self.getSourceDir(), "frame_provider.py") + + commands = ["command script import " + provider_path] + commands.append( + "target frame-provider register -C frame_provider.DictFrameProvider" + ) + # Run `bt` several times to raise the odds of hitting the race + # between the command thread and the debugger's event-handler + # thread within a single test invocation. + commands.extend(["bt"] * 20) + commands.append("quit") + + stdin_path = self.getBuildArtifact("stdin.txt") + stdout_path = self.getBuildArtifact("stdout.txt") + with open(stdin_path, "w") as f: + f.write("\n".join(commands) + "\n") + + with open(stdin_path, "r") as in_fileH, open(stdout_path, "w") as out_fileH: + in_sbf = lldb.SBFile(in_fileH.fileno(), "r", False) + out_sbf = lldb.SBFile(out_fileH.fileno(), "w", False) + self.assertSuccess(self.dbg.SetInputFile(in_sbf)) + self.assertSuccess(self.dbg.SetOutputFile(out_sbf)) + self.assertSuccess(self.dbg.SetErrorFile(out_sbf)) + + options = lldb.SBCommandInterpreterRunOptions() + options.SetEchoCommands(False) + options.SetPrintResults(True) + options.SetStopOnError(False) + options.SetStopOnCrash(False) + + # If the API-mutex deadlock regresses, this call hangs forever + # (timing out the test run). + n_errors, quit_requested, has_crashed = self.dbg.RunCommandInterpreter( + True, False, options, 0, False, False + ) + + with open(stdout_path, "r") as out_fileH: + output = out_fileH.read() + + self.assertFalse(has_crashed, "lldb should not have crashed") + self.assertTrue(quit_requested, "quit command should have been processed") + self.assertEqual(n_errors, 0, f"unexpected errors in output:\n{output}") + + self.assertIn("successfully registered scripted frame provider", output) diff --git a/lldb/test/API/functionalities/scripted_frame_provider/register_command_api_mutex_deadlock/frame_provider.py b/lldb/test/API/functionalities/scripted_frame_provider/register_command_api_mutex_deadlock/frame_provider.py new file mode 100644 index 0000000000000..2bfab64b1c4d8 --- /dev/null +++ b/lldb/test/API/functionalities/scripted_frame_provider/register_command_api_mutex_deadlock/frame_provider.py @@ -0,0 +1,28 @@ +""" +Frame provider that returns dict-based synthetic frames (never identity +forwarding), while touching self.input_frames from get_frame_at_index. + +Returning a dict keeps this test isolated from the frame-aliasing bug: +dict-based frames always go through ScriptedFrameProvider's +create_frame_from_dict helper, which builds a brand new StackFrame and +never reuses (or wraps via BorrowedStackFrame) the parent list's frame +object. Only the API-mutex deadlock is reachable through this path. +""" + +from lldb.plugins.scripted_frame_provider import ScriptedFrameProvider + + +class DictFrameProvider(ScriptedFrameProvider): + @staticmethod + def get_description(): + return "Provider that returns dict-based synthetic frames" + + def get_frame_at_index(self, index): + if index >= len(self.input_frames): + return None + # __getitem__ calls SBFrame.IsValid() internally, which is what + # exercises GetStoppedExecutionContext. + frame = self.input_frames[index] + if frame is None: + return None + return {"idx": index, "pc": frame.GetPC()} diff --git a/lldb/test/API/functionalities/scripted_frame_provider/register_command_api_mutex_deadlock/main.c b/lldb/test/API/functionalities/scripted_frame_provider/register_command_api_mutex_deadlock/main.c new file mode 100644 index 0000000000000..1aa56e3eddf7a --- /dev/null +++ b/lldb/test/API/functionalities/scripted_frame_provider/register_command_api_mutex_deadlock/main.c @@ -0,0 +1,7 @@ +int frame3() { return 3; } + +int frame2() { return frame3(); } + +int frame1() { return frame2(); } + +int main() { return frame1(); } diff --git a/lldb/test/API/functionalities/scripted_frame_provider/sbmutex_reflects_target_mutex/Makefile b/lldb/test/API/functionalities/scripted_frame_provider/sbmutex_reflects_target_mutex/Makefile new file mode 100644 index 0000000000000..c9319d6e6888a --- /dev/null +++ b/lldb/test/API/functionalities/scripted_frame_provider/sbmutex_reflects_target_mutex/Makefile @@ -0,0 +1,2 @@ +C_SOURCES := main.c +include Makefile.rules diff --git a/lldb/test/API/functionalities/scripted_frame_provider/sbmutex_reflects_target_mutex/TestHoldMutexNoDeadlock.py b/lldb/test/API/functionalities/scripted_frame_provider/sbmutex_reflects_target_mutex/TestHoldMutexNoDeadlock.py new file mode 100644 index 0000000000000..f50eb9f3ebc88 --- /dev/null +++ b/lldb/test/API/functionalities/scripted_frame_provider/sbmutex_reflects_target_mutex/TestHoldMutexNoDeadlock.py @@ -0,0 +1,88 @@ +""" +Test that a scripted frame provider can safely call a blocking +SBMutex.lock() from inside get_frame_at_index without deadlocking. + +LLDB's private state thread can reach this callback without already +holding the target's real API mutex, so a blocking lock() there is a +genuinely new acquisition attempt, not a safe same-thread recursive +re-lock. Target::APIMutexHandle re-resolves Target::GetAPIMutex() on +every call rather than aliasing whatever mutex was current when the +SBMutex was constructed, so this thread gets the same thread-local +bypass mutex the internal machinery does, and lock() never contends +with anyone. + +Like the sibling runlock_reentrant_deadlock/was_hit_deadlock/ +register_command_api_mutex_deadlock tests, this drives a genuine +cross-thread race and is best-effort: it raises the odds of exercising +the path within a single invocation but the important guarantee is that +it cannot hang, not that it hits any particular thread ordering. +""" + +import os +import lldb +import lldbsuite.test.lldbutil as lldbutil +from lldbsuite.test.decorators import * +from lldbsuite.test.lldbtest import * + + +class TestHoldMutexNoDeadlock(TestBase): + NO_DEBUG_INFO_TESTCASE = True + + def test_hold_mutex_no_deadlock(self): + """ + Register a scripted frame provider that locks and holds + target.GetAPIMutex() from get_frame_at_index, then run `bt` and + `continue` through RunCommandInterpreter. Should complete + without deadlocking. + """ + self.build() + + lldbutil.run_to_name_breakpoint(self, "frame3") + + provider_path = os.path.join( + self.getSourceDir(), "hold_mutex_frame_provider.py" + ) + commands = ["command script import " + provider_path] + commands.append( + "target frame-provider register " + "-C hold_mutex_frame_provider.HoldMutexFrameProvider" + ) + # Interleave `bt` with `continue` (hitting the same breakpoint + # again, via a loop in main.c) so get_frame_at_index runs + # repeatedly instead of once, raising the odds of hitting the + # race within a single test invocation. + commands.extend(["bt", "continue"] * 20) + commands.append("quit") + + stdin_path = self.getBuildArtifact("stdin.txt") + stdout_path = self.getBuildArtifact("stdout.txt") + with open(stdin_path, "w") as f: + f.write("\n".join(commands) + "\n") + + with open(stdin_path, "r") as in_fileH, open(stdout_path, "w") as out_fileH: + in_sbf = lldb.SBFile(in_fileH.fileno(), "r", False) + out_sbf = lldb.SBFile(out_fileH.fileno(), "w", False) + self.assertSuccess(self.dbg.SetInputFile(in_sbf)) + self.assertSuccess(self.dbg.SetOutputFile(out_sbf)) + self.assertSuccess(self.dbg.SetErrorFile(out_sbf)) + + options = lldb.SBCommandInterpreterRunOptions() + options.SetEchoCommands(False) + options.SetPrintResults(True) + options.SetStopOnError(False) + options.SetStopOnCrash(False) + + # If the bypass regresses, this call hangs forever (timing + # out the test run). + n_errors, quit_requested, has_crashed = self.dbg.RunCommandInterpreter( + True, False, options, 0, False, False + ) + + with open(stdout_path, "r") as out_fileH: + output = out_fileH.read() + + self.assertFalse(has_crashed, "lldb should not have crashed") + self.assertTrue(quit_requested, "quit command should have been processed") + self.assertEqual(n_errors, 0, f"unexpected errors in output:\n{output}") + self.assertIn("successfully registered scripted frame provider", output) + diff --git a/lldb/test/API/functionalities/scripted_frame_provider/sbmutex_reflects_target_mutex/TestSBMutexReflectsTargetMutex.py b/lldb/test/API/functionalities/scripted_frame_provider/sbmutex_reflects_target_mutex/TestSBMutexReflectsTargetMutex.py new file mode 100644 index 0000000000000..b596157c4cfd3 --- /dev/null +++ b/lldb/test/API/functionalities/scripted_frame_provider/sbmutex_reflects_target_mutex/TestSBMutexReflectsTargetMutex.py @@ -0,0 +1,125 @@ +""" +Test that a scripted frame provider calling SBTarget.GetAPIMutex() from +get_frame_at_index gets a handle that reflects the state of the target's +real, shared API mutex, even though the callback's own thread is exempt +from having to serialize on it. + +SBMutex is meant to be usable across threads and outlive the call that +created it (e.g. lldb-dap hands it to background workers), so it must +always alias the genuine target mutex rather than the thread-local mutex +the bypass hands out for internal callers. This test drives the same +kind of command-thread / internal-thread race as +TestFrameProviderRegisterCommandAPIMutexDeadlock, interleaving `bt` with +`continue` (hitting the same breakpoint again each time, via a loop in +main.c) so get_frame_at_index runs many times instead of once, and has +the provider call target.GetAPIMutex().try_lock() from inside the +callback: if some other thread happens to hold the real mutex at that +moment, this should observe it as contended. + +Only try_lock() is used, which never blocks, so this cannot deadlock +regardless of the outcome. An earlier version of this test tried to +widen the race window by having the callback actually lock() and hold +the mutex for a short duration, on the assumption that whichever thread +reaches this callback already holds the real mutex first. That +assumption is wrong -- LLDB's private state thread can reach this +callback without already holding it -- so that held mutex.lock() call +could genuinely block, and reproducibly deadlocked in practice. Do not +reintroduce a blocking acquisition here. + +Observing contention is a genuine cross-thread race, so -- like the +sibling runlock_reentrant_deadlock/was_hit_deadlock/ +register_command_api_mutex_deadlock tests -- this is best-effort: it +raises the odds of witnessing it within a single invocation but cannot +guarantee it, and the test does not require it to pass. +""" + +import os +import lldb +import lldbsuite.test.lldbutil as lldbutil +from lldbsuite.test.decorators import * +from lldbsuite.test.lldbtest import * + + +class TestSBMutexReflectsTargetMutex(TestBase): + NO_DEBUG_INFO_TESTCASE = True + + def test_sbmutex_reflects_target_mutex(self): + """ + Register a scripted frame provider that checks + target.GetAPIMutex().try_lock() from get_frame_at_index, then + repeatedly run `bt` and `continue` through RunCommandInterpreter. + Should complete without deadlocking, regardless of whether + contention is observed. + """ + self.build() + + lldbutil.run_to_name_breakpoint(self, "frame3") + + provider_path = os.path.join(self.getSourceDir(), "sbmutex_frame_provider.py") + artifact_path = self.getBuildArtifact("contention.txt") + if os.path.exists(artifact_path): + os.remove(artifact_path) + + commands = ["command script import " + provider_path] + commands.append( + "target frame-provider register " + "-C sbmutex_frame_provider.ContentionCheckFrameProvider " + "-k artifact_path -v " + artifact_path + ) + # `bt` only re-invokes get_frame_at_index when the thread's stack + # frame list was invalidated by a new stop, so interleave `bt` with + # `continue` (hitting the same breakpoint again, in a loop in + # main.c) to get repeated fresh invocations, raising the odds of + # hitting the race within a single test invocation. + commands.extend(["bt", "continue"] * 20) + commands.append("quit") + + stdin_path = self.getBuildArtifact("stdin.txt") + stdout_path = self.getBuildArtifact("stdout.txt") + with open(stdin_path, "w") as f: + f.write("\n".join(commands) + "\n") + + with open(stdin_path, "r") as in_fileH, open(stdout_path, "w") as out_fileH: + in_sbf = lldb.SBFile(in_fileH.fileno(), "r", False) + out_sbf = lldb.SBFile(out_fileH.fileno(), "w", False) + self.assertSuccess(self.dbg.SetInputFile(in_sbf)) + self.assertSuccess(self.dbg.SetOutputFile(out_sbf)) + self.assertSuccess(self.dbg.SetErrorFile(out_sbf)) + + options = lldb.SBCommandInterpreterRunOptions() + options.SetEchoCommands(False) + options.SetPrintResults(True) + options.SetStopOnError(False) + options.SetStopOnCrash(False) + + # try_lock() never blocks, so this can only hang if something + # else regresses (e.g. a leaked recursive lock count). + n_errors, quit_requested, has_crashed = self.dbg.RunCommandInterpreter( + True, False, options, 0, False, False + ) + + with open(stdout_path, "r") as out_fileH: + output = out_fileH.read() + + self.assertFalse(has_crashed, "lldb should not have crashed") + self.assertTrue(quit_requested, "quit command should have been processed") + self.assertEqual(n_errors, 0, f"unexpected errors in output:\n{output}") + self.assertIn("successfully registered scripted frame provider", output) + + self.assertTrue( + os.path.exists(artifact_path), + "get_frame_at_index should have run and recorded at least one outcome", + ) + with open(artifact_path, "r") as f: + outcomes = [line.strip() for line in f if line.strip()] + + self.assertTrue(outcomes, "expected at least one recorded outcome") + self.assertTrue( + all(o in ("CONTENDED", "UNCONTENDED") for o in outcomes), + f"unexpected outcome values: {outcomes}", + ) + # A "CONTENDED" outcome means some other thread held the real + # mutex at that moment, which proves SBMutex aliases the genuine, + # shared target mutex rather than the bypass mutex. Whether that + # race is hit is not guaranteed within a single invocation, so it + # isn't asserted on here. diff --git a/lldb/test/API/functionalities/scripted_frame_provider/sbmutex_reflects_target_mutex/hold_mutex_frame_provider.py b/lldb/test/API/functionalities/scripted_frame_provider/sbmutex_reflects_target_mutex/hold_mutex_frame_provider.py new file mode 100644 index 0000000000000..a6c36ce161ae4 --- /dev/null +++ b/lldb/test/API/functionalities/scripted_frame_provider/sbmutex_reflects_target_mutex/hold_mutex_frame_provider.py @@ -0,0 +1,46 @@ +""" +Frame provider whose get_frame_at_index locks the target's real API +mutex via SBMutex and holds it briefly, from inside the bypassed +scripted-extension callback. + +This is the exact pattern that used to deadlock: LLDB's private state +thread can reach this callback without already holding the real API +mutex, so a blocking lock() here is a genuinely new acquisition +attempt, not a safe same-thread recursive re-lock. Before +Target::APIMutexHandle re-resolved Target::GetAPIMutex() on every call +(rather than aliasing a fixed mutex captured at construction time), this +held mutex.lock() call would block waiting for another thread (e.g. the +command thread) that was itself waiting on this thread to finish +processing the stop -- an AB-BA deadlock. Now, because this thread is +running inside a scripted-extension callback, GetAPIMutex() hands it +the thread-local bypass mutex instead, so lock() never contends with +anyone. +""" + +import time + +from lldb.plugins.scripted_frame_provider import ScriptedFrameProvider + +# How long to hold the real API mutex on each get_frame_at_index(0) call. +HOLD_DURATION_SECONDS = 0.2 + + +class HoldMutexFrameProvider(ScriptedFrameProvider): + @staticmethod + def get_description(): + return "Provider that holds the real API mutex via SBMutex from get_frame_at_index" + + def get_frame_at_index(self, index): + if index >= len(self.input_frames): + return None + + if index == 0: + mutex = self.target.GetAPIMutex() + mutex.lock() + time.sleep(HOLD_DURATION_SECONDS) + mutex.unlock() + + frame = self.input_frames[index] + if frame is None: + return None + return {"idx": index, "pc": frame.GetPC()} diff --git a/lldb/test/API/functionalities/scripted_frame_provider/sbmutex_reflects_target_mutex/main.c b/lldb/test/API/functionalities/scripted_frame_provider/sbmutex_reflects_target_mutex/main.c new file mode 100644 index 0000000000000..ed95560986ac0 --- /dev/null +++ b/lldb/test/API/functionalities/scripted_frame_provider/sbmutex_reflects_target_mutex/main.c @@ -0,0 +1,12 @@ +int frame3() { return 3; } + +int frame2() { return frame3(); } + +int frame1() { return frame2(); } + +int main() { + int result = 0; + for (int i = 0; i < 25; ++i) + result += frame1(); + return result; +} diff --git a/lldb/test/API/functionalities/scripted_frame_provider/sbmutex_reflects_target_mutex/sbmutex_frame_provider.py b/lldb/test/API/functionalities/scripted_frame_provider/sbmutex_reflects_target_mutex/sbmutex_frame_provider.py new file mode 100644 index 0000000000000..26a6b7ecfe4ef --- /dev/null +++ b/lldb/test/API/functionalities/scripted_frame_provider/sbmutex_reflects_target_mutex/sbmutex_frame_provider.py @@ -0,0 +1,56 @@ +""" +Frame provider whose get_frame_at_index checks, from inside the bypassed +scripted-extension callback, whether the target's real API mutex is +currently held by a different thread. + +Used by TestSBMutexReflectsTargetMutex.py to confirm that SBMutex +(SBTarget::GetAPIMutex()) aliases the real, shared target mutex rather +than the thread-local mutex the bypass hands out for internal callers. + +Only try_lock() is used, and it is never held beyond the immediate +check: an earlier version of this provider held the mutex for a short +duration to try to widen the race window, but that meant a genuinely +blocking acquisition from whichever thread invoked this callback -- not +every internal caller (e.g. the private state thread) already holds the +real mutex by the time it gets here, so that held the mutex, which +caused a real deadlock in practice. try_lock() never blocks, so this +cannot deadlock regardless of the outcome. +""" + +from lldb.plugins.scripted_frame_provider import ScriptedFrameProvider + + +class ContentionCheckFrameProvider(ScriptedFrameProvider): + @staticmethod + def get_description(): + return "Provider that checks SBMutex contention from get_frame_at_index" + + def __init__(self, input_frames, args): + super().__init__(input_frames, args) + self.artifact_path = None + if self.args is not None: + value = self.args.GetValueForKey("artifact_path") + if value.IsValid(): + self.artifact_path = value.GetStringValue(4096) + + def get_frame_at_index(self, index): + if index >= len(self.input_frames): + return None + + if index == 0 and self.artifact_path: + mutex = self.target.GetAPIMutex() + if mutex.try_lock(): + # Uncontended: this thread already owned the real mutex + # (recursion) or nobody else holds it right now. Undo the + # extra recursive lock we just took. + mutex.unlock() + outcome = "UNCONTENDED" + else: + outcome = "CONTENDED" + with open(self.artifact_path, "a") as f: + f.write(outcome + "\n") + + frame = self.input_frames[index] + if frame is None: + return None + return {"idx": index, "pc": frame.GetPC()} diff --git a/lldb/tools/lldb-dap/InstructionBreakpoint.cpp b/lldb/tools/lldb-dap/InstructionBreakpoint.cpp index 89a0983b07809..738eee047c2c2 100644 --- a/lldb/tools/lldb-dap/InstructionBreakpoint.cpp +++ b/lldb/tools/lldb-dap/InstructionBreakpoint.cpp @@ -12,6 +12,7 @@ #include "lldb/API/SBBreakpoint.h" #include "lldb/API/SBTarget.h" #include "llvm/ADT/StringRef.h" +#include <mutex> namespace lldb_dap { diff --git a/lldb/tools/lldb-dap/ProtocolUtils.cpp b/lldb/tools/lldb-dap/ProtocolUtils.cpp index fb27859c5726f..d7d583a318d3a 100644 --- a/lldb/tools/lldb-dap/ProtocolUtils.cpp +++ b/lldb/tools/lldb-dap/ProtocolUtils.cpp @@ -21,6 +21,7 @@ #include "lldb/Host/PosixApi.h" // Adds PATH_MAX for windows #include <iomanip> +#include <mutex> #include <optional> #include <sstream> diff --git a/lldb/unittests/Utility/PolicyTest.cpp b/lldb/unittests/Utility/PolicyTest.cpp index 5ad045a03d30b..29d8bb884285b 100644 --- a/lldb/unittests/Utility/PolicyTest.cpp +++ b/lldb/unittests/Utility/PolicyTest.cpp @@ -145,7 +145,8 @@ TEST(PolicyTest, DumpPublicState) { EXPECT_EQ(s.GetString(), "policy: view=public, capabilities={" "eval_expr=true run_all=true try_all=true " - "bp_actions=true frame_providers=true frame_recognizers=true}"); + "bp_actions=true frame_providers=true frame_recognizers=true " + "bypass_api_mutex=false}"); } TEST(PolicyTest, DumpPrivateState) { @@ -154,7 +155,8 @@ 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=true frame_recognizers=true}"); + "bp_actions=true frame_providers=true frame_recognizers=true " + "bypass_api_mutex=false}"); } TEST(PolicyTest, DumpStack) { _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
