[lldb] Add try_lock to SBMutex #164109
[lldb] Add try_lock to SBMutex #164109
Conversation
|
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesAdd try_lock to confirm to Lockable, which is necessary to use it with std::scoped_lock. Full diff: https://github.com/llvm/llvm-project/pull/164109.diff 3 Files Affected:
diff --git a/lldb/include/lldb/API/SBMutex.h b/lldb/include/lldb/API/SBMutex.h
index 717d5f86cbc1c..826ad077f159f 100644
--- a/lldb/include/lldb/API/SBMutex.h
+++ b/lldb/include/lldb/API/SBMutex.h
@@ -31,6 +31,10 @@ class LLDB_API SBMutex {
/// Releases ownership of this lock.
void unlock() const;
+ /// Tries to lock the mutex. Returns immediately. On successful lock
+ /// acquisition returns true, otherwise returns false.
+ bool try_lock() const;
+
private:
// Private constructor used by SBTarget to create the Target API mutex.
// Requires a friend declaration.
diff --git a/lldb/source/API/SBMutex.cpp b/lldb/source/API/SBMutex.cpp
index 445076b5a9174..ded8038b92760 100644
--- a/lldb/source/API/SBMutex.cpp
+++ b/lldb/source/API/SBMutex.cpp
@@ -58,3 +58,12 @@ void SBMutex::unlock() const {
if (m_opaque_sp)
m_opaque_sp->unlock();
}
+
+bool SBMutex::try_lock() const {
+ LLDB_INSTRUMENT_VA(this);
+
+ if (m_opaque_sp)
+ m_opaque_sp->try_lock();
+
+ return false;
+}
diff --git a/lldb/unittests/API/SBMutexTest.cpp b/lldb/unittests/API/SBMutexTest.cpp
index aafad59d58c17..dd3258405544a 100644
--- a/lldb/unittests/API/SBMutexTest.cpp
+++ b/lldb/unittests/API/SBMutexTest.cpp
@@ -36,6 +36,10 @@ TEST_F(SBMutexTest, LockTest) {
std::future<void> f;
{
lldb::SBMutex lock = target.GetAPIMutex();
+
+ ASSERT_TRUE(lock.try_lock());
+ lock.unlock();
+
std::lock_guard<lldb::SBMutex> lock_guard(lock);
ASSERT_FALSE(locked.exchange(true));
|
Add try_lock to confirm to Lockable, which is necessary to use it with std::scoped_lock.
JDevlieghere
force-pushed
the
try_lock
branch
from
7a06676 to
32cbb92
Compare
| { | ||
| lldb::SBMutex lock = target.GetAPIMutex(); | ||
|
|
||
| ASSERT_TRUE(lock.try_lock()); |
There was a problem hiding this comment.
try_lock another time to confirm that it fails?
| bool SBMutex::try_lock() const { | ||
| LLDB_INSTRUMENT_VA(this); | ||
|
|
||
| if (m_opaque_sp) |
There was a problem hiding this comment.
I know this is what we do in all the other methods too, but under what circumstances can this be nullptr? Isn't its lifetime at least that of the owning SBMutex?
There was a problem hiding this comment.
Yes, it's a wrapper around a std::shared_ptr<std::recursive_mutex>. It could be null when you use the constructor that takes an SBTarget that is null.
There was a problem hiding this comment.
Correct me if I'm wrong, but I don't think the aliasing constructor allows SBTarget to ever be null. After all, we're aliasing one of its members, so we always dereference it.
There was a problem hiding this comment.
Yes, you're right. If that's the case then I don't think the pointer can be null. Doesn't look like we do this in SBTarget either, but we could update the classes where that's the case to have an assert, but that would be a bigger change.
There was a problem hiding this comment.
Yea definitely out-of-scope of this PR. Adding an assert and removing the null-checks would be a nice-to-have
|
Add to Release note new |
I don't think individual SB APIs are wort release noting, but @DavidSpickett has been championing the release notes so let's see what he thinks. The other alternative is to write something like "SBMutex can now be used with a scoped lock", but this class was added primarily to support locking in |
JDevlieghere
force-pushed
the
try_lock
branch
from
b9e549e to
a48b0e9
Compare
If we were already doing a better job about tracking SBAPI changes then yes they should be release noted but we aren't. A list of SBIAPI changes per release would be cool but is definitely in the "nice to have" bucket right now. I would add a note if it was:
|
|
The last force push undid the |
I moved it because it's a recursive mutex, so try_lock will continue succeeding if you do it on the same thread. We now do it from the async call. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/162/builds/33585 Here is the relevant piece of the build log for the reference |
Add `try_lock` to confirm to Lockable, which is necessary to use it with `std::scoped_lock`.
Add `try_lock` to confirm to Lockable, which is necessary to use it with `std::scoped_lock`.
Add `try_lock` to confirm to Lockable, which is necessary to use it with `std::scoped_lock`. (cherry picked from commit c01a223)
[lldb] Add try_lock to SBMutex (llvm#164109)
6 participants
Add try_lock to confirm to Lockable, which is necessary to use it with std::scoped_lock.