8304915: Create jdk.internal.util.Architecture enum and apply #13357

RogerRiggs · 2023-04-05T15:58:08Z

Define an internal jdk.internal.util.Architecture enumeration and static methods to replace uses of the system property os.arch.
The enumeration values are defined to match those used in the build.
The initial values are: X64, X86, AARCH64, RISCV64, S390, PPC64
Note that amd64 and x86_64 in the build are represented by X64.
The value of the system property os.arch is unchanged.

The API is similar to the jdk.internal.util.OperatingSystem enum created by #12931.
Uses in java.base and a few others are included but other modules will be done in separate PRs.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issue

JDK-8304915: Create jdk.internal.util.Architecture enum and apply

Reviewers

Erik Joelsson (@erikj79 - Reviewer) ⚠️ Review applies to 99a93b7e
Martin Doerr (@TheRealMDoerr - Reviewer)
@Glavo (no known openjdk.org user name / role) ⚠️ Review applies to 99a93b7e
Amit Kumar (@offamitkumar - Author) ⚠️ Review applies to 99a93b7e

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13357

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

Using diff file

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

Webrev

Link to Webrev Comment

Merge amd64 and X64 to a single Architecture

bridgekeeper · 2023-04-05T15:59:48Z

👋 Welcome back rriggs! 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.

openjdk · 2023-04-05T16:02:06Z

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

build
client
core-libs
serviceability

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.

mlbridge · 2023-04-05T16:06:23Z

Webrevs

src/java.base/share/classes/jdk/internal/util/Architecture.java

erikj79

Looks good from a build point of view.

/reviewers 2 reviewer

openjdk · 2023-04-05T16:38:15Z

@RogerRiggs 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:

8304915: Create jdk.internal.util.Architecture enum and apply

Reviewed-by: erikj, mdoerr, amitkumar

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 68 new commits pushed to the master branch:

4a9f8ef: 8057586: Explicit GC ignored if GCLocker is active
ce493dd: 8306435: Juggle04/TestDescription.java should be a booleanArr test and not a byteArr one
f7d45b8: 8306076: Open source AWT misc tests
4900517: 8306636: Disable compiler/c2/Test6905845.java with -XX:TieredStopAtLevel=3
0f51e63: 8305590: Remove nothrow exception specifications from operator new
8d696ae: 8306575: Clean up and open source four Dialog related tests
9ed456f: 8306634: Open source AWT Event related tests
b2240bf: 8304696: Duplicate class names in dynamicArchive tests can lead to test failure
cb158ff: 8296153: Bump minimum boot jdk to JDK 20
117c5b1: 8279216: Investigate implementation of premultiplied alpha in the Little-CMS 2.13
... and 58 more: https://git.openjdk.org/jdk/compare/0f3828dddd8d4a08677efcd15aa8dfde18540130...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.

openjdk · 2023-04-05T16:38:45Z

@erikj79
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

…num. Refactor the is64Bit method to be a static method for the current runtime.

src/java.base/share/classes/jdk/internal/util/Architecture.java

ecki · 2023-04-05T20:25:43Z

src/java.base/share/classes/jdk/internal/foreign/CABI.java

        ADDRESS_SIZE = ADDRESS.bitSize();
        // might be running in a 32-bit VM on a 64-bit platform.
        // addressSize will be correctly 32
-        if ((ARCH.equals("amd64") || ARCH.equals("x86_64")) && ADDRESS_SIZE == 64) {
+        if (Architecture.isX64() && ADDRESS_SIZE == 64) {


Is there a difference to Architecture.is64bit (I.e. is the later or the former runtime vs compiletime

There should be no difference; I was hesitant to drop the ADDRESS_SIZE check without knowing more about the foreign api dependencies. ADDRESS_SIZE is computed (I think) from UNSAFE.ADDRESS_SIZE * 8.
But I can't think of how it can be different than the CPU_BITS that are defined when the JDK is built.

Is there a difference to Architecture.is64bit (I.e. is the later or the former runtime vs compiletime

Theoretically, the two can be unrelated. Linux x32 ABI is a typical example of using 32-bit addresses on the x86-64 architecture.

Right, it is still possible to run a 32-bit VM build on a 64-bit platform. os.arch can be amd64, the runtime platform, in this case, but the VM variant can still be 32 bits.

Since Architecture.isX64 seems to be the target arch of the VM, I'm think that when running a 32-bit VM on a amd64 would result in isX64 being false? (In that case, only the isX64 check should be enough)

Right, it is still possible to run a 32-bit VM build on a 64-bit platform. os.arch can be amd64, the runtime platform, in this case, but the VM variant can still be 32 bits.

os.arch is always the target architecture for JVM, even using 32-bit JVM on 64-bit system, its value remains x86.

But as mentioned above, the size of the address can be independent of these.

Linux x32 ABI is an ABI for the x86-64 architecture, which uses unique features of 64-bit architectures such as 64-bit registers, but it uses 32-bit addresses.

Although OpenJDK does not support such a platform, technically speaking, it is still necessary to distinguish between architecture bits and address bits.

os.arch is always the target architecture for JVM, even using 32-bit JVM on 64-bit system, its value remains x86.

You're right. I could have sworn we had to add the ADDRESS_SIZE check because of this issue where a 32-bit VM running on x64 would erroneously select the x64 ABIs (#1386)

src/java.base/share/classes/jdk/internal/util/Architecture.java

test/jdk/jdk/internal/util/ArchTest.java

test/jdk/java/foreign/TestUnsupportedLinker.java

ExE-Boss

Per @Glavo’s comment:

src/java.base/share/classes/jdk/internal/util/Architecture.java

Rename S390X to S390, representing just the s390 architecture Removed ARM and IA64 enum values; they are unused; they can be added later if needed

src/jdk.attach/windows/classes/sun/tools/attach/AttachProviderImpl.java

All platforms and architectures that can be built support attach.

TheRealMDoerr · 2023-04-12T09:07:34Z

Works on PPC64 Big Endian, now. However, little Endian fails:
STARTED ArchTest::isArch '[6] PPC64, false'
org.opentest4j.AssertionFailedError: Mismatch PPC64 == PPC64 vs isPPC64 ==> expected: but was:
at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
at org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)
at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182)
at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1152)
at ArchTest.isArch(ArchTest.java:99)
...
System property os.arch: "ppc64le", Architecture.current(): "PPC64"

RealLucy · 2023-04-12T11:29:52Z

Another remark: Old JDK on s390 used "os.arch = zArch_64", current one "os.arch = s390x". @offamitkumar: You probably want to take a look.

zArch_64 is not relevant/not used in the OpenJDK port to IBM System z. As noted elsewhere in the PR, it's the architecture name that was used in the initial, proprietary port by SAP. I found just one occurrence of the string in the head revision, and that was in test/lib/jdk/test/lib/Platform.java.

Consolidated switch cases in ArchTest. Moved mapping of build TARGET_OS and TARGET_CPU to the build to avoid multiple mappings in more than one place.

RogerRiggs · 2023-04-12T17:43:29Z

@TheRealMDoerr Thanks for the ppc64 feedback and testing.

TheRealMDoerr

Thanks! PPC64 parts look good and the test has passed on both endianness versions.

RogerRiggs · 2023-04-14T14:31:31Z

make/modules/java.base/gensrc/GensrcMisc.gmk

+
+# Normalize OPENJDK_TARGET_CPU name to match jdk.internal.util.Architecture enum
+ifneq ($(filter $(OPENJDK_TARGET_CPU), ppc64le), )
+  OPENJDK_TARGET_ARCH_CANONICAL = ppc64
+else ifneq ($(filter $(OPENJDK_TARGET_CPU), s390x), )
+  OPENJDK_TARGET_ARCH_CANONICAL = s390
+else ifneq ($(filter $(OPENJDK_TARGET_CPU), x86_64 amd64), )
+  OPENJDK_TARGET_ARCH_CANONICAL = x64
+else
+  OPENJDK_TARGET_ARCH_CANONICAL := $(OPENJDK_TARGET_CPU)
+endif
+
+# Normalize OPENJDK_TARGET_OS operating system name to match jdk.internal.util.OperatingSystem enum
+ifeq ($(OPENJDK_TARGET_OS), macosx)
+  OPENJDK_TARGET_OS_CANONICAL = macos
+else
+  OPENJDK_TARGET_OS_CANONICAL := $(OPENJDK_TARGET_OS)
+endif
+
 $(eval $(call SetupTextFileProcessing, BUILD_PLATFORMPROPERTIES_JAVA, \


@erikj79 Is there a better/good way to do these mappings, or select "local" variable names?

Not sure if it's better, but you could consider doing this directly in the switch statements in make/autoconf/platform.m4 and add the corresponding variables in spec.gmk.in. That would make them available globally in the build, which may also be detrimental as parts of the build could start relying on them, and we end up with even more variants sprinkled around. I think what you have here is better for now.

ok, I'd rather keep it local. Thanks

Glavo

The RISC-V 64 part look good, ArchTest has passed on Debian RISC-V 64.

test/jdk/jdk/internal/util/ArchTest.java

RogerRiggs · 2023-04-18T20:31:42Z

Last call for Reviewers. Tnx

erikj79

Build changes look good.

Glavo

Looks good, tests still pass on Debian RISC-V 64.

test/jdk/jdk/internal/util/ArchTest.java

offamitkumar

for s390x, build is fine and tier1 (specifically ArchTest.java) passes.

Thanks for the change.

… mentioning PPC64LE

TheRealMDoerr

Thanks for the comment updates!

RogerRiggs · 2023-05-01T15:20:35Z

/integrate

openjdk · 2023-05-01T15:22:19Z

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

7d07d19: 8305201: Improve error message for GroupLayouts that are too large on SysV
67dd841: 8305093: Linker cache should not take layout names into account
d437c61: 8305672: Surprising definite assignment error after JDK-8043179
b39a9bf: 8301703: java.base jdk.internal.foreign.abi.BindingSpecializer uses ASM to generate classes
1de1a38: 8303002: Reject packed structs from linker
316d303: 8306851: Move Method access flags
a6b4f25: 8306825: Monitor deflation might be accidentally disabled by zero intervals
2d7c507: 8305778: javax/swing/JTableHeader/6884066/bug6884066.java: Unexpected header's value; index = 4 value = E
e1b06ea: 8305780: javax/swing/JTable/7068740/bug7068740.java fails on Ubunutu 20.04
b54c4a3: 8299713: Test javax/swing/JTableHeader/6889007/bug6889007.java failed: Wrong type of cursor
... and 189 more: https://git.openjdk.org/jdk/compare/0f3828dddd8d4a08677efcd15aa8dfde18540130...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-05-01T15:22:31Z

@RogerRiggs Pushed as commit f00a748.

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

8304915: Create jdk.internal.util.Architecture enum and apply

0c61382

Merge amd64 and X64 to a single Architecture

openjdk bot added the rfr Pull request is ready for review label Apr 5, 2023

openjdk bot added serviceability serviceability-dev@openjdk.org build build-dev@openjdk.org client client-libs-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Apr 5, 2023

Glavo reviewed Apr 5, 2023

View reviewed changes

src/java.base/share/classes/jdk/internal/util/Architecture.java Outdated Show resolved Hide resolved

erikj79 approved these changes Apr 5, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 5, 2023

openjdk bot removed the ready Pull request is ready to be integrated label Apr 5, 2023

Rename isXXX method to be uppercase architecture names matching the e…

bef30f9

…num. Refactor the is64Bit method to be a static method for the current runtime.

Glavo reviewed Apr 5, 2023

View reviewed changes

src/java.base/share/classes/jdk/internal/util/Architecture.java Outdated Show resolved Hide resolved

RogerRiggs added 2 commits April 5, 2023 14:59

Correct name of isRISCV64 method.

3349b46

Correct spelling of isAARCH64 in WIndows AttachProviderImpl

8916dca

ecki reviewed Apr 5, 2023

View reviewed changes

JornVernee reviewed Apr 5, 2023

View reviewed changes

test/jdk/java/foreign/TestUnsupportedLinker.java Outdated Show resolved Hide resolved

ExE-Boss reviewed Apr 6, 2023

View reviewed changes

minborg reviewed Apr 6, 2023

View reviewed changes

src/java.base/share/classes/jdk/internal/util/Architecture.java Outdated Show resolved Hide resolved

minborg reviewed Apr 6, 2023

View reviewed changes

src/java.base/share/classes/jdk/internal/util/Architecture.java Outdated Show resolved Hide resolved

minborg reviewed Apr 6, 2023

View reviewed changes

src/java.base/share/classes/jdk/internal/util/Architecture.java Outdated Show resolved Hide resolved

ExE-Boss reviewed Apr 6, 2023

View reviewed changes

jaikiran reviewed Apr 6, 2023

View reviewed changes

src/java.base/share/classes/jdk/internal/util/Architecture.java Outdated Show resolved Hide resolved

Revert changes to Foreign API to avoid class with another PR

105ce60

Rename S390X to S390, representing just the s390 architecture Removed ARM and IA64 enum values; they are unused; they can be added later if needed

AlanBateman reviewed Apr 6, 2023

View reviewed changes

src/jdk.attach/windows/classes/sun/tools/attach/AttachProviderImpl.java Outdated Show resolved Hide resolved

Remove obsolete conditions in Windows AttachProviderImpl.

ab084c2

All platforms and architectures that can be built support attach.

Fixed isPPC64().

0cd6ce7

Consolidated switch cases in ArchTest. Moved mapping of build TARGET_OS and TARGET_CPU to the build to avoid multiple mappings in more than one place.

TheRealMDoerr approved these changes Apr 12, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 12, 2023

RogerRiggs commented Apr 14, 2023

View reviewed changes

Glavo approved these changes Apr 15, 2023

View reviewed changes

test/jdk/jdk/internal/util/ArchTest.java Outdated Show resolved Hide resolved

RogerRiggs added 2 commits April 17, 2023 13:09

ArchTest on Debian RISC-V 64 confirmed by reviewer

7094db6

Merge branch 'master' into 8304915-arch-enum

99a93b7

Merge branch 'master' into 8304915-arch-enum

e8f1be6

erikj79 approved these changes Apr 18, 2023

View reviewed changes

Glavo approved these changes Apr 19, 2023

View reviewed changes

TheRealMDoerr reviewed Apr 19, 2023

View reviewed changes

test/jdk/jdk/internal/util/ArchTest.java Outdated Show resolved Hide resolved

offamitkumar approved these changes Apr 19, 2023

View reviewed changes

Use and test of "s390" verified by reviewer.

b95a312

openjdk bot mentioned this pull request Apr 21, 2023

8306678: Replace use of os.version with an internal Version record #13584

Closed

3 tasks

Correct comment on isPPC64() and refer to isLittleEndian() instead of…

accf67f

… mentioning PPC64LE

TheRealMDoerr approved these changes Apr 24, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label May 1, 2023

openjdk bot closed this May 1, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 1, 2023

andy-goryachev-oracle mentioned this pull request May 9, 2023

8278422: Replace use of deprecated single string variant of Runtime.exec method openjdk/jfx#1130

Closed

3 tasks

bulasevich mentioned this pull request May 17, 2023

8308270: ARM32 build broken after JDK-8304913 #14028

Closed

3 tasks

RogerRiggs deleted the 8304915-arch-enum branch December 11, 2023 17:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8304915: Create jdk.internal.util.Architecture enum and apply #13357

8304915: Create jdk.internal.util.Architecture enum and apply #13357

RogerRiggs commented Apr 5, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Apr 5, 2023

openjdk bot commented Apr 5, 2023

mlbridge bot commented Apr 5, 2023 •

edited

Loading

erikj79 left a comment

openjdk bot commented Apr 5, 2023 •

edited

Loading

openjdk bot commented Apr 5, 2023

ecki Apr 5, 2023

RogerRiggs Apr 5, 2023

Glavo Apr 5, 2023

JornVernee Apr 5, 2023 •

edited

Loading

Glavo Apr 6, 2023 •

edited

Loading

JornVernee Apr 6, 2023 •

edited

Loading

ExE-Boss left a comment

TheRealMDoerr commented Apr 12, 2023 •

edited

Loading

RealLucy commented Apr 12, 2023

RogerRiggs commented Apr 12, 2023

TheRealMDoerr left a comment

RogerRiggs Apr 14, 2023

erikj79 Apr 14, 2023

RogerRiggs Apr 14, 2023

Glavo left a comment

RogerRiggs commented Apr 18, 2023

erikj79 left a comment

Glavo left a comment

offamitkumar left a comment

TheRealMDoerr left a comment

RogerRiggs commented May 1, 2023

openjdk bot commented May 1, 2023

openjdk bot commented May 1, 2023

8304915: Create jdk.internal.util.Architecture enum and apply #13357

8304915: Create jdk.internal.util.Architecture enum and apply #13357

Conversation

RogerRiggs commented Apr 5, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Apr 5, 2023

openjdk bot commented Apr 5, 2023

mlbridge bot commented Apr 5, 2023 • edited Loading

Webrevs

erikj79 left a comment

Choose a reason for hiding this comment

openjdk bot commented Apr 5, 2023 • edited Loading

openjdk bot commented Apr 5, 2023

ecki Apr 5, 2023

Choose a reason for hiding this comment

RogerRiggs Apr 5, 2023

Choose a reason for hiding this comment

Glavo Apr 5, 2023

Choose a reason for hiding this comment

JornVernee Apr 5, 2023 • edited Loading

Choose a reason for hiding this comment

Glavo Apr 6, 2023 • edited Loading

Choose a reason for hiding this comment

JornVernee Apr 6, 2023 • edited Loading

Choose a reason for hiding this comment

ExE-Boss left a comment

Choose a reason for hiding this comment

TheRealMDoerr commented Apr 12, 2023 • edited Loading

RealLucy commented Apr 12, 2023

RogerRiggs commented Apr 12, 2023

TheRealMDoerr left a comment

Choose a reason for hiding this comment

RogerRiggs Apr 14, 2023

Choose a reason for hiding this comment

erikj79 Apr 14, 2023

Choose a reason for hiding this comment

RogerRiggs Apr 14, 2023

Choose a reason for hiding this comment

Glavo left a comment

Choose a reason for hiding this comment

RogerRiggs commented Apr 18, 2023

erikj79 left a comment

Choose a reason for hiding this comment

Glavo left a comment

Choose a reason for hiding this comment

offamitkumar left a comment

Choose a reason for hiding this comment

TheRealMDoerr left a comment

Choose a reason for hiding this comment

RogerRiggs commented May 1, 2023

openjdk bot commented May 1, 2023

openjdk bot commented May 1, 2023

RogerRiggs commented Apr 5, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Apr 5, 2023 •

edited

Loading

openjdk bot commented Apr 5, 2023 •

edited

Loading

JornVernee Apr 5, 2023 •

edited

Loading

Glavo Apr 6, 2023 •

edited

Loading

JornVernee Apr 6, 2023 •

edited

Loading

TheRealMDoerr commented Apr 12, 2023 •

edited

Loading