build: remove confusing and inconsistent disable-asm option

theuni commented at 8:21 pm on February 7, 2024: member

It didn’t actually disable asm usage in our code. Regardless of the setting, asm is used in random.cpp and support/cleanse.cpp.
The value wasn’t forwarded to libsecp as a user might have reasonably expected.
We now have the DISABLE_OPTIMIZED_SHA256 define which is what disable-asm actually did in practice.

If there is any desire, we can hook DISABLE_OPTIMIZED_SHA256 up to a new configure option that actually does what it says.

Additionally, this is one of the last (THE last?) remaining uses of autoconf defines in our crypto code. As such it seems like low-hanging fruit.

DrahtBot commented at 8:21 pm on February 7, 2024: 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	fanquake
Concept ACK	TheCharlatan, dergoegge, epiccurious, hebasto, Empact

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

theuni commented at 8:22 pm on February 7, 2024: member

Ping @hebasto @fanquake @TheCharlatan for buildsystem eyes. Ping @sipa for crypto concept ACK/NACK.

theuni commented at 8:24 pm on February 7, 2024: member

Also ping @dergoegge for fuzzer interactions ACK/NACK.

maflcko added the label DrahtBot Guix build requested on Feb 8, 2024

TheCharlatan commented at 12:13 pm on February 8, 2024: contributor

Concept ACK

dergoegge commented at 3:58 pm on February 8, 2024: member

Concept ACK

We only use --with-asm=no for the MSan builds in oss-fuzz but that only applies to secp, so we should be fine on that front.

Our docs (developer-notes.md and fuzzing.md) mention the use of --disable-asm to avoid address sanitizer failures. I’ve not used --disable-asm in any of my fuzzing and have not observed any ASan failures but they might depend on the specific sha256 optimizations used? Could also just be a relic of the past…

If there is any desire, we can hook DISABLE_OPTIMIZED_SHA256 up to a new configure option that actually does what it says.

I think we’re good for now but it probably makes sense if anyone runs into the ASan failures mentioned above.

maflcko commented at 4:22 pm on February 8, 2024: member

Our docs (developer-notes.md and fuzzing.md) mention the use of --disable-asm to avoid address sanitizer failures. I’ve not used --disable-asm in any of my fuzzing and have not observed any ASan failures but they might depend on the specific sha256 optimizations used? Could also just be a relic of the past…

This is macOS specific? It may be good if someone tested or bisected this.

theuni commented at 5:01 pm on February 8, 2024: member

@dergoegge Ah, thanks for the correction. The option does indeed currently pass through to secp, so that bullet-point in my commit message is wrong and I’ll update it.

It still forwards just fine with our option removed though, so I don’t think that changes anything here.

DrahtBot commented at 7:26 pm on February 8, 2024: contributor

Guix builds (on x86_64)

File	commit 801ef07ebd72fcd6544dcfb60536efd3a88178c1(master)	commit 83449332f95c0bb0891caf3080af12c1ba98b98e(master and this pull)
SHA256SUMS.part	`8285648633ad7851...`	`37ff7794fb79acdd...`
*-aarch64-linux-gnu-debug.tar.gz	`649c9cb86efb468c...`	`b55b7ad5de02c43c...`
*-aarch64-linux-gnu.tar.gz	`e9ecb75f6a291ae4...`	`7fe83b3fd375a894...`
*-arm-linux-gnueabihf-debug.tar.gz	`d8baacca70f003d0...`	`a98f8958e2d5b3a9...`
*-arm-linux-gnueabihf.tar.gz	`e8a4fa6cecea985a...`	`c6fcaed970bdba17...`
*-arm64-apple-darwin-unsigned.tar.gz	`04239fa625ee4657...`	`b140b7ed04be163a...`
*-arm64-apple-darwin-unsigned.zip	`524e2989621d68cb...`	`e2997e9e8250fdc1...`
*-arm64-apple-darwin.tar.gz	`4fc2a9e14c684b91...`	`1f2c361a6947dafc...`
*-powerpc64-linux-gnu-debug.tar.gz	`b5d59398a428b5ea...`	`f07eefc463836276...`
*-powerpc64-linux-gnu.tar.gz	`50797bfdeb4e5631...`	`9a50f17938cae2b9...`
*-powerpc64le-linux-gnu-debug.tar.gz	`bd4623e017ab2ba0...`	`0b0552ee63142ea3...`
*-powerpc64le-linux-gnu.tar.gz	`465eeafe1e6fdbdf...`	`68e9b7185d3580ab...`
*-riscv64-linux-gnu-debug.tar.gz	`18ef159d48797904...`	`64275cc1d83edb3d...`
*-riscv64-linux-gnu.tar.gz	`e4b1964213f49bd8...`	`8e87a63a9a6c2836...`
*-x86_64-apple-darwin-unsigned.tar.gz	`2b5cacdc02803eef...`	`22442c7e846b4471...`
*-x86_64-apple-darwin-unsigned.zip	`ef6820c80bc4b7e3...`	`451a4ea68e9bb62b...`
*-x86_64-apple-darwin.tar.gz	`ebdf99fd1bf2f454...`	`55c0ee2d8ff7b399...`
*-x86_64-linux-gnu-debug.tar.gz	`fe723f6279a5cabc...`	`db7fa25fc4ffcc8e...`
*-x86_64-linux-gnu.tar.gz	`2b436e79610795d9...`	`e05224842238bfb0...`
*.tar.gz	`b35916ce901abca7...`	`31ef0a2c1de590cc...`
guix_build.log	`5d758151bc432a49...`	`194a8178ec134672...`
guix_build.log.diff		`58602d0d24d6a6f8...`

DrahtBot removed the label DrahtBot Guix build requested on Feb 8, 2024

jonatack commented at 9:24 pm on February 8, 2024: contributor

My usual fuzz configure for ARM64 macOS is the one in doc/fuzzing.md in the macOS hints for libFuzzer section.

Just tried building (on master) with and without the --disable-asm option and then running FUZZ=random ./src/test/fuzz/fuzz.

The one difference I notice without --disable-asm is this additional entry near the beginning of the fuzz run (after which the fuzzer continues to run):

0crc32c/src/crc32c_arm64.cc:101:26: runtime error: load of misaligned address 0x62d000000406 for type 'uint64_t' (aka 'unsigned long long'), which requires 8 byte alignment
10x62d000000406: note: pointer points here
2 b9 c5 22 00 01 01  1a 6c 65 76 65 6c 64 62  2e 42 79 74 65 77 69 73  65 43 6f 6d 70 61 72 61  74 6f
3             ^ 
4SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior crc32c/src/crc32c_arm64.cc:101:26 in

dergoegge commented at 9:36 pm on February 8, 2024: member

@jonatack see #29178 (we prefer having this broken over reverting the change that introduced it for some reason)

I’m not sure if the random harness would trigger the ASan error, the docs mention sha256 which random does not use afaict. Could you try txorphan or a different harness that does some hashing?

Did you set UBSAN_OPTIONS to include halt_on_error=1? that is likely why it keeps running if you didn’t

epiccurious commented at 3:07 pm on February 9, 2024: none

Concept ACK 0eec878e514c23d33d2cd81c44566ea601eb263f.

TheCharlatan commented at 5:40 pm on February 9, 2024: contributor

Do the mentiones from the doc/fuzzing.md to --disable-asm still need to be removed? Seems like --disable-asm is not recognized by libsecp, it supports a package-specific asm option.

theuni commented at 9:48 pm on February 12, 2024: member

@dergoegge Ah, thanks for the correction. The option does indeed currently pass through to secp, so that bullet-point in my commit message is wrong and I’ll update it.

Sorry for the back-and-forth, but I confused myself here. --disable-asm doesn’t have any effect on libsecp, but --without-asm does. No need to update the PR description.

It still forwards just fine with our option removed though, so I don’t think that changes anything here.

I got this wrong but it still accidentally made sense, heh. To be clear, passing --without-asm will still do the same as before, which is to disable asm-optimized libsecp without affecting Core at all. It was never “forwarded” in the sense that we interpreted and copied it to libsecp, rather it passed through untouched and still will.

All that confusion just seems like more justification for removal imo :)

theuni commented at 11:05 pm on February 12, 2024: member

The one difference I notice without --disable-asm is this additional entry near the beginning of the fuzz run (after which the fuzzer continues to run):

@jonatack Since you can reproduce, any interest in testing this theory https://github.com/bitcoin-core/crc32c-subtree/pull/6#issuecomment-1936095430 ?

Edit: nevermind. Reproduced and explained here: https://github.com/bitcoin-core/crc32c-subtree/pull/6#issuecomment-1942698611

fanquake commented at 12:58 pm on February 13, 2024: member

Concept ACK

hebasto commented at 11:30 am on February 15, 2024: member

Concept ACK.

in src/Makefile.am:53 in 0eec878e51 outdated

49@@ -50,10 +50,8 @@ LIBBITCOIN_WALLET_TOOL=libbitcoin_wallet_tool.a
50 endif
51 
52 LIBBITCOIN_CRYPTO = $(LIBBITCOIN_CRYPTO_BASE)
53-if USE_ASM
54 LIBBITCOIN_CRYPTO_SSE4 = crypto/libbitcoin_crypto_sse4.la

hebasto commented at 11:50 am on February 15, 2024:

It seems there are no reasons to keep libbitcoin_crypto_sse4 library anymore. The crypto/sha256_sse4.cpp source might be a part of the libbitcoin_crypto_base, no?

theuni commented at 4:19 pm on February 15, 2024:

Agreed, but would you be ok with fixing this up in a follow-up? I think it’d make sense to address while dealing with this #29404#pullrequestreview-1870581585

fanquake commented at 4:58 pm on March 1, 2024:

See #29528.

hebasto commented at 4:00 pm on February 19, 2024: member

Our docs (developer-notes.md and fuzzing.md) mention the use of --disable-asm to avoid address sanitizer failures.

In the libsecp repo, the --with-asm=no option is still required to avoid MSan warnings. If this is a common issue for builtin assembly code, it seems possible that the sha256_sse4.cpp might cause similar issues in the future without an option to disable it.

Maybe gate the sha256_sse4::Transform function with #if !defined(DISABLE_OPTIMIZED_SHA256)?

theuni commented at 1:29 pm on February 21, 2024: member

@fanquake Do you think it’s too late for this for v27? It would imply require getting https://github.com/bitcoin-core/crc32c-subtree/pull/6 in, I think.

theuni commented at 1:34 pm on February 21, 2024: member

In the libsecp repo, the --with-asm=no option is still required to avoid MSan warnings.

Can you point to where this is an issue? It looks as though secp has proper __msan_* annotations in place these days.

hebasto commented at 3:11 pm on February 21, 2024: member

In the libsecp repo, the --with-asm=no option is still required to avoid MSan warnings.

Can you point to where this is an issue? It looks as though secp has proper __msan_* annotations in place these days.

Sure. For example, https://github.com/bitcoin-core/secp256k1/pull/1169#issuecomment-1370003449. It holds for the current libsecp master branch @ https://github.com/bitcoin-core/secp256k1/commit/0653a25d50f67c68bd2d196ecc7eddab067d95ef as well.

fanquake commented at 3:36 pm on February 21, 2024: member

@fanquake Do you think it’s too late for this for v27?

I think it’s still possible to do this for v27. It’s not clear to me that the other change is strictly a requirement for doing this change?, but I also think that could go in.

theuni commented at 4:46 pm on February 21, 2024: member

In the libsecp repo, the --with-asm=no option is still required to avoid MSan warnings.

Can you point to where this is an issue? It looks as though secp has proper __msan_* annotations in place these days.

Sure. For example, bitcoin-core/secp256k1#1169 (comment). It holds for the current libsecp master branch @ bitcoin-core/secp256k1@0653a25 as well.

Fixed here https://github.com/bitcoin-core/secp256k1/pull/1496 :)

theuni commented at 5:35 pm on February 21, 2024: member

Any other sanitizer false-positives while we’re at it? :)

fanquake commented at 2:13 pm on February 26, 2024: member

Any other sanitizer false-positives while we’re at it? :)

Anything in test/sanitizer_suppressions/* is up for grabs (if it can be fixed)

It would imply require getting https://github.com/bitcoin-core/crc32c-subtree/pull/6 in, I think.

Had another look, and this is correct. We need to merge a fix in the subtree, then do a subtree update here, and then this can be merged, otherwise we’ll be left with at least one bug without any workaround. This PR should also be updated to removed the --disable-asm mentions in fuzzing.md and developer-notes.md.

maflcko commented at 2:33 pm on February 26, 2024: member

Any other sanitizer false-positives while we’re at it? :)

Anything in test/sanitizer_suppressions/* is up for grabs (if it can be fixed)

I think there should be no suppressions related to asm in those files, at least in the code that is compiled as part of Bitcoin Core

fanquake commented at 2:34 pm on February 26, 2024: member

I think there should be no suppressions related to asm in those files

I’m just talking about general suppressions, nothing asm specific.

theuni commented at 3:54 pm on February 26, 2024: member

From developer-notes.md:

There are a number of known problems when using the address sanitizer. The address sanitizer is known to fail in sha256_sse4::Transform which makes it unusable unless you also use --disable-asm when running configure. We would like to fix sanitizer issues, so please send pull requests if you can fix any errors found by the address sanitizer (or any other sanitizer).

I’m unable to hit this locally. Presumably if it’s still an issue we should be able to fix it with some notations.

Does anyone happen to have a test-case for it?

theuni commented at 4:12 pm on February 26, 2024: member

I pushed a commit to remove references to disable-asm from our docs as @fanquake suggested, in anticipation of:

merging https://github.com/bitcoin-core/crc32c-subtree/pull/6 and syncing our subtree
confirmation that sha256_sse4::Transform is no longer an issue and/or supplying a fix for it

theuni commented at 7:49 pm on February 27, 2024: member

This now depends on #29493

Empact commented at 9:16 pm on February 27, 2024: member

Concept ACK

fanquake commented at 11:53 pm on February 28, 2024: member

This now depends on #29493

Want to rebase now?

build: remove confusing and inconsistent disable-asm option

1. It didn't actually disable asm usage in our code. Regardless of the setting,
   asm is used in random.cpp and support/cleanse.cpp.
2. The value wasn't forwarded to libsecp as a user might have reasonably
   expected.
3. We now have the DISABLE_OPTIMIZED_SHA256 define which is what disable-asm
   actually did in practice.

If there is any desire, we can hook DISABLE_OPTIMIZED_SHA256 up to a new
configure option that actually does what it says.

376f0f6d07

doc: remove references to disable-asm option now that it's gone

The comment about sha256_sse4::Transform is believed to be old and stale.

f8a06f7a02

theuni force-pushed on Feb 29, 2024

theuni renamed this:
~~RFC: build: remove confusing and inconsistent disable-asm option~~
build: remove confusing and inconsistent disable-asm option
on Feb 29, 2024

DrahtBot added the label Build system on Feb 29, 2024

theuni commented at 7:12 pm on February 29, 2024: member

Rebased and removed RFC from PR title.

fanquake approved

fanquake commented at 7:35 pm on February 29, 2024: member

ACK f8a06f7a02be83e9b76a1b31f1b66a965dbedfce

DrahtBot requested review from Empact on Feb 29, 2024

DrahtBot requested review from hebasto on Feb 29, 2024

DrahtBot requested review from dergoegge on Feb 29, 2024

DrahtBot requested review from TheCharlatan on Feb 29, 2024

fanquake merged this on Feb 29, 2024

fanquake closed this on Feb 29, 2024

fanquake referenced this in commit 2533405364 on Mar 1, 2024

fanquake referenced this in commit 521693378b on Mar 1, 2024

hebasto referenced this in commit 4e196925b6 on Mar 2, 2024

fanquake referenced this in commit fce53f132e on Mar 2, 2024

hebasto referenced this in commit 6b47227e15 on Mar 4, 2024

kevkevinpal referenced this in commit b8b22a1df8 on Mar 13, 2024

kevkevinpal referenced this in commit 9b462aef9a on Mar 13, 2024

achow101 commented at 0:28 am on March 30, 2024: member

I used --disable-asm in order to even be able to build when configured for fuzzing. Otherwise, I get this compiler error:

0crypto/sha256_sse4.cpp:44:9: error: expected relocatable expression
1   44 |         "shl    $0x6,%2;"
2      |         ^
3<inline asm>:1:15894: note: instantiated into assembly here

Since crypto/sha256_sse4.cpp is not guarded by DISABLE_OPTIMIZED_SHA256, there’s no way for me to compile for fuzzing now.

fanquake commented at 3:59 pm on April 1, 2024: member

Otherwise, I get this compiler error:

Can you open a new issue, with the complete config.log / build output?

theuni commented at 4:43 pm on April 1, 2024: member

Since crypto/sha256_sse4.cpp is not guarded by DISABLE_OPTIMIZED_SHA256, there’s no way for me to compile for fuzzing now.

Still trying to understand the problem here, but in the meantime I’ll PR an addition of the DISABLE_OPTIMIZED_SHA256 guard.

build: remove confusing and inconsistent disable-asm option #29407

Code Coverage

Reviews

Guix builds (on x86_64)