https://github.com/JDevlieghere updated https://github.com/llvm/llvm-project/pull/131404
>From 5ea2ad6ecb388818b009fa89c4aebe1e0e4213df Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere <jo...@devlieghere.com> Date: Fri, 14 Mar 2025 15:19:58 -0700 Subject: [PATCH 1/2] [lldb] Expose the Target API lock through the SB API --- lldb/include/lldb/API/SBDefines.h | 1 + lldb/include/lldb/API/SBLock.h | 45 ++++++++++++++++++++++ lldb/include/lldb/API/SBTarget.h | 6 ++- lldb/include/lldb/Target/Target.h | 16 +++++++- lldb/source/API/CMakeLists.txt | 1 + lldb/source/API/SBLock.cpp | 40 +++++++++++++++++++ lldb/source/API/SBTarget.cpp | 36 +++++++++-------- lldb/unittests/API/CMakeLists.txt | 1 + lldb/unittests/API/SBLockTest.cpp | 64 +++++++++++++++++++++++++++++++ 9 files changed, 193 insertions(+), 17 deletions(-) create mode 100644 lldb/include/lldb/API/SBLock.h create mode 100644 lldb/source/API/SBLock.cpp create mode 100644 lldb/unittests/API/SBLockTest.cpp diff --git a/lldb/include/lldb/API/SBDefines.h b/lldb/include/lldb/API/SBDefines.h index ed5a80da117a5..7fa5150d69d8a 100644 --- a/lldb/include/lldb/API/SBDefines.h +++ b/lldb/include/lldb/API/SBDefines.h @@ -84,6 +84,7 @@ class LLDB_API SBLanguageRuntime; class LLDB_API SBLaunchInfo; class LLDB_API SBLineEntry; class LLDB_API SBListener; +class LLDB_API SBLock; class LLDB_API SBMemoryRegionInfo; class LLDB_API SBMemoryRegionInfoList; class LLDB_API SBModule; diff --git a/lldb/include/lldb/API/SBLock.h b/lldb/include/lldb/API/SBLock.h new file mode 100644 index 0000000000000..8d6fdfb959dfb --- /dev/null +++ b/lldb/include/lldb/API/SBLock.h @@ -0,0 +1,45 @@ +//===-- SBLock.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_SBLOCK_H +#define LLDB_API_SBLOCK_H + +#include "lldb/API/SBDefines.h" +#include "lldb/lldb-forward.h" + +#include <mutex> + +namespace lldb_private { +struct APILock; +} + +namespace lldb { + +#ifndef SWIG +class LLDB_API SBLock { +public: + ~SBLock(); + + bool IsValid() const; + +private: + friend class SBTarget; + + SBLock() = default; + SBLock(std::recursive_mutex &mutex); + SBLock(std::recursive_mutex &mutex, lldb::TargetSP target_sp); + SBLock(const SBLock &rhs) = delete; + const SBLock &operator=(const SBLock &rhs) = delete; + + std::unique_ptr<lldb_private::APILock> m_opaque_up; +}; +#endif + +} // namespace lldb + +#endif diff --git a/lldb/include/lldb/API/SBTarget.h b/lldb/include/lldb/API/SBTarget.h index bb912ab41d0fe..6120c289743e3 100644 --- a/lldb/include/lldb/API/SBTarget.h +++ b/lldb/include/lldb/API/SBTarget.h @@ -342,7 +342,7 @@ class LLDB_API SBTarget { uint32_t GetAddressByteSize(); const char *GetTriple(); - + const char *GetABIName(); const char *GetLabel() const; @@ -946,6 +946,10 @@ class LLDB_API SBTarget { /// An error if a Trace already exists or the trace couldn't be created. lldb::SBTrace CreateTrace(SBError &error); +#ifndef SWIG + lldb::SBLock GetAPILock() const; +#endif + protected: friend class SBAddress; friend class SBAddressRange; diff --git a/lldb/include/lldb/Target/Target.h b/lldb/include/lldb/Target/Target.h index 80ce5f013344c..8321a963d4c5b 100644 --- a/lldb/include/lldb/Target/Target.h +++ b/lldb/include/lldb/Target/Target.h @@ -1327,7 +1327,7 @@ class Target : public std::enable_shared_from_this<Target>, StopHook(const StopHook &rhs); virtual ~StopHook() = default; - enum class StopHookKind : uint32_t { CommandBased = 0, ScriptBased }; + enum class StopHookKind : uint32_t { CommandBased = 0, ScriptBased }; enum class StopHookResult : uint32_t { KeepStopped = 0, RequestContinue, @@ -1692,6 +1692,20 @@ class Target : public std::enable_shared_from_this<Target>, } }; +/// The private implementation backing SBLock. +struct APILock { + APILock(std::recursive_mutex &mutex) : lock(mutex) {} + std::lock_guard<std::recursive_mutex> lock; +}; + +/// The private implementation used by SBLock to hand out the target API mutex. +/// It has a TargetSP to ensure the lock cannot outlive the target. +struct TargetAPILock : public APILock { + TargetAPILock(std::recursive_mutex &mutex, lldb::TargetSP target_sp) + : APILock(mutex), target_sp(target_sp) {} + lldb::TargetSP target_sp; +}; + } // namespace lldb_private #endif // LLDB_TARGET_TARGET_H diff --git a/lldb/source/API/CMakeLists.txt b/lldb/source/API/CMakeLists.txt index 48d5cde5bf592..c9a9433b2329d 100644 --- a/lldb/source/API/CMakeLists.txt +++ b/lldb/source/API/CMakeLists.txt @@ -77,6 +77,7 @@ add_lldb_library(liblldb SHARED ${option_framework} SBLaunchInfo.cpp SBLineEntry.cpp SBListener.cpp + SBLock.cpp SBMemoryRegionInfo.cpp SBMemoryRegionInfoList.cpp SBModule.cpp diff --git a/lldb/source/API/SBLock.cpp b/lldb/source/API/SBLock.cpp new file mode 100644 index 0000000000000..448c8ccd46747 --- /dev/null +++ b/lldb/source/API/SBLock.cpp @@ -0,0 +1,40 @@ +//===-- SBLock.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/SBLock.h" +#include "lldb/Target/Target.h" +#include "lldb/Utility/Instrumentation.h" +#include "lldb/lldb-forward.h" + +#include <memory> +#include <mutex> + +using namespace lldb; +using namespace lldb_private; + +#ifndef SWIG + +SBLock::SBLock(std::recursive_mutex &mutex) + : m_opaque_up(std::make_unique<APILock>(mutex)) { + LLDB_INSTRUMENT_VA(this); +} + +SBLock::SBLock(std::recursive_mutex &mutex, lldb::TargetSP target_sp) + : m_opaque_up(std::make_unique<TargetAPILock>(mutex, target_sp)) { + LLDB_INSTRUMENT_VA(this); +} + +SBLock::~SBLock() { LLDB_INSTRUMENT_VA(this); } + +bool SBLock::IsValid() const { + LLDB_INSTRUMENT_VA(this); + + return static_cast<bool>(m_opaque_up); +} + +#endif diff --git a/lldb/source/API/SBTarget.cpp b/lldb/source/API/SBTarget.cpp index dd9caa724ea36..a2761de8bfc5e 100644 --- a/lldb/source/API/SBTarget.cpp +++ b/lldb/source/API/SBTarget.cpp @@ -7,6 +7,7 @@ //===----------------------------------------------------------------------===// #include "lldb/API/SBTarget.h" +#include "lldb/API/SBLock.h" #include "lldb/Utility/Instrumentation.h" #include "lldb/Utility/LLDBLog.h" #include "lldb/lldb-public.h" @@ -18,6 +19,7 @@ #include "lldb/API/SBExpressionOptions.h" #include "lldb/API/SBFileSpec.h" #include "lldb/API/SBListener.h" +#include "lldb/API/SBLock.h" #include "lldb/API/SBModule.h" #include "lldb/API/SBModuleSpec.h" #include "lldb/API/SBProcess.h" @@ -544,9 +546,8 @@ lldb::SBProcess SBTarget::ConnectRemote(SBListener &listener, const char *url, if (target_sp) { std::lock_guard<std::recursive_mutex> guard(target_sp->GetAPIMutex()); if (listener.IsValid()) - process_sp = - target_sp->CreateProcess(listener.m_opaque_sp, plugin_name, nullptr, - true); + process_sp = target_sp->CreateProcess(listener.m_opaque_sp, plugin_name, + nullptr, true); else process_sp = target_sp->CreateProcess( target_sp->GetDebugger().GetListener(), plugin_name, nullptr, true); @@ -1040,7 +1041,7 @@ SBTarget::BreakpointCreateForException(lldb::LanguageType language, std::lock_guard<std::recursive_mutex> guard(target_sp->GetAPIMutex()); const bool hardware = false; sb_bp = target_sp->CreateExceptionBreakpoint(language, catch_bp, throw_bp, - hardware); + hardware); } return sb_bp; @@ -1060,14 +1061,9 @@ lldb::SBBreakpoint SBTarget::BreakpointCreateFromScript( Status error; StructuredData::ObjectSP obj_sp = extra_args.m_impl_up->GetObjectSP(); - sb_bp = - target_sp->CreateScriptedBreakpoint(class_name, - module_list.get(), - file_list.get(), - false, /* internal */ - request_hardware, - obj_sp, - &error); + sb_bp = target_sp->CreateScriptedBreakpoint( + class_name, module_list.get(), file_list.get(), false, /* internal */ + request_hardware, obj_sp, &error); } return sb_bp; @@ -1692,8 +1688,8 @@ uint32_t SBTarget::GetMaximumNumberOfChildrenToDisplay() const { LLDB_INSTRUMENT_VA(this); TargetSP target_sp(GetSP()); - if(target_sp){ - return target_sp->GetMaximumNumberOfChildrenToDisplay(); + if (target_sp) { + return target_sp->GetMaximumNumberOfChildrenToDisplay(); } return 0; } @@ -2193,7 +2189,7 @@ SBError SBTarget::SetModuleLoadAddress(lldb::SBModule module, } SBError SBTarget::SetModuleLoadAddress(lldb::SBModule module, - uint64_t slide_offset) { + uint64_t slide_offset) { SBError sb_error; @@ -2439,3 +2435,13 @@ lldb::SBTrace SBTarget::CreateTrace(lldb::SBError &error) { } return SBTrace(); } + +#ifndef SWIG +lldb::SBLock SBTarget::GetAPILock() const { + LLDB_INSTRUMENT_VA(this); + + if (TargetSP target_sp = GetSP()) + return lldb::SBLock(target_sp->GetAPIMutex(), target_sp); + return lldb::SBLock(); +} +#endif diff --git a/lldb/unittests/API/CMakeLists.txt b/lldb/unittests/API/CMakeLists.txt index fe2ff684a5d92..5a88d06b601a3 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 + SBLockTest.cpp LINK_LIBS liblldb diff --git a/lldb/unittests/API/SBLockTest.cpp b/lldb/unittests/API/SBLockTest.cpp new file mode 100644 index 0000000000000..1fa9c184bf79b --- /dev/null +++ b/lldb/unittests/API/SBLockTest.cpp @@ -0,0 +1,64 @@ +//===-- SBLockTest.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/SBLock.h" +#include "lldb/API/SBDebugger.h" +#include "lldb/API/SBTarget.h" +#include "gtest/gtest.h" +#include <atomic> +#include <chrono> +#include <thread> + +TEST(SBLockTest, LockTest) { + lldb::SBDebugger debugger = lldb::SBDebugger::Create(); + lldb::SBTarget target = debugger.GetDummyTarget(); + + std::mutex m; + std::condition_variable cv; + bool wakeup = false; + std::atomic<bool> locked = false; + + std::thread test_thread([&]() { + { + { + std::unique_lock lk(m); + cv.wait(lk, [&] { return wakeup; }); + } + + ASSERT_TRUE(locked); + target.BreakpointCreateByName("foo", "bar"); + ASSERT_FALSE(locked); + + cv.notify_one(); + } + }); + + // Take the API lock. + { + lldb::SBLock lock = target.GetAPILock(); + ASSERT_FALSE(locked.exchange(true)); + + // Wake up the test thread. + { + std::lock_guard lk(m); + wakeup = true; + } + cv.notify_one(); + + // Wait 500ms to confirm the thread is blocked. + { + std::unique_lock<std::mutex> lk(m); + auto result = cv.wait_for(lk, std::chrono::milliseconds(500)); + ASSERT_EQ(result, std::cv_status::timeout); + } + + ASSERT_TRUE(locked.exchange(false)); + } + + test_thread.join(); +} >From acb6a6ed6051d5d8297645f12a136ad7b2754b8e Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere <jo...@devlieghere.com> Date: Fri, 14 Mar 2025 16:20:35 -0700 Subject: [PATCH 2/2] Address code review feedback --- lldb/include/lldb/API/SBLock.h | 8 +++++--- lldb/include/lldb/API/SBTarget.h | 2 +- lldb/include/lldb/Target/Target.h | 2 ++ lldb/source/API/SBLock.cpp | 5 ----- lldb/source/API/SBTarget.cpp | 10 ++++------ lldb/unittests/API/SBLockTest.cpp | 2 +- 6 files changed, 13 insertions(+), 16 deletions(-) diff --git a/lldb/include/lldb/API/SBLock.h b/lldb/include/lldb/API/SBLock.h index 8d6fdfb959dfb..d13fec989990b 100644 --- a/lldb/include/lldb/API/SBLock.h +++ b/lldb/include/lldb/API/SBLock.h @@ -28,11 +28,13 @@ class LLDB_API SBLock { bool IsValid() const; private: - friend class SBTarget; - SBLock() = default; - SBLock(std::recursive_mutex &mutex); + + // Private constructor used by SBTarget to create the Target API lock. + // Requires a friend declaration. SBLock(std::recursive_mutex &mutex, lldb::TargetSP target_sp); + friend class SBTarget; + SBLock(const SBLock &rhs) = delete; const SBLock &operator=(const SBLock &rhs) = delete; diff --git a/lldb/include/lldb/API/SBTarget.h b/lldb/include/lldb/API/SBTarget.h index 6120c289743e3..03d6b623fd425 100644 --- a/lldb/include/lldb/API/SBTarget.h +++ b/lldb/include/lldb/API/SBTarget.h @@ -947,7 +947,7 @@ class LLDB_API SBTarget { lldb::SBTrace CreateTrace(SBError &error); #ifndef SWIG - lldb::SBLock GetAPILock() const; + lldb::SBLock AcquireAPILock() const; #endif protected: diff --git a/lldb/include/lldb/Target/Target.h b/lldb/include/lldb/Target/Target.h index 8321a963d4c5b..4379f73936d0c 100644 --- a/lldb/include/lldb/Target/Target.h +++ b/lldb/include/lldb/Target/Target.h @@ -1695,6 +1695,7 @@ class Target : public std::enable_shared_from_this<Target>, /// The private implementation backing SBLock. struct APILock { APILock(std::recursive_mutex &mutex) : lock(mutex) {} + virtual ~APILock() = default; std::lock_guard<std::recursive_mutex> lock; }; @@ -1703,6 +1704,7 @@ struct APILock { struct TargetAPILock : public APILock { TargetAPILock(std::recursive_mutex &mutex, lldb::TargetSP target_sp) : APILock(mutex), target_sp(target_sp) {} + ~TargetAPILock() override = default; lldb::TargetSP target_sp; }; diff --git a/lldb/source/API/SBLock.cpp b/lldb/source/API/SBLock.cpp index 448c8ccd46747..4338049cb1689 100644 --- a/lldb/source/API/SBLock.cpp +++ b/lldb/source/API/SBLock.cpp @@ -19,11 +19,6 @@ using namespace lldb_private; #ifndef SWIG -SBLock::SBLock(std::recursive_mutex &mutex) - : m_opaque_up(std::make_unique<APILock>(mutex)) { - LLDB_INSTRUMENT_VA(this); -} - SBLock::SBLock(std::recursive_mutex &mutex, lldb::TargetSP target_sp) : m_opaque_up(std::make_unique<TargetAPILock>(mutex, target_sp)) { LLDB_INSTRUMENT_VA(this); diff --git a/lldb/source/API/SBTarget.cpp b/lldb/source/API/SBTarget.cpp index a2761de8bfc5e..96d5fd1cbf31a 100644 --- a/lldb/source/API/SBTarget.cpp +++ b/lldb/source/API/SBTarget.cpp @@ -7,11 +7,6 @@ //===----------------------------------------------------------------------===// #include "lldb/API/SBTarget.h" -#include "lldb/API/SBLock.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" @@ -60,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" @@ -2437,7 +2435,7 @@ lldb::SBTrace SBTarget::CreateTrace(lldb::SBError &error) { } #ifndef SWIG -lldb::SBLock SBTarget::GetAPILock() const { +lldb::SBLock SBTarget::AcquireAPILock() const { LLDB_INSTRUMENT_VA(this); if (TargetSP target_sp = GetSP()) diff --git a/lldb/unittests/API/SBLockTest.cpp b/lldb/unittests/API/SBLockTest.cpp index 1fa9c184bf79b..33bf94ead1dd3 100644 --- a/lldb/unittests/API/SBLockTest.cpp +++ b/lldb/unittests/API/SBLockTest.cpp @@ -40,7 +40,7 @@ TEST(SBLockTest, LockTest) { // Take the API lock. { - lldb::SBLock lock = target.GetAPILock(); + lldb::SBLock lock = target.AcquireAPILock(); ASSERT_FALSE(locked.exchange(true)); // Wake up the test thread. _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
