Rethink thread_local (take 2)

maflcko commented at 1:18 pm on April 24, 2024: member

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

fanquake commented at 1:21 pm on April 24, 2024: member

Possibly. Someone will first need to test it on FreeBSD and mingw-w64. Given we still assume those implementations to be broken.

maflcko commented at 1:31 pm on April 24, 2024: member

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?

maflcko added the label Brainstorming on Apr 24, 2024

maflcko added the label Needs CMake port on Apr 24, 2024

maflcko added the label Build system on Apr 24, 2024

laanwj commented at 11:43 am on April 25, 2024: member

Concept ACK (if indeed all platforms we care about can do this now, including mingw)

maflcko commented at 12:29 pm on April 25, 2024: member

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.

laanwj commented at 12:35 pm on April 26, 2024: member

@vasild Do you know if C++11 thread-local storage is still broken on FreeBSD?

maflcko commented at 12:39 pm on April 26, 2024: member

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?

vasild commented at 5:12 pm on April 26, 2024: contributor

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:

0__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:

  0diff --git i/configure.ac w/configure.ac
  1index febb352cdb..3eb58e2558 100644
  2--- i/configure.ac
  3+++ w/configure.ac
  4@@ -1047,17 +1047,12 @@ if test "$use_thread_local" = "yes" || test "$use_thread_local" = "auto"; then
  5        *mingw*)
  6           dnl mingw32's implementation of thread_local has also been shown to behave
  7           dnl erroneously under concurrent usage; see:
  8           dnl https://gist.github.com/jamesob/fe9a872051a88b2025b1aa37bfa98605
  9           AC_MSG_RESULT([no])
 10           ;;
 11-        *freebsd*)
 12-          dnl FreeBSD's implementation of thread_local is also buggy (per
 13-          dnl https://groups.google.com/d/msg/bsdmailinglist/22ncTZAbDp4/Dii_pII5AwAJ)
 14-          AC_MSG_RESULT([no])
 15-          ;;
 16         *)
 17           AC_DEFINE([HAVE_THREAD_LOCAL], [1], [Define if thread_local is supported.])
 18           AC_MSG_RESULT([yes])
 19           ;;
 20       esac
 21     ],
 22diff --git i/src/logging.cpp w/src/logging.cpp
 23index 578650f856..1582bc1a17 100644
 24--- i/src/logging.cpp
 25+++ w/src/logging.cpp
 26@@ -358,13 +358,13 @@ void BCLog::Logger::LogPrintStr(const std::string& str, const std::string& loggi
 27     if (m_log_sourcelocations && m_started_new_line) {
 28         str_prefixed.insert(0, "[" + RemovePrefix(source_file, "./") + ":" + ToString(source_line) + "] [" + logging_function + "] ");
 29     }
 30 
 31     if (m_log_threadnames && m_started_new_line) {
 32         const auto& threadname = util::ThreadGetInternalName();
 33-        str_prefixed.insert(0, "[" + (threadname.empty() ? "unknown" : threadname) + "] ");
 34+        str_prefixed.insert(0, std::string{"["} + (std::strlen(threadname) == 0 ? "unknown" : threadname) + "] ");
 35     }
 36 
 37     str_prefixed = LogTimestampStr(str_prefixed);
 38 
 39     m_started_new_line = !str.empty() && str[str.size()-1] == '\n';
 40 
 41diff --git i/src/sync.cpp w/src/sync.cpp
 42index a8bdfc1dea..5fa9fbb7c0 100644
 43--- i/src/sync.cpp
 44+++ w/src/sync.cpp
 45@@ -34,13 +34,13 @@
 46 struct CLockLocation {
 47     CLockLocation(
 48         const char* pszName,
 49         const char* pszFile,
 50         int nLine,
 51         bool fTryIn,
 52-        const std::string& thread_name)
 53+        const char* thread_name)
 54         : fTry(fTryIn),
 55           mutexName(pszName),
 56           sourceFile(pszFile),
 57           m_thread_name(thread_name),
 58           sourceLine(nLine) {}
 59 
 60@@ -57,13 +57,13 @@ struct CLockLocation {
 61     }
 62 
 63 private:
 64     bool fTry;
 65     std::string mutexName;
 66     std::string sourceFile;
 67-    const std::string& m_thread_name;
 68+    const std::string m_thread_name;
 69     int sourceLine;
 70 };
 71 
 72 using LockStackItem = std::pair<void*, CLockLocation>;
 73 using LockStack = std::vector<LockStackItem>;
 74 using LockStacks = std::unordered_map<std::thread::id, LockStack>;
 75diff --git i/src/util/threadnames.cpp w/src/util/threadnames.cpp
 76index 91883fe4ff..2bb12f9da1 100644
 77--- i/src/util/threadnames.cpp
 78+++ w/src/util/threadnames.cpp
 79@@ -39,23 +39,26 @@ static void SetThreadName(const char* name)
 80 }
 81 
 82 // If we have thread_local, just keep thread ID and name in a thread_local
 83 // global.
 84 #if defined(HAVE_THREAD_LOCAL)
 85 
 86-static thread_local std::string g_thread_name;
 87-const std::string& util::ThreadGetInternalName() { return g_thread_name; }
 88+static thread_local char g_thread_name[128] = {'\0'};
 89+const char* util::ThreadGetInternalName() { return g_thread_name; }
 90 //! Set the in-memory internal name for this thread. Does not affect the process
 91 //! name.
 92-static void SetInternalName(std::string name) { g_thread_name = std::move(name); }
 93+static void SetInternalName(std::string name)
 94+{
 95+    std::memcpy(g_thread_name, name.c_str(), std::min(sizeof(g_thread_name), name.length() + 1));
 96+    g_thread_name[sizeof(g_thread_name) - 1] = '\0';
 97+}
 98 
 99 // Without thread_local available, don't handle internal name at all.
100 #else
101 
102-static const std::string empty_string;
103-const std::string& util::ThreadGetInternalName() { return empty_string; }
104+const char* util::ThreadGetInternalName() { return ""; }
105 static void SetInternalName(std::string name) { }
106 #endif
107 
108 void util::ThreadRename(std::string&& name)
109 {
110     SetThreadName(("b-" + name).c_str());
111diff --git i/src/util/threadnames.h w/src/util/threadnames.h
112index 64b2689cf1..a5b7581e99 100644
113--- i/src/util/threadnames.h
114+++ w/src/util/threadnames.h
115@@ -16,11 +16,11 @@ void ThreadRename(std::string&&);
116 
117 //! Set the internal (in-memory) name of the current thread only.
118 void ThreadSetInternalName(std::string&&);
119 
120 //! Get the thread's internal (in-memory) name; used e.g. for identification in
121 //! logging.
122-const std::string& ThreadGetInternalName();
123+const char* ThreadGetInternalName();
124 
125 } // namespace util
126 
127 #endif // BITCOIN_UTIL_THREADNAMES_H

maflcko commented at 8:08 am on April 27, 2024: member

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 #18678 (comment))

maflcko renamed this:
~~Require thread_local~~
Rethink thread_local (take 2)
on Apr 27, 2024

laanwj commented at 8:16 am on April 27, 2024: member

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

vasild commented at 6:52 am on April 30, 2024: contributor

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!

vasild commented at 6:56 pm on May 2, 2024: contributor

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: #29952 (comment) 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.

laanwj commented at 7:18 pm on May 2, 2024: member

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.

vasild commented at 3:08 pm on May 13, 2024: contributor

Is anybody interested in reviewing the patch I posted above if I PR it: #29952 (comment)

PRed at #30095

fanquake closed this on May 21, 2024

fanquake referenced this in commit 2ec0a28a37 on May 21, 2024

hebasto commented at 2:26 pm on June 5, 2024: member

Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/214 and https://github.com/hebasto/bitcoin/pull/220.

hebasto removed the label Needs CMake port on Jun 5, 2024

Rethink thread_local (take 2) #29952