8342822: jdk8u432-b06 does not compile on AIX#600
8342822: jdk8u432-b06 does not compile on AIX#600varada1110 wants to merge 5 commits intoopenjdk:masterfrom
Conversation
|
👋 Welcome back varadam! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
openjdk
bot
changed the title
|
/approval 8342822 request build error observed on aix is resolved with use of llabs(). Build and jtreg testing(hotspot/test) successful |
Webrevs
|
|
@varada1110 |
openjdk
bot
added
the
approval
|
@varada1110 Please enable GHA testing on your fork. Thanks! |
|
Hi @jerboaa, Could you please review the code changes. |
Some of the orginal reviewers should do that. @rwestrel @franferrax could you please take a look? Thank you! |
franferrax
left a comment
franferrax
left a comment
There was a problem hiding this comment.
Hi, I'm not a reviewer, but it looks good to me.
We are using:
long long int llabs (long long int n);Which should be fine for 32-bit systems because on those jlong is generally defined as long long (1).
It should also work fine in 64-bit systems because on those jlong is generally defined as long (1), and, in the LP64 data model, long long has the same width as long (2).
(1)
jdk8u-dev/hotspot/src/cpu/ppc/vm/jni_ppc.h
Lines 47 to 51 in 618917e
jdk8u-dev/hotspot/src/cpu/aarch64/vm/jni_aarch64.h
Lines 47 to 51 in 618917e
jdk8u-dev/hotspot/src/cpu/zero/vm/jni_zero.h
Lines 46 to 50 in 618917e
jdk8u-dev/hotspot/src/cpu/x86/vm/jni_x86.h
Lines 47 to 51 in 618917e
jdk8u-dev/hotspot/src/cpu/sparc/vm/jni_sparc.h
Lines 42 to 46 in 618917e
|
@varada1110: the cause of this build error seems to be that while long long int abs (long long int n);Haven't you found the same problem with |
|
Hi @franferrax , |
|
Hi @varada1110, Here is the jdk11u version of the failing code: src/hotspot/share/opto/superword.hpp:710. We may experience the same build error on AIX. I'm realizing my assumptions in the understanding of the issue were wrong. I thought it was a 32-bit build, but the build is for The problem seems to be in choosing between two equally suitable @varada1110 / @andrew-m-leonard: could you help me understand the root cause of this build issue? Did the With regard to the last change (8d18c55), I think it would be cleaner to just use the It's already being used in the file changed by this PR: jdk8u-dev/hotspot/src/share/vm/opto/superword.hpp Lines 292 to 295 in 618917e But I think we'll first want to understand why the build failed, and if it isn't failing in jdk11u, what's causing the difference in behavior. |
I found that for long long int abs can overload with 64 bit compilation. The similar compilation error is observed while building jdk8 on AIX. |
|
Where are we with this change? It doesn't seem that it is ready for approval, being still under review. |
|
That's an interesting question, and I suspect that must be because jdk8u is
built with IBM XL C/C++ v13.1.3, whereas jdk11u v16.1.0.
…On Fri, Nov 29, 2024 at 12:30 AM Andrew Hughes ***@***.***> wrote:
Where are we with this change? It doesn't seem that it is ready for
approval, being still under review.
It sounds like we need to know why this is being considered 8u only, when
the same abs call is used in later JDK versions.
—
Reply to this email directly, view it on GitHub
<#600 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHQDDN3HFEITURPZKX5BNM32C6YTVAVCNFSM6AAAAABQ563OJSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMBWHA4DIOBZGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Hi, sorry for my delayed reply, I'm resuming work on this issue. Here is the relevant part of the failure log (from We are building for ppc64, and searching the best candidate for
From @varada1110 tests, this candidate is available in Trying to get some external clues, I did a simple Fedora Linux x86-64 test with GCC. I found that g++ -pedantic -Wall -xc++ -std=c++11 -op -<<'EOF'
//////////////////////////////////////////////////
#include <iostream>
#include <stdlib.h>
int main() {
long var = -1234567890; // 0xffffffffb669fd2e
std::cout << abs(var) << std::endl;
}
//////////////////////////////////////////////////
EOF
./p
sha256sum p
objdump --insn-width=8 -dC p | grep -A10 'abs\|<main>'
rm -f pOutput: 1234567890
3657bd6d3bb686f0dc8e058cce73199bb38b7c20113f3c599a11629577dbb12b p
0000000000401146 <main>:
401146: 55 push %rbp
401147: 48 89 e5 mov %rsp,%rbp
40114a: 48 83 ec 10 sub $0x10,%rsp
40114e: 48 c7 45 f8 2e fd 69 b6 movq $0xffffffffb669fd2e,-0x8(%rbp)
401156: 48 8b 45 f8 mov -0x8(%rbp),%rax
40115a: 48 89 c7 mov %rax,%rdi
40115d: e8 21 00 00 00 call 401183 <std::abs(long)>
401162: 48 89 c6 mov %rax,%rsi
401165: bf 40 40 40 00 mov $0x404040,%edi
40116a: e8 e1 fe ff ff call 401050 <std::ostream::operator<<(long)@plt>
40116f: be 30 10 40 00 mov $0x401030,%esi
401174: 48 89 c7 mov %rax,%rdi
401177: e8 c4 fe ff ff call 401040 <std::ostream::operator<<(std::ostream& (*)(std::ostream&))@plt>
40117c: b8 00 00 00 00 mov $0x0,%eax
401181: c9 leave
401182: c3 ret
0000000000401183 <std::abs(long)>:
401183: 55 push %rbp
401184: 48 89 e5 mov %rsp,%rbp
401187: 48 89 7d f8 mov %rdi,-0x8(%rbp)
40118b: 48 8b 45 f8 mov -0x8(%rbp),%rax
40118f: 48 89 c2 mov %rax,%rdx
401192: 48 f7 da neg %rdx
401195: 48 0f 49 c2 cmovns %rdx,%rax
401199: 5d pop %rbp
40119a: c3 retIn fact, the following three combinations produce an identical binary:
Apparently, the 0000000000401146 <main>:
[...]
40114e: 48 c7 45 f8 2e fd 69 b6 movq $0xffffffffb669fd2e,-0x8(%rbp)
401156: 48 8b 45 f8 mov -0x8(%rbp),%rax
40115a: 89 c2 mov %eax,%edx
40115c: f7 da neg %edx
40115e: 0f 49 c2 cmovns %edx,%eax
[...]I will proceed to get Temporary test system access and continue experimentation in AIX ppc64. Some doubts I plan to clarify are:
Any other research suggestions or advices are welcome. |
|
The build error persists with xlc 16.1.0 version. Also observed that with <iostream.h> or adding inline abs() function resolves the issue. https://github.com/openjdk/jdk8u-dev/compare/master...varada1110:abs?expand=1 |
|
Hi, after getting access to an AIX system (thanks @sxa) I have a clearer understanding of what is going on. Tests I executed in AIX 7.2All the tests have the following structure, I will specify the C++ snippet and the results. cat >test.cpp <<'EOF'
// C++ snippet goes here
EOF
/opt/IBM/xlC/13.1.3/bin/xlC_r -q64 -c -o/dev/null test.cpp
/opt/IBM/xlC/16.1.0/bin/xlC_r -q64 -c -o/dev/null test.cpp
rm -f test.cppFinding the combination that causes the failureSnippets that workedAll the following snippets worked, both with Use
|
|
Thank you, @franferrax, for identifying the root cause of the issue. |
This seems to break Solaris which can't find the new header file: |
|
I have enclosed the header file within an #ifdef and made it specific to AIX to ensure compatibility |
franferrax
left a comment
franferrax
left a comment
There was a problem hiding this comment.
Hi @varada1110,
Great! The change looks good to me, is based on the root cause, and should be 100% innocuous for other platforms.
I would also like to know if this looks good to @rwestrel or @gnu-andrew. I'm not a reviewer and we will need approval from one anyway.
It looks pretty risk-free to me, if it is only applied where it is needed (on AIX) I would suggest moving this PR to github.com/openjdk/jdk8u as a regression fix aimed at the January release. Otherwise, 8u442 will be broken on AIX as well (8u-dev is now 8u452, due for release in April 2025). Thanks for the detailed analysis and testing. I wasn't aware there were public build runs on AIX either. Maybe this is worth referencing on https://wiki.openjdk.org/display/Build/Supported+Build+Platforms ? |
Bonus
I was wrong, after a quick trick to ignore a missing So the previous tests were wrong, and in fact cat >test.cpp <<'EOF'
#include <cmath>
#include <stdlib.h>
long test() {
return abs((long)1);
}
EOF
/opt/IBM/xlC/16.1.0/bin/xlclang++ -q64 -c -o/dev/null test.cpp
rm -f test.cppSo we won't hit this in 11u. For reference, here is the full /opt/IBM/xlC/16.1.0/bin/xlclang++ -qmakedep=gcc \
-MF ./build/aix-ppc64-normal-server-release/hotspot/variant-server/libjvm/objs/loopnode.d.tmp \
-D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -D_REENTRANT \
-qtbtable=full -qtune=balanced -qalias=noansi -qstrict -qtls=default -qnortti -qnoeh \
-qignerrno -qstackprotect -DLIBC=default -DSUPPORTS_CLOCK_MONOTONIC -DAIX \
-qpic=large -qarch=ppc64 -q64 -DNDEBUG -DPRODUCT -DTARGET_ARCH_ppc -DINCLUDE_SUFFIX_OS=_aix \
-DINCLUDE_SUFFIX_CPU=_ppc -DINCLUDE_SUFFIX_COMPILER=_xlc -DTARGET_COMPILER_xlc -DPPC64 \
-DHOTSPOT_LIB_ARCH='"ppc64"' -DCOMPILER1 -DCOMPILER2 -DINCLUDE_JVMCI=0 -DINCLUDE_CDS=0 \
-DINCLUDE_AOT=0 -DINCLUDE_ZGC=0 -DINCLUDE_SHENANDOAHGC=0 -DINCLUDE_JFR=0 \
-I ./build/aix-ppc64-normal-server-release/hotspot/variant-server/gensrc/adfiles \
-I ./src/hotspot/share \
-I ./src/hotspot/os/aix \
-I ./src/hotspot/os/posix \
-I ./src/hotspot/cpu/ppc \
-I ./src/hotspot/os_cpu/aix_ppc \
-I ./build/aix-ppc64-normal-server-release/hotspot/variant-server/gensrc \
-I ./src/hotspot/share/precompiled \
-I ./src/hotspot/share/include \
-I ./src/hotspot/os/posix/include \
-I ./build/aix-ppc64-normal-server-release/support/modules_include/java.base \
-I ./build/aix-ppc64-normal-server-release/support/modules_include/java.base/aix \
-I ./src/java.base/share/native/libjimage -q64 -DDONT_USE_PRECOMPILED_HEADER -g1 \
-qsuppress=1540-0216 -qsuppress=1540-0198 -qsuppress=1540-1090 -qsuppress=1540-1639 \
-qsuppress=1540-1088 -qsuppress=1500-010 -O3 -qhot=level=1 -qinline -qinlglue -c \
-o ./build/aix-ppc64-normal-server-release/hotspot/variant-server/libjvm/objs/loopnode.o \
./src/hotspot/share/opto/loopnode.cpp |
|
I've tested the updated branch on Solaris (and AIX and macos) and it passed the compilation phase on all three so this looks good now if we're going to stick with the include instead of the
Thanks @gnu-andrew - I agree that with this guard around it so it only takes effect on AIX there is as near zero risk as we can get (and we know it's definitely broken on AIX without this!)
Agreed - that's what I was hoping for - that this would get accepted in time to fix 8u442 next month. @varada1110 Can you handle migrating the PR into jdk8u?
Yeah I'd be good with that. the post at https://api.adoptium.net/v3/binary/latest/21/ea/linux/aarch64/jdk/hotspot/normal/adoptium talks about the builds we publish for each EA tag that comes out. The build link I referenced above was a special that I ran specifically against that branch (That isn't done automatically!) but in general they can be pulled from the API links referenced in that article e.g. https://api.adoptium.net/v3/binary/latest/8/ea/aix/ppc64/jdk/hotspot/normal/adoptium for the latest AIX jdk8u EA tag (unless it failed to build of course, like it does for AIX, so that link is currently serving up jdk8u432-b05 as the last one that built successfully) |
We need to do this ASAP as we freeze at the end of next week. I doubt many people will be around next week either, so it may already be too late. I'll try and keep an eye out, but today is my last work day of 2024.
Is the first link wrong? It take me to an AArch64 binary, not a post. It is really good to know this is being regularly tested. |
|
I was away and did not have access to my laptop. I have created the PR on jdk8u. Please review the code change, jtreg testing looks fine too |
7 participants
Use of llabs() for jlong resolves the build error on AIX.
Performed jtreg testing for hotspot/test on both aix-ppc64 and linux-ppc64le and no related failures observed
JBS : JDK-8342822
Progress
Issue
Backport <hash>with the hash of the original commit. See Backports.Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk8u-dev.git pull/600/head:pull/600$ git checkout pull/600Update a local copy of the PR:
$ git checkout pull/600$ git pull https://git.openjdk.org/jdk8u-dev.git pull/600/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 600View PR using the GUI difftool:
$ git pr show -t 600Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk8u-dev/pull/600.diff
Using Webrev
Link to Webrev Comment