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

gitian-linux: Build binaries for 64-bit POWER #14066

Open
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@luke-jr
Copy link
Member

luke-jr commented Aug 26, 2018

A set of both big and little endian binaries, should be compatible with POWER8+.

Also splits libpng out of Qt (since Qt's bundled copy is broken on POWER) and disables JPEG (since we don't use it).

Tested only the little endian variant, on Gentoo.

(Based on #14065)

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Aug 26, 2018

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #14849 (depends: qt 5.9.7 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

This comment has been minimized.

Copy link
Member

laanwj commented Aug 27, 2018

Concept ACK

Not convinced, though, that we need a set of both big and little-endian binaries. What is the default on this platform? If you can only test little-endian, I think it's better to leave it at that.
(looks like both Debian and Fedora support LE, or are moving to it)

Also (not for this PR): what would be really interesting is running the gitian build (for all platforms) on the secure workstation. This would remove the single point of failure of dependency Intel/AMD platforms for the build.

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Aug 30, 2018

There is no default. Some older systems can only boot to big endian. Newer systems come in either BE and LE variant. Unfortunately, one cannot run even static BE binaries on LE or vice-versa (on Linux, anyway).

@awilfox

This comment has been minimized.

Copy link

awilfox commented Aug 30, 2018

At least RHEL, SuSE, and Adélie ship big-endian images.

Big endian is simple to run in KVM paravirt on the little-endian systems with a performance hit of less than 1%. This should be very easy to test, especially with Gentoo, since Gentoo can make a cross-root somewhere and then you can just point KVM at that disk.

Any POWER7 or older system, including most workstations before the Talos (including IBM's post-RS and Apple's 970), are exclusively big endian.

Any system (of any generation) running openSuSE Tumbleweed or Adélie is exclusively big endian.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Sep 8, 2018

Any POWER7 or older system, including most workstations before the Talos (including IBM's post-RS and Apple's 970), are exclusively big endian.

Those are outside the scope of this PR, though, OP mentions this is POWER8+.

@luke-jr luke-jr force-pushed the luke-jr:gitian_power64 branch from 05cd16b to c921a19 Sep 18, 2018

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Sep 18, 2018

Rebased

@DrahtBot DrahtBot removed the Needs rebase label Sep 18, 2018

@theuni
Copy link
Member

theuni left a comment

Concept ACK, a few questions though.

words = line.split()
if 'Machine:' in words:
arch = words[-1]
m = re.match(r'^\s*\d+:\s*[\da-f]+\s+\d+\s(?:(?:\S+\s+){3})(?:\[.*\]\s+)?(\S+)\s+(\S+).*$', line)

This comment has been minimized.

@theuni

theuni Sep 26, 2018

Member

I have no clue what this is suppose to do.

Perhaps a different tool like objdump or nm has output that's more easily parsed?

This comment has been minimized.

@laanwj

laanwj Oct 18, 2018

Member

ahhh
even parsing the ELF directly would be easier here than this

This comment has been minimized.

@luke-jr

luke-jr Oct 20, 2018

Member

This is a fairly straightforward regex... Do we really need to ban regexs? :/

Looking at alternatives:

  • Debian's linter uses almost the same regex (they tolerate multiple spaces in one place this didn't).
  • Perl's Parse::Readelf module does some fancy parsing of column headers and stuff, and doesn't really seem like it works (not to mention only supports printing its output, not actual code usage), and adds a lot of complexity we really don't need.
  • pyelftools doesn't seem to get symbol version information for some reason.

This comment has been minimized.

@laanwj

laanwj Oct 20, 2018

Member

I don't think it's that complex now that you've explained what it does on IRC, and why the old parsing doesn't work for POWER (due to [<localentry>: 8])—please add a comment in that regard (and thanks for investigating that readelf is apparently still the right tool for the job)

@@ -674,6 +674,11 @@ if test x$use_glibc_compat != xno; then
AC_DEFINE_UNQUOTED(FDELT_TYPE, $fdelt_type,[parameter and return value type for __fdelt_chk])
AX_CHECK_LINK_FLAG([[-Wl,--wrap=__divmoddi4]], [COMPAT_LDFLAGS="$COMPAT_LDFLAGS -Wl,--wrap=__divmoddi4"])
AX_CHECK_LINK_FLAG([[-Wl,--wrap=log2f]], [COMPAT_LDFLAGS="$COMPAT_LDFLAGS -Wl,--wrap=log2f"])
case $host in
powerpc64* | ppc64*)
AX_CHECK_LINK_FLAG([[-Wl,--no-tls-get-addr-optimize]], [COMPAT_LDFLAGS="$COMPAT_LDFLAGS -Wl,--no-tls-get-addr-optimize"])

This comment has been minimized.

@theuni

theuni Sep 26, 2018

Member

If this were a gcc switch, we would test against --tls-get-addr-optimize, then add --no-tls-get-addr-optimize to COMPAT_LDFLAGS if it succeeded. That is because gcc issues warnings for --no-foo if unsupported, but errors for --foo.

I'm thinking we should do the same here. I don't know how most linkers handle this, but I can't imagine that causing any problems. That would also mean that we could safely run this check for all platforms without constraining it to specific ppc's.

This comment has been minimized.

@luke-jr

luke-jr Oct 20, 2018

Member

It should work without the no- prefix, but upon further research, I'm not sure if this is the correct solution at all.

According to the documentation, systems without the glibc-side part can still explicitly enable the option and produce binaries that work (with a performance hit) on older systems. I'm not sure how this is technically implemented.

This comment has been minimized.

@luke-jr

luke-jr Oct 21, 2018

Member

Managed to get an old glibc running and confirmed --no-tls-get-addr-optimize is in fact needed. No clue why the docs imply otherwise. :/

This comment has been minimized.

@luke-jr

luke-jr Oct 22, 2018

Member

As for the original comment:

  1. At least GNU ld errors if --no-foo is provided.
  2. Other platforms might not have compatibility issues using this flag.
endef

define $(package)_build_cmds
$(MAKE) $($(package)_build_opts) PNG_COPTS='-fPIC'

This comment has been minimized.

@theuni

theuni Sep 26, 2018

Member

Why this instead of doing it the usual way via configure?

This comment has been minimized.

@luke-jr

luke-jr Oct 21, 2018

Member

No idea why, but it doesn't seem to work the usual way via configure.

/usr/lib/gcc-cross/riscv64-linux-gnu/8/../../../../riscv64-linux-gnu/bin/ld: /home/ubuntu/build/bitcoin/depends/riscv64-linux-gnu/lib/libpng16.a(png.o): relocation R_RISCV_HI20 against `a local symbol' can not be used when making a shared object; recompile with -fPIC
/usr/lib/gcc-cross/riscv64-linux-gnu/8/../../../../riscv64-linux-gnu/bin/ld: /home/ubuntu/build/bitcoin/depends/riscv64-linux-gnu/lib/libpng16.a(pngerror.o): relocation R_RISCV_HI20 against `a local symbol' can not be used when making a shared object; recompile with -fPIC
/usr/lib/gcc-cross/riscv64-linux-gnu/8/../../../../riscv64-linux-gnu/bin/ld: /home/ubuntu/build/bitcoin/depends/riscv64-linux-gnu/lib/libpng16.a(pngget.o): relocation R_RISCV_HI20 against `__stack_chk_guard@@GLIBC_2.27' can not be used when making a shared object; recompile with -fPIC
...
@@ -79,7 +85,11 @@ script: |
echo "REAL=\`which -a ${i}-${prog}-8 | grep -v ${WRAP_DIR}/${i}-${prog} | head -1\`" >> ${WRAP_DIR}/${i}-${prog}
echo 'export LD_PRELOAD=/usr/lib/x86_64-linux-gnu/faketime/libfaketime.so.1' >> ${WRAP_DIR}/${i}-${prog}
echo "export FAKETIME=\"$1\"" >> ${WRAP_DIR}/${i}-${prog}
echo "exec \"\$REAL\" \"\$@\"" >> $WRAP_DIR/${i}-${prog}
if [ "${i:0:9}" = "powerpc64" ]; then
echo "exec \"\$REAL\" -mcpu=power8 -mtune=power9 \"\$@\"" >> $WRAP_DIR/${i}-${prog}

This comment has been minimized.

@theuni

theuni Sep 26, 2018

Member

This looks like something that should be handled in depends/hosts/linux.mk

This comment has been minimized.

@luke-jr

luke-jr Oct 20, 2018

Member

What if someone else wants to build for POWER7 (outside of gitian)?

This comment has been minimized.

@awilfox

awilfox Oct 21, 2018

Is there a reason for specifying -mcpu=power8 here at all? VSX goes back to P7 and VMX goes back to P4…

This comment has been minimized.

@luke-jr

luke-jr Oct 21, 2018

Member

Maybe not. How far back can we safely go without a noticeable performance hit?

Would also be good to confirm #13203 still works if compiled for pre-POWER8.

This comment has been minimized.

@awilfox

awilfox Oct 22, 2018

vec_vsx_ld is POWER7; but the SHA sigma instructions are exclusive to POWER8 and newer. However, it looks like the merged code does check the hardware cap before using it, so it depends on compiler support, not build hardware support. (That is, if my understanding is correct, one could build all the way down to -mcpu=generic and you'd still get speed on P8+.)

@@ -573,7 +573,7 @@ clean-local:
check-symbols: $(bin_PROGRAMS)
if GLIBC_BACK_COMPAT
@echo "Checking glibc back compat..."
$(AM_V_at) READELF=$(READELF) CPPFILT=$(CPPFILT) $(top_srcdir)/contrib/devtools/symbol-check.py < $(bin_PROGRAMS)
$(AM_V_at) READELF=$(READELF) CPPFILT=$(CPPFILT) $(top_srcdir)/contrib/devtools/symbol-check.py $(BITCOIN_SYMBOL_CHECK_OPTIONS) < $(bin_PROGRAMS)

This comment has been minimized.

@theuni

theuni Sep 26, 2018

Member

What is BITCOIN_SYMBOL_CHECK_OPTIONS ? I don't see any references to it, is this just intended to be an optional env var for local testing?

This comment has been minimized.

@luke-jr

luke-jr Oct 18, 2018

Member

Should have been dropped in rebase; will remove next revision.

@luke-jr luke-jr force-pushed the luke-jr:gitian_power64 branch from f81ba95 to 02ba489 Oct 22, 2018

@bitcoin bitcoin deleted a comment from DrahtBot Oct 24, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Oct 25, 2018

Gitian builds for commit e895fdc (master):

Gitian builds for commit e4123b3 (master and this pull):

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Dec 13, 2018

Needs rebase

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 24, 2018

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 24, 2018

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 24, 2018

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 24, 2018

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 24, 2018

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 24, 2018

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 24, 2018

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 29, 2018

@luke-jr luke-jr force-pushed the luke-jr:gitian_power64 branch from 02ba489 to f6c91d4 Dec 30, 2018

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