util: avoid using thread_local variable that has a destructor #30095

vasild · 2024-05-13T15:07:04Z

Store the thread name in a thread_local variable of type char[] instead of std::string. This avoids calling the destructor when the thread exits. This is a workaround for
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=278701

For type-safety, return std::string from
util::ThreadGetInternalName() instead of char[].

As a side effect of this change, we no longer store a reference to a thread_local variable in CLockLocation. This was dangerous because if the thread quits while the reference still exists (in the global variable lock_data, see inside GetLockData()) then the reference will become dangling.

DrahtBot · 2024-05-13T15:07:07Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto, theuni, laanwj

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

vasild · 2024-05-13T15:09:08Z

This came from the discussion in #29952

DrahtBot · 2024-05-13T16:35:39Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/24905574812}

fanquake · 2024-05-14T02:53:23Z

https://github.com/bitcoin/bitcoin/pull/30095/checks?check_run_id=24909710597

'./'util/threadnames.cpp
In file included from /usr/include/string.h:535,
                 from /usr/include/c++/11/cstring:42,
                 from util/threadnames.cpp:7:
In function ‘void* memcpy(void*, const void*, size_t)’,
    inlined from ‘void SetInternalName(std::string)’ at util/threadnames.cpp:50:16,
    inlined from ‘void util::ThreadSetInternalName(std::string&&)’ at util/threadnames.cpp:69:20:
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:33: error: ‘void* __builtin_memcpy(void*, const void*, long unsigned int)’ forming offset [32, 143] is out of the bounds [0, 32] of object ‘<anonymous>’ with type ‘std::string’ {aka ‘std::__cxx11::basic_string<char>’} [-Werror=array-bounds]
   29 |   return __builtin___memcpy_chk (__dest, __src, __len,
      |          ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
   30 |                                  __glibc_objsize0 (__dest));
      |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~
util/threadnames.cpp: In function ‘void util::ThreadSetInternalName(std::string&&)’:
util/threadnames.cpp:69:20: note: ‘<anonymous>’ declared here
   69 |     SetInternalName(std::move(name));
      |     ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
In file included from /usr/include/string.h:535,
                 from /usr/include/c++/11/cstring:42,
                 from util/threadnames.cpp:7:
In function ‘void* memcpy(void*, const void*, size_t)’,
    inlined from ‘void SetInternalName(std::string)’ at util/threadnames.cpp:50:16,
    inlined from ‘void util::ThreadRename(std::string&&)’ at util/threadnames.cpp:64:20:
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:33: error: ‘void* __builtin_memcpy(void*, const void*, long unsigned int)’ forming offset [32, 143] is out of the bounds [0, 32] of object ‘<anonymous>’ with type ‘std::string’ {aka ‘std::__cxx11::basic_string<char>’} [-Werror=array-bounds]
   29 |   return __builtin___memcpy_chk (__dest, __src, __len,
      |          ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
   30 |                                  __glibc_objsize0 (__dest));
      |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~
util/threadnames.cpp: In function ‘void util::ThreadRename(std::string&&)’:
util/threadnames.cpp:64:20: note: ‘<anonymous>’ declared here
   64 |     SetInternalName(std::move(name));
      |     ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~

vasild · 2024-05-14T09:48:22Z

13f438a667...c5f9afd946: fix the above and return std::string instead of std::string_view which internally has a pointer to the thread_local variable. This way the API can't the misused to store the reference to the thread_local longer than the thread's lifespan.

configure.ac

hebasto

Approach ACK c5f9afd.

src/util/threadnames.cpp

hebasto

ACK c5f9afd, tested on FreeBSD 14.0.

./src/bitcoind -logthreadnames works as expected.

fanquake · 2024-05-15T05:23:03Z

My understanding is that it is safe to use thread_local on FreeBSD for variables that do not have a destructor

So if we are moving forward with this assumption, what is preventing these kinds of variables being reintroduced (elsewhere) into the codebase? I'd rather thread_local be safe to use (in all circumstance) on a platform, or we just not use it.

This change feels a bit odd/forced because it's basically opting back into thread_local, but only certain usage/uncertain assumptions, and there's nothing to prevent regressions. Putting us into this middle-ground doesn't seem so productive, especially given there's currently no real need.

laanwj · 2024-05-15T09:51:16Z

Concept ACK as i said here: #29952 (comment)

I'd rather thread_local be safe to use (in all circumstance) on a platform, or we just not use it.

Sure, that'd be ideal, but handling destructors with TLS is very hard and easy to get wrong. It's not something we want to rely on.

Ideally we'd get rid of thread-local usage completely. But we've considered alternative solutions, and tracking some data until a thread disappears is hard to do also manually. i have a PR somewhere that keeps track of the thread names in a mutex-protected map, but it leaks (as well as is less efficient).

This should be the only place we really need is. So i think doing something special is fine here.

We should avoid any further TLS usage being introduced. But here it's warranted, imo.

Store the thread name in a `thread_local` variable of type `char[]` instead of `std::string`. This avoids calling the destructor when the thread exits. This is a workaround for https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=278701 For type-safety, return `std::string` from `util::ThreadGetInternalName()` instead of `char[]`. As a side effect of this change, we no longer store a reference to a `thread_local` variable in `CLockLocation`. This was dangerous because if the thread quits while the reference still exists (in the global variable `lock_data`, see inside `GetLockData()`) then the reference will become dangling.

vasild · 2024-05-16T16:17:56Z

c5f9afd946...d35ba1b3f1: add a comment as suggested above

hebasto

re-ACK d35ba1b.

theuni

Agree with @laanwj's take.

utACK d35ba1b

src/util/threadnames.cpp

laanwj · 2024-05-18T07:43:37Z

Code review ACK d35ba1b

Followup to bitcoin#30095 (comment).

maflcko · 2024-05-21T11:10:06Z

src/util/threadnames.cpp

+ * 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
+ * other thread_local variables.
+ * @see https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=278701


So when the minimum freeBSD is 15.0, this workaround can be reverted? It would be good to clarify this in the source code.

This was already backported to the 13 and 14 branches, but we could add another comment here.

Ok, so assuming this was fixed, it can probably be reverted some time next year, given the EOL policy https://www.freebsd.org/security/#sup

I think not using thread_local vars with destructors is a reasonable policy anyway, regardless of where it's supported.

I see, so the comment should say: "While this particular bug has been fixed, thread_local and especially thread_local with non-POD objects should be avoided."?

That'd be my preference, yes. I'll see about adding a clang-tidy check for that combo.

clang-tidy check

A faster and easier approximation would be to use git grep -l thread_local, excluding this file (threadnames.cpp), and the normal lint-exclude list.

DrahtBot added the Utils/log/libs label May 13, 2024

vasild mentioned this pull request May 13, 2024

Rethink thread_local (take 2) #29952

Closed

vasild force-pushed the thead_local_without_destructor branch from 0b1a5db to 13f438a Compare May 13, 2024 16:35

DrahtBot added the CI failed label May 13, 2024

vasild force-pushed the thead_local_without_destructor branch from 13f438a to c5f9afd Compare May 14, 2024 09:41

fanquake reviewed May 14, 2024

View reviewed changes

configure.ac Show resolved Hide resolved

DrahtBot removed the CI failed label May 14, 2024

hebasto reviewed May 14, 2024

View reviewed changes

src/util/threadnames.cpp Show resolved Hide resolved

hebasto added the Needs CMake port label May 14, 2024

hebasto approved these changes May 14, 2024

View reviewed changes

vasild force-pushed the thead_local_without_destructor branch from c5f9afd to d35ba1b Compare May 16, 2024 16:17

hebasto approved these changes May 16, 2024

View reviewed changes

DrahtBot requested a review from laanwj May 16, 2024 19:17

theuni approved these changes May 17, 2024

View reviewed changes

theuni mentioned this pull request May 17, 2024

build: Enable thread_local for MinGW-w64 builds #30099

Closed

fanquake reviewed May 18, 2024

View reviewed changes

src/util/threadnames.cpp Show resolved Hide resolved

fanquake mentioned this pull request May 18, 2024

build: Remove --enable-threadlocal #30137

Merged

fanquake merged commit 8804ec7 into bitcoin:master May 21, 2024
16 checks passed

fanquake added a commit to fanquake/bitcoin that referenced this pull request May 21, 2024

doc: remove comment about using thread_local

1e7c20b

Followup to bitcoin#30095 (comment).

maflcko reviewed May 21, 2024

View reviewed changes

vasild deleted the thead_local_without_destructor branch May 21, 2024 14:44

theuni mentioned this pull request May 21, 2024

Add clang-tidy check for thread_local vars #30146

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

util: avoid using thread_local variable that has a destructor #30095

util: avoid using thread_local variable that has a destructor #30095

vasild commented May 13, 2024 •

edited

DrahtBot commented May 13, 2024 •

edited

vasild commented May 13, 2024

DrahtBot commented May 13, 2024

fanquake commented May 14, 2024

vasild commented May 14, 2024

hebasto left a comment

hebasto left a comment

fanquake commented May 15, 2024

laanwj commented May 15, 2024 •

edited

vasild commented May 16, 2024

hebasto left a comment

theuni left a comment

laanwj commented May 18, 2024

maflcko May 21, 2024

fanquake May 21, 2024

maflcko May 21, 2024

theuni May 21, 2024

maflcko May 21, 2024

theuni May 21, 2024

maflcko May 21, 2024

util: avoid using thread_local variable that has a destructor #30095

util: avoid using thread_local variable that has a destructor #30095

Conversation

vasild commented May 13, 2024 • edited

DrahtBot commented May 13, 2024 • edited

Code Coverage

Reviews

vasild commented May 13, 2024

DrahtBot commented May 13, 2024

fanquake commented May 14, 2024

vasild commented May 14, 2024

hebasto left a comment

Choose a reason for hiding this comment

hebasto left a comment

Choose a reason for hiding this comment

fanquake commented May 15, 2024

laanwj commented May 15, 2024 • edited

vasild commented May 16, 2024

hebasto left a comment

Choose a reason for hiding this comment

theuni left a comment

Choose a reason for hiding this comment

laanwj commented May 18, 2024

maflcko May 21, 2024

Choose a reason for hiding this comment

fanquake May 21, 2024

Choose a reason for hiding this comment

maflcko May 21, 2024

Choose a reason for hiding this comment

theuni May 21, 2024

Choose a reason for hiding this comment

maflcko May 21, 2024

Choose a reason for hiding this comment

theuni May 21, 2024

Choose a reason for hiding this comment

maflcko May 21, 2024

Choose a reason for hiding this comment

vasild commented May 13, 2024 •

edited

DrahtBot commented May 13, 2024 •

edited

laanwj commented May 15, 2024 •

edited