configure: Fix thread_local detection #16059

pull dongcarl wants to merge 1 commits into bitcoin:master from dongcarl:2019-05-fix-thread_local-compat changing 1 files +49 −36
  1. dongcarl commented at 7:19 pm on May 20, 2019: member
    • 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.

  2. dongcarl added the label Bug on May 20, 2019
  3. dongcarl added the label Build system on May 20, 2019
  4. dongcarl added the label Needs gitian build on May 20, 2019
  5. MarcoFalke added this to the milestone 0.19.0 on May 20, 2019
  6. MarcoFalke commented at 7:34 pm on May 20, 2019: member

    ACK 3b549f4aae9bc5f5f9e9dee8476523daf44dd7ee (checked diff with –ignore-all-space, and verified that thread_local is now disabled on freebsd)

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 3b549f4aae9bc5f5f9e9dee8476523daf44dd7ee (checked diff with --ignore-all-space, and verified that thread_local is now disabled on freebsd)
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhhlwwAp1D8oI5zuBCaduML4SFgcShnTo6EeWUN6VrVQDj0Ja4d2iEvgog3iJP1
 8+KMwj0frmEKmoBxOSW3b72iTPRWT/TeYXgp/PgqAIvy8E+K6CCIdDyBY+c/2iZTc
 9PSN/GnDkHGNhJ8Cx+RCgB4Io6/df+44XgpvLNzBrFeLkuZhSE0RuzzwIG9NJ3lpY
10AEI/OAsiAZLZP23scVqo7z3T9rYM0uUtW5XpW98eJSYlQ+JFe0DE7/oKyeETqVRE
11GZ6XtAf+rVEZ/8awa7QBcfFmQ2qxYNQGPewudTVRR9//PhFBn2jCvmqDvg26+zA6
12CAkDDcfQb4NLIjvnviZxkQhaXUPdc+PaefCD6Pzx1qL0bUsY2zIUYnw1fV8uZ69K
13YTZBewMyJRDQE8oz0NB+9xSpftG8yUlY2H4Xrwo9IEBfGwGsITAc1dv61lfrGrDO
14hGC3bE8ucDb+IaClCtrBS3sOyxqRfS3BjrgjkmA4QUJYyXLsyvNYZeTWpcfhKopm
1583ydESNX
16=eGHB
17-----END PGP SIGNATURE-----

    Timestamp of file with hash cb3987fa66dbad9cd23f72f8b0cb42d860ccda592d0a20757c01c728c5eadf2e -

  7. jamesob approved
  8. jamesob commented at 8:01 pm on May 20, 2019: member

    utACK https://github.com/bitcoin/bitcoin/pull/16059/commits/3b549f4aae9bc5f5f9e9dee8476523daf44dd7ee

    To confirm: use_glibc_compat is by default disabled, correct?

  9. dongcarl commented at 8:03 pm on May 20, 2019: member

    @jamesob

    To confirm: use_glibc_compat is by default disabled, correct?

    Correct! https://github.com/bitcoin/bitcoin/blob/2d1583ee6aff4b68dc46bcb87eb7a85f90b465c3/configure.ac#L191-L195

  10. theuni commented at 8:04 pm on May 20, 2019: 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.
  11. dongcarl commented at 8:09 pm on May 20, 2019: member

    @theuni

    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.

    As in:

    1. Keep my current changes, add a --enable-threadlocal flag that will enable thread_local regardless of what --enable-glibc-back-compat says?
    2. Or… Keep my current changes, add a --enable-threadlocal flag that will fail the configure if --enable-glibc-back-compat is also there?
    3. Or… Remove the use_glibc_compat <-> thread_local interaction, add a --enable-threadlocal flag that will enable thread_local?
  12. MarcoFalke commented at 8:16 pm on May 20, 2019: 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.
  13. ryanofsky commented at 8:18 pm on May 20, 2019: member
    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.
  14. theuni commented at 8:33 pm on May 20, 2019: member

    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.

    +1

    Thread names give developers additional debug information.

    They had thread names before thread_local :)

  15. jamesob commented at 9:11 pm on May 20, 2019: member

    They had thread names before thread_local :)

    I think @MarcoFalke is referring to the [threadname] prefixes on each log line, which did not exist before the merge of #15849.

  16. dongcarl force-pushed on May 20, 2019
  17. dongcarl commented at 9:29 pm on May 20, 2019: member
    Implemented logic as documented here: #16059 (comment)
  18. in configure.ac:199 in caaa942a5f outdated 
    193@@ -194,6 +194,12 @@ AC_ARG_ENABLE([glibc-back-compat],
194   [use_glibc_compat=$enableval],
195   [use_glibc_compat=no])
196 
197+AC_ARG_ENABLE([threadlocal],
198+  [AS_HELP_STRING([--enable-threadlocal],
199+  [enable thread_local code])],
    ryanofsky commented at 9:42 pm on May 20, 2019:
    Maybe could have more descriptive help text “enable features that depend on the c++ thread_local keyword (currently just thread names in debug logs). If unspecified, this will be enabled if there is platform support and glibc-back-compat is not enabled”
  19. DrahtBot commented at 9:13 pm on May 21, 2019: member

    Gitian builds for commit 2d1583ee6aff4b68dc46bcb87eb7a85f90b465c3 (master):

    Gitian builds for commit 256e35348968dc048f175507999c466654294194 (master and this pull):

  20. DrahtBot removed the label Needs gitian build on May 21, 2019
  21. dongcarl added the label Needs gitian build on May 21, 2019
  22. DrahtBot commented at 1:28 pm on May 22, 2019: member

    Gitian builds for commit 3001cc61cf11e016c403ce83c9cbcfd3efcbcfd9 (master):

    Gitian builds for commit 3f02161dd63d4983ae2b356dfea3dd4e45ee1f4a (master and this pull):

  23. DrahtBot removed the label Needs gitian build on May 22, 2019
  24. MarcoFalke commented at 1:57 pm on May 22, 2019: member

    utACK caaa942a5ff372db723a224c35236df48b93ebcb

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3utACK caaa942a5ff372db723a224c35236df48b93ebcb
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgOcAwAqOf1q52e0OmUrUy+lg0FHlFZ/F4VXVN/NtRAa7wP1KO+GsQacvFuKlOT
 87T9V7KFqdRbj9RsxwXd77ZwXkhA7PDOn5Z8ja4GvnH/Z8KXOOKyPKJF9Bw6qtUH0
 97ghGVa1eawGNddId5WQfhdJygwjbuwPZbqkxY3dLW1puXXXV7+RkWnEz6c+eZG2b
10zJ84SlRd8BvGDcRqSoSGtH4kwXbfjYwQMk1+wt1j534/UgHq4ubHOPiSq38GMF2X
11PmK6LGlG0PO9sU4hV5a62k5aZ0cW22n3HlL6MVTODAkSci7Cfd2Mo84dNC8TevOU
12NU+J5wTaqDn84tLKRBWvPdtH3pb1L2WtKVPQ1sVCFrXFsguGnPhKy8r7rB8dgjkz
13efLYbXO8vGK1GRTqsFsyAsxi7/AMS4PyIG19nnazGw2WmVIqcRp7D6ZTE+uEpCyV
14Z7cB4UgtZo9MdrwJqmjkbHXD45NHjxvy5SsXtVRcWzQ2J50BsCn+rfLZxkc484jJ
15srQR/1SK
16=9lnG
17-----END PGP SIGNATURE-----

    Timestamp of file with hash 92094b899f3571c01b98c4d14e9586500a17402059ac90ca5200d93dc689eef3 -

  25. dongcarl commented at 3:38 pm on May 22, 2019: member
    From the @DrahtBot gitian builds, looks like bitcoin-core-linux-0.19-res.ymls are coming back again! :tada:
  26. sipa commented at 6:32 pm on May 22, 2019: member
    Concept ACK
  27. ryanofsky approved
  28. ryanofsky commented at 5:20 pm on May 23, 2019: member
    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.
  29. laanwj added this to the "Blockers" column in a project
  30. configure: Add flag for enabling thread_local. 
    - 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.
    480e3415d7
  31. dongcarl force-pushed on May 23, 2019
  32. dongcarl commented at 7:16 pm on May 23, 2019: member
    Update: better help text.
  33. MarcoFalke commented at 11:24 am on May 24, 2019: member
    utACK 480e3415d738a4e0e2ad9774c43f29937178ecae
  34. fanquake commented at 5:07 pm on May 24, 2019: member

    tACK 480e341

    macOS:

    0checking for thread_local support... no
1src/bitcoind -logthreadnames=1
22019-05-23T20:27:48Z [] Bitcoin Core version v0.18.99.0-480e3415d (release build)
32019-05-23T20:27:48Z [] Assuming ancestors of block 0000000000000000000f1c54590ee18d15ec70e68c8cd4cfbadb1b4f11697eee have valid signatures.

    openBSD 6.4:

    0checking for thread_local support... yes
1./src/bitcoind -logthreadnames=1
22019-05-23T20:41:11Z [init] mapBlockIndex.size() = 1
32019-05-23T20:41:11Z [loadblk] Failed to open mempool file from disk. Continuing anyway.
4......
52019-05-23T20:41:58Z [shutoff] [default wallet] Releasing wallet
62019-05-23T20:41:58Z [shutoff] Shutdown: done

    FreeBSD 12.0:

    0checking for thread_local support... no
1src/bitcoind -logthreadnames=1
22019-05-24T16:06:38Z [] Bitcoin Core version v0.18.99.0-480e3415d (release build)
32019-05-24T16:06:38Z [] Assuming ancestors of block 0000000000000000000f1c54590ee18d15ec70e68c8cd4cfbadb1b4f11697eee have valid signatures.

    Debian 9.9:

    0checking for thread_local support... yes
1src/bitcoind -logthreadnames=1
2src/bitcoind -logthreadnames=1
32019-05-24T17:07:14Z [init] Bitcoin Core version v0.18.99.0-480e3415d (release build)
42019-05-24T17:07:14Z [init] Assuming ancestors of block 0000000000000000000f1c54590ee18d15ec70e68c8cd4cfbadb1b4f11697eee have valid signatures.
  35. MarcoFalke merged this on May 25, 2019
  36. MarcoFalke closed this on May 25, 2019
  37. MarcoFalke referenced this in commit e043bfce68 on May 25, 2019
  38. fanquake removed this from the "Blockers" column in a project
  39. laanwj referenced this in commit acb4fa0741 on Apr 22, 2020
  40. sidhujag referenced this in commit 36106e9ef0 on Apr 23, 2020
  41. linuxsh2 referenced this in commit 3c6ae97332 on Aug 11, 2021
  42. DrahtBot locked this on Dec 16, 2021


dongcarl MarcoFalke jamesob theuni ryanofsky DrahtBot sipa fanquake

Labels
Bug Build system

Milestone
0.19.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 18:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me