-
Notifications
You must be signed in to change notification settings - Fork 35.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
util: avoid using thread_local variable that has a destructor #30095
util: avoid using thread_local variable that has a destructor #30095
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
This came from the discussion in #29952 |
0b1a5db
to
13f438a
Compare
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
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));
| ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~ |
13f438a
to
c5f9afd
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach ACK c5f9afd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK c5f9afd, tested on FreeBSD 14.0.
./src/bitcoind -logthreadnames
works as expected.
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 This change feels a bit odd/forced because it's basically opting back into |
Concept ACK as i said here: #29952 (comment)
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.
c5f9afd
to
d35ba1b
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-ACK d35ba1b.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK d35ba1b |
* 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So when the minimum freeBSD is 15.0, this workaround can be reverted? It would be good to clarify this in the source code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was already backported to the 13 and 14 branches, but we could add another comment here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think not using thread_local
vars with destructors is a reasonable policy anyway, regardless of where it's supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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."?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That'd be my preference, yes. I'll see about adding a clang-tidy check for that combo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Ported to the CMake-based build system in hebasto#214. |
Ah nm, I see, that's already done :) |
Store the thread name in a
thread_local
variable of typechar[]
instead ofstd::string
. This avoids calling the destructor when the thread exits. This is a workaround forhttps://bugs.freebsd.org/bugzilla/show_bug.cgi?id=278701
For type-safety, return
std::string
fromutil::ThreadGetInternalName()
instead ofchar[]
.As a side effect of this change, we no longer store a reference to a
thread_local
variable inCLockLocation
. This was dangerous because if the thread quits while the reference still exists (in the global variablelock_data
, see insideGetLockData()
) then the reference will become dangling.