[lldb] Expose the Target API lock through the SB API #131404
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesExpose the target API lock through the SB API. This is motivated by Full diff: https://github.com/llvm/llvm-project/pull/131404.diff 9 Files Affected:
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();
+}
|
labath
reviewed
Mar 17, 2025
|
The unit tests are crashing because the new test correctly initializes and terminates the debugger and there's a double free in the Edit: #131658 |
|
Rebase on top of #131658: The unit test should pass now. |
|
The one remaining question seems to be: Should we also support (and test) this API from Python? Also see comments in the RFC |
Yup, this patch now makes SBlock available from Python and Lua. I didn't add the test yet because I wanted to add the |
|
Rebased + implement the Python extension. |
labath
approved these changes
Mar 20, 2025
Comment on lines
+1701
to
+1704
| void Unlock() { m_lock.unlock(); } | ||
|
|
||
| operator bool() const { return static_cast<bool>(m_lock); } | ||
|
|
There was a problem hiding this comment.
What would you say to removing these, and having SBLock::Unlock do its work by clearing the unique_ptr ?
Comment on lines
4
to
8
| def __enter__(self): | ||
| return self | ||
|
|
||
| def __exit__(self, exc_type, exc_value, traceback): | ||
| self.Unlock() |
There was a problem hiding this comment.
It's a bit unfortunate, that
lock = target.AcquireLock();
with lock:
# we are correctly locked here
with lock:
# we are no longer locked here :/
does not work as expected.
In Python, the convention seems to be that locks can be locked or unlocked, and entering a with scope acquires the lock. We might want to model this after the Python's Lock or RLock objects, i.e. closer to a std::unique_lock than a std::lock_guard
WDYT?
There was a problem hiding this comment.
Can you think of a way to make this work in Python, without giving up on the RAII property on the C++ side? I my opinion the SBLock is a lot more important in C++ than it is in Python (where the GIL means you don't truly have multithreading anyway, though I know in the future there might be a mode without the GIL).
There was a problem hiding this comment.
What I can make work is this:
lock = target.AcquireLock();
[...]
lock.Unlock()
with lock:
# we are correctly locked here
with lock:
# we are correctly locked here
There was a problem hiding this comment.
Sounds good. If we would have an additional GetLock (beides AcquireLock) which returns the non-locked Lock, then I think we would have a full idiomatoc interface, also in Python
|
The pre-merge bot is using SWIG 4.0.2 which seems to have trouble with the move-only type. I'm going to build that SWIG and see if I can reproduce this. |
|
I'm not sure I can work around this with the version of SWIG installed on the bots. I've created an RFC about bumping the SWIG version: https://discourse.llvm.org/t/rfc-bumping-the-minimum-swig-version-to-4-1-0/85377 |
JDevlieghere
added a commit
to JDevlieghere/llvm-project
that referenced
this pull request
Mar 27, 2025
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 llvm#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<lldb::SBMutex>` and `std::unique_lock<lldb::SBMutex>`. [0]: https://discourse.llvm.org/t/rfc-exposing-the-target-api-lock-through-the-sb-api/85215/6 [1]: llvm#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
|
Superseded by #133295 |
JDevlieghere
added a commit
that referenced
this pull request
Mar 31, 2025
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<lldb::SBMutex>` and `std::unique_lock<lldb::SBMutex>`. [0]: https://discourse.llvm.org/t/rfc-exposing-the-target-api-lock-through-the-sb-api/85215/6 [1]: #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
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Mar 31, 2025
…3295) 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<lldb::SBMutex>` and `std::unique_lock<lldb::SBMutex>`. [0]: https://discourse.llvm.org/t/rfc-exposing-the-target-api-lock-through-the-sb-api/85215/6 [1]: llvm/llvm-project#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
JDevlieghere
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Apr 8, 2025
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 llvm#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<lldb::SBMutex>` and `std::unique_lock<lldb::SBMutex>`. [0]: https://discourse.llvm.org/t/rfc-exposing-the-target-api-lock-through-the-sb-api/85215/6 [1]: llvm#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 (cherry picked from commit 50949eb)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Expose the target API lock 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.