build: remove `--enable-lto`

fanquake commented at 3:20 pm on January 5, 2024: member

This has outlived its usefulness, doesn’t gel well with newer compilers & -flto related options, i.e thin vs full, or =auto, and having -flto as the only option means that sometimes this just needs to be worked around, i.e in oss-fuzz: https://github.com/google/oss-fuzz/blob/master/projects/bitcoin-core/build.sh.

While it was convenient when -flto was newer, support for -flto is now in all compilers we use, and there’s also no-longer any real need for us to treat -flto different to any other optimization option.

Remove it, to remove build complexity, and so there’s no need to port a similar option to CMake.

Note that the LTO option remains in depends, because we still need a way to build packages that have LTO specific patches/options.

build: remove --enable-lto

This has outlived its usefulness, doesn't gel well with
newer compilers & `-flto` related options, i.e thin vs full, or `=auto`,
and having `-flto` as the only option means that sometimes this just
needs to be worked around, i.e in oss-fuzz:
https://github.com/google/oss-fuzz/blob/master/projects/bitcoin-core/build.sh.

While it was convenient when `-flto` was newer, support for `-flto` is now
in all compilers we use, and there's also no-longer any real need
for us to treat `-flto` different to any other optimization option.

Remove it, to remove build complexity, and so there's no need
to port a similar option to CMake.

Note that the LTO option remains in depends, because we still a way to
build packages that have LTO specific patches/options.

If we decide to merge this, I'll follow up downstream in oss-fuzz first,
to make sure we don't break the build.

2d1b1c7dae

DrahtBot commented at 3:20 pm on January 5, 2024: 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.

Type	Reviewers
ACK	TheCharlatan, hebasto

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:

#29233 (build: depends move macOS C(XX) FLAGS out of C & CXX by fanquake)
#29205 (build: always set -g -O2 in CORE_CXXFLAGS by fanquake)
#25573 ([POC] guix: produce a fully -static-pie bitcoind by fanquake)
#21778 (build: LLD based macOS toolchain 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 Jan 5, 2024

maflcko commented at 3:26 pm on January 5, 2024: member

… treat -flto different to any other optimization option.

Can you explain what that means? Is the recommended way to manually override CXXFLAGS and LDFLAGS now?

hebasto commented at 3:34 pm on January 5, 2024: member

Concept ACK.

fanquake commented at 3:36 pm on January 5, 2024: member

Can you explain what that means? Is the recommended way to manually override CXXFLAGS and LDFLAGS now?

Yea. -flto is really no different to -O2 or any other compile option, so using it will require adding -flto=*, and any other related options you might want, i.e using a link cache via -Wl,-cache_path_lto, to the respective *FLAGS vars. Note that this is already required by anyone wanting to do anything other, or in addion to just using -flto with the current options (or they’d have to workaround by modifying the source etc).

maflcko commented at 3:43 pm on January 5, 2024: member

to the respective *FLAGS vars

Ah, I guess this would disable O2, see https://github.com/bitcoin/bitcoin/pull/28071/files. So I guess it would have to be appended to CXX and possibly LDFLAGS.

TheCharlatan approved

TheCharlatan commented at 4:38 pm on January 5, 2024: contributor

ACK 2d1b1c7daeeada3f737e62ceb2db7484cde5ff4e

Tested just passing in the required flags, seems to work as advertised.

DrahtBot requested review from hebasto on Jan 5, 2024

hebasto commented at 5:38 pm on January 6, 2024: member

Tested 2d1b1c7daeeada3f737e62ceb2db7484cde5ff4e on Ubuntu 22.04:

0$ ./configure CXXFLAGS="-g -O2 -flto=auto" SECP_CFLAGS="-flto=auto"
1$ make -C src bitcoind

0$ make -C depends CXXFLAGS="-O2 -flto=auto" LTO=1
1$ ./configure CONFIG_SITE=$PWD/depends/x86_64-pc-linux-gnu/share/config.site SECP_CFLAGS="-flto=auto"
2$ make

Ah, I guess this would disable O2, see https://github.com/bitcoin/bitcoin/pull/28071/files. So I guess it would have to be appended to CXX and possibly LDFLAGS.

Appending to CXXFLAGS works fine.

FWIW, there are three if test "$CXXFLAGS_overridden" = "no" cases in the configure.ac, which also includes processing of the warning options (see #25972).

UPD.

0$ ./configure CC=clang-15 CXX=clang++-15 CXXFLAGS="-g -O2 -flto=thin" SECP_CFLAGS="-flto=thin"
1$ make -C src bitcoind

maflcko commented at 4:03 pm on January 7, 2024: member

unrelated: I wonder if one of the sanitizer CI tasks should have LTO enabled. Otherwise it is just oss-fuzz, which enables it, so errors and warnings are easier to miss.

m3dwards commented at 5:06 pm on January 11, 2024: none

Tested 2d1b1c7daeeada3f737e62ceb2db7484cde5ff4e on MacOS

0$ ./configure CC=clang CXX=clang++ CXXFLAGS="-g -v -flto=thin" SECP_CFLAGS="-g -flto=thin" LDFLAGS="-flto-jobs=3"
1$ make check

With -v flag on CXXFLAGS I could see that -flto=thin was present.

hebasto approved

hebasto commented at 1:16 pm on January 13, 2024: member

ACK 2d1b1c7daeeada3f737e62ceb2db7484cde5ff4e.

fanquake referenced this in commit d72125b994 on Jan 15, 2024

fanquake referenced this in commit 6dbafdb27b on Jan 15, 2024

DrahtBot added the label CI failed on Jan 15, 2024

fanquake referenced this in commit 75f1ff253b on Jan 15, 2024

fanquake referenced this in commit 416ba3f353 on Jan 15, 2024

fanquake commented at 2:30 pm on January 15, 2024: member

This is waiting on https://github.com/google/oss-fuzz/pull/11498 downstream.

DavidKorczynski referenced this in commit abfa448c2a on Jan 16, 2024

fanquake merged this on Jan 16, 2024

fanquake closed this on Jan 16, 2024

fanquake deleted the branch on Jan 16, 2024

build: remove --enable-lto #29185

Code Coverage

Reviews

Conflicts

build: remove `--enable-lto` #29185