From 9df93da1695f1f3deb6ea71784b90bca1bf23d82 Mon Sep 17 00:00:00 2001 From: Patrick Lodder Date: Wed, 28 Feb 2024 16:01:49 -0500 Subject: [PATCH 1/8] bench: iwyu cleanup on non-test sources Cherry-picked from: 462ef2dc Github Pull Request: #3456 --- src/bench/bench.cpp | 11 +++++++---- src/bench/bench.h | 13 ++++++++----- src/bench/bench_bitcoin.cpp | 8 +++----- 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/src/bench/bench.cpp b/src/bench/bench.cpp index 3c9df4f71..98d3f3a7c 100644 --- a/src/bench/bench.cpp +++ b/src/bench/bench.cpp @@ -3,11 +3,14 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include "bench.h" -#include "perf.h" -#include -#include -#include +#include // for NULL +#include // for gettimeofday, timeval +#include // for operator<<, setprecision +#include // for operator<<, basic_ostream +#include // for pair, make_pair + +#include "perf.h" // for perf_cpucycles, perf_fini, perf_init benchmark::BenchRunner::BenchmarkMap &benchmark::BenchRunner::benchmarks() { static std::map benchmarks_map; diff --git a/src/bench/bench.h b/src/bench/bench.h index 0e7605c72..445be56d9 100644 --- a/src/bench/bench.h +++ b/src/bench/bench.h @@ -5,12 +5,15 @@ #ifndef BITCOIN_BENCH_BENCH_H #define BITCOIN_BENCH_BENCH_H -#include -#include +#include // for uint64_t -#include -#include -#include +#include // for function +#include // for BOOST_PP_CAT +#include // for BOOST_PP_STRINGIZE + +#include // for numeric_limits +#include // for map +#include // for string // Simple micro-benchmarking framework; API mostly matches a subset of the Google Benchmark // framework (see https://github.com/google/benchmark) diff --git a/src/bench/bench_bitcoin.cpp b/src/bench/bench_bitcoin.cpp index 61a0b31ae..c4e3fd5d0 100644 --- a/src/bench/bench_bitcoin.cpp +++ b/src/bench/bench_bitcoin.cpp @@ -2,11 +2,9 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. -#include "bench.h" - -#include "key.h" -#include "validation.h" -#include "util.h" +#include "bench.h" // for BenchRunner +#include "key.h" // for ECC_Start, ECC_Stop +#include "util.h" // for SetupEnvironment, fPrintToDebugLog int main(int argc, char** argv) From b097aa7e6802d41b395693a85b637dbb048bcee6 Mon Sep 17 00:00:00 2001 From: Patrick Lodder Date: Wed, 12 Jun 2024 05:21:53 -0400 Subject: [PATCH 2/8] fs: use path::is_absolute() instead of path::is_complete() path::is_complete() has been deprecated since boost::filesystem version 3, which was introduced with boost 1.44.0, and removed in 1.85.0. The minimum supported boost version has been 1.60.0 since Dogecoin Core release v1.14.3, so this can be safely replaced. Cherry-picked from: 1d84f4f8 Github Pull Request: #3558 Conflicts resolved: - boost fs prefix fix in src/rpc/protocol.cpp and src/util.cpp --- src/rpc/protocol.cpp | 5 ++--- src/util.cpp | 4 ++-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/rpc/protocol.cpp b/src/rpc/protocol.cpp index 3a24394a6..94ca5c37f 100644 --- a/src/rpc/protocol.cpp +++ b/src/rpc/protocol.cpp @@ -22,7 +22,7 @@ using namespace std; * JSON-RPC protocol. Bitcoin speaks version 1.0 for maximum compatibility, * but uses JSON-RPC 1.1/2.0 standards for parts of the 1.0 standard that were * unspecified (HTTP errors and contents of 'error'). - * + * * 1.0 spec: http://json-rpc.org/wiki/specification * 1.2 spec: http://jsonrpc.org/historical/json-rpc-over-http.html */ @@ -72,7 +72,7 @@ static const std::string COOKIEAUTH_FILE = ".cookie"; boost::filesystem::path GetAuthCookieFile() { boost::filesystem::path path(GetArg("-rpccookiefile", COOKIEAUTH_FILE)); - if (!path.is_complete()) path = GetDataDir() / path; + if (!path.is_absolute()) path = GetDataDir() / path; return path; } @@ -126,4 +126,3 @@ void DeleteAuthCookie() LogPrintf("%s: Unable to remove random auth cookie file: %s\n", __func__, e.what()); } } - diff --git a/src/util.cpp b/src/util.cpp index dc5d818ec..0cc261422 100644 --- a/src/util.cpp +++ b/src/util.cpp @@ -590,7 +590,7 @@ const boost::filesystem::path &GetBackupDir() boost::filesystem::path GetConfigFile(const std::string& confPath) { boost::filesystem::path pathConfigFile(confPath); - if (!pathConfigFile.is_complete()) + if (!pathConfigFile.is_absolute()) pathConfigFile = GetDataDir(false) / pathConfigFile; return pathConfigFile; @@ -626,7 +626,7 @@ void ReadConfigFile(const std::string& confPath) boost::filesystem::path GetPidFile() { boost::filesystem::path pathPidFile(GetArg("-pid", BITCOIN_PID_FILENAME)); - if (!pathPidFile.is_complete()) pathPidFile = GetDataDir() / pathPidFile; + if (!pathPidFile.is_absolute()) pathPidFile = GetDataDir() / pathPidFile; return pathPidFile; } From b9a99f613f1be997624a56031e1e6bedb9af100f Mon Sep 17 00:00:00 2001 From: Patrick Lodder Date: Wed, 12 Jun 2024 05:34:13 -0400 Subject: [PATCH 3/8] fs: don't use boost convenience functions fs::basename() and fs::extension() have been removed in boost 1.85, use path::stem() and path::extension() instead. This is safe since boost 1.44 (boost::filesystem version 3), and the minimum supported boost version for Dogecoin Core is 1.60. Cherry-picked from: 37909dd1 Github Pull Request: #3558 Conflicts resolved: - boost fs namespace fix in src/wallet/wallet.cpp --- src/wallet/wallet.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index b3542ea88..fd00e23a1 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -459,8 +459,10 @@ bool CWallet::Verify() uiInterface.InitMessage(_("Verifying wallet...")); // Wallet file must be a plain filename without a directory - if (walletFile != boost::filesystem::basename(walletFile) + boost::filesystem::extension(walletFile)) + boost::filesystem::path walletPath(walletFile); + if (walletFile != walletPath.stem().string() + walletPath.extension().string()) { return InitError(strprintf(_("Wallet %s resides outside data directory %s"), walletFile, GetDataDir().string())); + } if (!bitdb.Open(GetDataDir())) { From aa568d45d343f0f12554de03e4b392be050fa44b Mon Sep 17 00:00:00 2001 From: Patrick Lodder Date: Wed, 12 Jun 2024 05:39:24 -0400 Subject: [PATCH 4/8] fs: for boost 1.74 and up, use c++17 compatible copy_options In boost 1.74.0, boost::filesystem adapted path::copy_option enum to be the same as the enum in the fs::path from c++17, which is now called copy_options, and its member "overwrite_if_exists" is now called "overwrite_existing". In boost 1.85, the deprecated copy_option enum is fully removed. Because Dogecoin Core currently supports boost 1.60 and up, conditionally implement the new enum based on the boost version that is built against. Cherry-picked from: 95554c3e Github Pull Request: #3558 Conflicts resolved: - boost fs prefix added in src/wallet/wallet.cpp --- src/wallet/wallet.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index fd00e23a1..c65692a82 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4020,7 +4020,11 @@ bool CWallet::BackupWallet(const std::string& strDest) return false; } -#if BOOST_VERSION >= 104000 +#if BOOST_VERSION >= 107400 + // Boost 1.74.0 and up implements std++17 like "copy_options", and this + // is the only remaining enum after 1.85.0 + boost::filesystem::copy_file(pathSrc, pathDest, boost::filesystem::copy_options::overwrite_existing); +#elif BOOST_VERSION >= 104000 boost::filesystem::copy_file(pathSrc, pathDest, boost::filesystem::copy_option::overwrite_if_exists); #else boost::filesystem::copy_file(pathSrc, pathDest); From 1fb943f2ef0b02ca490422b3e57a87c143308732 Mon Sep 17 00:00:00 2001 From: Patrick Lodder Date: Wed, 12 Jun 2024 05:47:13 -0400 Subject: [PATCH 5/8] add missing include in validation.h This omission was masked by including boost/unordered_map.hpp prior to boost 1.85.0. Cherry-picked from: d021d75f Github Pull Request: #3558 --- src/validation.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/validation.h b/src/validation.h index 5380badaa..7a20d36b1 100644 --- a/src/validation.h +++ b/src/validation.h @@ -22,6 +22,7 @@ #include #include +#include #include #include #include From 447919565b25ad94aab0d477a8d1fd3fe5b5c24e Mon Sep 17 00:00:00 2001 From: Patrick Lodder Date: Wed, 9 Aug 2023 13:19:19 -0400 Subject: [PATCH 6/8] 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. Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Co-authored-by: Wladimir J. van der Laan Cherry-picked from: 06e40407 Github Pull Request: #3313 --- 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++); } From caefcf1c1cd402c956c10a3034b838e294c4f698 Mon Sep 17 00:00:00 2001 From: Patrick Lodder Date: Fri, 11 Aug 2023 06:46:28 -0400 Subject: [PATCH 7/8] cleanup: remove unneccesary boost dependencies from sync.cpp Removes boost dependencies in sync.cpp in favor of standard c++ for-loops, improving readability. Cherry-picked from: a9129315 Github Pull Request: #3313 --- src/sync.cpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/sync.cpp b/src/sync.cpp index a4518b09c..9eb2bceb4 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -9,8 +9,6 @@ #include -#include -#include #include #include #include @@ -86,7 +84,7 @@ static void potential_deadlock_detected(const LockPair& mismatch, const LockStac { LogPrintf("POTENTIAL DEADLOCK DETECTED\n"); LogPrintf("Previous lock order was:\n"); - BOOST_FOREACH (const LockStackItem& i, s2) { + for (const LockStackItem& i : s2) { if (i.first == mismatch.first) { LogPrintf(" (1)"); } @@ -96,7 +94,7 @@ static void potential_deadlock_detected(const LockPair& mismatch, const LockStac LogPrintf(" %s\n", i.second.ToString()); } LogPrintf("Current lock order is:\n"); - BOOST_FOREACH (const LockStackItem& i, s1) { + for (const LockStackItem& i : s1) { if (i.first == mismatch.first) { LogPrintf(" (1)"); } @@ -116,7 +114,7 @@ static void push_lock(void* c, const CLockLocation& locklocation, bool fTry) LockStack& lock_stack = lockdata.m_lock_stacks[std::this_thread::get_id()]; lock_stack.emplace_back(c, locklocation); - BOOST_FOREACH (const LockStackItem& i, lock_stack) { + for (const LockStackItem& i : lock_stack) { if (i.first == c) break; From ad3a9b800c7e1b1e5ce64e68e08a748b5a6f9e82 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Wed, 8 Nov 2017 15:28:35 -0500 Subject: [PATCH 8/8] Add unit test for DEBUG_LOCKORDER code Cherry-picked from: 11f5dd6e Github Pull Request: #3313 --- src/Makefile.test.include | 1 + src/sync.cpp | 8 +++++++- src/sync.h | 7 +++++++ src/test/sync_tests.cpp | 41 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 56 insertions(+), 1 deletion(-) create mode 100644 src/test/sync_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index d0dc3af6f..d83f96163 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -127,6 +127,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/test_random.h \ diff --git a/src/sync.cpp b/src/sync.cpp index 9eb2bceb4..f9e26299b 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -103,7 +103,11 @@ static void potential_deadlock_detected(const LockPair& mismatch, const LockStac } 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) @@ -206,4 +210,6 @@ void DeleteLock(void* cs) } } +bool g_debug_lockorder_abort = true; + #endif /* DEBUG_LOCKORDER */ diff --git a/src/sync.h b/src/sync.h index 3b29050e0..c79245325 100644 --- a/src/sync.h +++ b/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() {} diff --git a/src/test/sync_tests.cpp b/src/test/sync_tests.cpp new file mode 100644 index 000000000..2e0951c3a --- /dev/null +++ b/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 +#include + +#include + +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()