8323832: Load JVMCI with the platform class loader #17520

dougxc · 2024-01-22T17:34:16Z

This PR changes jdk.internal.vm.ci such that it is loaded by the platform class loader instead of the boot class loader. This allows Native Image to load a version of JVMCI different than the version on top of which Native Image is running. This capability is demonstrated and tested by LoadAlternativeJVMCI.java.

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-8323832: Load JVMCI with the platform class loader (Enhancement - P4) ⚠️ Issue is not open.

Reviewers

David Holmes (@dholmes-ora - Reviewer)
Magnus Ihse Bursie (@magicus - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17520

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-01-22T17:34:59Z

👋 Welcome back dnsimon! 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 · 2024-01-22T17:36:01Z

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

build
core-libs
graal
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.

dougxc · 2024-01-23T16:44:40Z

src/java.base/share/lib/security/default.policy

    permission java.security.AllPermission;
 };

+grant codeBase "jrt:/jdk.internal.vm.ci" {


This is required as JVMCI is no longer loaded by the boot loader but should retain all permissions.

mlbridge · 2024-01-23T16:47:40Z

Webrevs

01: Full - Incremental (1642276e)
00: Full (e7d5801a)

xxDark · 2024-01-23T17:00:20Z

Hello. I'm not a reviewer but I read through the conversation in JIRA and saw this comment:

[~pwoegerer] currently has a Native Image patch where he creates a URLClassLoader whose parent is jdk.internal.loader.ClassLoaders.BOOT_LOADER (retrieved via reflection and use of required --add-exports and --add-opens command line options). That is, he's using the non-delegating approach you mention.

There is zero reason to do this.
Passing null as parent class loader would suffice as boot loader just uses findBootstrapClassOrNull in JavaLangAccess either way.

dougxc · 2024-01-23T19:13:16Z

Passing null as parent class loader would suffice as boot loader just uses findBootstrapClassOrNull in JavaLangAccess either way

Thanks! I've simplified the test accordingly: 1642276

dholmes-ora

The actual changes to load JVMCI via the platform loader seem fine.

I'm still puzzled by the need to do this as any non-delegating classloader would have allowed this even if JVMCI were loaded by the bootloader. And I don't understand how your test is working as it is using a delegating classloader.

dholmes-ora · 2024-01-24T06:07:55Z

test/hotspot/jtreg/compiler/jvmci/LoadAlternativeJVMCI.java

+        ClassLoader pcl = ClassLoader.getPlatformClassLoader();
+        URLClassLoader ucl = new URLClassLoader(cp, null);


I am missing something here, a URLClassLoader first delegates to its parent before searching its URLs, so how does this not find the platform loader versions of the JVMCI classes?

With new URLClassLoader(cp, null), the URL loader delegates directly to the boot loader, by-passing the platform loader.

<face-palm>

Thanks Doug.

dougxc · 2024-01-24T08:56:10Z

I'm still puzzled by the need to do this as any non-delegating classloader would have allowed this even if JVMCI were loaded by the bootloader.

As far as I understand, even a non-delegating classloader cannot redefine a class loaded by the boot loader. I modified the test to show this and get:

java.lang.LinkageError: loader LoadAlternativeJVMCI$1 @4a1f4d08 attempted duplicate class definition for jdk.vm.ci.meta.ResolvedJavaType. (jdk.vm.ci.meta.ResolvedJavaType is in unnamed module of loader LoadAlternativeJVMCI$1 @4a1f4d08, parent loader 'bootstrap')
	at java.base/java.lang.ClassLoader.defineClass1(Native Method)
	at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1023)
	at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:150)
	at java.base/java.net.URLClassLoader.defineClass(URLClassLoader.java:524)
	at java.base/java.net.URLClassLoader$1.run(URLClassLoader.java:427)
	at java.base/java.net.URLClassLoader$1.run(URLClassLoader.java:421)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:714)
	at java.base/java.net.URLClassLoader.findClass(URLClassLoader.java:420)
	at LoadAlternativeJVMCI$1.loadClass(LoadAlternativeJVMCI.java:61)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:525)
	at LoadAlternativeJVMCI.main(LoadAlternativeJVMCI.java:77)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at com.sun.javatest.regtest.agent.MainWrapper$MainTask.run(MainWrapper.java:138)
	at java.base/java.lang.Thread.run(Thread.java:1575)

Test modification:

diff --git a/test/hotspot/jtreg/compiler/jvmci/LoadAlternativeJVMCI.java b/test/hotspot/jtreg/compiler/jvmci/LoadAlternativeJVMCI.java
index dd63867e7c2..28a6fedca38 100644
--- a/test/hotspot/jtreg/compiler/jvmci/LoadAlternativeJVMCI.java
+++ b/test/hotspot/jtreg/compiler/jvmci/LoadAlternativeJVMCI.java
@@ -51,7 +51,14 @@ public static void main(String[] args) throws Exception {
         }

         ClassLoader pcl = ClassLoader.getPlatformClassLoader();
-        URLClassLoader ucl = new URLClassLoader(cp, null);
+        URLClassLoader ucl = new URLClassLoader(cp, null) {
+            protected Class<?> loadClass(String name, boolean resolve) throws ClassNotFoundException {
+                if (!name.startsWith("jdk.vm.ci")) {
+                    return super.loadClass(name, resolve);
+                }
+                return findClass(name);
+            }
+        };

         String[] names = {
             "jdk.vm.ci.meta.ResolvedJavaType",

olpaw · 2024-01-24T09:47:34Z

There is zero reason to do this. Passing null as parent class loader would suffice as boot loader just uses findBootstrapClassOrNull in JavaLangAccess either way.

Right, using null does the same thing. In the final version we will use that instead of accessing private field jdk.internal.loader.ClassLoaders#BOOT_LOADER.

Would be nice if the JavaDoc for URLClassLoader mentioned that null is a valid option for parent.

AlanBateman · 2024-01-24T10:46:20Z

test/hotspot/jtreg/compiler/jvmci/LoadAlternativeJVMCI.java

+            if (new File(e).isDirectory()) {
+                e = e + File.separator;
+            }
+            cp[i] = new URI("file:" + e).toURL();


This should be cp[i] = file.toURI().toURL() as a file path needs encoding to be URI path component.

dholmes-ora · 2024-01-24T11:56:45Z

As far as I understand, even a non-delegating classloader cannot redefine a class loaded by the boot loader. I modified the test to show this and get:
java.lang.LinkageError: loader LoadAlternativeJVMCI$1 @4a1f4d08 attempted duplicate class definition for jdk.vm.ci.meta.ResolvedJavaType. (jdk.vm.ci.meta.ResolvedJavaType is in unnamed module of loader LoadAlternativeJVMCI$1 @4a1f4d08, parent loader 'bootstrap')
	at java.base/java.lang.ClassLoader.defineClass1(Native Method)

Interesting. I'm not sure why that should be happening in this case. I can imagine a potential split-package issue with the bootloader that doesn't happen with the platform loader. I will look into it.

openjdk · 2024-01-24T11:58:27Z

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

8323832: Load JVMCI with the platform class loader

Reviewed-by: dholmes, ihse

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

00bb6bf: 8324220: jdk/jfr/event/io/TestSerializationMisdeclarationEvent.java had 22 failures
bccd823: 8324613: Serial: Rename GenerationPool to TenuredGenerationPool
8c003d8: 8321512: runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java fails on 32-bit platforms
67f29b1: 8324537: Remove superfluous _FILE_OFFSET_BITS=64
1c1cb04: 8324512: Serial: Remove Generation::Name
b65e5eb: 8324543: Remove Space::object_iterate
6d2f640: 8324578: [BACKOUT] [IMPROVE] OPEN_MAX is no longer the max limit on macOS >= 10.6 for RLIMIT_NOFILE
c17059d: 8324334: Shenandoah: Improve end of process report
96607df: 8321545: Override toString() for Format subclasses
edfee7f: 8323546: Cleanup jcmd docs for Compiler.perfmap and VM.cds filename parameter
... and 14 more: https://git.openjdk.org/jdk/compare/c9cacfb25d1f15c879c961d2965a63c9fe4d9fa7...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.

xxDark · 2024-01-24T12:16:44Z

I'm still puzzled by the need to do this as any non-delegating classloader would have allowed this even if JVMCI were loaded by the bootloader.

As far as I understand, even a non-delegating classloader cannot redefine a class loaded by the boot loader. I modified the test to show this and get:

java.lang.LinkageError: loader LoadAlternativeJVMCI$1 @4a1f4d08 attempted duplicate class definition for jdk.vm.ci.meta.ResolvedJavaType. (jdk.vm.ci.meta.ResolvedJavaType is in unnamed module of loader LoadAlternativeJVMCI$1 @4a1f4d08, parent loader 'bootstrap')
	at java.base/java.lang.ClassLoader.defineClass1(Native Method)
	at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1023)
	at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:150)
	at java.base/java.net.URLClassLoader.defineClass(URLClassLoader.java:524)
	at java.base/java.net.URLClassLoader$1.run(URLClassLoader.java:427)
	at java.base/java.net.URLClassLoader$1.run(URLClassLoader.java:421)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:714)
	at java.base/java.net.URLClassLoader.findClass(URLClassLoader.java:420)
	at LoadAlternativeJVMCI$1.loadClass(LoadAlternativeJVMCI.java:61)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:525)
	at LoadAlternativeJVMCI.main(LoadAlternativeJVMCI.java:77)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at com.sun.javatest.regtest.agent.MainWrapper$MainTask.run(MainWrapper.java:138)
	at java.base/java.lang.Thread.run(Thread.java:1575)

Test modification:

diff --git a/test/hotspot/jtreg/compiler/jvmci/LoadAlternativeJVMCI.java b/test/hotspot/jtreg/compiler/jvmci/LoadAlternativeJVMCI.java
index dd63867e7c2..28a6fedca38 100644
--- a/test/hotspot/jtreg/compiler/jvmci/LoadAlternativeJVMCI.java
+++ b/test/hotspot/jtreg/compiler/jvmci/LoadAlternativeJVMCI.java
@@ -51,7 +51,14 @@ public static void main(String[] args) throws Exception {
         }

         ClassLoader pcl = ClassLoader.getPlatformClassLoader();
-        URLClassLoader ucl = new URLClassLoader(cp, null);
+        URLClassLoader ucl = new URLClassLoader(cp, null) {
+            protected Class<?> loadClass(String name, boolean resolve) throws ClassNotFoundException {
+                if (!name.startsWith("jdk.vm.ci")) {
+                    return super.loadClass(name, resolve);
+                }
+                return findClass(name);
+            }
+        };

         String[] names = {
             "jdk.vm.ci.meta.ResolvedJavaType",

It can. You need to check if class is already loaded by trying findLoadedClass first.

dougxc · 2024-01-24T13:47:17Z

You need to check if class is already loaded by trying findLoadedClass first.

You're right. I had forgotten the intricacies of class loader delegation. The only hard constraint on loading a class in multiple loaders is that java.* classes must (only) be loaded by the boot loader.

AlanBateman · 2024-01-24T15:34:39Z

You're right. I had forgotten the intricacies of class loader delegation. The only hard constraint on loading a class in multiple loaders is that java.* classes must (only) be loaded by the boot loader.

Just to add that this restriction was relaxed in Java 9 to allow java.* classes be defined by the platform class loader. The code that is linked to here throws if the class loader is not the platform class loader. There isn't a user accessible ClassLoader object for the boot loader and testing this == null doesn't make sense.

magicus

Build changes are trivially fine

dougxc · 2024-01-24T16:35:01Z

I'm closing this PR as the Native Image use case https://bugs.openjdk.org/browse/JDK-8323832 was opened for can be solved with an appropriately crafted custom loader that does not delegate loading of JVMCI classes.

Thanks for the reviews anyway, especially @xxDark for highlighting that this change is unnecessary.

dholmes-ora · 2024-01-24T21:41:03Z

You need to check if class is already loaded by trying findLoadedClass first.

Thanks @xxDark . I knew it should work. :)

openjdk bot added graal graal-dev@openjdk.org build build-dev@openjdk.org hotspot hotspot-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Jan 22, 2024

dougxc force-pushed the JDK-8323832 branch 4 times, most recently from 44c0742 to 36ea450 Compare January 22, 2024 19:39

load JVMCI with platform class loader

e7d5801

dougxc force-pushed the JDK-8323832 branch from 36ea450 to e7d5801 Compare January 22, 2024 20:16

dougxc marked this pull request as ready for review January 23, 2024 16:43

openjdk bot added the rfr Pull request is ready for review label Jan 23, 2024

dougxc commented Jan 23, 2024

View reviewed changes

use null to denote boot class loader as delegation parent

1642276

dholmes-ora reviewed Jan 24, 2024

View reviewed changes

AlanBateman reviewed Jan 24, 2024

View reviewed changes

dholmes-ora approved these changes Jan 24, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 24, 2024

magicus approved these changes Jan 24, 2024

View reviewed changes

dougxc closed this Jan 24, 2024

		ClassLoader pcl = ClassLoader.getPlatformClassLoader();
		URLClassLoader ucl = new URLClassLoader(cp, null);

8323832: Load JVMCI with the platform class loader #17520

8323832: Load JVMCI with the platform class loader #17520

Uh oh!

Conversation

dougxc commented Jan 22, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Jan 22, 2024

Uh oh!

openjdk bot commented Jan 22, 2024

Uh oh!

dougxc Jan 23, 2024

Choose a reason for hiding this comment

Uh oh!

mlbridge bot commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

xxDark commented Jan 23, 2024 • edited by bridgekeeper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dougxc commented Jan 23, 2024

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

dholmes-ora Jan 24, 2024

Choose a reason for hiding this comment

Uh oh!

dougxc Jan 24, 2024

Choose a reason for hiding this comment

Uh oh!

dholmes-ora Jan 24, 2024

Choose a reason for hiding this comment

Uh oh!

dougxc commented Jan 24, 2024

Uh oh!

olpaw commented Jan 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlanBateman Jan 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dholmes-ora commented Jan 24, 2024

Uh oh!

openjdk bot commented Jan 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xxDark commented Jan 24, 2024

Uh oh!

dougxc commented Jan 24, 2024

Uh oh!

AlanBateman commented Jan 24, 2024

Uh oh!

magicus left a comment

Choose a reason for hiding this comment

Uh oh!

dougxc commented Jan 24, 2024

Uh oh!

dholmes-ora commented Jan 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

dougxc commented Jan 22, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 23, 2024 •

edited

Loading

xxDark commented Jan 23, 2024 •

edited by bridgekeeper bot

Loading

olpaw commented Jan 24, 2024 •

edited

Loading

AlanBateman Jan 24, 2024 •

edited

Loading

openjdk bot commented Jan 24, 2024 •

edited

Loading