From 06e4040717ca3b0a1061e961d96f07c290f408e7 Mon Sep 17 00:00:00 2001 From: Patrick Lodder Date: Wed, 9 Aug 2023 13:19:19 -0400 Subject: [PATCH] refactor: remove DEBUG_LOCKORDER undefined behavior Removes undefined behavior by refactoring the way we destruct objects that track mutexes when debugging. 1. Prevent UB from LockData custom destructor by making it implicit 2. Replace the thread_specific_ptr holding the lock stack with a static map that stores it per thread. Also cleans up the aliasing to make the code better readable. Backported from: 458992b0 8d8921ab f511f61d 58e6881b 26c093a9 Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Co-authored-by: Wladimir J. van der Laan --- src/sync.cpp | 106 ++++++++++++++++++++++++++++++++------------------- 1 file changed, 67 insertions(+), 39 deletions(-) diff --git a/src/sync.cpp b/src/sync.cpp index fce57f1df..a4518b09c 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -11,6 +11,13 @@ #include #include +#include +#include +#include +#include +#include +#include +#include #ifdef DEBUG_LOCKCONTENTION void PrintLockContention(const char* pszName, const char* pszFile, int nLine) @@ -55,31 +62,31 @@ private: int sourceLine; }; -typedef std::vector > LockStack; -typedef std::map, LockStack> LockOrders; -typedef std::set > InvLockOrders; +typedef std::pair LockStackItem; +typedef std::vector LockStack; +typedef std::unordered_map LockStacks; + +typedef std::pair LockPair; +typedef std::map LockOrders; +typedef std::set InvLockOrders; 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; } - + LockStacks m_lock_stacks; LockOrders lockorders; InvLockOrders invlockorders; - boost::mutex dd_mutex; -} static lockdata; + std::mutex dd_mutex; +}; -boost::thread_specific_ptr lockstack; +LockData& GetLockData() { + static LockData& lock_data = *new LockData(); + return lock_data; +} -static void potential_deadlock_detected(const std::pair& mismatch, const LockStack& s1, const LockStack& s2) +static void potential_deadlock_detected(const LockPair& mismatch, const LockStack& s1, const LockStack& s2) { LogPrintf("POTENTIAL DEADLOCK DETECTED\n"); LogPrintf("Previous lock order was:\n"); - BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, s2) { + BOOST_FOREACH (const LockStackItem& i, s2) { if (i.first == mismatch.first) { LogPrintf(" (1)"); } @@ -89,7 +96,7 @@ static void potential_deadlock_detected(const std::pair& mismatch, LogPrintf(" %s\n", i.second.ToString()); } LogPrintf("Current lock order is:\n"); - BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, s1) { + BOOST_FOREACH (const LockStackItem& i, s1) { if (i.first == mismatch.first) { LogPrintf(" (1)"); } @@ -103,23 +110,22 @@ static void potential_deadlock_detected(const std::pair& mismatch, static void push_lock(void* c, const CLockLocation& locklocation, bool fTry) { - if (lockstack.get() == NULL) - lockstack.reset(new LockStack); + LockData& lockdata = GetLockData(); + std::lock_guard lock(lockdata.dd_mutex); - boost::unique_lock lock(lockdata.dd_mutex); + LockStack& lock_stack = lockdata.m_lock_stacks[std::this_thread::get_id()]; + lock_stack.emplace_back(c, locklocation); - (*lockstack).push_back(std::make_pair(c, locklocation)); - - BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, (*lockstack)) { + BOOST_FOREACH (const LockStackItem& i, lock_stack) { if (i.first == c) break; - std::pair p1 = std::make_pair(i.first, c); + const LockPair p1 = std::make_pair(i.first, c); if (lockdata.lockorders.count(p1)) continue; - lockdata.lockorders[p1] = (*lockstack); + lockdata.lockorders[p1] = lock_stack; - std::pair p2 = std::make_pair(c, i.first); + const LockPair 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]); @@ -128,7 +134,14 @@ static void push_lock(void* c, const CLockLocation& locklocation, bool fTry) static void pop_lock() { - (*lockstack).pop_back(); + LockData& lockdata = GetLockData(); + std::lock_guard lock(lockdata.dd_mutex); + + LockStack& lock_stack = lockdata.m_lock_stacks[std::this_thread::get_id()]; + lock_stack.pop_back(); + if (lock_stack.empty()) { + lockdata.m_lock_stacks.erase(std::this_thread::get_id()); + } } void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry) @@ -143,38 +156,53 @@ void LeaveCritical() std::string LocksHeld() { + LockData& lockdata = GetLockData(); + std::lock_guard lock(lockdata.dd_mutex); + + const LockStack& lock_stack = lockdata.m_lock_stacks[std::this_thread::get_id()]; + std::string result; - BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, *lockstack) + for (const LockStackItem& i : lock_stack) { result += i.second.ToString() + std::string("\n"); + } return result; } +static bool LockHeld(void* mutex) +{ + LockData& lockdata = GetLockData(); + std::lock_guard lock(lockdata.dd_mutex); + + const LockStack& lock_stack = lockdata.m_lock_stacks[std::this_thread::get_id()]; + for (const LockStackItem& i : lock_stack) { + if (i.first == mutex) return true; + } + + return false; +} + void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) { - BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, *lockstack) - if (i.first == cs) - return; + if (LockHeld(cs)) return; fprintf(stderr, "Assertion failed: lock %s not held in %s:%i; locks held:\n%s", pszName, pszFile, nLine, LocksHeld().c_str()); 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); + LockData& lockdata = GetLockData(); + std::lock_guard lock(lockdata.dd_mutex); + + const LockPair 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); + const LockPair 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); + const LockPair invinvitem = std::make_pair(invit->second, invit->first); lockdata.lockorders.erase(invinvitem); lockdata.invlockorders.erase(invit++); }