build, refactor: Rely on `AC_DEFINE` macro #24774

hebasto · 2022-04-05T16:12:48Z

On master (9ce1c50) 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 #24773.

src/Makefile.am

Empact · 2022-04-06T15:20:25Z

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.

hebasto · 2022-04-06T15:55:39Z

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

bitcoin/src/Makefile.am

Lines 40 to 56 in 41720a1

    
           LIBBITCOIN_CRYPTO= $(LIBBITCOIN_CRYPTO_BASE) 
        
           if ENABLE_SSE41 
        
           LIBBITCOIN_CRYPTO_SSE41 = crypto/libbitcoin_crypto_sse41.a 
        
           LIBBITCOIN_CRYPTO += $(LIBBITCOIN_CRYPTO_SSE41) 
        
           endif 
        
           if ENABLE_AVX2 
        
           LIBBITCOIN_CRYPTO_AVX2 = crypto/libbitcoin_crypto_avx2.a 
        
           LIBBITCOIN_CRYPTO += $(LIBBITCOIN_CRYPTO_AVX2) 
        
           endif 
        
           if ENABLE_X86_SHANI 
        
           LIBBITCOIN_CRYPTO_X86_SHANI = crypto/libbitcoin_crypto_x86_shani.a 
        
           LIBBITCOIN_CRYPTO += $(LIBBITCOIN_CRYPTO_X86_SHANI) 
        
           endif 
        
           if ENABLE_ARM_SHANI 
        
           LIBBITCOIN_CRYPTO_ARM_SHANI = crypto/libbitcoin_crypto_arm_shani.a 
        
           LIBBITCOIN_CRYPTO += $(LIBBITCOIN_CRYPTO_ARM_SHANI) 
        
           endif

laanwj · 2022-04-07T12:15:06Z

Build system review and concept ACK 8251431

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.

laanwj · 2022-04-07T12:16:11Z

Thinking of it, why do we compile these files at all when the respective option is disabled? #ifdefing out the entire source file is a bit curious (but maybe I'm missing something).

fanquake · 2022-04-07T12:44:08Z

Some of this might have been done (at some point) to support weird / nonstandard / non-autoconf builds in some way, i.e similar to

bitcoin/src/compat/endian.h

Lines 23 to 25 in 323d4c0

    
           // While not technically a supported configuration, defaulting to defining these 
        
           // DECLs when we were compiled without autotools makes it easier for other build 
        
           // systems to build things like libbitcoinconsensus for strange targets.

laanwj · 2022-04-07T13:39:07Z

I can see a slight point for a library like 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.

hebasto · 2022-04-07T16:46:32Z

@laanwj

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

hebasto · 2022-04-07T16:49:33Z

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.

DrahtBot · 2022-04-15T11:46:37Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25302 (build: Check usages of #if defined(...) by brokenprogrammer)

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.

fanquake · 2022-04-17T10:39:40Z

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.

hebasto · 2022-04-17T15:09:25Z

I bought this up very briefly with Cory, and he said that this was done on purpose.

What is the purpose?

theuni · 2022-04-21T20:11:11Z

@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.

hebasto · 2022-05-28T12:58:34Z

Rebased 8251431 -> 6241d16 (pr24774.02 -> pr24774.03) due to conflict with #24322.

hebasto · 2022-05-28T13:12:02Z

@theuni

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.

achow101 · 2022-10-12T19:09:27Z

Are you still working on this?

hebasto mentioned this pull request Apr 5, 2022

Enable HW-accelerated implementations of SHA256 for MSVC builds #24773

Closed

DrahtBot added Build system Utils/log/libs labels Apr 5, 2022

laanwj reviewed Apr 6, 2022

View reviewed changes

src/Makefile.am Outdated Show resolved Hide resolved

hebasto force-pushed the 220405-config branch from 047e7bd to 8251431 Compare April 6, 2022 12:44

DrahtBot mentioned this pull request Apr 15, 2022

[kernel 1/n] Introduce initial libbitcoinkernel #24322

Merged

DrahtBot added the Needs rebase label Apr 28, 2022

hebasto added 4 commits May 28, 2022 14:50

build, refactor: Rely on AC_DEFINE([ENABLE_ARM_SHANI])

a70b4db

This change deduplicates the `ENABLE_ARM_SHANI` macro in the build system.

build, refactor: Rely on AC_DEFINE([ENABLE_AVX2])

144dc2f

This change deduplicates the `ENABLE_AVX2` macro in the build system.

build, refactor: Rely on AC_DEFINE([ENABLE_SSE41])

0b5ec3a

This change deduplicates the `ENABLE_SSE41` macro in the build system.

build, refactor: Rely on AC_DEFINE([ENABLE_X86_SHANI])

6241d16

This change deduplicates the `ENABLE_X86_SHANI` macro in the build system.

hebasto force-pushed the 220405-config branch from 8251431 to 6241d16 Compare May 28, 2022 12:54

DrahtBot removed the Needs rebase label May 28, 2022

DrahtBot mentioned this pull request Jun 8, 2022

build: Check usages of #if defined(...) #25302

Closed

hebasto closed this Oct 12, 2022

bitcoin locked and limited conversation to collaborators Oct 12, 2023

build, refactor: Rely on AC_DEFINE macro #24774

build, refactor: Rely on AC_DEFINE macro #24774

Uh oh!

Conversation

hebasto commented Apr 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Empact commented Apr 6, 2022

Uh oh!

hebasto commented Apr 6, 2022

Uh oh!

laanwj commented Apr 7, 2022

Uh oh!

laanwj commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Apr 7, 2022

Uh oh!

laanwj commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Apr 7, 2022

Uh oh!

hebasto commented Apr 7, 2022

Uh oh!

DrahtBot commented Apr 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

fanquake commented Apr 17, 2022

Uh oh!

hebasto commented Apr 17, 2022

Uh oh!

theuni commented Apr 21, 2022

Uh oh!

hebasto commented May 28, 2022

Uh oh!

hebasto commented May 28, 2022

Uh oh!

achow101 commented Oct 12, 2022

Uh oh!

Uh oh!

build, refactor: Rely on `AC_DEFINE` macro #24774

build, refactor: Rely on `AC_DEFINE` macro #24774

hebasto commented Apr 5, 2022 •

edited

Loading

laanwj commented Apr 7, 2022 •

edited

Loading

laanwj commented Apr 7, 2022 •

edited

Loading

DrahtBot commented Apr 15, 2022 •

edited

Loading