https://github.com/medismailben updated https://github.com/llvm/llvm-project/pull/208242
>From d4d83314601d0c1a82e0563abcfa77ea74478d2d Mon Sep 17 00:00:00 2001 From: Med Ismail Bennani <[email protected]> Date: Wed, 19 Aug 2026 06:54:58 +0100 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: CreatePluginObject, 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. Target::GetAPIMutexForCurrentPolicy answers which mutex the calling thread has to serialize on, or none at all when its policy bypasses, which keeps the choice between the public and the private mutex inside Target. TargetAPIMutex resolves through it on every lock()/try_lock(), so every existing caller keeps its own lock_guard/unique_lock code unchanged and becomes deadlock-safe automatically: a no-op handle can be locked and unlocked from any thread with no cross-thread hazard. Extensions that run directly on the user's behalf opt out. A scripted command that the user invoked directly, rather than a callback running internally, needs to keep serializing on the API mutex. This is achieved by checking ScriptedInterface::UserCanRunDirectly so the exemption covers every scripting language rather than only Python. This patch adds regression tests for both the original deadlock and for a blocking SBMutex.lock() call made from inside a callback, including TestSBMutexReflectsTargetMutex, which confirms SBMutex aliases the real, shared target mutex rather than the bypass no-op. Both bypass tests are written so that they fail without the bypass. Depends on #212872, which introduces TargetAPIMutex's per-call resolve/replay behavior that makes this bypass safe to observe through a deferred-lock SBMutex. Signed-off-by: Med Ismail Bennani <[email protected]> --- .../Interfaces/ScriptedCommandInterface.h | 2 + .../Interfaces/ScriptedInterface.h | 6 + lldb/include/lldb/Target/Target.h | 4 + lldb/include/lldb/Target/TargetAPIMutex.h | 2 + lldb/include/lldb/Utility/Policy.h | 7 ++ .../Interfaces/ScriptedPythonInterface.h | 13 ++ .../ScriptedFrameProvider.cpp | 11 +- lldb/source/Target/Target.cpp | 8 ++ lldb/source/Target/TargetAPIMutex.cpp | 27 ++--- lldb/source/Utility/Policy.cpp | 7 ++ .../Makefile | 2 + ...ProviderRegisterCommandAPIMutexDeadlock.py | 87 ++++++++++++++ .../frame_provider.py | 23 ++++ .../main.c | 7 ++ .../sbmutex_reflects_target_mutex/Makefile | 2 + .../TestHoldMutexNoDeadlock.py | 89 ++++++++++++++ .../TestSBMutexReflectsTargetMutex.py | 111 ++++++++++++++++++ .../hold_mutex_frame_provider.py | 35 ++++++ .../sbmutex_reflects_target_mutex/main.c | 12 ++ .../sbmutex_frame_provider.py | 84 +++++++++++++ lldb/unittests/Interpreter/CMakeLists.txt | 1 + .../Interpreter/TestScriptedInterface.cpp | 55 +++++++++ lldb/unittests/Target/TargetAPIMutexTest.cpp | 61 ++++++++++ lldb/unittests/Utility/PolicyTest.cpp | 16 ++- 24 files changed, 651 insertions(+), 21 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 create mode 100644 lldb/unittests/Interpreter/TestScriptedInterface.cpp diff --git a/lldb/include/lldb/Interpreter/Interfaces/ScriptedCommandInterface.h b/lldb/include/lldb/Interpreter/Interfaces/ScriptedCommandInterface.h index 29f4d273e49f0..879aaa936f759 100644 --- a/lldb/include/lldb/Interpreter/Interfaces/ScriptedCommandInterface.h +++ b/lldb/include/lldb/Interpreter/Interfaces/ScriptedCommandInterface.h @@ -15,6 +15,8 @@ namespace lldb_private { class ScriptedCommandInterface : virtual public ScriptedInterface { public: + bool UserCanRunDirectly() const override { return true; } + virtual llvm::Expected<StructuredData::GenericSP> CreatePluginObject(llvm::StringRef class_name, lldb::DebuggerSP debugger_sp) = 0; diff --git a/lldb/include/lldb/Interpreter/Interfaces/ScriptedInterface.h b/lldb/include/lldb/Interpreter/Interfaces/ScriptedInterface.h index 21bb91960f777..417a143cb3e3d 100644 --- a/lldb/include/lldb/Interpreter/Interfaces/ScriptedInterface.h +++ b/lldb/include/lldb/Interpreter/Interfaces/ScriptedInterface.h @@ -37,6 +37,12 @@ class ScriptedInterface { return m_scripted_metadata; } + /// Whether the user can invoke this extension directly, the way a scripted + /// command can. Those never introduce the target's API mutex bypass, so at + /// top level they serialize like any other command; nested inside an + /// already-bypassed callback every extension inherits the ambient policy. + virtual bool UserCanRunDirectly() const { return false; } + struct AbstractMethodRequirement { llvm::StringLiteral name; size_t min_arg_count = 0; diff --git a/lldb/include/lldb/Target/Target.h b/lldb/include/lldb/Target/Target.h index 71ba92e624404..31a59a8501338 100644 --- a/lldb/include/lldb/Target/Target.h +++ b/lldb/include/lldb/Target/Target.h @@ -2050,6 +2050,10 @@ class Target : public std::enable_shared_from_this<Target>, void PrintDummySignals(Stream &strm, Args &signals); protected: + /// The mutex the calling thread must serialize on for its current policy, or + /// nullptr when that policy bypasses the API mutex entirely. + std::recursive_mutex *GetAPIMutexForCurrentPolicy(); + /// Implementing of ModuleList::Notifier. void NotifyModuleAdded(const ModuleList &module_list, diff --git a/lldb/include/lldb/Target/TargetAPIMutex.h b/lldb/include/lldb/Target/TargetAPIMutex.h index d822329842264..01af2ac7d31f1 100644 --- a/lldb/include/lldb/Target/TargetAPIMutex.h +++ b/lldb/include/lldb/Target/TargetAPIMutex.h @@ -56,6 +56,8 @@ class TargetAPIMutex { } private: + void Resolve(); + /// An aliasing shared_ptr into m_target_sp's own mutex, resolved fresh /// on every lock()/try_lock() call. Shares m_target_sp's control block /// (keeping the Target alive) while pointing at the mutex living inside diff --git a/lldb/include/lldb/Utility/Policy.h b/lldb/include/lldb/Utility/Policy.h index afeeab19c2ed0..b3f2e1ba04e9c 100644 --- a/lldb/include/lldb/Utility/Policy.h +++ b/lldb/include/lldb/Utility/Policy.h @@ -50,6 +50,7 @@ struct Policy { bool can_run_breakpoint_actions = true; bool can_load_frame_providers = true; bool can_run_frame_recognizers = true; + bool can_bypass_target_api_mutex = false; }; /// Why a private-state policy is being pushed. Distinguishes a PST's @@ -75,6 +76,7 @@ struct Policy { static Policy CreatePrivateState( PrivateStatePurpose purpose = PrivateStatePurpose::Default); static Policy CreatePublicStateRunningExpression(); + static Policy CreateScriptedExtensionCall(); /// @} void Dump(Stream &s) const; @@ -140,6 +142,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/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h index ce48f2468d380..91663d1293108 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h +++ b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h @@ -18,6 +18,7 @@ #include "lldb/API/SBCommandReturnObject.h" #include "lldb/Interpreter/Interfaces/ScriptedInterface.h" #include "lldb/Utility/DataBufferHeap.h" +#include "lldb/Utility/Policy.h" #include "../PythonDataObjects.h" #include "../SWIGPythonBridge.h" @@ -196,6 +197,10 @@ class ScriptedPythonInterface : virtual public ScriptedInterface { return create_error("Missing scripting object."); } + std::optional<PolicyStack::Guard> policy_guard; + if (!UserCanRunDirectly()) + policy_guard = PolicyStack::Get().PushScriptedExtensionCall(); + Locker py_lock(&m_interpreter, Locker::AcquireLock | Locker::NoSTDIN, Locker::FreeLock); @@ -413,6 +418,10 @@ class ScriptedPythonInterface : virtual public ScriptedInterface { return ErrorWithMessage<T>(caller_signature, "missing script class name", error); + std::optional<PolicyStack::Guard> policy_guard; + if (!UserCanRunDirectly()) + policy_guard = PolicyStack::Get().PushScriptedExtensionCall(); + Locker py_lock(&m_interpreter, Locker::AcquireLock | Locker::NoSTDIN, Locker::FreeLock); @@ -536,6 +545,10 @@ class ScriptedPythonInterface : virtual public ScriptedInterface { return ErrorWithMessage<T>(caller_signature, "python object ill-formed", error); + std::optional<PolicyStack::Guard> policy_guard; + if (!UserCanRunDirectly()) + policy_guard = PolicyStack::Get().PushScriptedExtensionCall(); + Locker py_lock(&m_interpreter, Locker::AcquireLock | Locker::NoSTDIN, Locker::FreeLock); diff --git a/lldb/source/Plugins/SyntheticFrameProvider/ScriptedFrameProvider/ScriptedFrameProvider.cpp b/lldb/source/Plugins/SyntheticFrameProvider/ScriptedFrameProvider/ScriptedFrameProvider.cpp index ab09d86b24d95..977ea5e0548e4 100644 --- a/lldb/source/Plugins/SyntheticFrameProvider/ScriptedFrameProvider/ScriptedFrameProvider.cpp +++ b/lldb/source/Plugins/SyntheticFrameProvider/ScriptedFrameProvider/ScriptedFrameProvider.cpp @@ -179,10 +179,13 @@ ScriptedFrameProvider::GetFrameAtIndex(uint32_t idx) { if (real_frame_index < m_input_frames->GetNumFrames()) { StackFrameSP real_frame_sp = m_input_frames->GetFrameAtIndex(real_frame_index); - synth_frame_sp = - (real_frame_index == idx) - ? real_frame_sp - : std::make_shared<BorrowedStackFrame>(real_frame_sp, idx); + // Always wrap in a BorrowedStackFrame, even when the index is + // unchanged. FetchFramesUpTo below unconditionally overwrites + // frame_sp->m_frame_list_id to tag the frame as belonging to this + // synthetic list; reusing real_frame_sp directly would corrupt the + // parent list's cached frame (still m_input_frames' object) to claim + // it belongs to this list instead. + synth_frame_sp = std::make_shared<BorrowedStackFrame>(real_frame_sp, idx); } } else if (StructuredData::Dictionary *dict = obj_sp->GetAsDictionary()) { // Check if it's a dictionary describing a frame. diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp index ca42d027f4c22..24f61519b8cdb 100644 --- a/lldb/source/Target/Target.cpp +++ b/lldb/source/Target/Target.cpp @@ -66,6 +66,7 @@ #include "lldb/Utility/LLDBAssert.h" #include "lldb/Utility/LLDBLog.h" #include "lldb/Utility/Log.h" +#include "lldb/Utility/Policy.h" #include "lldb/Utility/RealpathPrefixes.h" #include "lldb/Utility/State.h" #include "lldb/Utility/StreamString.h" @@ -6023,6 +6024,13 @@ TargetAPIMutex Target::GetAPIMutex() { return TargetAPIMutex(shared_from_this()); } +std::recursive_mutex *Target::GetAPIMutexForCurrentPolicy() { + Policy policy = PolicyStack::Get().Current(); + if (policy.capabilities.can_bypass_target_api_mutex) + return nullptr; + return policy.view == Policy::View::Private ? &m_private_mutex : &m_mutex; +} + /// Get metrics associated with this target in JSON format. llvm::json::Value Target::ReportStatistics(const lldb_private::StatisticsOptions &options) { diff --git a/lldb/source/Target/TargetAPIMutex.cpp b/lldb/source/Target/TargetAPIMutex.cpp index 26079c540ee2a..97d5fa98ac1a3 100644 --- a/lldb/source/Target/TargetAPIMutex.cpp +++ b/lldb/source/Target/TargetAPIMutex.cpp @@ -8,29 +8,26 @@ #include "lldb/Target/TargetAPIMutex.h" #include "lldb/Target/Target.h" -#include "lldb/Utility/Policy.h" using namespace lldb_private; +void TargetAPIMutex::Resolve() { + if (!m_target_sp) + return; + + std::recursive_mutex *real_mutex = m_target_sp->GetAPIMutexForCurrentPolicy(); + m_mutex = real_mutex + ? std::shared_ptr<std::recursive_mutex>(m_target_sp, real_mutex) + : nullptr; +} + void TargetAPIMutex::lock() { - if (m_target_sp) { - Policy policy = PolicyStack::Get().Current(); - std::recursive_mutex &real_mutex = policy.view == Policy::View::Private - ? m_target_sp->m_private_mutex - : m_target_sp->m_mutex; - m_mutex = std::shared_ptr<std::recursive_mutex>(m_target_sp, &real_mutex); - } + Resolve(); if (m_mutex) m_mutex->lock(); } bool TargetAPIMutex::try_lock() { - if (m_target_sp) { - Policy policy = PolicyStack::Get().Current(); - std::recursive_mutex &real_mutex = policy.view == Policy::View::Private - ? m_target_sp->m_private_mutex - : m_target_sp->m_mutex; - m_mutex = std::shared_ptr<std::recursive_mutex>(m_target_sp, &real_mutex); - } + Resolve(); return m_mutex ? m_mutex->try_lock() : true; } 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..e93ed6890fc04 --- /dev/null +++ b/lldb/test/API/functionalities/scripted_frame_provider/register_command_api_mutex_deadlock/TestFrameProviderRegisterCommandAPIMutexDeadlock.py @@ -0,0 +1,87 @@ +""" +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: 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 + # (see module docstring). + 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..9b4b948eb372d --- /dev/null +++ b/lldb/test/API/functionalities/scripted_frame_provider/register_command_api_mutex_deadlock/frame_provider.py @@ -0,0 +1,23 @@ +""" +Frame provider that returns dict-based synthetic frames while touching +self.input_frames from get_frame_at_index, to exercise the API-mutex +deadlock. +""" + +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..539c8949c4671 --- /dev/null +++ b/lldb/test/API/functionalities/scripted_frame_provider/sbmutex_reflects_target_mutex/TestHoldMutexNoDeadlock.py @@ -0,0 +1,89 @@ +""" +Test that a scripted frame provider can safely call a blocking +SBMutex.lock() from inside get_frame_at_index without deadlocking. + +The private state thread can reach this callback without already +holding the target's real API mutex. Without the bypass described +below, a blocking lock() call here could genuinely wait for that mutex, +and deadlock if some other thread (e.g. a `bt` command thread) holds it +at that moment. ScriptedPythonInterface::Dispatch prevents this by +pushing the can_bypass_target_api_mutex policy around the whole +callback. TargetAPIMutex re-checks that policy on every lock() call +rather than caching whatever was current when the SBMutex was +constructed, so lock() here resolves to a genuine no-op instead: no +synchronization primitive is touched at all, and it never contends with +anyone. + +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..fed5000daecbb --- /dev/null +++ b/lldb/test/API/functionalities/scripted_frame_provider/sbmutex_reflects_target_mutex/TestSBMutexReflectsTargetMutex.py @@ -0,0 +1,111 @@ +""" +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 obtainable inside +a bypassed scripted callback and locked later, once that bypass no +longer applies (e.g. on a different thread with no scripted-extension +call on its stack, as this test's own provider does; see +sbmutex_frame_provider.py). So it must always alias the genuine target +mutex rather than resolving to the no-op the bypass policy makes it for +internal callers. + +The provider obtains the mutex from inside get_frame_at_index, which is +safe since obtaining a handle doesn't resolve or lock anything, and does +every acquisition with try_lock() on threads it spawns and joins (see +sbmutex_frame_provider.py). Nothing may block on lock() there, on any +thread: the thread that reaches the callback may already hold the real +mutex while waiting on the provider, so a blocking acquisition deadlocks +the session. `bt` is interleaved with `continue` so the callback runs many +times rather than once. +""" + +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 of the check. + 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) + + 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") + # Either recorded outcome means a try_lock() failed, which a no-op + # handle can never do. Only the third outcome, two handles holding the + # mutex at once, indicates SBMutex resolved to the bypass no-op. + self.assertTrue( + set(outcomes) + <= { + "second handle contended with the first", + "another thread already held the real mutex", + }, + f"SBMutex did not alias the real target mutex: {outcomes}", + ) 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..a7d2db5d567d0 --- /dev/null +++ b/lldb/test/API/functionalities/scripted_frame_provider/sbmutex_reflects_target_mutex/hold_mutex_frame_provider.py @@ -0,0 +1,35 @@ +""" +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. See TestHoldMutexNoDeadlock.py for why this +must not deadlock. +""" + +import time + +from lldb.plugins.scripted_frame_provider import ScriptedFrameProvider + +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..655fb62f130ae --- /dev/null +++ b/lldb/test/API/functionalities/scripted_frame_provider/sbmutex_reflects_target_mutex/sbmutex_frame_provider.py @@ -0,0 +1,84 @@ +""" +Frame provider whose get_frame_at_index confirms SBMutex aliases the target's +genuine API mutex rather than the no-op TargetAPIMutex resolves to under the +can_bypass_target_api_mutex policy that ScriptedPythonInterface pushes for a +callback's entire duration. + +Every acquisition here uses try_lock() and runs on a freshly spawned thread, so +no scripted-extension call is on its stack and none of it is exempt from the +real mutex. Blocking on lock() is never an option: the thread that reaches this +callback may already hold the real mutex and is waiting on this code, so a +blocking acquisition on any thread deadlocks the session. + +TargetAPIMutex::try_lock() returns true unconditionally when it resolves to the +no-op, so any *failed* try_lock() proves the handle reached the real mutex. +""" + +import threading + +from lldb.plugins.scripted_frame_provider import ScriptedFrameProvider + +# A second handle could not take the mutex the first one holds, so the two alias +# the same real mutex. +CONTENDED = "second handle contended with the first" +# Some other thread already held the real mutex, which a no-op cannot do. +OTHER_HOLDER = "another thread already held the real mutex" +# Failure: two handles held the mutex at once, so at least one is a no-op. +UNCONTENDED = "two handles held the real mutex at once" + + +class ContentionCheckFrameProvider(ScriptedFrameProvider): + @staticmethod + def get_description(): + return "Provider that checks SBMutex contention from background threads" + + 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 _check_contention(self): + first = self.target.GetAPIMutex() + if not first.try_lock(): + self._record(OTHER_HOLDER) + return + + outcome = [UNCONTENDED] + + def check_from_another_thread(): + other = self.target.GetAPIMutex() + if other.try_lock(): + other.unlock() + else: + outcome[0] = CONTENDED + + other_thread = threading.Thread(target=check_from_another_thread) + other_thread.start() + other_thread.join() + first.unlock() + self._record(outcome[0]) + + def _record(self, outcome): + with open(self.artifact_path, "a") as f: + f.write(outcome + "\n") + + def get_frame_at_index(self, index): + if index >= len(self.input_frames): + return None + + if index == 0 and self.artifact_path: + # Obtaining a handle locks nothing, so it is safe on this thread; + # only the try_lock() calls have to run elsewhere. Every spawned + # thread is joined before returning, so nothing holds the mutex + # once the bypass ends. + checker = threading.Thread(target=self._check_contention) + checker.start() + checker.join() + + frame = self.input_frames[index] + if frame is None: + return None + return {"idx": index, "pc": frame.GetPC()} diff --git a/lldb/unittests/Interpreter/CMakeLists.txt b/lldb/unittests/Interpreter/CMakeLists.txt index 7eec76105aad2..80308b48b806f 100644 --- a/lldb/unittests/Interpreter/CMakeLists.txt +++ b/lldb/unittests/Interpreter/CMakeLists.txt @@ -7,6 +7,7 @@ add_lldb_unittest(InterpreterTests TestOptionValue.cpp TestOptionValueFileColonLine.cpp TestRegexCommand.cpp + TestScriptedInterface.cpp LINK_LIBS lldbCommands diff --git a/lldb/unittests/Interpreter/TestScriptedInterface.cpp b/lldb/unittests/Interpreter/TestScriptedInterface.cpp new file mode 100644 index 0000000000000..48fd98e601e0e --- /dev/null +++ b/lldb/unittests/Interpreter/TestScriptedInterface.cpp @@ -0,0 +1,55 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "lldb/Interpreter/Interfaces/ScriptedCommandInterface.h" +#include "lldb/Interpreter/Interfaces/ScriptedInterface.h" +#include "gtest/gtest.h" + +using namespace lldb_private; + +namespace { + +class DummyScriptedInterface : public ScriptedInterface { +public: + llvm::SmallVector<AbstractMethodRequirement> + GetAbstractMethodRequirements() const override { + return {}; + } +}; + +class DummyScriptedCommandInterface : public ScriptedCommandInterface { +public: + llvm::Expected<StructuredData::GenericSP> + CreatePluginObject(llvm::StringRef class_name, + lldb::DebuggerSP debugger_sp) override { + return nullptr; + } + + llvm::SmallVector<AbstractMethodRequirement> + GetAbstractMethodRequirements() const override { + return {}; + } +}; + +} // namespace + +TEST(ScriptedInterfaceTest, ExtensionsCannotBeRunDirectly) { + DummyScriptedInterface interface; + EXPECT_FALSE(interface.UserCanRunDirectly()); +} + +TEST(ScriptedInterfaceTest, CommandsCanBeRunDirectly) { + DummyScriptedCommandInterface command_interface; + EXPECT_TRUE(command_interface.UserCanRunDirectly()); + + // The scripted-extension policy is pushed through a ScriptedInterface, so the + // override has to be reachable from the base: a command that looks like any + // other extension there would silently lose its API mutex. + ScriptedInterface &as_base = command_interface; + EXPECT_TRUE(as_base.UserCanRunDirectly()); +} diff --git a/lldb/unittests/Target/TargetAPIMutexTest.cpp b/lldb/unittests/Target/TargetAPIMutexTest.cpp index 2650723ec6f4e..7311ed0f125e6 100644 --- a/lldb/unittests/Target/TargetAPIMutexTest.cpp +++ b/lldb/unittests/Target/TargetAPIMutexTest.cpp @@ -14,6 +14,7 @@ #include "lldb/Target/Platform.h" #include "lldb/Target/Target.h" #include "lldb/Utility/ArchSpec.h" +#include "lldb/Utility/Policy.h" #include "gtest/gtest.h" #include <thread> @@ -193,3 +194,63 @@ TEST_F(TargetAPIMutexTargetTest, ResolvesFreshOnEachLockCall) { contended.join(); lock.unlock(); } + +TEST_F(TargetAPIMutexTargetTest, + UnlockReplaysLockResolutionAcrossPolicyChange) { + // lock() and unlock() must agree on which mutex they touch even if the + // calling thread's policy changes in between: unlock() replays lock()'s + // resolution rather than re-resolving from the current policy. + TargetSP target_sp = CreateTarget(); + ASSERT_TRUE(target_sp); + + TargetAPIMutex lock(target_sp); + lock.lock(); + + // If unlock() re-resolved here it would see the bypass and skip releasing + // the mutex it actually locked. + { + PolicyStack::Guard guard = PolicyStack::Get().PushScriptedExtensionCall(); + lock.unlock(); + } + + // The real mutex must have actually been released: a fresh acquisition + // from a different thread (outside the bypass policy) must succeed + // immediately. A same-thread try_lock() would pass even if unlock() had + // incorrectly no-op'd, since std::recursive_mutex lets the same thread + // reenter a lock it still holds. + std::thread t([target_sp]() { + TargetAPIMutex background_lock(target_sp); + EXPECT_TRUE(background_lock.try_lock()); + background_lock.unlock(); + }); + t.join(); +} + +TEST_F(TargetAPIMutexTargetTest, BypassPolicyMakesTryLockANoOp) { + TargetSP target_sp = CreateTarget(); + ASSERT_TRUE(target_sp); + + TargetAPIMutex holder(target_sp); + holder.lock(); + + // The contention has to come from another thread: std::recursive_mutex lets + // the owning thread reenter a lock it already holds, so a same-thread + // try_lock() would succeed whether or not the bypass is in effect. + std::thread contended([target_sp]() { + TargetAPIMutex background_lock(target_sp); + EXPECT_FALSE(background_lock.try_lock()); + }); + contended.join(); + + // The bypass touches no primitive, so the same acquisition succeeds while + // the real mutex is held elsewhere. + std::thread bypassed([target_sp]() { + PolicyStack::Guard guard = PolicyStack::Get().PushScriptedExtensionCall(); + TargetAPIMutex background_lock(target_sp); + EXPECT_TRUE(background_lock.try_lock()); + background_lock.unlock(); + }); + bypassed.join(); + + holder.unlock(); +} diff --git a/lldb/unittests/Utility/PolicyTest.cpp b/lldb/unittests/Utility/PolicyTest.cpp index 5ad045a03d30b..57919b57bdf0a 100644 --- a/lldb/unittests/Utility/PolicyTest.cpp +++ b/lldb/unittests/Utility/PolicyTest.cpp @@ -70,6 +70,16 @@ TEST(PolicyTest, PublicStateRunningExpression) { EXPECT_TRUE(p.capabilities.can_run_frame_recognizers); } +TEST(PolicyTest, ScriptedExtensionCall) { + Policy p = Policy::CreateScriptedExtensionCall(); + EXPECT_TRUE(p.capabilities.can_bypass_target_api_mutex); + + PolicyStack::Guard guard = PolicyStack::Get().PushPrivateState(); + Policy nested = Policy::CreateScriptedExtensionCall(); + EXPECT_EQ(nested.view, Policy::View::Private); + EXPECT_TRUE(nested.capabilities.can_bypass_target_api_mutex); +} + TEST(PolicyTest, StackDefaultIsPublicState) { Policy current = PolicyStack::Get().Current(); EXPECT_EQ(current.view, Policy::View::Public); @@ -145,7 +155,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 +165,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
