Skip to content

Commit

Permalink
Merge bitcoin#29407: build: remove confusing and inconsistent disable…
Browse files Browse the repository at this point in the history
…-asm option

f8a06f7 doc: remove references to disable-asm option now that it's gone (Cory Fields)
376f0f6 build: remove confusing and inconsistent disable-asm option (Cory Fields)

Pull request description:

  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.

  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.

ACKs for top commit:
  fanquake:
    ACK f8a06f7

Tree-SHA512: 4a99c2130225acbe9dc7399ed572a04ca155cbfa3eef8178a632ba533017d264691e6482cceb1d8f9c5d768619d99a2466dea4b82b27b18b872bceae91b92fbb
  • Loading branch information
fanquake committed Feb 29, 2024
2 parents be5399e + f8a06f7 commit dfc35c9
Show file tree
Hide file tree
Showing 5 changed files with 4 additions and 36 deletions.
16 changes: 0 additions & 16 deletions configure.ac
Original file line number Diff line number Diff line change
Expand Up @@ -249,16 +249,6 @@ AC_ARG_ENABLE([threadlocal],
[use_thread_local=$enableval],
[use_thread_local=auto])

AC_ARG_ENABLE([asm],
[AS_HELP_STRING([--disable-asm],
[disable assembly routines (enabled by default)])],
[use_asm=$enableval],
[use_asm=yes])

if test "$use_asm" = "yes"; then
AC_DEFINE([USE_ASM], [1], [Define this symbol to build in assembly routines])
fi

AC_ARG_ENABLE([zmq],
[AS_HELP_STRING([--disable-zmq],
[disable ZMQ notifications])],
Expand Down Expand Up @@ -460,8 +450,6 @@ enable_sse41=no
enable_avx2=no
enable_x86_shani=no

if test "$use_asm" = "yes"; then

dnl Check for optional instruction set support. Enabling these does _not_ imply that all code will
dnl be compiled with them, rather that specific objects/libs may use them after checking for runtime
dnl compatibility.
Expand Down Expand Up @@ -600,8 +588,6 @@ AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[
)
CXXFLAGS="$TEMP_CXXFLAGS"

fi

CORE_CPPFLAGS="$CORE_CPPFLAGS -DHAVE_BUILD_INFO"

AC_ARG_WITH([utils],
Expand Down Expand Up @@ -1817,7 +1803,6 @@ AM_CONDITIONAL([ENABLE_AVX2], [test "$enable_avx2" = "yes"])
AM_CONDITIONAL([ENABLE_X86_SHANI], [test "$enable_x86_shani" = "yes"])
AM_CONDITIONAL([ENABLE_ARM_CRC], [test "$enable_arm_crc" = "yes"])
AM_CONDITIONAL([ENABLE_ARM_SHANI], [test "$enable_arm_shani" = "yes"])
AM_CONDITIONAL([USE_ASM], [test "$use_asm" = "yes"])
AM_CONDITIONAL([WORDS_BIGENDIAN], [test "$ac_cv_c_bigendian" = "yes"])
AM_CONDITIONAL([USE_NATPMP], [test "$use_natpmp" = "yes"])
AM_CONDITIONAL([USE_UPNP], [test "$use_upnp" = "yes"])
Expand Down Expand Up @@ -1972,7 +1957,6 @@ echo " with fuzz binary = $enable_fuzz_binary"
echo " with bench = $use_bench"
echo " with upnp = $use_upnp"
echo " with natpmp = $use_natpmp"
echo " use asm = $use_asm"
echo " USDT tracing = $use_usdt"
echo " sanitizers = $use_sanitizers"
echo " debug enabled = $enable_debug"
Expand Down
7 changes: 0 additions & 7 deletions doc/developer-notes.md
Original file line number Diff line number Diff line change
Expand Up @@ -577,13 +577,6 @@ export UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:prin
See the CI config for more examples, and upstream documentation for more information
about any additional options.

There are a number of known problems when using the `address` sanitizer. The
address sanitizer is known to fail in
[sha256_sse4::Transform](/src/crypto/sha256_sse4.cpp) 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).

Not all sanitizer options can be enabled at the same time, e.g. trying to build
with `--with-sanitizers=address,thread` will fail in the configure script as
these sanitizers are mutually incompatible. Refer to your compiler manual to
Expand Down
7 changes: 1 addition & 6 deletions doc/fuzzing.md
Original file line number Diff line number Diff line change
Expand Up @@ -127,19 +127,14 @@ The default Clang/LLVM version supplied by Apple on macOS does not include
fuzzing libraries, so macOS users will need to install a full version, for
example using `brew install llvm`.
Should you run into problems with the address sanitizer, it is possible you
may need to run `./configure` with `--disable-asm` to avoid errors
with certain assembly code from Bitcoin Core's code. See [developer notes on sanitizers](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#sanitizers)
for more information.
You may also need to take care of giving the correct path for `clang` and
`clang++`, like `CC=/path/to/clang CXX=/path/to/clang++` if the non-systems
`clang` does not come first in your path.
Full configure that was tested on macOS with `brew` installed `llvm`:
```sh
./configure --enable-fuzz --with-sanitizers=fuzzer,address,undefined --disable-asm CC=$(brew --prefix llvm)/bin/clang CXX=$(brew --prefix llvm)/bin/clang++
./configure --enable-fuzz --with-sanitizers=fuzzer,address,undefined CC=$(brew --prefix llvm)/bin/clang CXX=$(brew --prefix llvm)/bin/clang++
```
Read the [libFuzzer documentation](https://llvm.org/docs/LibFuzzer.html) for more information. This [libFuzzer tutorial](https://github.com/google/fuzzing/blob/master/tutorial/libFuzzerTutorial.md) might also be of interest.
Expand Down
2 changes: 0 additions & 2 deletions src/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -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
LIBBITCOIN_CRYPTO += $(LIBBITCOIN_CRYPTO_SSE4)
endif
if ENABLE_SSE41
LIBBITCOIN_CRYPTO_SSE41 = crypto/libbitcoin_crypto_sse41.la
LIBBITCOIN_CRYPTO += $(LIBBITCOIN_CRYPTO_SSE41)
Expand Down
8 changes: 3 additions & 5 deletions src/crypto/sha256.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,11 @@
#endif

#if defined(__x86_64__) || defined(__amd64__) || defined(__i386__)
#if defined(USE_ASM)
namespace sha256_sse4
{
void Transform(uint32_t* s, const unsigned char* chunk, size_t blocks);
}
#endif
#endif

namespace sha256d64_sse41
{
Expand Down Expand Up @@ -574,7 +572,7 @@ bool SelfTest() {
}

#if !defined(DISABLE_OPTIMIZED_SHA256)
#if defined(USE_ASM) && (defined(__x86_64__) || defined(__amd64__) || defined(__i386__))
#if (defined(__x86_64__) || defined(__amd64__) || defined(__i386__))
/** Check whether the OS has enabled AVX registers. */
bool AVXEnabled()
{
Expand All @@ -597,7 +595,7 @@ std::string SHA256AutoDetect(sha256_implementation::UseImplementation use_implem
TransformD64_8way = nullptr;

#if !defined(DISABLE_OPTIMIZED_SHA256)
#if defined(USE_ASM) && defined(HAVE_GETCPUID)
#if defined(HAVE_GETCPUID)
bool have_sse4 = false;
bool have_xsave = false;
bool have_avx = false;
Expand Down Expand Up @@ -654,7 +652,7 @@ std::string SHA256AutoDetect(sha256_implementation::UseImplementation use_implem
ret += ",avx2(8way)";
}
#endif
#endif // defined(USE_ASM) && defined(HAVE_GETCPUID)
#endif // defined(HAVE_GETCPUID)

#if defined(ENABLE_ARM_SHANI)
bool have_arm_shani = false;
Expand Down

0 comments on commit dfc35c9

Please sign in to comment.