gitian-linux: Build binaries for 64-bit POWER (continued) #20963

laanwj commented at 1:44 PM on January 19, 2021: member

Rebase of #14066 by luke-jr.

Let's try to get PowerPC support in in the beginning of the 22.0 cycle so that it gets some testing, and is not a last-minute decision this time, like for last … 2 or 3 major versions.

The symbol/security tooling-related changes have been dropped since they were part of #20434.

Support glibc-back-compat on 64-bit POWER 798bc0b29a

gitian: Properly quote arguments in wrappers 63fc2b1782

laanwj added the label Build system on Jan 19, 2021

laanwj added this to the milestone 22.0 on Jan 19, 2021

laanwj added the label Needs gitian build on Jan 19, 2021

DrahtBot commented at 6:51 PM on January 19, 2021: member

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

Conflicts

No conflicts as of last run.

laanwj commented at 6:59 PM on January 19, 2021: member

@dongcarl I guess a parallel set of changes to the guix build would be necessary ?

dongcarl commented at 7:41 PM on January 19, 2021: member

I believe so, will look into it!

in contrib/gitian-descriptors/gitian-linux.yml:127 in 25cd539e75 outdated

 122 | -      # Workaround for https://bugs.launchpad.net/ubuntu/+source/gcc-8-cross-ports/+bug/1853740
 123 | -      # TODO: remove this when no longer needed
 124 | -      HOST_LDFLAGS="${HOST_LDFLAGS_BASE} -Wl,-z,noexecstack"
 125 | -    else
 126 | -      HOST_LDFLAGS="${HOST_LDFLAGS_BASE}"
 127 | -    fi

laanwj commented at 12:22 PM on January 20, 2021:

So the reason that this is a workaround is that it's not 100% safe. Normally the object files are supposed to flag that they support non-executable stack. Due to a bug in that verson of RISC-V gcc, this is not signaled.

This linker flag forces non-executable stack, overriding individual object files' preference.

I'm not sure it's a good idea to set this by default. If it's required for PPC here, we should add a specific exception.

laanwj requested review from theuni on Jan 20, 2021

laanwj requested review from dongcarl on Jan 20, 2021

DrahtBot commented at 12:10 AM on January 21, 2021: member

Gitian builds

File	commit 80486e7e2d8c360839cffc5a0c597b5c745433a7<br>(master)	commit ee03c94b6133fb84c4006f040705fbb87182e01f<br>(master and this pull)
*-aarch64-linux-gnu-debug.tar.gz	`c465d0562d4f4b36...`	`e8aabfb614ab25e0...`
*-aarch64-linux-gnu.tar.gz	`a6641c173362914a...`	`2223b2b7c531a1d3...`
*-arm-linux-gnueabihf-debug.tar.gz	`ba68555a607871cc...`	`ca7d3b96eb37b4bb...`
*-arm-linux-gnueabihf.tar.gz	`4ce722a1269a9d2d...`	`ca65fc8dc9d6b949...`
*-osx-unsigned.dmg	`bdccbb7e72b4e99b...`	`1b2078a7ffee5aa7...`
*-osx64.tar.gz	`4472625dadeb2508...`	`58fa19de2989b673...`
*-riscv64-linux-gnu-debug.tar.gz	`04924da7a5c56db6...`	`ae61fdde150b6161...`
*-riscv64-linux-gnu.tar.gz	`459b64c697c4420f...`	`dad7214844fe8295...`
*-win64-debug.zip	`7285ea82230fcfaf...`	`c8d0ce1114d72689...`
*-win64-setup-unsigned.exe	`0119bbc9b21c059f...`	`0f7e626e6be14754...`
*-win64.zip	`74a588c5b8b08fec...`	`f2433f22292a412f...`
*-x86_64-linux-gnu-debug.tar.gz	`a4857fbfaffbd5e4...`	`c6d4f855475eaf90...`
*-x86_64-linux-gnu.tar.gz	`327cc993115833a3...`	`4ab6aba49f03c4c8...`
*.tar.gz	`48c6d93a410e9c2f...`	`fd7a18d12847e62d...`
bitcoin-core-linux-22-res.yml	`be9f74e2ed2432dc...`	`45b04bb5e2790748...`
bitcoin-core-osx-22-res.yml	`be215d7dd97ca6a8...`	`8c445dd70e3f9e3a...`
bitcoin-core-win-22-res.yml	`874912f46b5cbe79...`	`5f881bb5fc7d026c...`
linux-build.log	`69e53fde951b7629...`	`1b595f50c366683f...`
osx-build.log	`9f75df11b2c9c875...`	`a59c78f25753082f...`
win-build.log	`85d5b9e92561b947...`	`a68b54187fe173e8...`
bitcoin-core-linux-22-res.yml.diff		`064f01f2fc8d8c00...`
bitcoin-core-osx-22-res.yml.diff		`1ee4f4ae99ccf9d8...`
bitcoin-core-win-22-res.yml.diff		`dbc1cf0563700ae8...`
linux-build.log.diff		`400b943562af2541...`
osx-build.log.diff		`b9a28df5a89863e4...`
win-build.log.diff		`06b194d3fbc8b40e...`

DrahtBot removed the label Needs gitian build on Jan 21, 2021

MarcoFalke commented at 6:52 PM on January 21, 2021: member

This is missing b74c21fad1ac2822ccdfa733d39265156afb31bb ...?

laanwj commented at 6:23 PM on January 24, 2021: member

This is missing b74c21f ...?

While rebasing it looked to me that that was only a change to the security script. But it seems not. Will add the gitian yml change.

Edit: Done, and removed 25cd539e756f6e2d514e21cfc902ff44abef7d10 "gitian: Always specify noexecstack since we enforce it unconditionally", replacing it with a commit that extends the workaround for the powerpc architectures.

gitian-linux: Build binaries for 64-bit POWER 00f67c8aa1

gitian-linux: Extend noexec-stack workaround to powerpc 543bf745d3

laanwj force-pushed on Jan 24, 2021

in configure.ac:818 in 798bc0b29a outdated

 812 | @@ -813,6 +813,11 @@ AX_GCC_FUNC_ATTRIBUTE([dllimport])
 813 |  if test x$use_glibc_compat != xno; then
 814 |    AX_CHECK_LINK_FLAG([[-Wl,--wrap=__divmoddi4]], [COMPAT_LDFLAGS="$COMPAT_LDFLAGS -Wl,--wrap=__divmoddi4"])
 815 |    AX_CHECK_LINK_FLAG([[-Wl,--wrap=log2f]], [COMPAT_LDFLAGS="$COMPAT_LDFLAGS -Wl,--wrap=log2f"])
 816 | +  case $host in
 817 | +    powerpc64* | ppc64*)
 818 | +      AX_CHECK_LINK_FLAG([[-Wl,--no-tls-get-addr-optimize]], [COMPAT_LDFLAGS="$COMPAT_LDFLAGS -Wl,--no-tls-get-addr-optimize"])

sipa commented at 9:32 AM on January 25, 2021:

Does this disable some optimization related to thread local storage? Is there a way to avoid that?

(just wondering why this is here; not an objection if it's needed)

laanwj commented at 10:51 AM on January 25, 2021:

I don't know. @luke-jr ? If this is a workaround for a platform-specific compiler issue, let's add a comment (preferably mentioning the upstream issue) so that it can be removed when no longer necessary.

luke-jr commented at 6:53 PM on January 25, 2021:

Older glibc don't have support for the optimisation (hence it being part of glibc compat ;) )

sipa commented at 7:43 PM on January 25, 2021:

@luke-jr Yes, of course. I guess my question is: do we know whether that has any performance impact (presumably not much for now, as thread_local variables are only used for thread names AFAIK), and/or has the alternative of requiring a higher minimum glibc been considered?

No problem if the answer is we don't know, or there are reasons for doing it this way. I just want to make sure we don't end up years from now chasing a weird performance regression on POWER9 in release binaries.

laanwj commented at 8:59 PM on January 25, 2021:

FWIW in #18851 I had implemented the thread name system without use of TLS, which would have dropped that entire dependency.

That said, I doubt for our use (one query per log message?) a slightly slower TLS makes any noticeable difference in performance.

laanwj commented at 2:08 PM on January 27, 2021: member

Also, if we're looking into command-line options for optimizing further the first thing would be to consider using -mcpu=power9 instead of power8/970 respectively. I strongly suspect that everyone interested in these binaries wants to run them on Talos II workstations. But this also can be changed later, it doesn't have to be decided in this PR.

laanwj added the label Needs gitian build on Jan 27, 2021

DrahtBot commented at 2:22 PM on January 28, 2021: member

Gitian builds

File	commit 15a9df070615e7e8f29c17a92b889f19218f25ac<br>(master)	commit 627e297a3ed3af0621ebff67de11a04f38e4e2a2<br>(master and this pull)
*-aarch64-linux-gnu-debug.tar.gz	`2de46bcf0f879816...`	`0869beff056c5552...`
*-aarch64-linux-gnu.tar.gz	`195f74e31b494b0b...`	`a38a4453b683a115...`
*-arm-linux-gnueabihf-debug.tar.gz	`745064644885b403...`	`00a86433087dddfa...`
*-arm-linux-gnueabihf.tar.gz	`fc0843f9c76fa049...`	`deffbbd78440e700...`
*-osx-unsigned.dmg	`d7c2b3b331c0170d...`	`cc9e1bb1b0a551dd...`
*-osx64.tar.gz	`b83656c71a45dafb...`	`6f62fdb74f503006...`
*-riscv64-linux-gnu-debug.tar.gz	`da866a3d0511d523...`	`52dd96c75294c4aa...`
*-riscv64-linux-gnu.tar.gz	`27d3649f343e0ad6...`	`b1419057c91ac3b0...`
*-win64-debug.zip	`30189a63e207f9c3...`	`78480c2ac739d352...`
*-win64-setup-unsigned.exe	`807d2d9629ec72ef...`	`69e225bb0bbd0f10...`
*-win64.zip	`706047c7a5b49d73...`	`796393c233b6313c...`
*-x86_64-linux-gnu-debug.tar.gz	`599768629238d732...`	`cb1bec7a1831b5d1...`
*-x86_64-linux-gnu.tar.gz	`88f8b0625fd2e065...`	`76f0aa08fe46a94f...`
*.tar.gz	`b7377a5354e934db...`	`111a13c87dc2dcd9...`
bitcoin-core-linux-22-res.yml	`0cec6dc965b5a1e5...`	`e5365ec962e53528...`
bitcoin-core-osx-22-res.yml	`08cb2567f8470ee0...`	`2629252af46d82f8...`
bitcoin-core-win-22-res.yml	`9d8aa3f7e0636e6e...`	`f5fb15efd00b4de5...`
linux-build.log	`cd52ede60a8dbbde...`	`4f99d3fa044f2e80...`
osx-build.log	`86ea2870a70d9d2d...`	`2a6a78b201e8647c...`
win-build.log	`edfffe6d6b7b25d0...`	`1c92a6037289c69c...`
*-powerpc64-linux-gnu-debug.tar.gz		`ccb1fdb2c7d966b6...`
*-powerpc64-linux-gnu.tar.gz		`2df6d7ff9f92dc1d...`
*-powerpc64le-linux-gnu-debug.tar.gz		`4b2b837a7020aca9...`
*-powerpc64le-linux-gnu.tar.gz		`429133693e740b1b...`
bitcoin-core-linux-22-res.yml.diff		`8c2d77155b8c213e...`
bitcoin-core-osx-22-res.yml.diff		`b64967946ae06812...`
bitcoin-core-win-22-res.yml.diff		`7fabc826b3573afa...`
linux-build.log.diff		`7a0bf66c7060f547...`
osx-build.log.diff		`a3941fc2d80ee41d...`
win-build.log.diff		`343a6f62a2bc65ab...`

DrahtBot removed the label Needs gitian build on Jan 28, 2021

laanwj commented at 5:29 PM on January 28, 2021: member

Looks like the gitian build is working, nice.

in contrib/gitian-descriptors/gitian-linux.yml:133 in 543bf745d3

 129 | @@ -118,7 +130,7 @@ script: |
 130 |    # Extract the git archive into a dir for each host and build
 131 |    for i in ${HOSTS}; do
 132 |      export PATH=${BASEPREFIX}/${i}/native/bin:${ORIGPATH}
 133 | -    if [ "${i}" = "riscv64-linux-gnu" ]; then
 134 | +    if [ "${i}" = "riscv64-linux-gnu" ] || [ "${i}" = "powerpc64-linux-gnu" ] || [ "${i}" = "powerpc64le-linux-gnu" ]; then

luke-jr commented at 9:04 PM on January 28, 2021:

Why does this remain conditional? We enforce it on all platforms; might as well make it explicit.

laanwj commented at 9:28 PM on January 28, 2021:

Because that is unsafe, see rationale here: #20963 (review)

These lines can go away completely after gcc upgrade that fixes the underlying issue. But no sooner.

luke-jr approved

luke-jr commented at 9:04 PM on January 28, 2021: member

utACK

laanwj merged this on Jan 28, 2021

laanwj closed this on Jan 28, 2021

DrahtBot locked this on Aug 18, 2022