From 50949ebf523cc09cc911a12691fb79b6ac97102a Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere Date: Mon, 31 Mar 2025 08:19:41 -0700 Subject: [PATCH] [lldb] Expose the Target API mutex through the SB API (#133295) Expose u target API mutex through the SB API. This is motivated by lldb-dap, which is built on top of the SB API and needs a way to execute a series of SB API calls in an atomic manner (see #131242). We can solve this problem by either introducing an additional layer of locking at the DAP level or by exposing the existing locking at the SB API level. This patch implements the second approach. This was discussed in an RFC on Discourse [0]. The original implementation exposed a move-only lock rather than a mutex [1] which doesn't work well with SWIG 4.0 [2]. This implement the alternative solution of exposing the mutex rather than the lock. The SBMutex conforms to the BasicLockable requirement [3] (which is why the methods are called `lock` and `unlock` rather than Lock and Unlock) so it can be used as `std::lock_guard` and `std::unique_lock`. [0]: https://discourse.llvm.org/t/rfc-exposing-the-target-api-lock-through-the-sb-api/85215/6 [1]: https://github.com/llvm/llvm-project/pull/131404 [2]: https://discourse.llvm.org/t/rfc-bumping-the-minimum-swig-version-to-4-1-0/85377/9 [3]: https://en.cppreference.com/w/cpp/named_req/BasicLockable --- lldb/bindings/interface/SBMutexExtensions.i | 12 ++++ lldb/bindings/interfaces.swig | 4 +- lldb/include/lldb/API/LLDB.h | 1 + lldb/include/lldb/API/SBDefines.h | 1 + lldb/include/lldb/API/SBMutex.h | 45 ++++++++++++++ lldb/include/lldb/API/SBTarget.h | 4 +- lldb/source/API/CMakeLists.txt | 1 + lldb/source/API/SBMutex.cpp | 60 +++++++++++++++++++ lldb/source/API/SBTarget.cpp | 16 +++-- .../API/python_api/target/TestTargetAPI.py | 24 ++++++++ lldb/unittests/API/CMakeLists.txt | 1 + lldb/unittests/API/SBMutexTest.cpp | 57 ++++++++++++++++++ 12 files changed, 220 insertions(+), 6 deletions(-) create mode 100644 lldb/bindings/interface/SBMutexExtensions.i create mode 100644 lldb/include/lldb/API/SBMutex.h create mode 100644 lldb/source/API/SBMutex.cpp create mode 100644 lldb/unittests/API/SBMutexTest.cpp diff --git a/lldb/bindings/interface/SBMutexExtensions.i b/lldb/bindings/interface/SBMutexExtensions.i new file mode 100644 index 000000000000..32d3fee46869 --- /dev/null +++ b/lldb/bindings/interface/SBMutexExtensions.i @@ -0,0 +1,12 @@ +%extend lldb::SBMutex { +#ifdef SWIGPYTHON + %pythoncode %{ + def __enter__(self): + self.lock() + return self + + def __exit__(self, exc_type, exc_value, traceback): + self.unlock() + %} +#endif +} diff --git a/lldb/bindings/interfaces.swig b/lldb/bindings/interfaces.swig index 6da56e4e0fa5..e71ed136f20e 100644 --- a/lldb/bindings/interfaces.swig +++ b/lldb/bindings/interfaces.swig @@ -51,6 +51,7 @@ %include "./interface/SBMemoryRegionInfoListDocstrings.i" %include "./interface/SBModuleDocstrings.i" %include "./interface/SBModuleSpecDocstrings.i" +%include "./interface/SBMutexExtensions.i" %include "./interface/SBPlatformDocstrings.i" %include "./interface/SBProcessDocstrings.i" %include "./interface/SBProcessInfoDocstrings.i" @@ -121,8 +122,8 @@ %include "lldb/API/SBHostOS.h" %include "lldb/API/SBInstruction.h" %include "lldb/API/SBInstructionList.h" -%include "lldb/API/SBLanguages.h" %include "lldb/API/SBLanguageRuntime.h" +%include "lldb/API/SBLanguages.h" %include "lldb/API/SBLaunchInfo.h" %include "lldb/API/SBLineEntry.h" %include "lldb/API/SBListener.h" @@ -130,6 +131,7 @@ %include "lldb/API/SBMemoryRegionInfoList.h" %include "lldb/API/SBModule.h" %include "lldb/API/SBModuleSpec.h" +%include "lldb/API/SBMutex.h" %include "lldb/API/SBPlatform.h" %include "lldb/API/SBProcess.h" %include "lldb/API/SBProcessInfo.h" diff --git a/lldb/include/lldb/API/LLDB.h b/lldb/include/lldb/API/LLDB.h index 126fcef31b41..6485f35302a1 100644 --- a/lldb/include/lldb/API/LLDB.h +++ b/lldb/include/lldb/API/LLDB.h @@ -50,6 +50,7 @@ #include "lldb/API/SBMemoryRegionInfoList.h" #include "lldb/API/SBModule.h" #include "lldb/API/SBModuleSpec.h" +#include "lldb/API/SBMutex.h" #include "lldb/API/SBPlatform.h" #include "lldb/API/SBProcess.h" #include "lldb/API/SBProcessInfo.h" diff --git a/lldb/include/lldb/API/SBDefines.h b/lldb/include/lldb/API/SBDefines.h index ed5a80da117a..85f6bbeea5bf 100644 --- a/lldb/include/lldb/API/SBDefines.h +++ b/lldb/include/lldb/API/SBDefines.h @@ -89,6 +89,7 @@ class LLDB_API SBMemoryRegionInfoList; class LLDB_API SBModule; class LLDB_API SBModuleSpec; class LLDB_API SBModuleSpecList; +class LLDB_API SBMutex; class LLDB_API SBPlatform; class LLDB_API SBPlatformConnectOptions; class LLDB_API SBPlatformShellCommand; diff --git a/lldb/include/lldb/API/SBMutex.h b/lldb/include/lldb/API/SBMutex.h new file mode 100644 index 000000000000..717d5f86cbc1 --- /dev/null +++ b/lldb/include/lldb/API/SBMutex.h @@ -0,0 +1,45 @@ +//===-- SBMutex.h ---------------------------------------------------------===// +// +// 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 +// +//===----------------------------------------------------------------------===// + +#ifndef LLDB_API_SBMUTEX_H +#define LLDB_API_SBMUTEX_H + +#include "lldb/API/SBDefines.h" +#include "lldb/lldb-forward.h" +#include + +namespace lldb { + +class LLDB_API SBMutex { +public: + SBMutex(); + SBMutex(const SBMutex &rhs); + const SBMutex &operator=(const SBMutex &rhs); + ~SBMutex(); + + /// Returns true if this lock has ownership of the underlying mutex. + bool IsValid() const; + + /// Blocking operation that takes ownership of this lock. + void lock() const; + + /// Releases ownership of this lock. + void unlock() const; + +private: + // Private constructor used by SBTarget to create the Target API mutex. + // Requires a friend declaration. + SBMutex(lldb::TargetSP target_sp); + friend class SBTarget; + + std::shared_ptr m_opaque_sp; +}; + +} // namespace lldb + +#endif diff --git a/lldb/include/lldb/API/SBTarget.h b/lldb/include/lldb/API/SBTarget.h index bb912ab41d0f..17735fdca655 100644 --- a/lldb/include/lldb/API/SBTarget.h +++ b/lldb/include/lldb/API/SBTarget.h @@ -342,7 +342,7 @@ public: uint32_t GetAddressByteSize(); const char *GetTriple(); - + const char *GetABIName(); const char *GetLabel() const; @@ -946,6 +946,8 @@ public: /// An error if a Trace already exists or the trace couldn't be created. lldb::SBTrace CreateTrace(SBError &error); + lldb::SBMutex GetAPIMutex() const; + protected: friend class SBAddress; friend class SBAddressRange; diff --git a/lldb/source/API/CMakeLists.txt b/lldb/source/API/CMakeLists.txt index 48d5cde5bf59..3bc569608e45 100644 --- a/lldb/source/API/CMakeLists.txt +++ b/lldb/source/API/CMakeLists.txt @@ -81,6 +81,7 @@ add_lldb_library(liblldb SHARED ${option_framework} SBMemoryRegionInfoList.cpp SBModule.cpp SBModuleSpec.cpp + SBMutex.cpp SBPlatform.cpp SBProcess.cpp SBProcessInfo.cpp diff --git a/lldb/source/API/SBMutex.cpp b/lldb/source/API/SBMutex.cpp new file mode 100644 index 000000000000..445076b5a917 --- /dev/null +++ b/lldb/source/API/SBMutex.cpp @@ -0,0 +1,60 @@ +//===-- SBMutex.cpp -------------------------------------------------------===// +// +// 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/API/SBMutex.h" +#include "lldb/Target/Target.h" +#include "lldb/Utility/Instrumentation.h" +#include "lldb/lldb-forward.h" +#include +#include + +using namespace lldb; +using namespace lldb_private; + +SBMutex::SBMutex() : m_opaque_sp(std::make_shared()) { + LLDB_INSTRUMENT_VA(this); +} + +SBMutex::SBMutex(const SBMutex &rhs) : m_opaque_sp(rhs.m_opaque_sp) { + LLDB_INSTRUMENT_VA(this); +} + +const SBMutex &SBMutex::operator=(const SBMutex &rhs) { + LLDB_INSTRUMENT_VA(this); + + m_opaque_sp = rhs.m_opaque_sp; + return *this; +} + +SBMutex::SBMutex(lldb::TargetSP target_sp) + : m_opaque_sp(std::shared_ptr( + target_sp, &target_sp->GetAPIMutex())) { + LLDB_INSTRUMENT_VA(this, target_sp); +} + +SBMutex::~SBMutex() { LLDB_INSTRUMENT_VA(this); } + +bool SBMutex::IsValid() const { + LLDB_INSTRUMENT_VA(this); + + return static_cast(m_opaque_sp); +} + +void SBMutex::lock() const { + LLDB_INSTRUMENT_VA(this); + + if (m_opaque_sp) + m_opaque_sp->lock(); +} + +void SBMutex::unlock() const { + LLDB_INSTRUMENT_VA(this); + + if (m_opaque_sp) + m_opaque_sp->unlock(); +} diff --git a/lldb/source/API/SBTarget.cpp b/lldb/source/API/SBTarget.cpp index dd9caa724ea3..0fed1bbfed6a 100644 --- a/lldb/source/API/SBTarget.cpp +++ b/lldb/source/API/SBTarget.cpp @@ -7,10 +7,6 @@ //===----------------------------------------------------------------------===// #include "lldb/API/SBTarget.h" -#include "lldb/Utility/Instrumentation.h" -#include "lldb/Utility/LLDBLog.h" -#include "lldb/lldb-public.h" - #include "lldb/API/SBBreakpoint.h" #include "lldb/API/SBDebugger.h" #include "lldb/API/SBEnvironment.h" @@ -20,6 +16,7 @@ #include "lldb/API/SBListener.h" #include "lldb/API/SBModule.h" #include "lldb/API/SBModuleSpec.h" +#include "lldb/API/SBMutex.h" #include "lldb/API/SBProcess.h" #include "lldb/API/SBSourceManager.h" #include "lldb/API/SBStream.h" @@ -58,11 +55,14 @@ #include "lldb/Utility/ArchSpec.h" #include "lldb/Utility/Args.h" #include "lldb/Utility/FileSpec.h" +#include "lldb/Utility/Instrumentation.h" +#include "lldb/Utility/LLDBLog.h" #include "lldb/Utility/ProcessInfo.h" #include "lldb/Utility/RegularExpression.h" #include "lldb/ValueObject/ValueObjectConstResult.h" #include "lldb/ValueObject/ValueObjectList.h" #include "lldb/ValueObject/ValueObjectVariable.h" +#include "lldb/lldb-public.h" #include "Commands/CommandObjectBreakpoint.h" #include "lldb/Interpreter/CommandReturnObject.h" @@ -2439,3 +2439,11 @@ lldb::SBTrace SBTarget::CreateTrace(lldb::SBError &error) { } return SBTrace(); } + +lldb::SBMutex SBTarget::GetAPIMutex() const { + LLDB_INSTRUMENT_VA(this); + + if (TargetSP target_sp = GetSP()) + return lldb::SBMutex(target_sp); + return lldb::SBMutex(); +} diff --git a/lldb/test/API/python_api/target/TestTargetAPI.py b/lldb/test/API/python_api/target/TestTargetAPI.py index 155a25b576b0..67b9d192bc62 100644 --- a/lldb/test/API/python_api/target/TestTargetAPI.py +++ b/lldb/test/API/python_api/target/TestTargetAPI.py @@ -537,3 +537,27 @@ class TargetAPITestCase(TestBase): """Make sure we don't crash when trying to select invalid target.""" target = lldb.SBTarget() self.dbg.SetSelectedTarget(target) + + @no_debug_info_test + def test_get_api_mutex(self): + """Make sure we can lock and unlock the API mutex from Python.""" + target = self.dbg.GetDummyTarget() + + mutex = target.GetAPIMutex() + self.assertTrue(mutex.IsValid()) + mutex.lock() + # The API call below doesn't actually matter, it's just there to + # confirm we don't block on the API lock. + target.BreakpointCreateByName("foo", "bar") + mutex.unlock() + + @no_debug_info_test + def test_get_api_mutex_with_statement(self): + """Make sure we can lock and unlock the API mutex using a with-statement from Python.""" + target = self.dbg.GetDummyTarget() + + with target.GetAPIMutex() as mutex: + self.assertTrue(mutex.IsValid()) + # The API call below doesn't actually matter, it's just there to + # confirm we don't block on the API lock. + target.BreakpointCreateByName("foo", "bar") diff --git a/lldb/unittests/API/CMakeLists.txt b/lldb/unittests/API/CMakeLists.txt index fe2ff684a5d9..8bdc80687823 100644 --- a/lldb/unittests/API/CMakeLists.txt +++ b/lldb/unittests/API/CMakeLists.txt @@ -1,6 +1,7 @@ add_lldb_unittest(APITests SBCommandInterpreterTest.cpp SBLineEntryTest.cpp + SBMutexTest.cpp LINK_LIBS liblldb diff --git a/lldb/unittests/API/SBMutexTest.cpp b/lldb/unittests/API/SBMutexTest.cpp new file mode 100644 index 000000000000..0b888c2725aa --- /dev/null +++ b/lldb/unittests/API/SBMutexTest.cpp @@ -0,0 +1,57 @@ +//===-- SBMutexTest.cpp ---------------------------------------------------===// +// +// 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 +// +//===----------------------------------------------------------------------===// + +// Use the umbrella header for -Wdocumentation. +#include "lldb/API/LLDB.h" + +#include "TestingSupport/SubsystemRAII.h" +#include "lldb/API/SBDebugger.h" +#include "lldb/API/SBTarget.h" +#include "gtest/gtest.h" +#include +#include +#include +#include + +using namespace lldb; +using namespace lldb_private; + +class SBMutexTest : public testing::Test { +protected: + void SetUp() override { debugger = SBDebugger::Create(); } + void TearDown() override { SBDebugger::Destroy(debugger); } + + SubsystemRAII subsystems; + SBDebugger debugger; +}; + +TEST_F(SBMutexTest, LockTest) { + lldb::SBTarget target = debugger.GetDummyTarget(); + + std::future f; + { + std::atomic locked = false; + lldb::SBMutex lock = target.GetAPIMutex(); + std::lock_guard lock_guard(lock); + ASSERT_FALSE(locked.exchange(true)); + + f = std::async(std::launch::async, [&]() { + ASSERT_TRUE(locked); + target.BreakpointCreateByName("foo", "bar"); + ASSERT_FALSE(locked); + }); + ASSERT_TRUE(f.valid()); + + // Wait 500ms to confirm the thread is blocked. + auto status = f.wait_for(std::chrono::milliseconds(500)); + ASSERT_EQ(status, std::future_status::timeout); + + ASSERT_TRUE(locked.exchange(false)); + } + f.wait(); +}