From c88b1cbf57a333261dbb8cf2eae91cf76e056d96 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Thu, 8 May 2025 13:32:12 +0000 Subject: [PATCH 1/5] tests: get rid of remaining manual critsect usage --- src/test/sync_tests.cpp | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/src/test/sync_tests.cpp b/src/test/sync_tests.cpp index 0576bf1633f..dcd1b1ce367 100644 --- a/src/test/sync_tests.cpp +++ b/src/test/sync_tests.cpp @@ -37,8 +37,7 @@ void TestPotentialDeadLockDetected(MutexType& mutex1, MutexType& mutex2) template void TestDoubleLock2(MutexType& m) { - ENTER_CRITICAL_SECTION(m); - LEAVE_CRITICAL_SECTION(m); + LOCK(m); } template @@ -48,15 +47,15 @@ void TestDoubleLock(bool should_throw) g_debug_lockorder_abort = false; MutexType m; - ENTER_CRITICAL_SECTION(m); - if (should_throw) { - BOOST_CHECK_EXCEPTION(TestDoubleLock2(m), std::logic_error, + { + LOCK(m); + if (should_throw) { + BOOST_CHECK_EXCEPTION(TestDoubleLock2(m), std::logic_error, HasReason("double lock detected")); - } else { - BOOST_CHECK_NO_THROW(TestDoubleLock2(m)); + } else { + BOOST_CHECK_NO_THROW(TestDoubleLock2(m)); + } } - LEAVE_CRITICAL_SECTION(m); - BOOST_CHECK(LockStackEmpty()); g_debug_lockorder_abort = prev; @@ -64,15 +63,15 @@ void TestDoubleLock(bool should_throw) #endif /* DEBUG_LOCKORDER */ template -void TestInconsistentLockOrderDetected(MutexType& mutex1, MutexType& mutex2) NO_THREAD_SAFETY_ANALYSIS +void TestInconsistentLockOrderDetected(MutexType& mutex1, MutexType& mutex2) { - ENTER_CRITICAL_SECTION(mutex1); - ENTER_CRITICAL_SECTION(mutex2); + { + WAIT_LOCK(mutex1, lock1); + LOCK(mutex2); #ifdef DEBUG_LOCKORDER - BOOST_CHECK_EXCEPTION(LEAVE_CRITICAL_SECTION(mutex1), std::logic_error, HasReason("mutex1 was not most recent critical section locked")); + BOOST_CHECK_EXCEPTION(REVERSE_LOCK(lock1, mutex1), std::logic_error, HasReason("mutex1 was not most recent critical section locked")); #endif // DEBUG_LOCKORDER - LEAVE_CRITICAL_SECTION(mutex2); - LEAVE_CRITICAL_SECTION(mutex1); + } BOOST_CHECK(LockStackEmpty()); } } // namespace From 3ddd554d318110e8270498760340854929c59405 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Thu, 22 May 2025 17:26:13 +0000 Subject: [PATCH 2/5] tests: Add Assertions in reverse_lock tests to exercise thread-safety annotations --- src/test/reverselock_tests.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/test/reverselock_tests.cpp b/src/test/reverselock_tests.cpp index 64b3ddf2cd7..6abf55c8f7c 100644 --- a/src/test/reverselock_tests.cpp +++ b/src/test/reverselock_tests.cpp @@ -17,8 +17,10 @@ BOOST_AUTO_TEST_CASE(reverselock_basics) WAIT_LOCK(mutex, lock); BOOST_CHECK(lock.owns_lock()); + AssertLockHeld(mutex); { REVERSE_LOCK(lock, mutex); + AssertLockNotHeld(mutex); BOOST_CHECK(!lock.owns_lock()); } BOOST_CHECK(lock.owns_lock()); From 0d0e0a39b4a58fc48d2f0107e8c08bece58130bc Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Tue, 6 May 2025 15:39:12 +0000 Subject: [PATCH 3/5] threading: use a reverse lock rather than manual critsect macros No functional change. --- src/rpc/mining.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 36adf154f15..b710c605bcb 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -704,7 +704,7 @@ static RPCHelpMan getblocktemplate() NodeContext& node = EnsureAnyNodeContext(request.context); ChainstateManager& chainman = EnsureChainman(node); Mining& miner = EnsureMining(node); - LOCK(cs_main); + WAIT_LOCK(cs_main, csmain_lock); uint256 tip{CHECK_NONFATAL(miner.getTip()).value().hash}; std::string strMode = "template"; @@ -810,8 +810,8 @@ static RPCHelpMan getblocktemplate() } // Release lock while waiting - LEAVE_CRITICAL_SECTION(cs_main); { + REVERSE_LOCK(csmain_lock, cs_main); MillisecondsDouble checktxtime{std::chrono::minutes(1)}; while (IsRPCRunning()) { // If hashWatchedChain is not a real block hash, this will @@ -830,8 +830,6 @@ static RPCHelpMan getblocktemplate() checktxtime = std::chrono::seconds(10); } } - ENTER_CRITICAL_SECTION(cs_main); - tip = CHECK_NONFATAL(miner.getTip()).value().hash; if (!IsRPCRunning()) From b537a6a6dbd3b2a6725ccfafd57c9cc50cd617b0 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Tue, 6 May 2025 16:47:59 +0000 Subject: [PATCH 4/5] threading: remove obsolete critsect macros --- src/sync.h | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/src/sync.h b/src/sync.h index ee189bee105..f863e5f5822 100644 --- a/src/sync.h +++ b/src/sync.h @@ -39,12 +39,6 @@ LOCK2(mutex1, mutex2); TRY_LOCK(mutex, name); std::unique_lock name(mutex, std::try_to_lock_t); - -ENTER_CRITICAL_SECTION(mutex); // no RAII - mutex.lock(); - -LEAVE_CRITICAL_SECTION(mutex); // no RAII - mutex.unlock(); */ /////////////////////////////// @@ -270,20 +264,6 @@ inline MutexType* MaybeCheckNotHeld(MutexType* m) LOCKS_EXCLUDED(m) LOCK_RETURNE #define TRY_LOCK(cs, name) UniqueLock name(LOCK_ARGS(cs), true) #define WAIT_LOCK(cs, name) UniqueLock name(LOCK_ARGS(cs)) -#define ENTER_CRITICAL_SECTION(cs) \ - { \ - EnterCritical(#cs, __FILE__, __LINE__, &cs); \ - (cs).lock(); \ - } - -#define LEAVE_CRITICAL_SECTION(cs) \ - { \ - std::string lockname; \ - CheckLastCritical((void*)(&cs), lockname, #cs, __FILE__, __LINE__); \ - (cs).unlock(); \ - LeaveCritical(); \ - } - //! Run code while locking a mutex. //! //! Examples: From 46ca7712cb5fcf759cfc9f4f32d74215c8c83763 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Wed, 28 May 2025 15:48:15 +0000 Subject: [PATCH 5/5] threading: remove unused template instantiations These were only required for the ENTER_CRITICAL_SECTION macro. --- src/sync.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/sync.cpp b/src/sync.cpp index e5be6fd1156..fb60e3cf14c 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -206,8 +206,6 @@ void EnterCritical(const char* pszName, const char* pszFile, int nLine, MutexTyp { push_lock(cs, CLockLocation(pszName, pszFile, nLine, fTry, util::ThreadGetInternalName())); } -template void EnterCritical(const char*, const char*, int, Mutex*, bool); -template void EnterCritical(const char*, const char*, int, RecursiveMutex*, bool); template void EnterCritical(const char*, const char*, int, std::mutex*, bool); template void EnterCritical(const char*, const char*, int, std::recursive_mutex*, bool);