configure: Fix thread_local detection#16059
Merged
maflcko merged 1 commit intobitcoin:masterfrom May 25, 2019
Merged
Conversation
Member
|
ACK 3b549f4aae9bc5f5f9e9dee8476523daf44dd7ee (checked diff with --ignore-all-space, and verified that thread_local is now disabled on freebsd) Show signature and timestampSignature: Timestamp of file with hash |
Contributor
Author
Correct! Lines 191 to 195 in 2d1583e |
Member
|
Concept ACK, but I think that the logic here is complex enough that an --enable-threadlocal is necessary so that it can be chosen explicitly. |
Contributor
Author
As in:
|
Member
|
I think it is nice to have thread_local enabled by default on the generic "64-bit Ubuntu Bionic" (and similar) environments. Thread names give developers additional debug information. |
Contributor
|
Option (1) is the way to go. If a user passes explicitly passes --enable-threadlocal or --disable-threadlocal, it should enable or disable thread local code respectively, regardless of other options. If neither is specified, it should choose an intelligent default based on the platform and maybe based on --glibc-back-compat. |
Member
+1
They had thread names before thread_local :) |
Contributor
I think @MarcoFalke is referring to the |
dongcarl
force-pushed
the
2019-05-fix-thread_local-compat
branch
from
3b549f4 to
caaa942
Compare
Contributor
Author
|
Implemented logic as documented here: #16059 (comment) |
Contributor
|
Gitian builds for commit 2d1583e (master):
Gitian builds for commit 256e35348968dc048f175507999c466654294194 (master and this pull):
|
Contributor
|
Gitian builds for commit 3001cc6 (master):
Gitian builds for commit 3f02161dd63d4983ae2b356dfea3dd4e45ee1f4a (master and this pull):
|
Member
|
utACK caaa942a5ff372db723a224c35236df48b93ebcb Show signature and timestampSignature: Timestamp of file with hash |
Contributor
Author
|
From the @DrahtBot gitian builds, looks like |
Member
|
Concept ACK |
ryanofsky
approved these changes
May 23, 2019
Contributor
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
utACK caaa942a5ff372db723a224c35236df48b93ebcb. Change looks good to me. I just suggested adding some more documentation below. IMO, the glibc-back-compat option is also underdocumented, and is perhaps trying to control too many disparate things. My instinct would be to make the --enable-threadlocal and --enable-glibc-back-compat options independent and just pass them both where needed. But I don't know all the context, and the change seems fine in its present form.
- When aiming for glibc compatibility, don't use thread_local. - Add a flag --enable-threadlocal, which, when specified, will enable/disable thread_local regardless of the value of glibc_compat. - FreeBSD has a buggy thread_local, don't use it.
dongcarl
force-pushed
the
2019-05-fix-thread_local-compat
branch
from
caaa942 to
480e341
Compare
Contributor
Author
|
Update: better help text. |
Member
|
utACK 480e341 |
Member
|
tACK 480e341 macOS: checking for thread_local support... no
src/bitcoind -logthreadnames=1
2019-05-23T20:27:48Z [] Bitcoin Core version v0.18.99.0-480e3415d (release build)
2019-05-23T20:27:48Z [] Assuming ancestors of block 0000000000000000000f1c54590ee18d15ec70e68c8cd4cfbadb1b4f11697eee have valid signatures.openBSD 6.4: checking for thread_local support... yes
./src/bitcoind -logthreadnames=1
2019-05-23T20:41:11Z [init] mapBlockIndex.size() = 1
2019-05-23T20:41:11Z [loadblk] Failed to open mempool file from disk. Continuing anyway.
......
2019-05-23T20:41:58Z [shutoff] [default wallet] Releasing wallet
2019-05-23T20:41:58Z [shutoff] Shutdown: doneFreeBSD 12.0: checking for thread_local support... no
src/bitcoind -logthreadnames=1
2019-05-24T16:06:38Z [] Bitcoin Core version v0.18.99.0-480e3415d (release build)
2019-05-24T16:06:38Z [] Assuming ancestors of block 0000000000000000000f1c54590ee18d15ec70e68c8cd4cfbadb1b4f11697eee have valid signatures.Debian 9.9: checking for thread_local support... yes
src/bitcoind -logthreadnames=1
src/bitcoind -logthreadnames=1
2019-05-24T17:07:14Z [init] Bitcoin Core version v0.18.99.0-480e3415d (release build)
2019-05-24T17:07:14Z [init] Assuming ancestors of block 0000000000000000000f1c54590ee18d15ec70e68c8cd4cfbadb1b4f11697eee have valid signatures. |
maflcko
pushed a commit
that referenced
this pull request
May 25, 2019
480e341 configure: Add flag for enabling thread_local. (Carl Dong) Pull request description: - When aiming for glibc compatibility, don't use thread_local. Fixes #15958. - FreeBSD has a buggy thread_local, don't use it. Fixes #16055. I've done a Gitian build on my local machine and the symbol tests seem to pass. ACKs for commit 480e34: MarcoFalke: utACK 480e341 fanquake: tACK 480e341 Tree-SHA512: 334f21f7cf271c261b115a6410afd4ed4db3e84ad79b98c6c684c1dfa42b081f16d58e77695929e27b0fa173a894b959a327fe82821a3f3ed708b305a906ddd3
18 tasks
This was referenced Apr 16, 2020
laanwj
added a commit
that referenced
this pull request
Apr 22, 2020
… not work b91e4ae Do not expose and consider -logthreadnames when it does not work (Hennadii Stepanov) Pull request description: There are conditions when the `HAVE_THREAD_LOCAL` macro is undefined what causes the `-logthreadnames` option does not work -- instead of thread names empty strings `[]` only are printed in the `debug.log` file. This PR does not exposes the `-logthreadnames` option in such cases. Refs: - #16059 - #18652 ACKs for top commit: MarcoFalke: ACK b91e4ae, looked at the diff, didn't test Tree-SHA512: 3bd58e5ea603c69686589ddc94d6fa441cab4f712004378f2f1661e12638804ca03cfb6426e6393e55b6a095b325f3161d3c5371af05d7fc79d6d328227bf40c
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Apr 23, 2020
…it does not work b91e4ae Do not expose and consider -logthreadnames when it does not work (Hennadii Stepanov) Pull request description: There are conditions when the `HAVE_THREAD_LOCAL` macro is undefined what causes the `-logthreadnames` option does not work -- instead of thread names empty strings `[]` only are printed in the `debug.log` file. This PR does not exposes the `-logthreadnames` option in such cases. Refs: - bitcoin#16059 - bitcoin#18652 ACKs for top commit: MarcoFalke: ACK b91e4ae, looked at the diff, didn't test Tree-SHA512: 3bd58e5ea603c69686589ddc94d6fa441cab4f712004378f2f1661e12638804ca03cfb6426e6393e55b6a095b325f3161d3c5371af05d7fc79d6d328227bf40c
linuxsh2
pushed a commit
to linuxsh2/dash
that referenced
this pull request
Aug 11, 2021
480e341 configure: Add flag for enabling thread_local. (Carl Dong) Pull request description: - When aiming for glibc compatibility, don't use thread_local. Fixes bitcoin#15958. - FreeBSD has a buggy thread_local, don't use it. Fixes bitcoin#16055. I've done a Gitian build on my local machine and the symbol tests seem to pass. ACKs for commit 480e34: MarcoFalke: utACK 480e341 fanquake: tACK 480e341 Tree-SHA512: 334f21f7cf271c261b115a6410afd4ed4db3e84ad79b98c6c684c1dfa42b081f16d58e77695929e27b0fa173a894b959a327fe82821a3f3ed708b305a906ddd3
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I've done a Gitian build on my local machine and the symbol tests seem to pass.