Repository: bitcoin/bitcoin (language: cpp)

Issue:
Rethink thread_local (take 2)
After dbfca4a815d2dbef69f3b634c24b875bc1d22afc from the 23.x release (and fe1b3256888bd0e70d0c9655f565e139ec87b606 from 25.x), I think the `--disable-threadlocal` can now be dropped, as those commits removed the need for it?

cc @hebasto @fanquake 


Discussion / hints:
Possibly. Someone will first need to test it on FreeBSD and mingw-w64. Given we still assume those implementations to be broken.
I couldn't get wine running locally, so my testing failed. But given that we require a C++20 compiler, I'd be surprised if there is still a platform out there that hasn't correctly implemented C++11 at this point.

I presume, at least for the mingw issue, the existing unit tests should detect the issue, as I presume the `RenameEnMasse` unit test is equivalent to the C++ shared in the linked gist?
Concept ACK (if indeed all platforms we care about can do this now, including mingw)
> I couldn't get wine running locally

I hopped into the win64 CI container and ran https://gist.github.com/jamesob/fe9a872051a88b2025b1aa37bfa98605 there. It passed.

But given that the previous test was done on Trusty (14.04?), it will probably be hard to check when it started passing.
@vasild Do you know if C++11 thread-local storage is still broken on FreeBSD?
> FreeBSD

I am not familiar with the *BSD, but is there indication that OpenBSD or NetBSD are unaffected? What are the steps to test this?
https://github.com/bitcoin/bitcoin/blob/7973a670915632b75a6aa16f24f98b936865c48f/configure.ac#L1054-L1057

The file https://github.com/freebsd/freebsd-src/blob/master/lib/libc/stdlib/cxa_thread_atexit_impl.c has not been changed since 2017.

Running the unit tests on FreeBSD with thread_local enabled (had to edit `configure.ac`) prints a bunch of those:
```
__cxa_thread_call_dtors: dtr 0x375164b2d70 from unloaded dso, skipping
```
which is at least annoying and scary.

We only use `thread_local` for storing the thread name in `std::string`. Can use dumb `char` array for this:

<details>
<summary>[patch] avoid thread_local with std::string</summary>

```diff
diff --git i/configure.ac w/configure.ac
index febb352cdb..3eb58e2558 100644
--- i/configure.ac
+++ w/configure.ac
@@ -1047,17 +1047,12 @@ if test "$use_thread_local" = "yes" || test "$use_thread_local" = "auto"; then
        *mingw*)
           dnl mingw32's implementation of thread_local has also been shown to behave
           dnl erroneously under concurrent usage; see:
           dnl https://gist.github.com/jamesob/fe9a872051a88b2025b1aa37bfa98605
           AC_MSG_RESULT([no])
           ;;
-        *freebsd*)
-          dnl FreeBSD's implementation of thread_local is also buggy (per
-          dnl https://groups.google.com/d/msg/bsdmailinglist/22ncTZAbDp4/Dii_pII5AwAJ)
-          AC_MSG_RESULT([no])
-          ;;
         *)
           AC_DEFINE([HAVE_THREAD_LOCAL], [1], [Define if thread_local is supported.])
           AC_MSG_RESULT([yes])
           ;;
       esac
     ],
diff --git i/src/logging.cpp w/src/logging.cpp
index 578650f856..1582bc1a17 100644
--- i/src/logging.cpp
+++ w/src/logging.cpp
@@ -358,13 +358,13 @@ void BCLog::Logger::LogPrintStr(const std::string& str, const std::string& loggi
     if (m_log_sourcelocations && m_started_new_line) {
         str_prefixed.insert(0, "[" + RemovePrefix(source_file, "./") + ":" + ToString(source_line) + "] [" + logging_function + "] ");
     }
 
     if (m_log_threadnames && m_started_new_line) {
         const auto& threadname = util::ThreadGetInternalName();
-        str_prefixed.insert(0, "[" + (threadname.empty() ? "unknown" : threadname) + "] ");
+        str_prefixed.insert(0, std::string{"["} + (std::strlen(threadname) == 0 ? "unknown" : threadname) + "] ");
     }
 
     str_prefixed = LogTimestampStr(str_prefixed);
 
     m_started_new_line = !str.empty() && str[str.size()-1] == '\n';
 
diff --git i/src/sync.cpp w/src/sync.cpp
index a8bdfc1dea..5fa9fbb7c0 100644
--- i/src/sync.cpp
+++ w/src/sync.cpp
@@ -34,13 +34,13 @@
 struct CLockLocation {
     CLockLocation(
         const char* pszName,
         const char* pszFile,
         int nLine,
         bool fTryIn,
-        const std::string& thread_name)
+        const char* thread_name)
         : fTry(fTryIn),
           mutexName(pszName),
           sourceFile(pszFile),
           m_thread_name(thread_name),
           sourceLine(nLine) {}
 
@@ -57,13 +57,13 @@ struct CLockLocation {
     }
 
 private:
     bool fTry;
     std::string mutexName;
     std::string sourceFile;
-    const std::string& m_thread_name;
+    const std::string m_thread_name;
     int sourceLine;
 };
 
 using LockStackItem = std::pair<void*, CLockLocation>;
 using LockStack = std::vector<LockStackItem>;
 using LockStacks = std::unordered_map<std::thread::id, LockStack>;
diff --git i/src/util/threadnames.cpp w/src/util/threadnames.cpp
index 91883fe4ff..2bb12f9da1 100644
--- i/src/util/threadnames.cpp
+++ w/src/util/threadnames.cpp
@@ -39,23 +39,26 @@ static void SetThreadName(const char* name)
 }
 
 // If we have thread_local, just keep thread ID and name in a thread_local
 // global.
 #if defined(HAVE_THREAD_LOCAL)
 
-static thread_local std::string g_thread_name;
-const std::string& util::ThreadGetInternalName() { return g_thread_name; }
+static thread_local char g_thread_name[128] = {'\0'};
+const char* util::ThreadGetInternalName() { return g_thread_name; }
 //! Set the in-memory internal name for this thread. Does not affect the process
 //! name.
-static void SetInternalName(std::string name) { g_thread_name = std::move(name); }
+static void SetInternalName(std::string name)
+{
+    std::memcpy(g_thread_name, name.c_str(), std::min(sizeof(g_thread_name), name.length() + 1));
+    g_thread_name[sizeof(g_thread_name) - 1] = '\0';
+}
 
 // Without thread_local available, don't handle internal name at all.
 #else
 
-static const std::string empty_string;
-const std::string& util::ThreadGetInternalName() { return empty_string; }
+const char* util::ThreadGetInternalName() { return ""; }
 static void SetInternalName(std::string name) { }
 #endif
 
 void util::ThreadRename(std::string&& name)
 {
     SetThreadName(("b-" + name).c_str());
diff --git i/src/util/threadnames.h w/src/util/threadnames.h
index 64b2689cf1..a5b7581e99 100644
--- i/src/util/threadnames.h
+++ w/src/util/threadnames.h
@@ -16,11 +16,11 @@ void ThreadRename(std::string&&);
 
 //! Set the internal (in-memory) name of the current thread only.
 void ThreadSetInternalName(std::string&&);
 
 //! Get the thread's internal (in-memory) name; used e.g. for identification in
 //! logging.
-const std::string& ThreadGetInternalName();
+const char* ThreadGetInternalName();
 
 } // namespace util
 
 #endif // BITCOIN_UTIL_THREADNAMES_H
```
</details>
> We only use `thread_local` for storing the thread name in `std::string`. Can use dumb `char` array for this:

If this works, then string_view may also work? Yet another alternative may be to just completely nuke thread_local with a map from id to $obj (See https://github.com/bitcoin/bitcoin/issues/18678#issuecomment-621717592)
i think the main problem with the map approach is that it doesn't clean up data when threads disappear, this is something that TLS handles automatically, and also why it's so hard for platforms to get right
It just occurred to me on Friday evening and I forgot about this during the weekend - we may have a bug in our code and FreeBSD may just be the messenger - we return a reference to the `thread_local`, store it in `CLockLocation`, from there in the global `lockdata` / `lock_stack`. It looks like the reference in `lockdata` may still be existent after the thread has exited.

@maflcko, thanks for the `string_view` hint!
This is indeed some problem in FreeBSD's libc which I reported upstream with a minimal, reproducable test case:

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=278701

Is anybody interested in reviewing the patch I posted above if I PR it:
https://github.com/bitcoin/bitcoin/issues/29952#issuecomment-2079776241
reworked to return `std::string_view` to the callers, but still storing the thread name in a `thread_local char []`? That would work around the FreeBSD weird message printout because the `thread_local` variable will not have a destructor.

As for storing a reference to `thread_local` in a global variable - that is dangerous and should be avoided IMO. I checked that there is no bug currently in the code but it looks fragile.
> reworked to return std::string_view to the callers, but still storing the thread name in a thread_local char []?

Yes. i think (for this one very rare exception) it's acceptable to store a string in a fixed-size buffer. To not need a destructor and heap deallocation when a thread goes away, works around a large part of the complexity of handling thread-local data.

And making it use `string_view` throughout the changed functions instead of `char*` is a good idea, a lot less ugly.

> As for storing a reference to thread_local in a global variable - that is dangerous and should be avoided IMO. I checked that there is no bug currently in the code but it looks fragile.

Agree. The memory can go away at any time when the thread goes away, and it will be a dangling reference. It's brittle.
> Is anybody interested in reviewing the patch I posted above if I PR it: [#29952 (comment)](https://github.com/bitcoin/bitcoin/issues/29952#issuecomment-2079776241)

PRed at https://github.com/bitcoin/bitcoin/pull/30095



Fix patch:
diff --git a/configure.ac b/configure.ac
index bfc1e54921dda..d539e4436f3a0 100644
--- a/configure.ac
+++ b/configure.ac
@@ -244,12 +244,6 @@ AC_ARG_ENABLE([lcov-branch-coverage],
   [use_lcov_branch=yes],
   [use_lcov_branch=no])
 
-AC_ARG_ENABLE([threadlocal],
-  [AS_HELP_STRING([--enable-threadlocal],
-  [enable features that depend on the c++ thread_local keyword (currently just thread names in debug logs). (default is to enable if there is platform support)])],
-  [use_thread_local=$enableval],
-  [use_thread_local=auto])
-
 AC_ARG_ENABLE([zmq],
   [AS_HELP_STRING([--disable-zmq],
   [disable ZMQ notifications])],
@@ -1028,40 +1022,6 @@ AC_COMPILE_IFELSE([AC_LANG_SOURCE([
   [AC_MSG_RESULT([no])]
 )
 
-if test "$use_thread_local" = "yes" || test "$use_thread_local" = "auto"; then
-  TEMP_LDFLAGS="$LDFLAGS"
-  LDFLAGS="$TEMP_LDFLAGS $PTHREAD_CFLAGS"
-  AC_MSG_CHECKING([for thread_local support])
-  AC_LINK_IFELSE([AC_LANG_SOURCE([
-    #include <thread>
-    static thread_local int foo = 0;
-    static void run_thread() { foo++;}
-    int main(){
-    for(int i = 0; i < 10; i++) { std::thread(run_thread).detach();}
-    return foo;
-    }
-    ])],
-    [
-     case $host in
-       *mingw*)
-          dnl mingw32's implementation of thread_local has also been shown to behave
-          dnl erroneously under concurrent usage; see:
-          dnl https://gist.github.com/jamesob/fe9a872051a88b2025b1aa37bfa98605
-          AC_MSG_RESULT([no])
-          ;;
-        *)
-          AC_DEFINE([HAVE_THREAD_LOCAL], [1], [Define if thread_local is supported.])
-          AC_MSG_RESULT([yes])
-          ;;
-      esac
-    ],
-    [
-      AC_MSG_RESULT([no])
-    ]
-  )
-  LDFLAGS="$TEMP_LDFLAGS"
-fi
-
 dnl Check for different ways of gathering OS randomness
 AC_MSG_CHECKING([for Linux getrandom function])
 AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[
diff --git a/src/init/common.cpp b/src/init/common.cpp
index 3a6df3e8bd519..f473ee6d66544 100644
--- a/src/init/common.cpp
+++ b/src/init/common.cpp
@@ -31,11 +31,7 @@ void AddLoggingArgs(ArgsManager& argsman)
     argsman.AddArg("-logips", strprintf("Include IP addresses in debug output (default: %u)", DEFAULT_LOGIPS), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-loglevel=<level>|<category>:<level>", strprintf("Set the global or per-category severity level for logging categories enabled with the -debug configuration option or the logging RPC. Possible values are %s (default=%s). The following levels are always logged: error, warning, info. If <category>:<level> is supplied, the setting will override the global one and may be specified multiple times to set multiple category-specific levels. <category> can be: %s.", LogInstance().LogLevelsString(), LogInstance().LogLevelToStr(BCLog::DEFAULT_LOG_LEVEL), LogInstance().LogCategoriesString()), ArgsManager::DISALLOW_NEGATION | ArgsManager::DISALLOW_ELISION | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-logtimestamps", strprintf("Prepend debug output with timestamp (default: %u)", DEFAULT_LOGTIMESTAMPS), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
-#ifdef HAVE_THREAD_LOCAL
-    argsman.AddArg("-logthreadnames", strprintf("Prepend debug output with name of the originating thread (only available on platforms supporting thread_local) (default: %u)", DEFAULT_LOGTHREADNAMES), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
-#else
-    argsman.AddHiddenArgs({"-logthreadnames"});
-#endif
+    argsman.AddArg("-logthreadnames", strprintf("Prepend debug output with name of the originating thread (default: %u)", DEFAULT_LOGTHREADNAMES), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-logsourcelocations", strprintf("Prepend debug output with name of the originating source location (source file, line number and function name) (default: %u)", DEFAULT_LOGSOURCELOCATIONS), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-logtimemicros", strprintf("Add microsecond precision to debug timestamps (default: %u)", DEFAULT_LOGTIMEMICROS), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-loglevelalways", strprintf("Always prepend a category and level (default: %u)", DEFAULT_LOGLEVELALWAYS), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
@@ -50,9 +46,7 @@ void SetLoggingOptions(const ArgsManager& args)
     LogInstance().m_print_to_console = args.GetBoolArg("-printtoconsole", !args.GetBoolArg("-daemon", false));
     LogInstance().m_log_timestamps = args.GetBoolArg("-logtimestamps", DEFAULT_LOGTIMESTAMPS);
     LogInstance().m_log_time_micros = args.GetBoolArg("-logtimemicros", DEFAULT_LOGTIMEMICROS);
-#ifdef HAVE_THREAD_LOCAL
     LogInstance().m_log_threadnames = args.GetBoolArg("-logthreadnames", DEFAULT_LOGTHREADNAMES);
-#endif
     LogInstance().m_log_sourcelocations = args.GetBoolArg("-logsourcelocations", DEFAULT_LOGSOURCELOCATIONS);
     LogInstance().m_always_print_category_level = args.GetBoolArg("-loglevelalways", DEFAULT_LOGLEVELALWAYS);
 
diff --git a/src/util/threadnames.cpp b/src/util/threadnames.cpp
index ba074e30e072d..0249de37e345d 100644
--- a/src/util/threadnames.cpp
+++ b/src/util/threadnames.cpp
@@ -37,10 +37,6 @@ static void SetThreadName(const char* name)
 #endif
 }
 
-// If we have thread_local, just keep thread ID and name in a thread_local
-// global.
-#if defined(HAVE_THREAD_LOCAL)
-
 /**
  * The name of the thread. We use char array instead of std::string to avoid
  * complications with running a destructor when the thread exits. Avoid adding
@@ -58,13 +54,6 @@ static void SetInternalName(const std::string& name)
     g_thread_name[copy_bytes] = '\0';
 }
 
-// Without thread_local available, don't handle internal name at all.
-#else
-
-std::string util::ThreadGetInternalName() { return ""; }
-static void SetInternalName(const std::string& name) { }
-#endif
-
 void util::ThreadRename(const std::string& name)
 {
     SetThreadName(("b-" + name).c_str());


Test patch:
diff --git a/src/test/util/random.h b/src/test/util/random.h
index c910bd6a3a005..18ab425e4813a 100644
--- a/src/test/util/random.h
+++ b/src/test/util/random.h
@@ -14,9 +14,8 @@
 /**
  * This global and the helpers that use it are not thread-safe.
  *
- * If thread-safety is needed, the global could be made thread_local (given
- * that thread_local is supported on all architectures we support) or a
- * per-thread instance could be used in the multi-threaded test.
+ * If thread-safety is needed, a per-thread instance could be
+ * used in the multi-threaded test.
  */
 extern FastRandomContext g_insecure_rand_ctx;
 
diff --git a/src/test/util_threadnames_tests.cpp b/src/test/util_threadnames_tests.cpp
index df5b1a4461d77..174052d5fa66e 100644
--- a/src/test/util_threadnames_tests.cpp
+++ b/src/test/util_threadnames_tests.cpp
@@ -11,8 +11,6 @@
 #include <thread>
 #include <vector>
 
-#include <config/bitcoin-config.h> // IWYU pragma: keep
-
 #include <boost/test/unit_test.hpp>
 
 BOOST_AUTO_TEST_SUITE(util_threadnames_tests)
@@ -52,11 +50,6 @@ std::set<std::string> RenameEnMasse(int num_threads)
  */
 BOOST_AUTO_TEST_CASE(util_threadnames_test_rename_threaded)
 {
-#if !defined(HAVE_THREAD_LOCAL)
-    // This test doesn't apply to platforms where we don't have thread_local.
-    return;
-#endif
-
     std::set<std::string> names = RenameEnMasse(100);
 
     BOOST_CHECK_EQUAL(names.size(), 100U);