From 1382913e61f5db6ba849b1e261e8aefcd5a1ae68 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Wed, 8 Nov 2017 16:21:13 -0500 Subject: [PATCH] Make LOCK, LOCK2, TRY_LOCK work with CWaitableCriticalSection They should also work with any other mutex type which std::unique_lock supports. There is no change in behavior for current code that calls these macros with CCriticalSection mutexes. --- src/sync.h | 61 ++++++++++++++++++++++------------------- src/test/sync_tests.cpp | 29 ++++++++++++++------ 2 files changed, 53 insertions(+), 37 deletions(-) diff --git a/src/sync.h b/src/sync.h index c9a950799..339a7e2c1 100644 --- a/src/sync.h +++ b/src/sync.h @@ -71,13 +71,17 @@ void static inline DeleteLock(void* cs) {} #define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs) /** - * Template mixin that adds -Wthread-safety locking - * annotations to a subset of the mutex API. + * Template mixin that adds -Wthread-safety locking annotations and lock order + * checking to a subset of the mutex API. */ template class LOCKABLE AnnotatedMixin : public PARENT { public: + ~AnnotatedMixin() { + DeleteLock((void*)this); + } + void lock() EXCLUSIVE_LOCK_FUNCTION() { PARENT::lock(); @@ -92,19 +96,15 @@ public: { return PARENT::try_lock(); } + + using UniqueLock = std::unique_lock; }; /** * Wrapped mutex: supports recursive locking, but no waiting * TODO: We should move away from using the recursive lock by default. */ -class CCriticalSection : public AnnotatedMixin -{ -public: - ~CCriticalSection() { - DeleteLock((void*)this); - } -}; +typedef AnnotatedMixin CCriticalSection; /** Wrapped mutex: supports waiting but not recursive locking */ typedef AnnotatedMixin CWaitableCriticalSection; @@ -119,20 +119,19 @@ typedef std::unique_lock WaitableLock; void PrintLockContention(const char* pszName, const char* pszFile, int nLine); #endif -/** Wrapper around std::unique_lock */ -class SCOPED_LOCKABLE CCriticalBlock +/** Wrapper around std::unique_lock style lock for Mutex. */ +template +class SCOPED_LOCKABLE CCriticalBlock : public Base { private: - std::unique_lock lock; - void Enter(const char* pszName, const char* pszFile, int nLine) { - EnterCritical(pszName, pszFile, nLine, (void*)(lock.mutex())); + EnterCritical(pszName, pszFile, nLine, (void*)(Base::mutex())); #ifdef DEBUG_LOCKCONTENTION - if (!lock.try_lock()) { + if (!Base::try_lock()) { PrintLockContention(pszName, pszFile, nLine); #endif - lock.lock(); + Base::lock(); #ifdef DEBUG_LOCKCONTENTION } #endif @@ -140,15 +139,15 @@ private: bool TryEnter(const char* pszName, const char* pszFile, int nLine) { - EnterCritical(pszName, pszFile, nLine, (void*)(lock.mutex()), true); - lock.try_lock(); - if (!lock.owns_lock()) + EnterCritical(pszName, pszFile, nLine, (void*)(Base::mutex()), true); + Base::try_lock(); + if (!Base::owns_lock()) LeaveCritical(); - return lock.owns_lock(); + return Base::owns_lock(); } public: - CCriticalBlock(CCriticalSection& mutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) EXCLUSIVE_LOCK_FUNCTION(mutexIn) : lock(mutexIn, std::defer_lock) + CCriticalBlock(Mutex& mutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) EXCLUSIVE_LOCK_FUNCTION(mutexIn) : Base(mutexIn, std::defer_lock) { if (fTry) TryEnter(pszName, pszFile, nLine); @@ -156,11 +155,11 @@ public: Enter(pszName, pszFile, nLine); } - CCriticalBlock(CCriticalSection* pmutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) EXCLUSIVE_LOCK_FUNCTION(pmutexIn) + CCriticalBlock(Mutex* pmutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) EXCLUSIVE_LOCK_FUNCTION(pmutexIn) { if (!pmutexIn) return; - lock = std::unique_lock(*pmutexIn, std::defer_lock); + *static_cast(this) = Base(*pmutexIn, std::defer_lock); if (fTry) TryEnter(pszName, pszFile, nLine); else @@ -169,22 +168,28 @@ public: ~CCriticalBlock() UNLOCK_FUNCTION() { - if (lock.owns_lock()) + if (Base::owns_lock()) LeaveCritical(); } operator bool() { - return lock.owns_lock(); + return Base::owns_lock(); } }; +template +using DebugLock = CCriticalBlock::type>::type>; + #define PASTE(x, y) x ## y #define PASTE2(x, y) PASTE(x, y) -#define LOCK(cs) CCriticalBlock PASTE2(criticalblock, __COUNTER__)(cs, #cs, __FILE__, __LINE__) -#define LOCK2(cs1, cs2) CCriticalBlock criticalblock1(cs1, #cs1, __FILE__, __LINE__), criticalblock2(cs2, #cs2, __FILE__, __LINE__) -#define TRY_LOCK(cs, name) CCriticalBlock name(cs, #cs, __FILE__, __LINE__, true) +#define LOCK(cs) DebugLock PASTE2(criticalblock, __COUNTER__)(cs, #cs, __FILE__, __LINE__) +#define LOCK2(cs1, cs2) \ + DebugLock criticalblock1(cs1, #cs1, __FILE__, __LINE__); \ + DebugLock criticalblock2(cs2, #cs2, __FILE__, __LINE__); +#define TRY_LOCK(cs, name) DebugLock name(cs, #cs, __FILE__, __LINE__, true) +#define WAIT_LOCK(cs, name) DebugLock name(cs, #cs, __FILE__, __LINE__) #define ENTER_CRITICAL_SECTION(cs) \ { \ diff --git a/src/test/sync_tests.cpp b/src/test/sync_tests.cpp index 2e0951c3a..539e2ff3a 100644 --- a/src/test/sync_tests.cpp +++ b/src/test/sync_tests.cpp @@ -7,16 +7,10 @@ #include -BOOST_FIXTURE_TEST_SUITE(sync_tests, BasicTestingSetup) - -BOOST_AUTO_TEST_CASE(potential_deadlock_detected) +namespace { +template +void TestPotentialDeadLockDetected(MutexType& mutex1, MutexType& mutex2) { - #ifdef DEBUG_LOCKORDER - bool prev = g_debug_lockorder_abort; - g_debug_lockorder_abort = false; - #endif - - CCriticalSection mutex1, mutex2; { LOCK2(mutex1, mutex2); } @@ -32,6 +26,23 @@ BOOST_AUTO_TEST_CASE(potential_deadlock_detected) #else BOOST_CHECK(!error_thrown); #endif +} +} // namespace + +BOOST_FIXTURE_TEST_SUITE(sync_tests, BasicTestingSetup) + +BOOST_AUTO_TEST_CASE(potential_deadlock_detected) +{ + #ifdef DEBUG_LOCKORDER + bool prev = g_debug_lockorder_abort; + g_debug_lockorder_abort = false; + #endif + + CCriticalSection rmutex1, rmutex2; + TestPotentialDeadLockDetected(rmutex1, rmutex2); + + CWaitableCriticalSection mutex1, mutex2; + TestPotentialDeadLockDetected(mutex1, mutex2); #ifdef DEBUG_LOCKORDER g_debug_lockorder_abort = prev;