Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

guix: Use LTO to build releases #25391

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

fanquake
Copy link
Member

@fanquake fanquake commented Jun 16, 2022

Changes required to use LTO in Guix (release) builds.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 16, 2022

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
Concept ACK laanwj, TheCharlatan
Approach ACK 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:

  • #30201 (depends: remove FORCE_USE_SYSTEM_CLANG 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.

@laanwj
Copy link
Member

laanwj commented Jun 22, 2022

Concept ACK. This would be great.

@hebasto
Copy link
Member

hebasto commented Jul 5, 2022

... build all binaries except for bitcoin-qt

See #25542.

@hebasto
Copy link
Member

hebasto commented Jul 10, 2022

Tested 076aa5e on Ubuntu 22.04:

$ make -C depends HOST=riscv64-linux-gnu LTO=1 NO_QT=1
$ ./autogen.sh
$ ./configure CONFIG_SITE=depends/riscv64-linux-gnu/share/config.site
$ make clean
$ make
...
  CXXLD    libbitcoinconsensus.la
/usr/lib/gcc-cross/riscv64-linux-gnu/11/../../../../riscv64-linux-gnu/bin/ld: secp256k1/.libs/libsecp256k1.a(libsecp256k1_la-secp256k1.o): Relocations in generic ELF (EM: 62)
/usr/lib/gcc-cross/riscv64-linux-gnu/11/../../../../riscv64-linux-gnu/bin/ld: secp256k1/.libs/libsecp256k1.a(libsecp256k1_la-secp256k1.o): Relocations in generic ELF (EM: 62)
/usr/lib/gcc-cross/riscv64-linux-gnu/11/../../../../riscv64-linux-gnu/bin/ld: secp256k1/.libs/libsecp256k1.a(libsecp256k1_la-secp256k1.o): Relocations in generic ELF (EM: 62)
/usr/lib/gcc-cross/riscv64-linux-gnu/11/../../../../riscv64-linux-gnu/bin/ld: secp256k1/.libs/libsecp256k1.a(libsecp256k1_la-secp256k1.o): Relocations in generic ELF (EM: 62)
/usr/lib/gcc-cross/riscv64-linux-gnu/11/../../../../riscv64-linux-gnu/bin/ld: secp256k1/.libs/libsecp256k1.a(libsecp256k1_la-secp256k1.o): Relocations in generic ELF (EM: 62)
/usr/lib/gcc-cross/riscv64-linux-gnu/11/../../../../riscv64-linux-gnu/bin/ld: secp256k1/.libs/libsecp256k1.a(libsecp256k1_la-secp256k1.o): Relocations in generic ELF (EM: 62)
/usr/lib/gcc-cross/riscv64-linux-gnu/11/../../../../riscv64-linux-gnu/bin/ld: secp256k1/.libs/libsecp256k1.a(libsecp256k1_la-secp256k1.o): Relocations in generic ELF (EM: 62)
/usr/lib/gcc-cross/riscv64-linux-gnu/11/../../../../riscv64-linux-gnu/bin/ld: secp256k1/.libs/libsecp256k1.a(libsecp256k1_la-secp256k1.o): Relocations in generic ELF (EM: 62)
/usr/lib/gcc-cross/riscv64-linux-gnu/11/../../../../riscv64-linux-gnu/bin/ld: secp256k1/.libs/libsecp256k1.a: error adding symbols: file in wrong format
collect2: error: ld returned 1 exit status
make[2]: *** [Makefile:6729: libbitcoinconsensus.la] Error 1
make[2]: *** Waiting for unfinished jobs....
make[2]: Leaving directory '/home/hebasto/git/bitcoin/src'
make[1]: *** [Makefile:18615: all-recursive] Error 1
make[1]: Leaving directory '/home/hebasto/git/bitcoin/src'
make: *** [Makefile:818: all-recursive] Error 1

UPDATE: CONFIG_SITE needs a absolute path. Sorry for the noise.

@fanquake
Copy link
Member Author

Tested 076aa5e on Ubuntu 22.04:

I'm not sure what's causing that (maybe a not-completely-clean build dir, and mix of compiled objects), but I don't have any issues with this branch, following the same steps, using Ubuntu 22.04 and riscv64-linux-gnu-g++-11.

depends/packages/expat.mk Outdated Show resolved Hide resolved
fanquake added a commit to bitcoin-core/gui that referenced this pull request Jul 18, 2022
… code on Linux

3442865 build: Use Link Time Optimization for Qt code on Linux (Hennadii Stepanov)
ebce66e build: pass -fno-lto when building expat (fanquake)

Pull request description:

  See: https://www.qt.io/blog/2019/01/02/qt-applications-lto

  `bitcon-qt` unstripped size:
  | host | master (31c6309) | this PR, depends built with `LTO=1` |
  |---|:-:|:-:|
  | x86_64-pc-linux-gnu | 42 MB | 35 MB |
  | arm-linux-gnueabihf | 31 MB | 26 MB |
  | aarch64-linux-gnu | 41 MB | 32 MB |
  | powerpc64-linux-gnu | 51 MB | 41 MB |
  | powerpc64le-linux-gnu | 48 MB | 39 MB |
  | riscv64-linux-gnu | 35 MB | 29 MB |

  Based on the first commit from bitcoin/bitcoin#25391.

  Using LTO for macOS and Windows hosts has some issues which could be addressed in follow ups.

  x86_64 build:
  ![image](https://user-images.githubusercontent.com/32963518/179326902-f91853ca-23c1-4c04-9a6d-161b695f27b5.png)

ACKs for top commit:
  fanquake:
    ACK 3442865

Tree-SHA512: 03eef2568358df9336e24d6c4e12f28b89d649076fb74e7e5303d61e52865c2360c5345a4fb2b1e4bdfdae194f273fc27a5f67e6cf797ed01a154f3da9117247
@fanquake fanquake changed the title [POC] guix: Use LTO to build releases guix: Use LTO to build releases Jul 18, 2022
@fanquake
Copy link
Member Author

Rebased for #25542. All binaries for our Linux HOSTS now build.

fanquake added a commit that referenced this pull request Jul 19, 2022
…th GCC)

658685a depends: default to using GCC tool wrappers (with GCC) (fanquake)
6fdc13c build: Fix autoconf variable names for tools found by `AC_PATH_TOOL` (Hennadii Stepanov)

Pull request description:

  This improves support for LTO by using gcc wrappers for `ar`, `nm`, `ranlib`,
  that correctly setup plugin arguments for LTO, when using GCC.

  Other HOSTS are using clang.

  Portion of #25391.

  Guix Build (x86_64):
  ```bash
  ```

  Guix Build (arm64):
  ```bash
  ```

ACKs for top commit:
  dongcarl:
    Code Review ACK 658685a
  hebasto:
    ACK 658685a
  jarolrod:
    ACK 658685a

Tree-SHA512: 28d6127c118f74336c97e2523117f8a0d11b32cd565124cd4052baeb7cc53e71909d3037cb080d996ae4e3ce600326fced37ee36adcc53d839ba7dd7974ebcd2
@hebasto
Copy link
Member

hebasto commented Jan 6, 2024

Related:

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 14, 2024
4133c81 guix: use gcc tool wrappers (fanquake)

Pull request description:

  This way, correct `--plugin` arguments are passed through.

  This is a prerequisite for LTO (see bitcoin#25391). Split out, to try move things along, as this change is isolated, and should be straight-forward.

ACKs for top commit:
  TheCharlatan:
    ACK [4133c81](bitcoin@4133c81)
  hebasto:
    ACK 4133c81

Tree-SHA512: 4311a72a613cf027bd4490caa29604c985ed455589acd972285f13cbdf4806d2184a4dc6f20cb6f47c3fa751d58bfd0bacc257b87d4a804bf5ecf5b240e4a757
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 16, 2024
4133c81 guix: use gcc tool wrappers (fanquake)

Pull request description:

  This way, correct `--plugin` arguments are passed through.

  This is a prerequisite for LTO (see bitcoin#25391). Split out, to try move things along, as this change is isolated, and should be straight-forward.

ACKs for top commit:
  TheCharlatan:
    ACK [4133c81](bitcoin@4133c81)
  hebasto:
    ACK 4133c81

Tree-SHA512: 4311a72a613cf027bd4490caa29604c985ed455589acd972285f13cbdf4806d2184a4dc6f20cb6f47c3fa751d58bfd0bacc257b87d4a804bf5ecf5b240e4a757
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 19, 2024
4133c81 guix: use gcc tool wrappers (fanquake)

Pull request description:

  This way, correct `--plugin` arguments are passed through.

  This is a prerequisite for LTO (see bitcoin#25391). Split out, to try move things along, as this change is isolated, and should be straight-forward.

ACKs for top commit:
  TheCharlatan:
    ACK [4133c81](bitcoin@4133c81)
  hebasto:
    ACK 4133c81

Tree-SHA512: 4311a72a613cf027bd4490caa29604c985ed455589acd972285f13cbdf4806d2184a4dc6f20cb6f47c3fa751d58bfd0bacc257b87d4a804bf5ecf5b240e4a757
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 24, 2024
4133c81 guix: use gcc tool wrappers (fanquake)

Pull request description:

  This way, correct `--plugin` arguments are passed through.

  This is a prerequisite for LTO (see bitcoin#25391). Split out, to try move things along, as this change is isolated, and should be straight-forward.

ACKs for top commit:
  TheCharlatan:
    ACK [4133c81](bitcoin@4133c81)
  hebasto:
    ACK 4133c81

Tree-SHA512: 4311a72a613cf027bd4490caa29604c985ed455589acd972285f13cbdf4806d2184a4dc6f20cb6f47c3fa751d58bfd0bacc257b87d4a804bf5ecf5b240e4a757
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 28, 2024
4133c81 guix: use gcc tool wrappers (fanquake)

Pull request description:

  This way, correct `--plugin` arguments are passed through.

  This is a prerequisite for LTO (see bitcoin#25391). Split out, to try move things along, as this change is isolated, and should be straight-forward.

ACKs for top commit:
  TheCharlatan:
    ACK [4133c81](bitcoin@4133c81)
  hebasto:
    ACK 4133c81

Tree-SHA512: 4311a72a613cf027bd4490caa29604c985ed455589acd972285f13cbdf4806d2184a4dc6f20cb6f47c3fa751d58bfd0bacc257b87d4a804bf5ecf5b240e4a757
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 31, 2024
Summary:
This way, correct `--plugin` argument are passed through.

This is a prerequisite for LTO (see [[bitcoin/bitcoin#25391 | core#25391]]).

This is a backport of [[bitcoin/bitcoin#27345 | core#27345]]

Depends on D15328

Test Plan: `contrib/guix.guix-build`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D15329
PastaPastaPasta pushed a commit to dashpay/dash that referenced this pull request Feb 7, 2024
4133c81 guix: use gcc tool wrappers (fanquake)

Pull request description:

  This way, correct `--plugin` arguments are passed through.

  This is a prerequisite for LTO (see bitcoin#25391). Split out, to try move things along, as this change is isolated, and should be straight-forward.

ACKs for top commit:
  TheCharlatan:
    ACK [4133c81](bitcoin@4133c81)
  hebasto:
    ACK 4133c81

Tree-SHA512: 4311a72a613cf027bd4490caa29604c985ed455589acd972285f13cbdf4806d2184a4dc6f20cb6f47c3fa751d58bfd0bacc257b87d4a804bf5ecf5b240e4a757
@fanquake fanquake force-pushed the lto_in_guix branch 2 times, most recently from 9907c82 to 739826f Compare May 22, 2024 09:32
@fanquake fanquake marked this pull request as ready for review May 22, 2024 09:58
```bash
bitcoind: export of symbol in6addr_any not allowed!
bitcoind: failed EXPORTED_SYMBOLS
test/test_bitcoin: export of symbol in6addr_any not allowed!
test/test_bitcoin: failed EXPORTED_SYMBOLS
```

Also new exports for bitcoin-qt.
@maflcko
Copy link
Member

maflcko commented Jun 4, 2024

Is this safe to do in light of https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105469 ?

Also https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113359 (in combination with #29881)

Would be nice to extend the motivation (pull request description) a bit about the benefits and risks.

@DrahtBot
Copy link
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants