doc: update docs for `CHECK_ATOMIC` macro #28777

fanquake commented at 3:37 PM on November 2, 2023: member

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.

DrahtBot commented at 3:37 PM on November 2, 2023: contributor

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

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

DrahtBot added the label Build system on Nov 2, 2023

maflcko added the label DrahtBot Guix build requested on Nov 2, 2023

maflcko commented at 3:51 PM on November 2, 2023: member

It may be good to get matching guix hashes from different arches before merging this?

fanquake commented at 4:16 PM on November 2, 2023: member

Looks like it's still required for at least 32-bit Linux, using ~~GCC 11~~ Clang.

fanquake commented at 4:22 PM on November 2, 2023: member

Although not always, because the 32-bit CentOS build using GCC 11 compiled fine. Will adjust the docs.

fanquake marked this as a draft on Nov 2, 2023

doc: update docs for CHECK_ATOMIC macro

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.

ebc7063c80

fanquake force-pushed on Nov 2, 2023

DrahtBot added the label CI failed on Nov 2, 2023

fanquake renamed this:
~~build: remove `CHECK_ATOMIC`~~
doc: update docs for `CHECK_ATOMIC` macro
on Nov 2, 2023

fanquake marked this as ready for review on Nov 2, 2023

fanquake commented at 4:56 PM on November 2, 2023: member

Clang prior to version 15, when compiling for 32-bit, is the culprit here (I misread the CI). Have updated the docs.

maflcko commented at 5:20 PM on November 2, 2023: member

Jup, it is still needed. Steps to reproduce on a fresh install of bullseye:

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-zmq make automake cmake curl clang-13 llvm-13 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-13 -m32' CXX='clang++-13 -m32' --enable-fuzz --with-sanitizers=fuzzer && make -j $(nproc)

DrahtBot removed the label CI failed on Nov 2, 2023

DrahtBot commented at 5:55 AM on November 3, 2023: contributor

Guix builds (on x86_64)

File	commit 9b68c9b85efebfa23daec6471b87e9cbb514a006<br>(master)	commit 525ed4f03e71450d2b31bbf7877bc1ef9abdd480<br>(master and this pull)
SHA256SUMS.part	`053fee2c87199a7f...`	`7438564bfdb20be6...`
*-aarch64-linux-gnu-debug.tar.gz	`548602b10b8ddd5f...`	`76795d9f8072233e...`
*-aarch64-linux-gnu.tar.gz	`982e7b1e41ca3b06...`	`68770ea4c88e75e8...`
*-arm-linux-gnueabihf-debug.tar.gz	`6a9c04a5d007b5e3...`	`06fae5f22c51e4a6...`
*-arm-linux-gnueabihf.tar.gz	`88a06217c39a9c09...`	`c86407dc6f19b38f...`
*-arm64-apple-darwin-unsigned.tar.gz	`c353233a6ae44fd8...`	`9c2c4c6e87e91c40...`
*-arm64-apple-darwin-unsigned.zip	`aaacd6c86c651209...`	`e7503c9ff79eaa3f...`
*-arm64-apple-darwin.tar.gz	`74da972a4a6d739f...`	`fe60a23d709678e9...`
*-powerpc64-linux-gnu-debug.tar.gz	`6c6922e6fdbd68eb...`	`98638dc6920d2dcb...`
*-powerpc64-linux-gnu.tar.gz	`bc21d6e9071d5cc6...`	`7fc11c60657d60a0...`
*-powerpc64le-linux-gnu-debug.tar.gz	`94a578b2bdc54ba1...`	`0237e5d4dce93054...`
*-powerpc64le-linux-gnu.tar.gz	`92c7a2bd7897e9b2...`	`73e3683652ba2a40...`
*-riscv64-linux-gnu-debug.tar.gz	`125fb59a7b43e6d4...`	`be481b60a497deb4...`
*-riscv64-linux-gnu.tar.gz	`2f52fb07c57612dc...`	`4a716a3f438c55e9...`
*-x86_64-apple-darwin-unsigned.tar.gz	`57683be77c342c7c...`	`f6a8da1beef3d072...`
*-x86_64-apple-darwin-unsigned.zip	`4c03151a658554cd...`	`4bea7f743540ef52...`
*-x86_64-apple-darwin.tar.gz	`606dbf1a6dd07797...`	`b97f71017bdec1db...`
*-x86_64-linux-gnu-debug.tar.gz	`9b33618fcffc7f26...`	`156cae07d61ed69a...`
*-x86_64-linux-gnu.tar.gz	`e54d96622d9c1d02...`	`3afa2d45324bf83a...`
*.tar.gz	`779747874577c109...`	`acb477d68e67ea0d...`
guix_build.log	`7ca44361b0ca6fb9...`	`1c349a70dd7c8930...`
guix_build.log.diff		`dfc51c3f6221d8a5...`

DrahtBot removed the label DrahtBot Guix build requested on Nov 3, 2023

fanquake requested review from TheCharlatan on Nov 3, 2023

hebasto commented at 9:44 PM on November 8, 2023: member

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?

fanquake commented at 10:49 AM on November 9, 2023: member

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.

hebasto approved

hebasto commented at 2:24 PM on November 12, 2023: member

ACK ebc7063c80135dd6f3e7b9418e8f4bf217bd8db7.

fanquake commented at 9:59 AM on November 13, 2023: member

See also the CMake implementation of this check: https://github.com/hebasto/bitcoin/pull/50.

fanquake merged this on Nov 13, 2023

fanquake closed this on Nov 13, 2023

fanquake deleted the branch on Nov 13, 2023

hebasto referenced this in commit 61744854d1 on Nov 15, 2023

in build-aux/m4/l_atomic.m4:7 in ebc7063c80

   3 | @@ -4,8 +4,10 @@ dnl permitted in any medium without royalty provided the copyright notice
   4 |  dnl and this notice are preserved. This file is offered as-is, without any
   5 |  dnl warranty.
   6 |  
   7 | -# Some versions of gcc/libstdc++ require linking with -latomic if
   8 | -# using the C++ atomic library.
   9 | +# Clang prior to version 15, when building for 32-bit,

maflcko commented at 11:55 AM on January 3, 2024:

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)

PastaPastaPasta referenced this in commit 7b0e84f49c on Oct 24, 2024

PastaPastaPasta referenced this in commit 733fe560b4 on Oct 24, 2024

PastaPastaPasta referenced this in commit 34551de20f on Oct 24, 2024

PastaPastaPasta referenced this in commit ba97f57c70 on Oct 24, 2024

PastaPastaPasta referenced this in commit a9021db4ec on Oct 24, 2024

PastaPastaPasta referenced this in commit 0587790c01 on Oct 24, 2024

bitcoin locked this on Jan 2, 2025