build: Fix LTO functionality #28876

hebasto commented at 5:53 PM on November 14, 2023: member

The current LTO functionality implementation has multiple issues:

Duplicated -flto flags when linking.
The secp256k1 subtree code is built without LTO.
In AX_CHECK_LINK_FLAG macro the wrong CXXFLAG_WERROR flag is used instead of the correct LDFLAG_WERROR one.

This PR fixes all of them.

Might be verified as follows:

$ readelf -S src/secp256k1/src/libsecp256k1_la-secp256k1.o | grep .gnu.lto_ | wc -l
274

The depends build system has similar issues that will be addressed in a separated PR.

Noticed while porting this functionality to a new CMake-based build system.

build: Drop excessive appending `-flto` to `LDFLAGS`

A compiler driver will pass `-flto`, which is set in CXXFLAGS, during
both compiling and linking.

e8c5e448ac

build: Pass LTO flags to `libsecp256k1` code ffa1e0cfe4

scripted-diff: Rename `LTO_CXXFLAGS` to `LTO_FLAGS`

LTO flags are the same for C and C++ compilers. And the same flags are
used by a compiler driver during the linking as well.

-BEGIN VERIFY SCRIPT-
sed -i -e 's/LTO_CXXFLAGS/LTO_FLAGS/g' configure.ac src/Makefile.am
-END VERIFY SCRIPT-

50dffe895d

DrahtBot commented at 5:53 PM on November 14, 2023: contributor

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. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29205 (build: always set -g -O2 in CORE_CXXFLAGS by fanquake)
#29185 (build: remove --enable-lto by fanquake)
#28983 (Stratum v2 Template Provider (take 2) by Sjors)
#28875 (build: Pass sanitize flags to instrument libsecp256k1 code by hebasto)
#28241 (Silent payment index (for light wallets and consistency check) by Sjors)
#28122 (Silent Payments: Implement BIP352 by josibake)
#25573 ([POC] guix: produce a fully -static-pie bitcoind by fanquake)

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 14, 2023

fanquake commented at 7:31 PM on November 14, 2023: member

In case anyone is unsure, the --enable-lto option, as-is, does work, no broken binaries etc (and we use (thin) LTO in oss-fuzz).

Changes 1 & 3 here don't affect the compiled binary. The compiler will eat any duplicate -flto flags; the change to the -Werror flag is correct, but also does not affect the compiled binary.

Passing the flags through to libsecp is an improvement, and I'm pretty sure that wasn't done originally, because when the --enable-lto option was implemented, we didn't have a clean way to do it. SECP_CFLAGS didn't exist.

That being said, I'm not sure if this option is something we should actually maintain going forward. It's been a useful convenience for testing, but as LTO has matured, and more configurations/options have become available (thin,auto etc), I'm not sure it makes sense for us to have a usage like this. We could probably revert back to c(xx)flags and friends, for passing options through.

Extending the current build option to account for all new/different LTO functionality probably doesn't make sense (that isn't being done here, but before we change things further I think this is worth considering).

hebasto commented at 8:52 AM on November 15, 2023: member

The other point of this PR is to make the current build system comparable to a new CMake-based one, as there is no reason to pessimize the latter, for example, by not applying LTO for libsecp code, for the sake of compatibility with the master branch.

See: https://github.com/hebasto/bitcoin/pull/52.

fanquake commented at 9:46 AM on November 15, 2023: member

See: https://github.com/hebasto/bitcoin/pull/52.

I think the example usage there:

cmake -B build -DLTO=ON -DLTO_FLAGS="-flto"

is somewhat helping make my point. Seems odd to have a LTO=ON option, but then still have to explicitly pass -flto into a non-standard LTO_FLAGS variable. Shouldn't that be what LTO=ON would do for you? I'm not sure how that is better/clearer than just cmake -B build CXXFLAGS="-flto"?

It's also not clear if LTO_FLAGS is a catch-all for compile and link flags, i.e if I'm using a link cache, do I pass the link options (i.e -Wl,-cache_path_lto) through there too, and then the build system figures out what's a compile flag, and what's a link flag? Is that better than LDFLAGS="-Wl,-cache_path_lto=/some/cache"? Will followup in the PR in any case.

hebasto commented at 12:09 PM on January 6, 2024: member

FWIW, the #29185 makes this PR outdated.

fanquake commented at 1:22 PM on January 15, 2024: member

FWIW, the #29185 makes this PR outdated.

Closing this for now, given it looks like we are going ahead with #29185.

fanquake closed this on Jan 15, 2024

bitcoin locked this on Jan 14, 2025