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 <laanwj@protonmail.com>

src/sync.cpp

@@ -11,6 +11,13 @@
 
 #include <boost/foreach.hpp>
 #include <boost/thread.hpp>
+#include <thread>
+#include <unordered_map>
+#include <utility>
+#include <vector>
+#include <set>
+#include <map>
+#include <mutex>
 
 #ifdef DEBUG_LOCKCONTENTION
 void PrintLockContention(const char* pszName, const char* pszFile, int nLine)
@@ -55,31 +62,31 @@ private:
     int sourceLine;
 };
 
-typedef std::vector<std::pair<void*, CLockLocation> > LockStack;
+typedef std::pair<void*, CLockLocation> LockStackItem;
-typedef std::map<std::pair<void*, void*>, LockStack> LockOrders;
+typedef std::vector<LockStackItem> LockStack;
-typedef std::set<std::pair<void*, void*> > InvLockOrders;
+typedef std::unordered_map<std::thread::id, LockStack> LockStacks;
+
+typedef std::pair<void*, void*> LockPair;
+typedef std::map<LockPair, LockStack> LockOrders;
+typedef std::set<LockPair> InvLockOrders;
 
 struct LockData {
-    // Very ugly hack: as the global constructs and destructors run single
+    LockStacks m_lock_stacks;
-    // 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;
+    std::mutex dd_mutex;
-} static lockdata;
+};
 
-boost::thread_specific_ptr<LockStack> lockstack;
+LockData& GetLockData() {
+    static LockData& lock_data = *new LockData();
+    return lock_data;
+}
 
-static void potential_deadlock_detected(const std::pair<void*, void*>& 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<void*, void*>& 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<void*, void*>& mismatch,
 
 static void push_lock(void* c, const CLockLocation& locklocation, bool fTry)
 {
-    if (lockstack.get() == NULL)
+    LockData& lockdata = GetLockData();
-        lockstack.reset(new LockStack);
+    std::lock_guard<std::mutex> lock(lockdata.dd_mutex);
 
-    boost::unique_lock<boost::mutex> 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 LockStackItem& i, lock_stack) {
-
-    BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, (*lockstack)) {
         if (i.first == c)
             break;
 
-        std::pair<void*, void*> 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<void*, void*> 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<std::mutex> 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<std::mutex> 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<std::mutex> 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 (LockHeld(cs)) return;
-        if (i.first == 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) {
+    LockData& lockdata = GetLockData();
-        // We're already shutting down.
+    std::lock_guard<std::mutex> lock(lockdata.dd_mutex);
-        return;
+
-    }
+    const LockPair item = std::make_pair(cs, (void*)0);
-    boost::unique_lock<boost::mutex> lock(lockdata.dd_mutex);
-    std::pair<void*, void*> 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<void*, void*> 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<void*, void*> 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++);
     }