8327701: Remove the xlc toolchain #18172

magicus · 2024-03-08T15:29:58Z

As of JDK-8325880, building the JDK requires version 17 of IBM Open XL C/C++ (xlc). This is in effect clang by another name, and it uses the clang toolchain in the JDK build. Thus the old xlc toolchain is no longer supported, and should be removed.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8327701: Remove the xlc toolchain (Bug - P3)

Reviewers

Julian Waters (@TheShermanTanker - Committer) ⚠️ Review applies to 53a05019
Erik Joelsson (@erikj79 - Reviewer) ⚠️ Review applies to 53a05019

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18172/head:pull/18172
$ git checkout pull/18172

Update a local copy of the PR:
$ git checkout pull/18172
$ git pull https://git.openjdk.org/jdk.git pull/18172/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18172

View PR using the GUI difftool:
$ git pr show -t 18172

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18172.diff

Webrev

Link to Webrev Comment

make/autoconf/flags-cflags.m4

bridgekeeper · 2024-03-08T15:31:59Z

👋 Welcome back ihse! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

make/autoconf/flags-cflags.m4

openjdk · 2024-03-08T15:32:36Z

@magicus The following labels will be automatically applied to this pull request:

build
client
core-libs
hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

make/autoconf/flags.m4

mlbridge · 2024-03-08T15:37:00Z

Webrevs

make/autoconf/toolchain.m4

magicus · 2024-03-08T15:53:08Z

Also, I believe that the HOTSPOT_TOOLCHAIN_TYPE=xlc quirk actually is bad. This means that the clang functionality in compilerWarnings_gcc.hpp (where the _gcc is hotspot-speak for "clang or gcc") is being ignored, and it means that globalDefinitions_xlc.hpp is in big parts a direct copy of globalDefinitions_gcc.hpp, but apparently lagging in some fixes that has been made in that file.

And it means a lot of lines like this:

#if defined(TARGET_COMPILER_gcc) || defined(TARGET_COMPILER_xlc)

But cleaning up that is left as an exercise to the AIX team; my goal here just primarily to get rid of the old xlc stuff from the build system.

TheShermanTanker

Build changes look ok, but I'm not an AIX developer

erikj79

Build changes look good to me, but needs review and verification from the AIX folks.

openjdk · 2024-03-08T18:50:16Z

@magicus This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8327701: Remove the xlc toolchain

Reviewed-by: jwaters, erikj

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 43 new commits pushed to the master branch:

782206b: 8327748: Convert javax/swing/JFileChooser/6798062/bug6798062.java applet test to main
0776fff: 8327794: RISC-V: enable extension features based on impid (Rivos specific change)
cfd9209: 8327751: Convert javax/swing/JInternalFrame/6726866/bug6726866.java applet test to main
2cf3524: 8325433: Type annotations on primitive types are not linked
5056902: 8327361: Update some comments after JDK-8139457
78beb03: 8327750: Convert javax/swing/JFileChooser/FileFilterDescription/FileFilterDescription.java applet test to main
1f43fa0: 8326661: sun/java2d/cmm/ColorConvertOp/ColConvTest.java assumes profiles were generated by LCMS
013aff8: 8326606: Test javax/swing/text/BoxView/6494356/bug6494356.java performs a synchronization on a value based class
b92440f: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage
139681a: 8326497: Window.toFront() fails for iconified windows on Linux
... and 33 more: https://git.openjdk.org/jdk/compare/d0d4912c3bbc06e9a9c5273308d5f4ef7bac1b24...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

MBaesken · 2024-03-11T07:51:03Z

Hi, thanks for doing this cleanup change. I put it into our build/test queue to see the results on AIX.

doc/building.html

doc/building.md

make/autoconf/flags-cflags.m4

make/autoconf/toolchain.m4

MBaesken · 2024-03-11T12:44:55Z

With this change added, currently configure fails

checking for ibm-llvm-cxxfilt... /opt/IBM/openxlC/17.1.1/tools/ibm-llvm-cxxfilt
configure: error: ibm-clang_r version output check failed, output: 
configure exiting with result code

maybe related to what Joachim pointed out ?

make/autoconf/flags-cflags.m4

magicus · 2024-03-12T15:24:21Z

@MBaesken Yes, it was the bug Joachim found. It should be fixed now, together will all other comments (except the example version string in the comments).

Please re-test.

MBaesken · 2024-03-12T16:02:41Z

Please re-test.

Hi Magnus, thanks for the adjustments. I reenabled your patch in our build/test queue .

MBaesken · 2024-03-13T08:27:29Z

Please re-test.

Hi Magnus, thanks for the adjustments. I reenabled your patch in our build/test queue .

Builds and test results on AIX (product and fastdebug) are fine with the latest version of the PR .

magicus · 2024-03-13T08:41:43Z

/integrate

openjdk · 2024-03-13T08:42:13Z

Going to push as commit 107cb53.
Since your change was applied there have been 60 commits pushed to the master branch:

07acc0b: 8326385: [aarch64] C2: lightweight locking nodes kill the box register without specifying this effect
cc9a8ab: 8327460: Compile tests with the same visibility rules as product code
3b18c5d: 8323605: Java source launcher should not require --source ... to enable preview
5d4bfad: 8327693: C1: LIRGenerator::_instruction_for_operand is only read by assertion code
f3d0c45: 8327829: [JVMCI] runtime/ClassUnload/ConstantPoolDependsTest.java fails on libgraal
d5b95a0: 8327631: Update IANA Language Subtag Registry to Version 2024-03-07
966a42f: 8324868: debug agent does not properly handle interrupts of a virtual thread
22f10e0: 8327856: Convert applet test SpanishDiacriticsTest.java to a main program
7283c8b: 8327972: Convert java/awt/FileDialog/SaveFileNameOverrideTest/SaveFileNameOverrideTest.html applet test to main
30249c4: 8327838: Convert java/awt/FileDialog/MultipleMode/MultipleMode.html applet test to main
... and 50 more: https://git.openjdk.org/jdk/compare/d0d4912c3bbc06e9a9c5273308d5f4ef7bac1b24...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-03-13T08:42:22Z

@magicus Pushed as commit 107cb53.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

MBaesken · 2024-03-13T08:50:10Z

Seems HOTSPOT_TOOLCHAIN_TYPE=xlc and TARGET_COMPILER_xlc macros stay, is this intended ?
(not saying this is a wrong thing to do, but I believed first that all the xlc toolchain references are replaced by clang?)

magicus · 2024-03-13T09:00:39Z

@MBaesken Yes, I discussed this in length above: #18172 (comment).

In short, changing that would mean changing behavior, and that is not something I want to do in a removal refactoring. Also, it is up to you guys to make it work correctly.

But I really believe you should address this. Let me know if you need help with the build aspects.

JoKern65 · 2024-03-13T09:50:20Z

e.g. We should change the HOTSPOT_TOOLCHAIN_TYPE=xlc to aix, because it is not toolchain, but OS related. As a consequence the globalDefinitions_xlc.hpp will become globalDefinitions_aix.hpp

magicus · 2024-03-13T10:25:11Z

e.g. We should change the HOTSPOT_TOOLCHAIN_TYPE=xlc to aix, because it is not toolchain, but OS related. As a consequence the globalDefinitions_xlc.hpp will become globalDefinitions_aix.hpp

No, it's not that simple. First, the globalDefinitions_<compiler> files are included on a per-toolchain basis, not OS basis. Secondly, I think you will want to have the stuff in globalDefinitions_gcc.h. In fact, if you compare globalDefinitions_gcc.h and globalDefinitions_xlc.h side-by-side, you see that they are much more similar than you'd perhaps naively expect. The remaining differences will probably be better expressed as #ifdef AIX inside globalDefinitions_gcc.h, I assume. (But of course, in the end this is up to the hotspot team.)

I recommend doing such a side-by-side check yourself to get an understanding of the actual differences.

8327701: Remove the xlc toolchain

9f4a059