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

theuni · 2024-02-07T20:21:20Z

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 · 2024-02-07T20:21:23Z

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 · 2024-02-07T20:22:41Z

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

theuni · 2024-02-07T20:24:41Z

Also ping @dergoegge for fuzzer interactions ACK/NACK.

TheCharlatan · 2024-02-08T12:13:08Z

Concept ACK

dergoegge · 2024-02-08T15:58:12Z

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 · 2024-02-08T16:22:44Z

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 · 2024-02-08T17:01:32Z

@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 · 2024-02-08T19:26:22Z

Guix builds (on x86_64)

File	commit `801ef07` (master)	commit `8344933` (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...`

jonatack · 2024-02-08T21:24:15Z

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):

crc32c/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
0x62d000000406: note: pointer points here
 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
             ^ 
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior crc32c/src/crc32c_arm64.cc:101:26 in

dergoegge · 2024-02-08T21:36:59Z

@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 · 2024-02-09T15:07:20Z

Concept ACK 0eec878.

TheCharlatan

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 · 2024-02-12T21:48:12Z

@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 · 2024-02-12T23:05:33Z

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 bitcoin-core/crc32c-subtree#6 (comment) ?

Edit: nevermind. Reproduced and explained here: bitcoin-core/crc32c-subtree#6 (comment)

fanquake · 2024-02-13T12:58:47Z

Concept ACK

hebasto

Concept ACK.

hebasto · 2024-02-15T11:50:39Z

src/Makefile.am

@@ -50,10 +50,8 @@ LIBBITCOIN_WALLET_TOOL=libbitcoin_wallet_tool.a
 endif

 LIBBITCOIN_CRYPTO = $(LIBBITCOIN_CRYPTO_BASE)
-if USE_ASM
 LIBBITCOIN_CRYPTO_SSE4 = crypto/libbitcoin_crypto_sse4.la


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?

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 (review)

See #29528.

hebasto · 2024-02-19T16:00:38Z

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 · 2024-02-21T13:29:18Z

@fanquake Do you think it's too late for this for v27? It would imply require getting bitcoin-core/crc32c-subtree#6 in, I think.

theuni · 2024-02-21T13:34:53Z

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 · 2024-02-21T15:11:57Z

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.

fanquake · 2024-02-21T15:36:41Z

@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 · 2024-02-21T16:46:12Z

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 bitcoin-core/secp256k1#1496 :)

theuni · 2024-02-21T17:35:12Z

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

fanquake · 2024-02-26T14:13:49Z

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 bitcoin-core/crc32c-subtree#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 · 2024-02-26T14:33:42Z

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 · 2024-02-26T14:34:33Z

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

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

theuni · 2024-02-26T15:54:32Z

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?

Empact · 2024-02-27T21:16:03Z

Concept ACK

fanquake · 2024-02-28T23:53:17Z

This now depends on #29493

Want to rebase now?

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.

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

theuni · 2024-02-29T19:12:20Z

Rebased and removed RFC from PR title.

fanquake

ACK f8a06f7

Followup to discussion in bitcoin#29407. Drops LIBBITCOIN_CRYPTO_SSE4.

Align `bitcoin_crypto` library implementation with: - bitcoin#29407 - bitcoin#29528 Note. Failed to present this change as git-autosquash-ready.

5216933 build: move sha256_sse4 into libbitcoin_crypto_base (fanquake) Pull request description: Followup to discussion in #29407. Drops `LIBBITCOIN_CRYPTO_SSE4`. ACKs for top commit: theuni: utACK 5216933. hebasto: ACK 5216933. TheCharlatan: ACK 5216933 Tree-SHA512: 44889340b7647409a439ebe97012f66383e0e5f3d99200ffd55c124e91d3ed164f02b736ff9dafca2d9ba7e365fcdc79aff054471d4bc240d035b58659407420

…ecent sync to the master branch 4e19692 FIXUP: cmake: Build `bitcoin_crypto` library (Hennadii Stepanov) fc662d9 fixup! cmake: Build `secp256k1` static library (Hennadii Stepanov) Pull request description: Align `bitcoin_crypto` library implementation with: - bitcoin#29407 - bitcoin#29528 Note. Failed to present this change as git-autosquash-ready. ACKs for top commit: pablomartin4btc: crACK 4e19692 Tree-SHA512: 2d87cc7dc8423bcd345eb9282bd4091a6c9a4e768fe6f7fb6a96922f40335d5f85ef288a5af5279aa9795832fd617c8167fc776b6fb06bb7d3f698866cc5e3e5

Followup to discussion in bitcoin#29407. Drops LIBBITCOIN_CRYPTO_SSE4.

achow101 · 2024-03-30T00:28:35Z

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

crypto/sha256_sse4.cpp:44:9: error: expected relocatable expression
   44 |         "shl    $0x6,%2;"
      |         ^
<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 · 2024-04-01T15:59:45Z

Otherwise, I get this compiler error:

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

theuni · 2024-04-01T16:43:46Z

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.

It was removed in bitcoin/bitcoin#29407.

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

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

TheCharlatan reviewed Feb 9, 2024

View reviewed changes

hebasto reviewed Feb 15, 2024

View reviewed changes

theuni added 2 commits February 29, 2024 19:05

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

f8a06f7

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

theuni force-pushed the remove-use-asm branch from 26f67b1 to f8a06f7 Compare February 29, 2024 19:11

theuni changed the title ~~RFC: build: remove confusing and inconsistent disable-asm option~~ build: remove confusing and inconsistent disable-asm option Feb 29, 2024

DrahtBot added the Build system label Feb 29, 2024

fanquake approved these changes Feb 29, 2024

View reviewed changes

DrahtBot requested review from Empact, hebasto, dergoegge and TheCharlatan February 29, 2024 19:35

fanquake merged commit dfc35c9 into bitcoin:master Feb 29, 2024

fanquake added a commit to fanquake/bitcoin that referenced this pull request Mar 1, 2024

build: move sha256_sse4 into libcrypto

2533405

Followup to discussion in bitcoin#29407. Drops LIBBITCOIN_CRYPTO_SSE4.

fanquake added a commit to fanquake/bitcoin that referenced this pull request Mar 1, 2024

build: move sha256_sse4 into libbitcoin_crypto_base

5216933

Followup to discussion in bitcoin#29407. Drops LIBBITCOIN_CRYPTO_SSE4.

fanquake mentioned this pull request Mar 1, 2024

build: move sha256_sse4 into libbitcoin_crypto_base #29528

Merged

hebasto added a commit to hebasto/bitcoin that referenced this pull request Mar 2, 2024

FIXUP: cmake: Build bitcoin_crypto library

4e19692

Align `bitcoin_crypto` library implementation with: - bitcoin#29407 - bitcoin#29528 Note. Failed to present this change as git-autosquash-ready.

hebasto mentioned this pull request Mar 2, 2024

cmake: Rework bitcoin_crypto library script after the recent sync to the master branch hebasto/bitcoin#108

Merged

kevkevinpal pushed a commit to kevkevinpal/bitcoin that referenced this pull request Mar 13, 2024

build: move sha256_sse4 into libbitcoin_crypto_base

b8b22a1

Followup to discussion in bitcoin#29407. Drops LIBBITCOIN_CRYPTO_SSE4.

kevkevinpal pushed a commit to kevkevinpal/bitcoin that referenced this pull request Mar 13, 2024

build: move sha256_sse4 into libbitcoin_crypto_base

9b462ae

Followup to discussion in bitcoin#29407. Drops LIBBITCOIN_CRYPTO_SSE4.

fanquake mentioned this pull request Apr 3, 2024

build: align debugging flags to -O0 #29796

Closed

achow101 mentioned this pull request Apr 3, 2024

Compilation failure with -O0 + -fsanitize=address due to inline asm #29801

Closed

1 task

jonatack added a commit to jonatack/jonatack.github.io that referenced this pull request Apr 3, 2024

How to Compile: rm now-unused --disable-asm configure option

e58f6ef

It was removed in bitcoin/bitcoin#29407.

bitcoin locked and limited conversation to collaborators Apr 1, 2025

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

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

Uh oh!

Conversation

theuni commented Feb 7, 2024

Uh oh!

DrahtBot commented Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Uh oh!

theuni commented Feb 7, 2024

Uh oh!

theuni commented Feb 7, 2024

Uh oh!

TheCharlatan commented Feb 8, 2024

Uh oh!

dergoegge commented Feb 8, 2024

Uh oh!

maflcko commented Feb 8, 2024

Uh oh!

theuni commented Feb 8, 2024

Uh oh!

DrahtBot commented Feb 8, 2024

Guix builds (on x86_64)

Uh oh!

jonatack commented Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dergoegge commented Feb 8, 2024

Uh oh!

epiccurious commented Feb 9, 2024

Uh oh!

TheCharlatan left a comment

Choose a reason for hiding this comment

Uh oh!

theuni commented Feb 12, 2024

Uh oh!

theuni commented Feb 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Feb 13, 2024

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto Feb 15, 2024

Choose a reason for hiding this comment

Uh oh!

theuni Feb 15, 2024

Choose a reason for hiding this comment

Uh oh!

fanquake Mar 1, 2024

Choose a reason for hiding this comment

Uh oh!

hebasto commented Feb 19, 2024

Uh oh!

theuni commented Feb 21, 2024

Uh oh!

theuni commented Feb 21, 2024

Uh oh!

hebasto commented Feb 21, 2024

Uh oh!

fanquake commented Feb 21, 2024

Uh oh!

theuni commented Feb 21, 2024

Uh oh!

theuni commented Feb 21, 2024

Uh oh!

fanquake commented Feb 26, 2024

Uh oh!

maflcko commented Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Feb 26, 2024

Uh oh!

theuni commented Feb 26, 2024

Uh oh!

Empact commented Feb 27, 2024

Uh oh!

DrahtBot commented Feb 7, 2024 •

edited

Loading

jonatack commented Feb 8, 2024 •

edited

Loading

theuni commented Feb 12, 2024 •

edited

Loading

maflcko commented Feb 26, 2024 •

edited

Loading