cmake: add optional source files to bitcoin

purpleKarrot commented at 11:22 am on November 11, 2024: none

Avoid having many static libraries by adding the optional sources to the target bitcoin_crypto directly. Set the necessary compile options at the source file level, rather than the target level.

DrahtBot commented at 11:22 am on November 11, 2024: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31268.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	sipa
Approach ACK	BrandonOdiwuor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31394 ([POC] cmake: Introduce LLVM’s Source-based Code Coverage reports by hebasto)
#31161 (cmake: Set top-level target output locations by hebasto)

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.

DrahtBot added the label Build system on Nov 11, 2024

in src/crypto/CMakeLists.txt:57 in 7cdae6d7e0 outdated

72 if(HAVE_ARM_SHANI)
73-  add_library(bitcoin_crypto_arm_shani STATIC EXCLUDE_FROM_ALL
74-    sha256_arm_shani.cpp
75+  target_compile_definitions(bitcoin_crypto PRIVATE ENABLE_ARM_SHANI)
76+  target_sources(bitcoin_crypto PRIVATE sha256_arm_shani.cpp)
77+  set_property(SOURCE sha256_x86_shani.cpp PROPERTY

willcl-ark commented at 11:26 am on November 11, 2024:

I think you have a copy-paste error here. x86 should be arm?

purpleKarrot force-pushed on Nov 11, 2024

DrahtBot added the label CI failed on Nov 11, 2024

DrahtBot commented at 11:29 am on November 11, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/32802162894

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

cmake: add optional source files to bitcoin_crypto directly ed2cd60878

purpleKarrot force-pushed on Nov 11, 2024

DrahtBot removed the label CI failed on Nov 11, 2024

fanquake commented at 2:17 pm on November 11, 2024: member

Avoid having many static libraries

Can you expand the motivation.

purpleKarrot commented at 2:44 pm on November 11, 2024: none

There are four alternative implementations for sha256, each in its own source file which has to be compiled with dedicated compiler options. The current approach is to build a static library per source file, set the necessary compiler options for that library, and eventually link bitcoin_crypto against those libraries. This is needless complexity.

My proposed approach is to compile the source file with its dedicated options, but link the object files into bitcoin_crypto directly.

sipa commented at 3:28 pm on November 11, 2024: member

Concept ACK. If it’s possible to have differently-compiled compilation units without needing separate libraries for each, then this does seem like a cleaner solution. I suspect we perhaps ended up in this state because in autotools separate libraries were the only way to achieve this?

BrandonOdiwuor commented at 10:59 am on November 14, 2024: contributor

Approach ACK. Great catch!

Nice simplification! Adding the optional source files directly to bitcoin_crypto eliminating the need for intermediate libraries like bitcoin_crypto_sse41, bitcoin_crypto_avx2, and bitcoin_crypto_x86_shani

hebasto commented at 8:01 pm on November 16, 2024: member

Avoid having many static libraries by adding the optional sources to the target bitcoin_crypto directly. Set the necessary compile options at the source file level, rather than the target level.

While working on the CMake staging branch, this was done on purpose (see https://github.com/hebasto/bitcoin/pull/172 and similar https://github.com/hebasto/bitcoin/pull/171). However, the goal PR has since been replaced.

Additionally, from the Professional CMake: A Practical Guide 19th Edition:

Developers should be aware of an implementation detail which may present a strong deterrent to their use in some situations. For some CMake generators (notably the Unix Makefiles generator), the dependencies between sources and source properties are stronger than one might expect. If source properties are used to modify the compiler flags for specific source files rather than for a whole target, changing the source’s compiler flags will still result in all of the target’s sources being rebuilt, not just the affected source file. This is a limitation of how the dependency details are handled in the Makefile, where testing whether each source file’s compiler flags have changed brings with it a prohibitively big performance hit. The relevant Makefile dependencies were implemented at the target level instead to avoid that problem.

purpleKarrot commented at 7:18 am on December 1, 2024: none

If source properties are used to modify the compiler flags for specific source files rather than for a whole target, changing the source’s compiler flags will still result in all of the target’s sources being rebuilt, not just the affected source file.

I don’t think that applies here. The source’s compile flags are never changed between builds: sha256_avx2.cpp is either compiled with ${AVX2_CXXFLAGS}, or it is not compiled at all. And AVX2_CXXFLAGS is only ever set to -mavx -mavx2. The issue that all of the libraries source files get recompiled never occurs. @hebasto, can you elaborate why you think the source-specific compile options are hard to reason about?

hebasto commented at 10:41 am on December 1, 2024: member

@hebasto, can you elaborate why you think the source-specific compile options are hard to reason about?

Sure. Consider this minimal example:

0cmake_minimum_required(VERSION 3.22)
1project(test CXX)
2
3add_library(interface INTERFACE)
4target_compile_options(interface INTERFACE -Wno-mock-interface)
5
6add_executable(test main.cpp)
7set_property(SOURCE main.cpp PROPERTY COMPILE_OPTIONS -Wno-mock-source)
8target_compile_options(test PRIVATE -Wno-mock-target)
9target_link_libraries(test PRIVATE interface)

It is not straightforward to determine the actual order of compiler flags passed to the compiler when the COMPILE_OPTIONS target property is used.

0-Wno-mock-target -Wno-mock-interface -Wno-mock-source

purpleKarrot commented at 11:31 am on December 1, 2024: none

Why is that hard to reason about? If I add

0add_compile_options(-Wno-mock-global)

the resulting order will be -Wno-mock-global -Wno-mock-target -Wno-mock-interface -Wno-mock-source, so the flags are ordered from most generic to most specific.

That aside, I don’t think the order of arguments is relevant here.

hebasto commented at 11:38 am on December 1, 2024: member

Why is that hard to reason about?

People unfamiliar with such peculiarities might not realise that compiler options specified as a source’s property take precedence over other compiler options.

purpleKarrot commented at 12:30 pm on December 1, 2024: none

Yeah, they might also not realize that compiler options specified at the target level might take precedence over those specified globally…

theuni commented at 7:01 pm on December 5, 2024: member

I also find the reasons against doing this do be pretty uncompelling.

I consider the current approach to be a leftover autootools artifact. This is a nice cleanup and simplification, and the (stated) downsides seem pretty minimal. It looks like the crc32c lib would benefit from this approach as well.

Unless there’s some actual use case or generator that this breaks, +1 from me.

cmake: add optional source files to bitcoin_crypto directly #31268

Code Coverage & Benchmarks

Reviews

Conflicts