AC_DEFINE macro
#24774
On master (9ce1c506a3a5d20b1bf254235bfae48af592d86c) the ENABLE_ARM_SHANI, ENABLE_AVX2, ENABLE_SSE41, and ENABLE_X86_SHANI macros are defined twice in the build system. First time, using the AC_DEFINE macro in the configure script. And second time, in Makefile as a part of CPPFLAGS.
This PR removes such duplication.
This change is required for bitcoin/bitcoin#24773.
498@@ -499,27 +499,19 @@ crypto_libbitcoin_crypto_base_a_SOURCES += crypto/sha256_sse4.cpp
499 endif
500
501 crypto_libbitcoin_crypto_sse41_a_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
502-crypto_libbitcoin_crypto_sse41_a_CPPFLAGS = $(AM_CPPFLAGS)
CPPFLAGS is not needed anymore?
Just to be clear on my understanding: this removes the definitions in these files in the case where they do not pass the test in configure.ac. The reason the current behavior is not broken where these functions are defined despite missing required behavior, is that while these functions are always included, their callers also check for the definitions, and that prevents their being called in those cases. Is that right?
If so, would it be worthwhile and feasible to remove these archives entirely, in the cases where the functionality is not present? E.g. crypto_libbitcoin_crypto_sse41_a includes only crypto/sha256_sse41.cpp. which only defines functionality if ENABLE_SSE41 is defined.
Just to be clear on my understanding: this removes the definitions in these files in the case where they do not pass the test in
configure.ac.
The build system includes the crypto parts only when the related checks passed https://github.com/bitcoin/bitcoin/blob/41720a1f540ef3c16a283a6cce6f0a63552a4937/src/Makefile.am#L40-L56
Build system review and concept ACK 825143174e7a91bf644b980b4e133ace13e898fe
Including the config header instead of relying on definitions on the command line is a no-brainer.
I would guess this doesn’t need parallel changes to the MSVC build system because it doesn’t support special instruction sets yet.
#ifdefing out the entire source file is a bit curious (but maybe I’m missing something).
libbitcoin_consensus; e.g. if you want to build it as part of a.rust binding, you don’t particularly want to call into automake/autoconf. Still. I’m not sure having files compiled only when e.g. ENABLE_AVX2 is set, then having the whole file guarded with #ifdef ENABLE_AVX2…#endif helps there.
Thinking of it, why do we compile these files at all when the respective option is disabled?
What does make you think so?
For example, when the configure script fails to check AC_MSG_CHECKING([for x86 SHA-NI intrinsics]) ..., the sha256_x86_shani.cpp source file is not compiled. Also #24774 (comment).
e.g. if you want to build it as part of a.rust binding, you don’t particularly want to call into automake/autoconf.
Also we do not use automake/autoconf when building with MSVC.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
This PR removes such duplication.
I bought this up very briefly with Cory, and he said that this was done on purpose. So this change shouldn’t be made under the guise of “duplication”, and/or move ahead until he weighs in here. If this code does have other use cases then we need to evaluate whether removing it just to support things in the MSVC build is actually more worthwhile.
I bought this up very briefly with Cory, and he said that this was done on purpose.
What is the purpose?
@hebasto whoops, sorry for the delayed response.
It’s rather convenient imo to have the crypto folder work outside of Core. I use it often as a quick way to add a known/trustworthy sha2 impl to another project, like here: https://github.com/theuni/libutreexo
I believe @TheBlueMatt has done/does something similar for some of his projects.
So for what’s in crypto/ at least, unless it’s somehow problematic, I’d much prefer to keep the need for bitcoin-config.h to a minimum.
This change deduplicates the `ENABLE_ARM_SHANI` macro in the build
system.
This change deduplicates the `ENABLE_AVX2` macro in the build system.
This change deduplicates the `ENABLE_SSE41` macro in the build system.
This change deduplicates the `ENABLE_X86_SHANI` macro in the build
system.
I’d much prefer to keep the need for bitcoin-config.h to a minimum.
This change does not make the bitcoin-config.h required. It remains optional, i.e., only when the HAVE_CONFIG_H is defined.
It’s rather convenient imo to have the crypto folder work outside of Core. I use it often as a quick way to add a known/trustworthy sha2 impl to another project, like here: https://github.com/theuni/libutreexo
I believe @TheBlueMatt has done/does something similar for some of his projects.
I believe all of the cases mentioned above are still working with this PR without any adjustments.
In other words, this PR makes crypto recognize bitcoin-config.h as an additional build option. All other build options, which do not use bitcoin-config.h, are not affected.
