Merge pull request #3313 from patricklodder/1.14.7-fix-lock-debugging

refactor: fix undefined behavior and cleanup lock debugging code

src/Makefile.test.include

@@ -128,6 +128,7 @@ BITCOIN_TESTS =\
   test/sigopcount_tests.cpp \
   test/skiplist_tests.cpp \
   test/streams_tests.cpp \
+  test/sync_tests.cpp \
   test/test_bitcoin.cpp \
   test/test_bitcoin.h \
   test/testutil.cpp \

src/sync.cpp

@@ -9,8 +9,13 @@
 
 #include <stdio.h>
 
-#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 +60,31 @@ private:
     int sourceLine;
 };
 
-typedef std::vector<std::pair<void*, CLockLocation> > LockStack;
-typedef std::map<std::pair<void*, void*>, LockStack> LockOrders;
-typedef std::set<std::pair<void*, void*> > InvLockOrders;
+typedef std::pair<void*, CLockLocation> LockStackItem;
+typedef std::vector<LockStackItem> LockStack;
+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
-    // 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> 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) {
+    for (const LockStackItem& i : s2) {
         if (i.first == mismatch.first) {
             LogPrintf(" (1)");
         }
@@ -89,7 +94,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) {
+    for (const LockStackItem& i : s1) {
         if (i.first == mismatch.first) {
             LogPrintf(" (1)");
         }
@@ -98,28 +103,31 @@ static void potential_deadlock_detected(const std::pair<void*, void*>& mismatch,
         }
         LogPrintf(" %s\n", i.second.ToString());
     }
-    assert(false);
+    if (g_debug_lockorder_abort) {
+        fprintf(stderr, "Assertion failed: detected inconsistent lock order at %s:%i, details in debug log.\n", __FILE__, __LINE__);
+        abort();
+    }
+    throw std::logic_error("potential deadlock detected");
 }
 
 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<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 PAIRTYPE(void*, CLockLocation) & i, (*lockstack)) {
+    for (const LockStackItem& i : lock_stack) {
         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 +136,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,41 +158,58 @@ 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 (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<boost::mutex> lock(lockdata.dd_mutex);
-    std::pair<void*, void*> item = std::make_pair(cs, (void*)0);
+    LockData& lockdata = GetLockData();
+    std::lock_guard<std::mutex> 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<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++);
     }
 }
 
+bool g_debug_lockorder_abort = true;
+
 #endif /* DEBUG_LOCKORDER */

src/sync.h

@@ -77,6 +77,13 @@ void LeaveCritical();
 std::string LocksHeld();
 void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs);
 void DeleteLock(void* cs);
+
+/**
+ * Call abort() if a potential lock order deadlock bug is detected, instead of
+ * just logging information and throwing a logic_error. Defaults to true, and
+ * set to false in DEBUG_LOCKORDER unit tests.
+ */
+extern bool g_debug_lockorder_abort;
 #else
 void static inline EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false) {}
 void static inline LeaveCritical() {}

src/test/sync_tests.cpp

@@ -0,0 +1,41 @@
+// Copyright (c) 2012-2017 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <sync.h>
+#include <test/test_bitcoin.h>
+
+#include <boost/test/unit_test.hpp>
+
+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 mutex1, mutex2;
+    {
+        LOCK2(mutex1, mutex2);
+    }
+    bool error_thrown = false;
+    try {
+        LOCK2(mutex2, mutex1);
+    } catch (const std::logic_error& e) {
+        BOOST_CHECK_EQUAL(e.what(), "potential deadlock detected");
+        error_thrown = true;
+    }
+    #ifdef DEBUG_LOCKORDER
+    BOOST_CHECK(error_thrown);
+    #else
+    BOOST_CHECK(!error_thrown);
+    #endif
+
+    #ifdef DEBUG_LOCKORDER
+    g_debug_lockorder_abort = prev;
+    #endif
+}
+
+BOOST_AUTO_TEST_SUITE_END()