gitian-linux: Build binaries for 64-bit POWER (continued) #20963
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
|
@dongcarl I guess a parallel set of changes to the guix build would be necessary ? |
|
I believe so, will look into it! |
|
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 25cd539 "gitian: Always specify noexecstack since we enforce it unconditionally", replacing it with a commit that extends the workaround for the powerpc architectures. |
| @@ -813,6 +813,11 @@ AX_GCC_FUNC_ATTRIBUTE([dllimport]) | |||
| if test x$use_glibc_compat != xno; then | |||
| 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"]) | |||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Older glibc don't have support for the optimisation (hence it being part of glibc compat ;) )
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
|
Also, if we're looking into command-line options for optimizing further the first thing would be to consider using |
Gitian builds
|
|
Looks like the gitian build is working, nice. |
| @@ -118,7 +130,7 @@ script: | | |||
| # Extract the git archive into a dir for each host and build | |||
| for i in ${HOSTS}; do | |||
| export PATH=${BASEPREFIX}/${i}/native/bin:${ORIGPATH} | |||
| if [ "${i}" = "riscv64-linux-gnu" ]; then | |||
| if [ "${i}" = "riscv64-linux-gnu" ] || [ "${i}" = "powerpc64-linux-gnu" ] || [ "${i}" = "powerpc64le-linux-gnu" ]; then | |||
There was a problem hiding this comment.
Why does this remain conditional? We enforce it on all platforms; might as well make it explicit.
There was a problem hiding this comment.
Because that is unsafe, see rationale here: #20963 (comment)
These lines can go away completely after gcc upgrade that fixes the underlying issue. But no sooner.
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.