https://github.com/medismailben updated https://github.com/llvm/llvm-project/pull/208242
>From 5806b905c3428373b359053f4667ba42aa7ae83a Mon Sep 17 00:00:00 2001 From: Med Ismail Bennani <[email protected]> Date: Thu, 6 Aug 2026 15:06:56 -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. TargetAPILock::lock()/try_lock() check this capability when resolving which mutex to use: when the current thread's policy says it can bypass, they leave the handle pointing at nothing -- no synchronization primitive touched at all -- instead of the real mutex. Every existing caller keeps its own lock_guard/unique_lock code unchanged and becomes deadlock-safe automatically, since a no-op handle can be locked/unlocked from any thread with no cross-thread hazard. 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. Depends on #212872, which introduces TargetAPILock'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]> --- lldb/include/lldb/Utility/Policy.h | 11 ++ .../Interfaces/ScriptedPythonInterface.h | 7 + lldb/source/Target/TargetAPIMutex.cpp | 24 ++-- 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 | 123 ++++++++++++++++++ .../hold_mutex_frame_provider.py | 35 +++++ .../sbmutex_reflects_target_mutex/main.c | 12 ++ .../sbmutex_frame_provider.py | 58 +++++++++ lldb/unittests/Target/TargetAPIMutexTest.cpp | 50 +++++++ lldb/unittests/Utility/PolicyTest.cpp | 17 ++- 16 files changed, 544 insertions(+), 10 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/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/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h index ce48f2468d380..9ad4d4e40cecf 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" @@ -413,6 +414,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); @@ -536,6 +540,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/TargetAPIMutex.cpp b/lldb/source/Target/TargetAPIMutex.cpp index 26079c540ee2a..aefd10e7326e6 100644 --- a/lldb/source/Target/TargetAPIMutex.cpp +++ b/lldb/source/Target/TargetAPIMutex.cpp @@ -15,10 +15,14 @@ using namespace lldb_private; 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); + if (policy.capabilities.can_bypass_target_api_mutex) { + m_mutex = nullptr; + } else { + 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); + } } if (m_mutex) m_mutex->lock(); @@ -27,10 +31,14 @@ void TargetAPIMutex::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); + if (policy.capabilities.can_bypass_target_api_mutex) { + m_mutex = nullptr; + } else { + 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); + } } 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..9fc83b1d47553 --- /dev/null +++ b/lldb/test/API/functionalities/scripted_frame_provider/sbmutex_reflects_target_mutex/TestSBMutexReflectsTargetMutex.py @@ -0,0 +1,123 @@ +""" +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. This test drives the same kind of command-thread and +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. + +The provider obtains the mutex from inside get_frame_at_index, which is +safe since obtaining a handle doesn't resolve or lock anything, but the +actual try_lock() runs on a plain background thread it spawns for the +check (see sbmutex_frame_provider.py for why). Only try_lock() is used, +never lock(), because an earlier version of this test tried to widen +the race window by blocking here, on the assumption that whichever +thread reaches this callback already holds the real mutex first; that +assumption is wrong, since LLDB's private state thread can reach this +callback without already holding it, and that blocking call +reproducibly deadlocked in practice. Observing contention is a genuine +cross-thread race, so this test is best-effort: it raises the odds of +witnessing it within a single invocation but cannot guarantee it, and +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) + + 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 ( + "another thread held the real target API mutex", + "no other thread held the real target API mutex", + ) + for o in outcomes + ), + f"unexpected outcome values: {outcomes}", + ) + # Whether this specific outcome occurs is a race (see module + # docstring); not 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..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..63dbaabc0daca --- /dev/null +++ b/lldb/test/API/functionalities/scripted_frame_provider/sbmutex_reflects_target_mutex/sbmutex_frame_provider.py @@ -0,0 +1,58 @@ +""" +Frame provider whose get_frame_at_index spawns a background thread to +try_lock() the target's real API mutex, confirming SBMutex aliases the +genuine mutex rather than the no-op TargetAPIMutex resolves to under the +can_bypass_target_api_mutex policy that ScriptedPythonInterface::Dispatch +pushes for the callback's entire duration. The background thread has no +scripted-extension call on its stack, so it is exempt from that policy. +""" + +import threading + +from lldb.plugins.scripted_frame_provider import ScriptedFrameProvider + + +class ContentionCheckFrameProvider(ScriptedFrameProvider): + @staticmethod + def get_description(): + return "Provider that checks SBMutex contention from a background thread" + + 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, mutex): + # Runs on a fresh thread with no scripted-extension call (and so no + # can_bypass_target_api_mutex) on its stack. See module docstring. + if mutex.try_lock(): + # Uncontended: nobody else holds the real mutex right now. + # Undo the lock we just took. + mutex.unlock() + outcome = "no other thread held the real target API mutex" + else: + outcome = "another thread held the real target API mutex" + 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 the mutex handle itself doesn't lock anything. + # It's safe to do from inside the bypassed callback. Only the + # actual try_lock() call, on the background thread, needs to + # happen outside the bypass. + mutex = self.target.GetAPIMutex() + checker = threading.Thread(target=self._check_contention, args=(mutex,)) + 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/Target/TargetAPIMutexTest.cpp b/lldb/unittests/Target/TargetAPIMutexTest.cpp index 2650723ec6f4e..22e3da835a5ea 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,52 @@ TEST_F(TargetAPIMutexTargetTest, ResolvesFreshOnEachLockCall) { contended.join(); lock.unlock(); } + +TEST_F(TargetAPIMutexTargetTest, + UnlockReplaysLockResolutionAcrossPolicyChange) { + // Regression test for the cross-thread bypass bug: lock() and unlock() + // must agree on which mutex they touch even if the calling thread's + // policy changes in between, because 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(); + + // Simulate the calling thread now running inside a scripted-extension + // callback: 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 outer_lock(target_sp); + outer_lock.lock(); + + // A handle resolved while the bypass policy is active never touches + // the real (already-held) mutex, so it succeeds even though the real + // mutex is contended. + PolicyStack::Guard guard = PolicyStack::Get().PushScriptedExtensionCall(); + TargetAPIMutex lock(target_sp); + EXPECT_TRUE(lock.try_lock()); + lock.unlock(); +} diff --git a/lldb/unittests/Utility/PolicyTest.cpp b/lldb/unittests/Utility/PolicyTest.cpp index 5ad045a03d30b..56edfb68f6855 100644 --- a/lldb/unittests/Utility/PolicyTest.cpp +++ b/lldb/unittests/Utility/PolicyTest.cpp @@ -70,6 +70,17 @@ 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); + + // Inherits the current view/capabilities rather than resetting them. + 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 +156,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 +166,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
