build: Restrict check for CRC32C intrinsic to aarch64

laanwj commented at 10:27 am on September 20, 2021: member

crc32c’s hardware accelerated code doesn’t handle ARM 32-bit at all, but only 64-bit. Make the check in configure.ac check for this architecture explicitly. This change does not affect non-ARM architectures.

For the release binaries, the current configure.ac check happens to work: it enables it on aarch64 but disables it for armhf. However some combination of compiler version and settings can cause this check to succeed on armhf (as reported on IRC). So make the 64-bit platform requirement explicit.

(details: while the check already explicitly checks the __crc32d intrinsic, which strictly doesn’t exist on 32-bit ARM, this is not enough! gcc happens to helpfully emulate it:

These built-in intrinsics for the ARMv8-A CRC32 extension are available when the -march=armv8-a+crc switch is used “uint32_t __crc32d (uint32_t, uint64_t)” Form of expected instruction(s): Two crc32w r0, r0, r0 instructions for AArch32

)

laanwj added the label Build system on Sep 20, 2021

laanwj added the label Needs backport (22.x) on Sep 20, 2021

laanwj commented at 10:28 am on September 20, 2021: member

Thanks to @luke-jr for reporting this.

fanquake added the label DrahtBot Guix build requested on Sep 20, 2021

laanwj force-pushed on Sep 20, 2021

laanwj added the label Needs backport (0.21) on Sep 20, 2021

luke-jr commented at 2:07 pm on September 20, 2021: member

ARM “emulates” it with two crc32w from what you said on IRC. But is that actually slower than the fallback non-assembly code? If it performs better, maybe we’d be better off making it work (as it apparently did in 0.21)

in configure.ac:571 in f6a7ed55c9 outdated

567@@ -568,13 +568,17 @@ AX_CHECK_COMPILE_FLAG([-march=armv8-a+crc+crypto],[[ARM_CRC_CXXFLAGS="-march=arm
568 
569 TEMP_CXXFLAGS="$CXXFLAGS"
570 CXXFLAGS="$CXXFLAGS $ARM_CRC_CXXFLAGS"
571-AC_MSG_CHECKING(for ARM CRC32 intrinsics)
572+AC_MSG_CHECKING(for ARM64 CRC32 intrinsics)

luke-jr commented at 2:07 pm on September 20, 2021:

nit: AArch64 is a more accurate name, since it’s not really related to the 32-bit ARM architecture AIUI.

laanwj commented at 2:54 pm on September 20, 2021:

Sure, fine with changing the name, though what crc32c library uses internally is ARM64 so I mimiced that.

laanwj commented at 2:53 pm on September 20, 2021: member

Tested the GUIX build:

0$ aarch64-linux-gnu-objdump -d 64/bitcoin-f6a7ed55c9bc/bin/bitcoind |grep crc32 | wc
1    136     816    5341
2$ arm-linux-gnueabihf-objdump -d 32/bitcoin-f6a7ed55c9bc/bin/bitcoind |grep crc32 | wc
3      0       0       0

ARM “emulates” it with two crc32w from what you said on IRC. But is that actually slower than the fallback non-assembly code? If it performs better, maybe we’d be better off making it work (as it apparently did in 0.21)

No, it can never have worked like this. I’m obviously fine with someone adapting crc32c’s hardware acceleration to work on 32-bit ARM, but be aware that 32-bit hardware with support for CRC32C is exceedingly rare. It’s probably why Google didn’t bother.

Until then this is the fix to make it at least not crash.

luke-jr commented at 11:13 pm on September 20, 2021: member

tACK f6a7ed55c9bc16fcf48d3d4b957170f12393049e (also ACK AArch64 rename variant thereof)

build: Restrict check for CRC32C intrinsic to aarch64

`crc32c`'s hardware accelerated code doesn't handle ARM 32-bit at all.
Make the check in `configure.ac` check for this architecture explicitly.

For the release binaries, the current `configure.ac` check happens
to work: it enables it on aarch64 but disables it for armhf. However
some combination of compiler version and settings might ostensibly cause
this check to succeed on armhf (as reported on IRC). So make the 64-bit
platform requirement explicit.

f2747d1602

laanwj force-pushed on Sep 21, 2021

laanwj commented at 10:38 am on September 21, 2021: member

Force pushed, only change ARM64→Aarch64 f6a7ed55c9bc16fcf48d3d4b957170f12393049e…f2747d1602ec4e1128356b861b2167daf66a845b

DrahtBot commented at 1:19 am on September 22, 2021: contributor

Guix builds

File	commit 226731ac1144886d693d3508b331f98727ab883c(master)	commit ee8c3a19260a07455aa7c6fcdabfc459f30e6b2e(master and this pull)
SHA256SUMS.part	`3b0fb952af02fea7...`	`12e6ec1bfbd4bb60...`
*-aarch64-linux-gnu-debug.tar.gz	`ebddd981105cda54...`	`29cee6f0fd831056...`
*-aarch64-linux-gnu.tar.gz	`06ed68405650507a...`	`43ca1309f2a661b7...`
*-arm-linux-gnueabihf-debug.tar.gz	`171f501c0774635d...`	`4721a4f9ed0bcd77...`
*-arm-linux-gnueabihf.tar.gz	`6d4934aa9df9d38f...`	`ea0154146a4a5faa...`
*-osx-unsigned.dmg	`e4263f82001b2a20...`	`52546c3edfac3147...`
*-osx-unsigned.tar.gz	`dcc64c104b66b92b...`	`db54064edb3980c3...`
*-osx64.tar.gz	`1ccdb0871e9ec7df...`	`567ca71dd6845046...`
*-powerpc64-linux-gnu-debug.tar.gz	`30ccf10d5d3f946c...`	`3b6821d55e6832e0...`
*-powerpc64-linux-gnu.tar.gz	`ae28a2385243126d...`	`936b440ac751070d...`
*-powerpc64le-linux-gnu-debug.tar.gz	`b6a28f795856c441...`	`4b8367c3aa9f8f7a...`
*-powerpc64le-linux-gnu.tar.gz	`17b6fc3521884b9c...`	`a6845022b493786e...`
*-riscv64-linux-gnu-debug.tar.gz	`80a16cb8b0d6b45d...`	`6c1cc3ec97baffc8...`
*-riscv64-linux-gnu.tar.gz	`d1d22dd4669869a0...`	`3cdd1d8b0f6aa119...`
*-win-unsigned.tar.gz	`42dcdb895a2b48ee...`	`68414f3889b0bffc...`
*-win64-debug.zip	`0ba1f6dbbfcd40e2...`	`e42f9037b72ac6dc...`
*-win64-setup-unsigned.exe	`764532404d55f7d9...`	`47f17865c19f905d...`
*-win64.zip	`4a57723d8a0c5684...`	`9c47c6bc39d3d56e...`
*-x86_64-linux-gnu-debug.tar.gz	`46f79071f4053088...`	`af58d5d12b7134d7...`
*-x86_64-linux-gnu.tar.gz	`aa2063637f637fcb...`	`cc0aca53afe108bf...`
*.tar.gz	`a8c6814a55f7445e...`	`ae4d27c4eb4e79c2...`
guix_build.log	`511b84519ce45cdd...`	`fa5684498ce1757c...`
guix_build.log.diff		`c495b251a38153df...`

DrahtBot removed the label DrahtBot Guix build requested on Sep 22, 2021

luke-jr commented at 1:56 pm on September 22, 2021: member

re-tACK f2747d1602ec4e1128356b861b2167daf66a845b

jarolrod commented at 8:56 pm on September 22, 2021: member

ACK f2747d1602ec4e1128356b861b2167daf66a845b

Not to familiar with the inner workings here, but I read up a bit on this. Additionally, I ran a GUIX build successfully on this PR branch, and subsequently tested the ARM binary on a 32-bit beaglebone by running bitcoind.

GUIX hashes:

0$ env HOSTS='arm-linux-gnueabihf aarch64-linux-gnu' ./contrib/guix/guix-build
1$ find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
2
35b1885691d142f4a37b5823380c5818428cf90182e30673c410c29edbcc137ca  guix-build-f2747d1602ec/output/aarch64-linux-gnu/SHA256SUMS.part
459e1cf3d773a4ea046ab4f6aec790c1d90a96ed2e70e9c2c7b8bd88d5dbeac75  guix-build-f2747d1602ec/output/aarch64-linux-gnu/bitcoin-f2747d1602ec-aarch64-linux-gnu-debug.tar.gz
5b692531334d33cc356772b0813f4497880b00f978623a15919697c41e66c2b58  guix-build-f2747d1602ec/output/aarch64-linux-gnu/bitcoin-f2747d1602ec-aarch64-linux-gnu.tar.gz
661f403c56ee0bdeaeb7af8859057adae52c7acb02fd55c822c33ec186ea51880  guix-build-f2747d1602ec/output/arm-linux-gnueabihf/SHA256SUMS.part
77f87851a8c5adde3bdcdecbd9bcd1c6631540def1d616297a3c542373cb9a412  guix-build-f2747d1602ec/output/arm-linux-gnueabihf/bitcoin-f2747d1602ec-arm-linux-gnueabihf-debug.tar.gz
8fdf5ae161a5314fe9119ebb49e9b2344be8b8cd7465a20047632b06419f5fa03  guix-build-f2747d1602ec/output/arm-linux-gnueabihf/bitcoin-f2747d1602ec-arm-linux-gnueabihf.tar.gz
9fb6f2ebe3f30608c3e05baac3373aeea41032515cedc482b88efe0136377a901  guix-build-f2747d1602ec/output/dist-archive/bitcoin-f2747d1602ec.tar.gz

laanwj merged this on Sep 23, 2021

laanwj closed this on Sep 23, 2021

sidhujag referenced this in commit 767fcf9d8e on Sep 24, 2021

luke-jr referenced this in commit 61248e3ff7 on Oct 10, 2021

fanquake referenced this in commit 7ac6c54fdf on Oct 14, 2021

fanquake removed the label Needs backport (22.x) on Oct 14, 2021

fanquake commented at 2:54 am on October 14, 2021: member

Being backported to 22.x in #23276.

fanquake referenced this in commit 43e186361b on Oct 15, 2021

fanquake referenced this in commit 019753513e on Oct 21, 2021

fanquake referenced this in commit aace7cc74a on Feb 14, 2022

fanquake referenced this in commit 85c78e08ec on Feb 15, 2022

fanquake referenced this in commit 9b5f674abb on Mar 1, 2022

PastaPastaPasta referenced this in commit d03696ca67 on Mar 13, 2022

fanquake referenced this in commit efb9f00f07 on Jun 9, 2022

fanquake removed the label Needs backport (0.21) on Jun 9, 2022

fanquake commented at 11:57 am on June 9, 2022: member

Backported to 0.21 in #25318.

fanquake referenced this in commit dca463bd81 on Jun 10, 2022

hebasto commented at 11:11 am on June 22, 2022: member

Should it be submitted to the upstream:

0check_cxx_source_compiles("
1#include <arm_acle.h>
2#include <arm_neon.h>
3
4int main() {
5  __crc32cb(0, 0); __crc32ch(0, 0); __crc32cw(0, 0); __crc32cd(0, 0);
6  vmull_p64(0, 0);
7  return 0;
8}
9" HAVE_ARM64_CRC32C)

?

laanwj commented at 11:21 am on June 22, 2022: member

Maybe! If they have the same check, and haven’t actually fixed ARM32 support, they’ll have the same problem.

delta1 referenced this in commit 6313268cc0 on Apr 23, 2023

hebasto commented at 5:57 pm on June 5, 2023: member

Should it be submitted to the upstream?

Maybe! If they have the same check, and haven’t actually fixed ARM32 support, they’ll have the same problem.

Done in https://github.com/google/crc32c/pull/61.

gades referenced this in commit 20ef1b28e0 on Nov 9, 2023

gades referenced this in commit 31c61f7d00 on Dec 9, 2023

bitcoin locked this on Jun 4, 2024

build: Restrict check for CRC32C intrinsic to aarch64 #23045

Guix builds