Yeah, I had hoped for the same :(
The problem here (as usual with these annotations) is aliasing.
As a simplified example:
CCheckQueueControl<CScriptCheck> control(m_chainman.GetCheckQueue());
CCheckQueueControl<CScriptCheck> control2(m_chainman.GetCheckQueue());
Here, clang has no idea that &control.m_lock.m_control_mutex == &control2.m_lock.m_control_mutex.
The only alternative, really, would be something like:
diff --git a/src/checkqueue.h b/src/checkqueue.h
index ade793ab392..5249e040d24 100644
--- a/src/checkqueue.h
+++ b/src/checkqueue.h
@@ -188,6 +188,11 @@ public:
}
}
+ UniqueLock<Mutex> TryLock()
+ {
+ return {IN_PLACE_TRY_LOCK(m_control_mutex)};
+ }
+
~CCheckQueue()
{
WITH_LOCK(m_mutex, m_request_stop = true);
@@ -209,13 +214,14 @@ class CCheckQueueControl
{
private:
CCheckQueue<T, R>& pqueue;
+ UniqueLock<Mutex> m_lock;
bool fDone;
public:
CCheckQueueControl() = delete;
CCheckQueueControl(const CCheckQueueControl&) = delete;
CCheckQueueControl& operator=(const CCheckQueueControl&) = delete;
- explicit CCheckQueueControl(CCheckQueue<T>& pqueueIn) : pqueue(pqueueIn), fDone(false)
+ explicit CCheckQueueControl(CCheckQueue<T>& pqueueIn, UniqueLock<Mutex>&& lock) : pqueue(pqueueIn), m_lock(std::move(lock)), fDone(false)
{
ENTER_CRITICAL_SECTION(pqueue.m_control_mutex);
}
diff --git a/src/sync.h b/src/sync.h
index b22956ef1ab..db4fd977cee 100644
--- a/src/sync.h
+++ b/src/sync.h
@@ -193,6 +193,8 @@ public:
Enter(pszName, pszFile, nLine);
}
+ UniqueLock(UniqueLock&&) = default;
+
~UniqueLock() UNLOCK_FUNCTION()
{
if (Base::owns_lock())
@@ -260,6 +262,7 @@ inline MutexType* MaybeCheckNotHeld(MutexType* m) LOCKS_EXCLUDED(m) LOCK_RETURNE
UniqueLock criticalblock2(MaybeCheckNotHeld(cs2), #cs2, __FILE__, __LINE__)
#define TRY_LOCK(cs, name) UniqueLock name(MaybeCheckNotHeld(cs), #cs, __FILE__, __LINE__, true)
#define WAIT_LOCK(cs, name) UniqueLock name(MaybeCheckNotHeld(cs), #cs, __FILE__, __LINE__)
+#define IN_PLACE_TRY_LOCK(cs) MaybeCheckNotHeld(cs), #cs, __FILE__, __LINE__, true
#define ENTER_CRITICAL_SECTION(cs) \
{ \
diff --git a/src/validation.cpp b/src/validation.cpp
index 3fa7afe3090..d690191b38e 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2612,7 +2612,10 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
// doesn't invalidate pointers into the vector, and keep txsdata in scope
// for as long as `control`.
std::optional<CCheckQueueControl<CScriptCheck>> control;
- if (fScriptChecks && parallel_script_checks) control.emplace(m_chainman.GetCheckQueue());
+ auto& queue = m_chainman.GetCheckQueue();
+ auto queue_lock = queue.TryLock();
+ assert(queue_lock);
+ if (fScriptChecks && parallel_script_checks) control.emplace(m_chainman.GetCheckQueue(), std::move(queue_lock));
std::vector<PrecomputedTransactionData> txsdata(block.vtx.size());
Unfortunately, try_lock is allowed to fail spuriously, so that's not safe :(
