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

fanquake · 2024-01-05T15:20:07Z

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.

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.

DrahtBot · 2024-01-05T15:20:09Z

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.

maflcko · 2024-01-05T15:26:58Z

... 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 · 2024-01-05T15:34:19Z

Concept ACK.

fanquake · 2024-01-05T15:36:27Z

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 · 2024-01-05T15:43:55Z

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

ACK 2d1b1c7

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

hebasto · 2024-01-06T17:38:38Z

Tested 2d1b1c7 on Ubuntu 22.04:

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

$ make -C depends CXXFLAGS="-O2 -flto=auto" LTO=1
$ ./configure CONFIG_SITE=$PWD/depends/x86_64-pc-linux-gnu/share/config.site SECP_CFLAGS="-flto=auto"
$ 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.

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

maflcko · 2024-01-07T16:03:38Z

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 · 2024-01-11T17:06:29Z

Tested 2d1b1c7 on MacOS

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

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

hebasto

ACK 2d1b1c7.

This is going to be removed upstream. bitcoin/bitcoin#29185.

fanquake · 2024-01-15T14:30:50Z

This is waiting on google/oss-fuzz#11498 downstream.

This is going to be removed upstream: bitcoin/bitcoin#29185. Skip `CFLAGS` for now. It looks like previously this was not getting properly applied, but would now run into a (fixed in later versions) LLVM bug. Set `NM` & `STRIP` to `llvm-*` so we are consistently using LLVM tools.

, bitcoin#28622, bitcoin#28880, bitcoin#29185, bitcoin#29170, bitcoin#29233, bitcoin#29298, bitcoin#29598, bitcoin#29732, bitcoin#29890, bitcoin#29739, bitcoin#30074, bitcoin#30198, bitcoin#29072 (toolchain backports: part 2) 1506d9d merge bitcoin#29072: use `-no_exported_symbols` on macOS (Kittywhiskers Van Gogh) 9247960 merge bitcoin#30198: qt 5.15.14 and fix macOS build with Clang 18 (Kittywhiskers Van Gogh) 5585e7a merge bitcoin#30074: use ENV flags in get_arch (Kittywhiskers Van Gogh) decd420 merge bitcoin#29739: swap cctools otool for llvm-objdump (Kittywhiskers Van Gogh) 0f8c420 merge bitcoin#29890: remove some tools when cross-compiling for macOS (Kittywhiskers Van Gogh) 936da1a merge bitcoin#29732: qt 5.15.13 (Kittywhiskers Van Gogh) c294b47 revert: patch qt to make placeholders differ from actual text (Kittywhiskers Van Gogh) af7090c merge bitcoin#29598: don't use -h with touch on OpenBSD (Kittywhiskers Van Gogh) ebf8ff2 merge bitcoin#29298: patch libtool out of libnatpmp/miniupnpc (Kittywhiskers Van Gogh) 070b876 merge bitcoin#29233: depends move macOS C(XX) FLAGS out of C & CXX (Kittywhiskers Van Gogh) d838481 revert dash#2398: Force fvisibility=hidden when compiling on macos (Kittywhiskers Van Gogh) 59a18f9 merge bitcoin#29170: add macho branch protection check (Kittywhiskers Van Gogh) cb024d9 merge bitcoin#29185: remove `--enable-lto` (Kittywhiskers Van Gogh) 6d75a81 merge bitcoin#28880: switch to using LLVM 17.x for macOS builds (Kittywhiskers Van Gogh) 7b0a1f2 merge bitcoin#28622: use macOS 14 SDK (Xcode 15.0) (Kittywhiskers Van Gogh) 02eb735 merge bitcoin#24948: fix typo in permissions (Kittywhiskers Van Gogh) 2739107 merge bitcoin#24534: make gen-sdk deterministic (Kittywhiskers Van Gogh) ab10bf9 merge bitcoin#24241: cleanup doc on need of Developer Account to obtain macOS SDK (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependent on #6384 * Dependency for #6389 * The Qt patch introduced in [dash#5596](#5596), `fix_qt_placeholders.patch`, was a portion of a suggested workaround for QTBUG-92199 ([source](https://bugreports.qt.io/browse/QTBUG-92199?focusedId=669719&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-669719)) but since then, a fix ([here](https://codereview.qt-project.org/c/qt/qtbase/+/434310)) has made its way to 5.15.12 and we are upgrading to 5.15.14 from 5.15.11. So we can safely remove this patch. ## Breaking Changes None expected ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 1506d9d PastaPastaPasta: utACK 1506d9d Tree-SHA512: df8e4ea0ce9e7b269d248518698f0566b5eca1a54cdfb53f5b213b90fb5177e5a5df44eaeb6f3fc014cd93351c9245736bb2fd52bc2af4ae274d8fa93e601b07

DrahtBot added the Build system label Jan 5, 2024

TheCharlatan approved these changes Jan 5, 2024

View reviewed changes

DrahtBot requested a review from hebasto January 5, 2024 16:38

This was referenced Jan 6, 2024

build: Fix LTO functionality #28876

Closed

[POC] guix: produce a fully -static-pie bitcoind #25573

Draft

hebasto mentioned this pull request Jan 6, 2024

guix: Use LTO to build releases #25391

Closed

This was referenced Jan 8, 2024

cmake: Add LTO functionality hebasto/bitcoin#52

Closed

CMake-based build system tracking issue #28607

Closed

This was referenced Jan 8, 2024

build: always set -g -O2 in CORE_CXXFLAGS #29205

Merged

build: LLD based macOS toolchain #21778

Merged

DrahtBot mentioned this pull request Jan 12, 2024

build: depends move macOS C(XX) FLAGS out of C & CXX #29233

Merged

hebasto approved these changes Jan 13, 2024

View reviewed changes

DrahtBot added the CI failed label Jan 15, 2024

fanquake added a commit to fanquake/oss-fuzz that referenced this pull request Jan 15, 2024

bitcoin: remove --enable-lto usage

416ba3f

This is going to be removed upstream. bitcoin/bitcoin#29185.

fanquake mentioned this pull request Jan 15, 2024

bitcoin: remove --enable-lto usage google/oss-fuzz#11498

Merged

fanquake merged commit 2ac2821 into bitcoin:master Jan 16, 2024

fanquake deleted the remove_enable_lto branch January 16, 2024 09:54

kwvg added a commit to kwvg/dash that referenced this pull request Nov 7, 2024

merge bitcoin#29185: remove --enable-lto

af34ca6

kwvg added a commit to kwvg/dash that referenced this pull request Nov 7, 2024

merge bitcoin#29185: remove --enable-lto

bfed6fc

kwvg added a commit to kwvg/dash that referenced this pull request Nov 15, 2024

merge bitcoin#29185: remove --enable-lto

c2d19e0

kwvg added a commit to kwvg/dash that referenced this pull request Nov 16, 2024

merge bitcoin#29185: remove --enable-lto

c09572e

kwvg added a commit to kwvg/dash that referenced this pull request Nov 17, 2024

merge bitcoin#29185: remove --enable-lto

cb024d9

bitcoin locked and limited conversation to collaborators Jan 15, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

fanquake commented Jan 5, 2024 •

edited

Loading

Uh oh!

DrahtBot commented Jan 5, 2024 •

edited

Loading

Uh oh!

maflcko commented Jan 5, 2024

Uh oh!

hebasto commented Jan 5, 2024

Uh oh!

fanquake commented Jan 5, 2024

Uh oh!

maflcko commented Jan 5, 2024

Uh oh!

TheCharlatan left a comment

Uh oh!

hebasto commented Jan 6, 2024 •

edited

Loading

Uh oh!

maflcko commented Jan 7, 2024

Uh oh!

m3dwards commented Jan 11, 2024

Uh oh!

hebasto left a comment

Uh oh!

fanquake commented Jan 15, 2024

Uh oh!

Uh oh!

build: remove --enable-lto #29185

build: remove --enable-lto #29185

Uh oh!

Conversation

fanquake commented Jan 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jan 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

maflcko commented Jan 5, 2024

Uh oh!

hebasto commented Jan 5, 2024

Uh oh!

fanquake commented Jan 5, 2024

Uh oh!

maflcko commented Jan 5, 2024

Uh oh!

TheCharlatan left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jan 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jan 7, 2024

Uh oh!

m3dwards commented Jan 11, 2024

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented Jan 15, 2024

Uh oh!

Uh oh!

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

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

fanquake commented Jan 5, 2024 •

edited

Loading

DrahtBot commented Jan 5, 2024 •

edited

Loading

hebasto commented Jan 6, 2024 •

edited

Loading