doc: update docs for CHECK_ATOMIC macro
#28777
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. |
|
It may be good to get matching guix hashes from different arches before merging this? |
|
Looks like it's still required for at least 32-bit Linux, using |
|
Although not always, because the 32-bit CentOS build using GCC 11 compiled fine. Will adjust the docs. |
Clarify that supported versions of GCC are not affected, and that Clang prior to version 15 still requires the explicit -latomic linking, when compiling for 32-bit.
fanquake
force-pushed
the
drop_explicit_libatomic
branch
from
d46908c to
ebc7063
Compare
fanquake
changed the title
CHECK_ATOMICCHECK_ATOMIC macro
|
Clang prior to version 15, when compiling for 32-bit, is the culprit here (I misread the CI). Have updated the docs. |
|
Jup, it is still needed. Steps to reproduce on a fresh install of bullseye:
|
|
Not directly related to this PR patch, but... At some point in the future, the Bitcoin Core project will drop support for i686 platforms. It looks unreasonable to run the modern Bitcoin Core on CPUs without instruction sets for h/w optimizations etc. And the market is full of cheap alternatives. By what criteria will we make such a decision? Could this moment be now? |
Feel free to open an issue for broader discussion. Given the overhead of supporting it is low, I don't see a pressing reason to do so. There is also a difference between us officially dropping support, and purposefully breaking the ability to compile for certain targets. |
|
See also the CMake implementation of this check: hebasto#50. |
b2cfe67 ci: Test CMake edge cases (Hennadii Stepanov) ee2ac14 fixup! cmake: Redefine configuration flags (Hennadii Stepanov) 7f2d751 fixup! cmake: Check system symbols (Hennadii Stepanov) b8a01cf fixup! cmake: Check system symbols (Hennadii Stepanov) 1340921 fixup! cmake: Check system symbols (Hennadii Stepanov) 4b198e4 fixup! cmake: Warn about not encapsulated build properties (Hennadii Stepanov) c92c4c1 fixup! cmake: Warn about not encapsulated build properties (Hennadii Stepanov) Pull request description: 1. Added an analogue of https://github.com/bitcoin/bitcoin/blob/master/build-aux/m4/l_atomic.m4. 2. Included changes from bitcoin#28777. 3. Added (temporarily?) a test case to the GHA CI to avoid regressions in the future. Might be tested, for example, as follows: ``` make -j $(nproc) -C depends HOST=i686-pc-linux-gnu CC="clang -m32" CXX="clang++ -m32" NO_QT=1 NO_WALLET=1 NO_ZMQ=1 NO_NATPMP=1 NO_UPNP=1 NO_USDT=1 cmake -B build --toolchain depends/i686-pc-linux-gnu/share/toolchain.cmake cmake --build build -j $(nproc) -t bitcoind ``` Here is an excerpt from the CI [log](https://github.com/hebasto/bitcoin/actions/runs/6842095586/job/18603196018): ``` -- Performing Test STD_ATOMIC_LINKS_WITHOUT_LIBATOMIC -- Performing Test STD_ATOMIC_LINKS_WITHOUT_LIBATOMIC - Failed -- Performing Test STD_ATOMIC_NEEDS_LINK_TO_LIBATOMIC -- Performing Test STD_ATOMIC_NEEDS_LINK_TO_LIBATOMIC - Success ``` Top commit has no ACKs. Tree-SHA512: 3af1524dc0f8e06c8b1f5398417c3d53cd7d1fb41ffd6602407b6c79d7d891784447832f443434998b524b9a7c3360ff07c5396d660aa6f9185718fa2af4a3e8
| @@ -4,8 +4,10 @@ dnl permitted in any medium without royalty provided the copyright notice | |||
| dnl and this notice are preserved. This file is offered as-is, without any | |||
| dnl warranty. | |||
|
|
|||
| # Some versions of gcc/libstdc++ require linking with -latomic if | |||
| # using the C++ atomic library. | |||
| # Clang prior to version 15, when building for 32-bit, | |||
There was a problem hiding this comment.
Same with clang-17. Steps to reproduce on a fresh install of Ubuntu 24.04:
CXXLD test/fuzz/fuzz
/usr/bin/ld: libbitcoin_node.a(libbitcoin_node_a-net_processing.o): in function `(anonymous namespace)::PeerManagerImpl::BlockConnected(ChainstateRole, std::shared_ptr<CBlock const> const&, CBlockIndex const*)':
net_processing.cpp:(.text._ZN12_GLOBAL__N_115PeerManagerImpl14BlockConnectedE14ChainstateRoleRKSt10shared_ptrIK6CBlockEPK11CBlockIndex[_ZN12_GLOBAL__N_115PeerManagerImpl14BlockConnectedE14ChainstateRoleRKSt10shared_ptrIK6CBlockEPK11CBlockIndex]+0x1ef): undefined reference to `__atomic_compare_exchange'
/usr/bin/ld: libbitcoin_node.a(libbitcoin_node_a-net_processing.o): in function `(anonymous namespace)::PeerManagerImpl::SendMessages(CNode*)':
net_processing.cpp:(.text._ZN12_GLOBAL__N_115PeerManagerImpl12SendMessagesEP5CNode[_ZN12_GLOBAL__N_115PeerManagerImpl12SendMessagesEP5CNode]+0x6755): undefined reference to `__atomic_compare_exchange'
clang++-17: error: linker command failed with exit code 1 (use -v to see invocation)
export DEBIAN_FRONTEND=noninteractive && apt update && apt install curl wget htop git vim ccache -y && git clone https://github.com/bitcoin/bitcoin.git --depth=1 && cd bitcoin && apt install build-essential libtool autotools-dev automake pkg-config bsdmainutils python3 make automake cmake curl clang-17 llvm-17 g++-multilib libtool binutils-gold bsdmainutils pkg-config python3 patch bison -y && ( cd depends && make DEBUG=1 HOST=i686-pc-linux-gnu NO_QT=1 NO_WALLET=1 NO_ZMQ=1 NO_UPNP=1 NO_NATPMP=1 -j $(nproc) ) && ./autogen.sh && CONFIG_SITE="$PWD/depends/i686-pc-linux-gnu/share/config.site" ./configure CC='clang-17 -m32' CXX='clang++-17 -m32' --enable-fuzz --with-sanitizers=fuzzer && make -j $(nproc)
ebc7063 doc: update docs for CHECK_ATOMIC macro (fanquake) Pull request description: Clarify that supported versions of GCC are not affected, and that Clang prior to version 15 still requires the explicit `-latomic` linking, when compiling for 32-bit. ACKs for top commit: hebasto: ACK ebc7063. Tree-SHA512: 6044dc28547431cfde7e89b663b5f9a86a4cb801212a21c3dbb18a1c41a53640480c3e4e944050dc3ec4cded9bc4c1f8eae8dbb60596289fef49bb13a8b53b76
ebc7063 doc: update docs for CHECK_ATOMIC macro (fanquake) Pull request description: Clarify that supported versions of GCC are not affected, and that Clang prior to version 15 still requires the explicit `-latomic` linking, when compiling for 32-bit. ACKs for top commit: hebasto: ACK ebc7063. Tree-SHA512: 6044dc28547431cfde7e89b663b5f9a86a4cb801212a21c3dbb18a1c41a53640480c3e4e944050dc3ec4cded9bc4c1f8eae8dbb60596289fef49bb13a8b53b76
ebc7063 doc: update docs for CHECK_ATOMIC macro (fanquake) Pull request description: Clarify that supported versions of GCC are not affected, and that Clang prior to version 15 still requires the explicit `-latomic` linking, when compiling for 32-bit. ACKs for top commit: hebasto: ACK ebc7063. Tree-SHA512: 6044dc28547431cfde7e89b663b5f9a86a4cb801212a21c3dbb18a1c41a53640480c3e4e944050dc3ec4cded9bc4c1f8eae8dbb60596289fef49bb13a8b53b76
ebc7063 doc: update docs for CHECK_ATOMIC macro (fanquake) Pull request description: Clarify that supported versions of GCC are not affected, and that Clang prior to version 15 still requires the explicit `-latomic` linking, when compiling for 32-bit. ACKs for top commit: hebasto: ACK ebc7063. Tree-SHA512: 6044dc28547431cfde7e89b663b5f9a86a4cb801212a21c3dbb18a1c41a53640480c3e4e944050dc3ec4cded9bc4c1f8eae8dbb60596289fef49bb13a8b53b76
ebc7063 doc: update docs for CHECK_ATOMIC macro (fanquake) Pull request description: Clarify that supported versions of GCC are not affected, and that Clang prior to version 15 still requires the explicit `-latomic` linking, when compiling for 32-bit. ACKs for top commit: hebasto: ACK ebc7063. Tree-SHA512: 6044dc28547431cfde7e89b663b5f9a86a4cb801212a21c3dbb18a1c41a53640480c3e4e944050dc3ec4cded9bc4c1f8eae8dbb60596289fef49bb13a8b53b76
c75a0d4 Merge bitcoin#29177: build: Fix check whether `-latomic` needed (fanquake) f670118 Merge bitcoin#28851: build: Patch Qt to handle minimum macOS version properly (fanquake) 685ee8a Merge bitcoin#28884: doc: remove x86_64 build assumption from depends doc (fanquake) 47f6126 Merge bitcoin#28881: doc: remove mention of missing bdb being a configure error (fanquake) a9021db Merge bitcoin#28777: doc: update docs for `CHECK_ATOMIC` macro (fanquake) d5e15df Merge bitcoin#26839: Add support for RNDR/RNDRRS for AArch64 on Linux (Andrew Chow) 5aedcbf Merge bitcoin#28778: depends: drop -O1 workaround from arm64 apple Qt build (fanquake) 95a8d8c Merge bitcoin#21161: Fee estimation: extend bucket ranges consistently (glozow) f4ea48e Merge bitcoin#28693: build: Include `config/bitcoin-config.h` explicitly in `util/trace.h` (fanquake) f160e0d Merge bitcoin#28691: refactor: Remove CBlockFileInfo::SetNull (fanquake) 0278163 Merge bitcoin#28697: fuzz: Increase merge -rss_limit_mb (fanquake) 90a1fb0 Merge bitcoin#28650: fuzz: Merge with -set_cover_merge=1 (fanquake) f007abd Merge bitcoin#28459: build: add `-mbranch-protection=bti` (aarch64) to hardening flags (fanquake) af8d124 Merge bitcoin#28624: docs: fix typo (fanquake) c740264 Merge bitcoin#28532: qt: enable` -ltcg` for windows under LTO (fanquake) ccd3920 Merge bitcoin#28556: doc: fix link to developer-notes.md file in multiprocess.md (fanquake) Pull request description: ## Issue being fixed or feature implemented Batch of trivial backports ## What was done? See commits ## How Has This Been Tested? built locally; large combined merge passed tests locally ## Breaking Changes Should be none ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK c75a0d4 Tree-SHA512: 035dc3fa9812c7f381946ae4798b8e729a58b38a090d94502a8d992e9cfaab3307173c602d7b782c637a79c5c41b62570dc73bb4bb367e4505a039964926181b
Clarify that supported versions of GCC are not affected, and that Clang
prior to version 15 still requires the explicit
-latomiclinking, whencompiling for 32-bit.