llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Med Ismail Bennani (medismailben) <details> <summary>Changes</summary> `ScriptedFrameProvider::GetFrameAtIndex` reused the parent list's live StackFrame directly whenever a provider's `get_frame_at_index` forwarded a frame under its own index (`real_frame_index` == `idx`), instead of wrapping it in a `BorrowedStackFrame` like every other forwarding case. `SyntheticStackFrameList::FetchFramesUpTo` unconditionally re-tags the returned frame's `m_frame_list_id` to this child list before caching it. Reusing the parent list's object means this re-tag corrupts the parent list's own cached frame to claim it belongs to the child list instead. Later, resolving that frame's identity back to a frame list (via `ExecutionContextRef::GetFrameSP`, used by `SBFrame` validity checks) follows the corrupted tag to the child list. If the resolving thread is the one already fetching frames on that child list -- holding its writer lock via `GetFramesUpTo`, it self-deadlocks trying to take the (non-reentrant) reader lock on the same list. This changes the frame to be always wrapped in a `BorrowedStackFrame`, even when the index is unchanged, so the object added to the child list is never the same object cached in the parent list. This patch also adds a regression test using an identity-forwarding provider (mirroring the shape that surfaced this bug) to isolate it from the API-mutex deadlock fixed in the parent commit. Like that test, this is a best-effort race reproduction, not a guaranteed one. Signed-off-by: Med Ismail Bennani <ismail@<!-- -->bennani.ma> --- Patch is 23.18 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/208992.diff 17 Files Affected: - (modified) lldb/include/lldb/Target/ExecutionContext.h (+2-2) - (modified) lldb/include/lldb/Target/Target.h (+5) - (modified) lldb/include/lldb/Utility/Policy.h (+11) - (modified) lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h (+7) - (modified) lldb/source/Plugins/SyntheticFrameProvider/ScriptedFrameProvider/ScriptedFrameProvider.cpp (+7-4) - (modified) lldb/source/Target/ExecutionContext.cpp (+1-2) - (modified) lldb/source/Target/Target.cpp (+10) - (modified) lldb/source/Utility/Policy.cpp (+7) - (added) lldb/test/API/functionalities/scripted_frame_provider/register_command_api_mutex_deadlock/Makefile (+2) - (added) lldb/test/API/functionalities/scripted_frame_provider/register_command_api_mutex_deadlock/TestFrameProviderRegisterCommandAPIMutexDeadlock.py (+89) - (added) lldb/test/API/functionalities/scripted_frame_provider/register_command_api_mutex_deadlock/frame_provider.py (+28) - (added) lldb/test/API/functionalities/scripted_frame_provider/register_command_api_mutex_deadlock/main.c (+7) - (added) lldb/test/API/functionalities/scripted_frame_provider/register_command_frame_alias_deadlock/Makefile (+3) - (added) lldb/test/API/functionalities/scripted_frame_provider/register_command_frame_alias_deadlock/TestFrameProviderRegisterCommandFrameAliasDeadlock.py (+94) - (added) lldb/test/API/functionalities/scripted_frame_provider/register_command_frame_alias_deadlock/frame_provider.py (+30) - (added) lldb/test/API/functionalities/scripted_frame_provider/register_command_frame_alias_deadlock/main.c (+7) - (modified) lldb/unittests/Utility/PolicyTest.cpp (+4-2) ``````````diff 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 c8b7c477505ee..9eb17dda1755f 100644 --- a/lldb/include/lldb/Target/Target.h +++ b/lldb/include/lldb/Target/Target.h @@ -761,6 +761,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 + /// doesn't need to serialize, 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..63a17523c8b36 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 SB API calls made on this thread may skip re-acquiring the + /// target's API mutex, 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_reenter_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 7d0d4cdd3c6d1..79ca76577ffa9 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" @@ -415,6 +416,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); @@ -510,6 +514,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/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/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 daa1b1c43efa8..2777f29a3f65c 100644 --- a/lldb/source/Target/Target.cpp +++ b/lldb/source/Target/Target.cpp @@ -5994,6 +5994,16 @@ Target::TargetEventData::GetModuleListFromEvent(const Event *event_ptr) { std::recursive_mutex &Target::GetAPIMutex() { Policy policy = PolicyStack::Get().Current(); + + // A thread whose policy says it doesn't need to serialize on 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_reenter_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 301f7ee36e897..a375cf03181b3 100644 --- a/lldb/source/Utility/Policy.cpp +++ b/lldb/source/Utility/Policy.cpp @@ -59,6 +59,12 @@ Policy Policy::CreatePublicStateRunningExpression() { return p; } +Policy Policy::CreateScriptedExtensionCall() { + Policy p = PolicyStack::Get().Current(); + p.capabilities.can_reenter_target_api_mutex = true; + return p; +} + PolicyStack::Guard::~Guard() { if (!m_active) return; @@ -103,6 +109,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 << " reenter_api_mutex=" << capabilities.can_reenter_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/register_command_frame_alias_deadlock/Makefile b/lldb/test/API/functionalities/scripted_frame_provider/register_command_frame_alias_deadlock/Makefile new file mode 100644 index 0000000000000..0b710c6e298ae --- /dev/null +++ b/lldb/test/API/functionalities/scripted_frame_provider/register_command_frame_alias_deadlock/Makefile @@ -0,0 +1,3 @@ +C_SOURCES := main.c +CFLAGS_EXTRAS := -std=c99 +include Makefile.rules diff --git a/lldb/test/API/functionalities/scripted_frame_provider/register_command_frame_alias_deadlock/TestFrameProviderRegisterCommandFrameAliasDeadlock.py b/lldb/test/API/functionalities/scripted_frame_provider/register_command_frame_alias_deadlock/TestFrameProviderRegisterCommandFrameAliasDeadlock.py new file mode 100644 index 0000000000000..dee8bf01c7195 --- /dev/null +++ b/lldb/test/API/functionalities/scripted_frame_provider/register_command_frame_alias_deadlock/TestFrameProviderRegisterCommandFrameAliasDeadlock.py @@ -0,0 +1,94 @@ +""" +Test that a scripted frame provider that forwards frames under their own +index (identity forwarding) does not deadlock when running `bt` from the +command interpreter. + +ScriptedFrameProvider::GetFrameAtIndex reused the parent list's live +StackFrame object directly whenever a provider forwarded a frame under +its own index, instead of wrapping it in a BorrowedStackFrame. Frame +construction unconditionally re-tags the returned frame as belonging to +the child list, corrupting the parent list's cached frame. Once that +frame's corrupted list identity is later resolved, it points back at +the (possibly still-being-built) child list, and a thread already +holding that list's writer lock can self-deadlock taking the reader +lock. + +The event-handler thread that can trigger this only runs when commands +are driven through SBDebugger.RunCommandInterpreter (what the lldb +driver itself uses), not 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 TestFrameProviderRegisterCommandFrameAliasDeadlock(TestBase): + NO_DEBUG_INFO_TESTCASE = True + + def test_register_command_then_bt_no_deadlock(self): + """ + Register a scripted frame provider that identity-forwards every + frame, 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.IdentityProvider" + ) + # 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 frame-aliasing self-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) + self.assertIn("frame3", output) + self.assertIn("frame2", output) + self.assertIn("frame1", output) diff --git a/lldb/test/API/functionalit... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/208992 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
