Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]
On Wed, 24 Jan 2024 13:47:17 GMT, Doug Simon wrote: > You need to check if class is already loaded by trying findLoadedClass first. Thanks @xxDark . I knew it should work. :) - PR Comment: https://git.openjdk.org/jdk/pull/17520#issuecomment-1908961416
Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]
On Tue, 23 Jan 2024 19:16:49 GMT, Doug Simon wrote: >> 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`. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > use null to denote boot class loader as delegation parent 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. - PR Comment: https://git.openjdk.org/jdk/pull/17520#issuecomment-1908498530
Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]
On Tue, 23 Jan 2024 19:16:49 GMT, Doug Simon wrote: >> 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`. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > use null to denote boot class loader as delegation parent Build changes are trivially fine - Marked as reviewed by ihse (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17520#pullrequestreview-1841751451
Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]
On Wed, 24 Jan 2024 13:47:17 GMT, Doug Simon wrote: > 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](https://github.com/openjdk/jdk/blob/bccd823c8e40863bed70ff5b24772843203871a5/src/java.base/share/classes/java/lang/ClassLoader.java#L904). 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. - PR Comment: https://git.openjdk.org/jdk/pull/17520#issuecomment-1908375220
Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]
On Wed, 24 Jan 2024 12:16:44 GMT, xxDark wrote: > 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](https://github.com/openjdk/jdk/blob/bccd823c8e40863bed70ff5b24772843203871a5/src/java.base/share/classes/java/lang/ClassLoader.java#L904). - PR Comment: https://git.openjdk.org/jdk/pull/17520#issuecomment-1908157130
Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]
On Wed, 24 Jan 2024 08:56:10 GMT, Doug Simon wrote:
> > 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.
-
PR Comment: https://git.openjdk.org/jdk/pull/17520#issuecomment-1908013421
Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]
On Wed, 24 Jan 2024 08:56:10 GMT, Doug Simon wrote: > 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. - PR Comment: https://git.openjdk.org/jdk/pull/17520#issuecomment-1907983591
Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]
On Tue, 23 Jan 2024 19:16:49 GMT, Doug Simon wrote: >> 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`. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > use null to denote boot class loader as delegation parent Marked as reviewed by dholmes (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17520#pullrequestreview-1841205744
Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]
On Wed, 24 Jan 2024 08:46:08 GMT, Doug Simon wrote: >> test/hotspot/jtreg/compiler/jvmci/LoadAlternativeJVMCI.java line 54: >> >>> 52: >>> 53: ClassLoader pcl = ClassLoader.getPlatformClassLoader(); >>> 54: 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. Thanks Doug. - PR Review Comment: https://git.openjdk.org/jdk/pull/17520#discussion_r1464798827
Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]
On Tue, 23 Jan 2024 19:16:49 GMT, Doug Simon wrote:
>> 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`.
>
> Doug Simon has updated the pull request incrementally with one additional
> commit since the last revision:
>
> use null to denote boot class loader as delegation parent
test/hotspot/jtreg/compiler/jvmci/LoadAlternativeJVMCI.java line 50:
> 48: e = e + File.separator;
> 49: }
> 50: 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.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17520#discussion_r1464719091
Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]
On Wed, 24 Jan 2024 06:11:30 GMT, David Holmes wrote:
> 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",
-
PR Comment: https://git.openjdk.org/jdk/pull/17520#issuecomment-1907671987
Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]
On Wed, 24 Jan 2024 06:07:55 GMT, David Holmes wrote: >> Doug Simon has updated the pull request incrementally with one additional >> commit since the last revision: >> >> use null to denote boot class loader as delegation parent > > test/hotspot/jtreg/compiler/jvmci/LoadAlternativeJVMCI.java line 54: > >> 52: >> 53: ClassLoader pcl = ClassLoader.getPlatformClassLoader(); >> 54: 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/17520#discussion_r1464529290
Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]
On Tue, 23 Jan 2024 19:16:49 GMT, Doug Simon wrote: >> 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`. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > use null to denote boot class loader as delegation parent 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. test/hotspot/jtreg/compiler/jvmci/LoadAlternativeJVMCI.java line 54: > 52: > 53: ClassLoader pcl = ClassLoader.getPlatformClassLoader(); > 54: 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? - PR Review: https://git.openjdk.org/jdk/pull/17520#pullrequestreview-1840477028 PR Review Comment: https://git.openjdk.org/jdk/pull/17520#discussion_r1464348710
Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]
> 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`. Doug Simon has updated the pull request incrementally with one additional commit since the last revision: use null to denote boot class loader as delegation parent - Changes: - all: https://git.openjdk.org/jdk/pull/17520/files - new: https://git.openjdk.org/jdk/pull/17520/files/e7d5801a..1642276e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17520=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17520=00-01 Stats: 8 lines in 1 file changed: 1 ins; 7 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17520.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17520/head:pull/17520 PR: https://git.openjdk.org/jdk/pull/17520
