From 0fcc4e1e04082daf6e97e05bfb26e4b94e54ea53 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sat, 20 Dec 2014 05:35:33 -0500 Subject: Assert on probable deadlocks if the second lock isnt try_lock --- src/sync.cpp | 49 +++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 39 insertions(+), 10 deletions(-) (limited to 'src/sync.cpp') diff --git a/src/sync.cpp b/src/sync.cpp index a42293996..1837e8d53 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -33,20 +33,22 @@ void PrintLockContention(const char* pszName, const char* pszFile, int nLine) // struct CLockLocation { - CLockLocation(const char* pszName, const char* pszFile, int nLine) + CLockLocation(const char* pszName, const char* pszFile, int nLine, bool fTryIn) { mutexName = pszName; sourceFile = pszFile; sourceLine = nLine; + fTry = fTryIn; } std::string ToString() const { - return mutexName + " " + sourceFile + ":" + itostr(sourceLine); + return mutexName + " " + sourceFile + ":" + itostr(sourceLine) + (fTry ? " (TRY)" : ""); } std::string MutexName() const { return mutexName; } + bool fTry; private: std::string mutexName; std::string sourceFile; @@ -62,23 +64,52 @@ static boost::thread_specific_ptr lockstack; static void potential_deadlock_detected(const std::pair& mismatch, const LockStack& s1, const LockStack& s2) { + // We attempt to not assert on probably-not deadlocks by assuming that + // a try lock will immediately have otherwise bailed if it had + // failed to get the lock + // We do this by, for the locks which triggered the potential deadlock, + // in either lockorder, checking that the second of the two which is locked + // is only a TRY_LOCK, ignoring locks if they are reentrant. + bool firstLocked = false; + bool secondLocked = false; + bool onlyMaybeDeadlock = false; + LogPrintf("POTENTIAL DEADLOCK DETECTED\n"); LogPrintf("Previous lock order was:\n"); BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, s2) { - if (i.first == mismatch.first) + if (i.first == mismatch.first) { LogPrintf(" (1)"); - if (i.first == mismatch.second) + if (!firstLocked && secondLocked && i.second.fTry) + onlyMaybeDeadlock = true; + firstLocked = true; + } + if (i.first == mismatch.second) { LogPrintf(" (2)"); + if (!secondLocked && firstLocked && i.second.fTry) + onlyMaybeDeadlock = true; + secondLocked = true; + } LogPrintf(" %s\n", i.second.ToString()); } + firstLocked = false; + secondLocked = false; LogPrintf("Current lock order is:\n"); BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, s1) { - if (i.first == mismatch.first) + if (i.first == mismatch.first) { LogPrintf(" (1)"); - if (i.first == mismatch.second) + if (!firstLocked && secondLocked && i.second.fTry) + onlyMaybeDeadlock = true; + firstLocked = true; + } + if (i.first == mismatch.second) { LogPrintf(" (2)"); + if (!secondLocked && firstLocked && i.second.fTry) + onlyMaybeDeadlock = true; + secondLocked = true; + } LogPrintf(" %s\n", i.second.ToString()); } + assert(onlyMaybeDeadlock); } static void push_lock(void* c, const CLockLocation& locklocation, bool fTry) @@ -101,10 +132,8 @@ static void push_lock(void* c, const CLockLocation& locklocation, bool fTry) lockorders[p1] = (*lockstack); std::pair p2 = std::make_pair(c, i.first); - if (lockorders.count(p2)) { + if (lockorders.count(p2)) potential_deadlock_detected(p1, lockorders[p2], lockorders[p1]); - break; - } } } dd_mutex.unlock(); @@ -119,7 +148,7 @@ static void pop_lock() void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry) { - push_lock(cs, CLockLocation(pszName, pszFile, nLine), fTry); + push_lock(cs, CLockLocation(pszName, pszFile, nLine, fTry), fTry); } void LeaveCritical() -- cgit v1.2.3 From a4fe57da6207c1e5691a1e843d22db571f3f0186 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Thu, 6 Aug 2015 14:14:48 +0200 Subject: Revert "Assert on probable deadlocks if the second lock isnt try_lock" Disabling this for now - too many intermittent Travis issues. This reverts commit 0fcc4e1e04082daf6e97e05bfb26e4b94e54ea53 (pull #5515). --- src/sync.cpp | 49 ++++++++++--------------------------------------- 1 file changed, 10 insertions(+), 39 deletions(-) (limited to 'src/sync.cpp') diff --git a/src/sync.cpp b/src/sync.cpp index 1837e8d53..a42293996 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -33,22 +33,20 @@ void PrintLockContention(const char* pszName, const char* pszFile, int nLine) // struct CLockLocation { - CLockLocation(const char* pszName, const char* pszFile, int nLine, bool fTryIn) + CLockLocation(const char* pszName, const char* pszFile, int nLine) { mutexName = pszName; sourceFile = pszFile; sourceLine = nLine; - fTry = fTryIn; } std::string ToString() const { - return mutexName + " " + sourceFile + ":" + itostr(sourceLine) + (fTry ? " (TRY)" : ""); + return mutexName + " " + sourceFile + ":" + itostr(sourceLine); } std::string MutexName() const { return mutexName; } - bool fTry; private: std::string mutexName; std::string sourceFile; @@ -64,52 +62,23 @@ static boost::thread_specific_ptr lockstack; static void potential_deadlock_detected(const std::pair& mismatch, const LockStack& s1, const LockStack& s2) { - // We attempt to not assert on probably-not deadlocks by assuming that - // a try lock will immediately have otherwise bailed if it had - // failed to get the lock - // We do this by, for the locks which triggered the potential deadlock, - // in either lockorder, checking that the second of the two which is locked - // is only a TRY_LOCK, ignoring locks if they are reentrant. - bool firstLocked = false; - bool secondLocked = false; - bool onlyMaybeDeadlock = false; - LogPrintf("POTENTIAL DEADLOCK DETECTED\n"); LogPrintf("Previous lock order was:\n"); BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, s2) { - if (i.first == mismatch.first) { + if (i.first == mismatch.first) LogPrintf(" (1)"); - if (!firstLocked && secondLocked && i.second.fTry) - onlyMaybeDeadlock = true; - firstLocked = true; - } - if (i.first == mismatch.second) { + if (i.first == mismatch.second) LogPrintf(" (2)"); - if (!secondLocked && firstLocked && i.second.fTry) - onlyMaybeDeadlock = true; - secondLocked = true; - } LogPrintf(" %s\n", i.second.ToString()); } - firstLocked = false; - secondLocked = false; LogPrintf("Current lock order is:\n"); BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, s1) { - if (i.first == mismatch.first) { + if (i.first == mismatch.first) LogPrintf(" (1)"); - if (!firstLocked && secondLocked && i.second.fTry) - onlyMaybeDeadlock = true; - firstLocked = true; - } - if (i.first == mismatch.second) { + if (i.first == mismatch.second) LogPrintf(" (2)"); - if (!secondLocked && firstLocked && i.second.fTry) - onlyMaybeDeadlock = true; - secondLocked = true; - } LogPrintf(" %s\n", i.second.ToString()); } - assert(onlyMaybeDeadlock); } static void push_lock(void* c, const CLockLocation& locklocation, bool fTry) @@ -132,8 +101,10 @@ static void push_lock(void* c, const CLockLocation& locklocation, bool fTry) lockorders[p1] = (*lockstack); std::pair p2 = std::make_pair(c, i.first); - if (lockorders.count(p2)) + if (lockorders.count(p2)) { potential_deadlock_detected(p1, lockorders[p2], lockorders[p1]); + break; + } } } dd_mutex.unlock(); @@ -148,7 +119,7 @@ static void pop_lock() void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry) { - push_lock(cs, CLockLocation(pszName, pszFile, nLine, fTry), fTry); + push_lock(cs, CLockLocation(pszName, pszFile, nLine), fTry); } void LeaveCritical() -- cgit v1.2.3 From 9493803f4a60a234c2712bba000a6b9c93ffba94 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Fri, 7 Aug 2015 16:18:16 -0400 Subject: Revert "Revert "Assert on probable deadlocks if the second lock isnt try_lock"" This reverts commit a4fe57da6207c1e5691a1e843d22db571f3f0186. The issue here should be fixed by the previous commit. --- src/sync.cpp | 49 +++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 39 insertions(+), 10 deletions(-) (limited to 'src/sync.cpp') diff --git a/src/sync.cpp b/src/sync.cpp index a42293996..1837e8d53 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -33,20 +33,22 @@ void PrintLockContention(const char* pszName, const char* pszFile, int nLine) // struct CLockLocation { - CLockLocation(const char* pszName, const char* pszFile, int nLine) + CLockLocation(const char* pszName, const char* pszFile, int nLine, bool fTryIn) { mutexName = pszName; sourceFile = pszFile; sourceLine = nLine; + fTry = fTryIn; } std::string ToString() const { - return mutexName + " " + sourceFile + ":" + itostr(sourceLine); + return mutexName + " " + sourceFile + ":" + itostr(sourceLine) + (fTry ? " (TRY)" : ""); } std::string MutexName() const { return mutexName; } + bool fTry; private: std::string mutexName; std::string sourceFile; @@ -62,23 +64,52 @@ static boost::thread_specific_ptr lockstack; static void potential_deadlock_detected(const std::pair& mismatch, const LockStack& s1, const LockStack& s2) { + // We attempt to not assert on probably-not deadlocks by assuming that + // a try lock will immediately have otherwise bailed if it had + // failed to get the lock + // We do this by, for the locks which triggered the potential deadlock, + // in either lockorder, checking that the second of the two which is locked + // is only a TRY_LOCK, ignoring locks if they are reentrant. + bool firstLocked = false; + bool secondLocked = false; + bool onlyMaybeDeadlock = false; + LogPrintf("POTENTIAL DEADLOCK DETECTED\n"); LogPrintf("Previous lock order was:\n"); BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, s2) { - if (i.first == mismatch.first) + if (i.first == mismatch.first) { LogPrintf(" (1)"); - if (i.first == mismatch.second) + if (!firstLocked && secondLocked && i.second.fTry) + onlyMaybeDeadlock = true; + firstLocked = true; + } + if (i.first == mismatch.second) { LogPrintf(" (2)"); + if (!secondLocked && firstLocked && i.second.fTry) + onlyMaybeDeadlock = true; + secondLocked = true; + } LogPrintf(" %s\n", i.second.ToString()); } + firstLocked = false; + secondLocked = false; LogPrintf("Current lock order is:\n"); BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, s1) { - if (i.first == mismatch.first) + if (i.first == mismatch.first) { LogPrintf(" (1)"); - if (i.first == mismatch.second) + if (!firstLocked && secondLocked && i.second.fTry) + onlyMaybeDeadlock = true; + firstLocked = true; + } + if (i.first == mismatch.second) { LogPrintf(" (2)"); + if (!secondLocked && firstLocked && i.second.fTry) + onlyMaybeDeadlock = true; + secondLocked = true; + } LogPrintf(" %s\n", i.second.ToString()); } + assert(onlyMaybeDeadlock); } static void push_lock(void* c, const CLockLocation& locklocation, bool fTry) @@ -101,10 +132,8 @@ static void push_lock(void* c, const CLockLocation& locklocation, bool fTry) lockorders[p1] = (*lockstack); std::pair p2 = std::make_pair(c, i.first); - if (lockorders.count(p2)) { + if (lockorders.count(p2)) potential_deadlock_detected(p1, lockorders[p2], lockorders[p1]); - break; - } } } dd_mutex.unlock(); @@ -119,7 +148,7 @@ static void pop_lock() void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry) { - push_lock(cs, CLockLocation(pszName, pszFile, nLine), fTry); + push_lock(cs, CLockLocation(pszName, pszFile, nLine, fTry), fTry); } void LeaveCritical() -- cgit v1.2.3 From fa24439ff3d8ab5b9efaf66ef4dae6713b88cb35 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sun, 13 Dec 2015 17:58:29 +0100 Subject: Bump copyright headers to 2015 --- src/sync.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/sync.cpp') diff --git a/src/sync.cpp b/src/sync.cpp index 1837e8d53..8df8ae43f 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -1,4 +1,4 @@ -// Copyright (c) 2011-2012 The Bitcoin Core developers +// Copyright (c) 2011-2015 The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. -- cgit v1.2.3 From 5eeb913d6cff9cfe9a6769d7efe4a7b9f23de0f4 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Fri, 8 Apr 2016 22:14:19 +0200 Subject: Clean up lockorder data of destroyed mutexes The lockorder potential deadlock detection works by remembering for each lock A that is acquired while holding another B the pair (A,B), and triggering a warning when (B,A) already exists in the table. A and B in the above text are represented by pointers to the CCriticalSection object that is acquired. This does mean however that we need to clean up the table entries that refer to any critical section which is destroyed, as it memory address can potentially be used for another unrelated lock in the future. Implement this clean up by remembering not only the pairs in forward direction, but also backward direction. This allows for fast iteration over all pairs that use a deleted CCriticalSection in either the first or the second position. --- src/sync.cpp | 55 ++++++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 44 insertions(+), 11 deletions(-) (limited to 'src/sync.cpp') diff --git a/src/sync.cpp b/src/sync.cpp index 8df8ae43f..641ed2c8c 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -56,11 +56,24 @@ private: }; typedef std::vector > LockStack; +typedef std::map, LockStack> LockOrders; +typedef std::set > InvLockOrders; -static boost::mutex dd_mutex; -static std::map, LockStack> lockorders; -static boost::thread_specific_ptr lockstack; +struct LockData { + // Very ugly hack: as the global constructs and destructors run single + // threaded, we use this boolean to know whether LockData still exists, + // as DeleteLock can get called by global CCriticalSection destructors + // after LockData disappears. + bool available; + LockData() : available(true) {} + ~LockData() { available = false; } + LockOrders lockorders; + InvLockOrders invlockorders; + boost::mutex dd_mutex; +} static lockdata; + +boost::thread_specific_ptr lockstack; static void potential_deadlock_detected(const std::pair& mismatch, const LockStack& s1, const LockStack& s2) { @@ -117,7 +130,7 @@ static void push_lock(void* c, const CLockLocation& locklocation, bool fTry) if (lockstack.get() == NULL) lockstack.reset(new LockStack); - dd_mutex.lock(); + boost::unique_lock lock(lockdata.dd_mutex); (*lockstack).push_back(std::make_pair(c, locklocation)); @@ -127,23 +140,21 @@ static void push_lock(void* c, const CLockLocation& locklocation, bool fTry) break; std::pair p1 = std::make_pair(i.first, c); - if (lockorders.count(p1)) + if (lockdata.lockorders.count(p1)) continue; - lockorders[p1] = (*lockstack); + lockdata.lockorders[p1] = (*lockstack); std::pair p2 = std::make_pair(c, i.first); - if (lockorders.count(p2)) - potential_deadlock_detected(p1, lockorders[p2], lockorders[p1]); + lockdata.invlockorders.insert(p2); + if (lockdata.lockorders.count(p2)) + potential_deadlock_detected(p1, lockdata.lockorders[p2], lockdata.lockorders[p1]); } } - dd_mutex.unlock(); } static void pop_lock() { - dd_mutex.lock(); (*lockstack).pop_back(); - dd_mutex.unlock(); } void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry) @@ -173,4 +184,26 @@ void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, abort(); } +void DeleteLock(void* cs) +{ + if (!lockdata.available) { + // We're already shutting down. + return; + } + boost::unique_lock lock(lockdata.dd_mutex); + std::pair item = std::make_pair(cs, (void*)0); + LockOrders::iterator it = lockdata.lockorders.lower_bound(item); + while (it != lockdata.lockorders.end() && it->first.first == cs) { + std::pair invitem = std::make_pair(it->first.second, it->first.first); + lockdata.invlockorders.erase(invitem); + lockdata.lockorders.erase(it++); + } + InvLockOrders::iterator invit = lockdata.invlockorders.lower_bound(item); + while (invit != lockdata.invlockorders.end() && invit->first == cs) { + std::pair invinvitem = std::make_pair(invit->second, invit->first); + lockdata.lockorders.erase(invinvitem); + lockdata.invlockorders.erase(invit++); + } +} + #endif /* DEBUG_LOCKORDER */ -- cgit v1.2.3 From 27765b6403cece54320374b37afb01a0cfe571c3 Mon Sep 17 00:00:00 2001 From: isle2983 Date: Sat, 31 Dec 2016 11:01:21 -0700 Subject: Increment MIT Licence copyright header year on files modified in 2016 Edited via: $ contrib/devtools/copyright_header.py update . --- src/sync.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/sync.cpp') diff --git a/src/sync.cpp b/src/sync.cpp index 641ed2c8c..a18d0f148 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -1,4 +1,4 @@ -// Copyright (c) 2011-2015 The Bitcoin Core developers +// Copyright (c) 2011-2016 The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. -- cgit v1.2.3 From 8465631845eac3db834942a4feb50f65c3401c68 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 2 Feb 2017 22:22:01 -0500 Subject: Always enforce lock strict lock ordering (try or not) --- src/sync.cpp | 26 +------------------------- 1 file changed, 1 insertion(+), 25 deletions(-) (limited to 'src/sync.cpp') diff --git a/src/sync.cpp b/src/sync.cpp index a18d0f148..25773f08e 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -77,52 +77,28 @@ boost::thread_specific_ptr lockstack; static void potential_deadlock_detected(const std::pair& mismatch, const LockStack& s1, const LockStack& s2) { - // We attempt to not assert on probably-not deadlocks by assuming that - // a try lock will immediately have otherwise bailed if it had - // failed to get the lock - // We do this by, for the locks which triggered the potential deadlock, - // in either lockorder, checking that the second of the two which is locked - // is only a TRY_LOCK, ignoring locks if they are reentrant. - bool firstLocked = false; - bool secondLocked = false; - bool onlyMaybeDeadlock = false; - LogPrintf("POTENTIAL DEADLOCK DETECTED\n"); LogPrintf("Previous lock order was:\n"); BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, s2) { if (i.first == mismatch.first) { LogPrintf(" (1)"); - if (!firstLocked && secondLocked && i.second.fTry) - onlyMaybeDeadlock = true; - firstLocked = true; } if (i.first == mismatch.second) { LogPrintf(" (2)"); - if (!secondLocked && firstLocked && i.second.fTry) - onlyMaybeDeadlock = true; - secondLocked = true; } LogPrintf(" %s\n", i.second.ToString()); } - firstLocked = false; - secondLocked = false; LogPrintf("Current lock order is:\n"); BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, s1) { if (i.first == mismatch.first) { LogPrintf(" (1)"); - if (!firstLocked && secondLocked && i.second.fTry) - onlyMaybeDeadlock = true; - firstLocked = true; } if (i.first == mismatch.second) { LogPrintf(" (2)"); - if (!secondLocked && firstLocked && i.second.fTry) - onlyMaybeDeadlock = true; - secondLocked = true; } LogPrintf(" %s\n", i.second.ToString()); } - assert(onlyMaybeDeadlock); + assert(false); } static void push_lock(void* c, const CLockLocation& locklocation, bool fTry) -- cgit v1.2.3 From 618ee9249b178d94911ea66cb4b5291f000ef1fb Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 7 Feb 2017 14:15:28 -0500 Subject: Further-enforce lockordering by enforcing directly after TRY_LOCKs --- src/sync.cpp | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) (limited to 'src/sync.cpp') diff --git a/src/sync.cpp b/src/sync.cpp index 25773f08e..fce57f1df 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -110,21 +110,19 @@ static void push_lock(void* c, const CLockLocation& locklocation, bool fTry) (*lockstack).push_back(std::make_pair(c, locklocation)); - if (!fTry) { - BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, (*lockstack)) { - if (i.first == c) - break; - - std::pair p1 = std::make_pair(i.first, c); - if (lockdata.lockorders.count(p1)) - continue; - lockdata.lockorders[p1] = (*lockstack); - - std::pair p2 = std::make_pair(c, i.first); - lockdata.invlockorders.insert(p2); - if (lockdata.lockorders.count(p2)) - potential_deadlock_detected(p1, lockdata.lockorders[p2], lockdata.lockorders[p1]); - } + BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, (*lockstack)) { + if (i.first == c) + break; + + std::pair p1 = std::make_pair(i.first, c); + if (lockdata.lockorders.count(p1)) + continue; + lockdata.lockorders[p1] = (*lockstack); + + std::pair p2 = std::make_pair(c, i.first); + lockdata.invlockorders.insert(p2); + if (lockdata.lockorders.count(p2)) + potential_deadlock_detected(p1, lockdata.lockorders[p2], lockdata.lockorders[p1]); } } -- cgit v1.2.3