Thanks for doing this. I have a hard time following what’s happening :-)
src/jdk.internal.vm.compiler/.mx.graal/suite.py
+ "jdklibraries" : {
+ "JVMCI_SERVICES" : {
+ "path" : "lib/jvmci-services.jar",
+ "sourcePath" : "lib/jvmci-services.src.zip",
+ "optional" : False,
+ "jdkStandardizedSince" : "9",
+ "module" : "jdk.internal.vm.ci"
+ },
+ "JVMCI_API" : {
+ "path" : "lib/jvmci/jvmci-api.jar",
+ "sourcePath" : "lib/jvmci/jvmci-api.src.zip",
+ "dependencies" : [
+ "JVMCI_SERVICES",
+ ],
+ "optional" : False,
+ "jdkStandardizedSince" : "9",
+ "module" : "jdk.internal.vm.ci"
+ },
+ "JVMCI_HOTSPOT" : {
+ "path" : "lib/jvmci/jvmci-hotspot.jar",
+ "sourcePath" : "lib/jvmci/jvmci-hotspot.src.zip",
+ "dependencies" : [
+ "JVMCI_API",
+ ],
+ "optional" : False,
+ "jdkStandardizedSince" : "9",
+ "module" : "jdk.internal.vm.ci"
+ },
+ },
Can you explain to me why we need this? I don’t think these files are built in
9.
src/jdk.internal.vm.ci/share/classes/module-info.java
Why can we remove the exports to jdk.internal.vm.compiler? Because of this in
JVMCIServiceLocator:
+ ReflectionAccessJDK.openJVMCITo(getClass());
?
> On Apr 27, 2017, at 7:47 AM, Doug Simon <[email protected]> wrote:
>
>>
>> On 21 Apr 2017, at 13:46, Doug Simon <[email protected]> wrote:
>>
>> There has been some discussion about whether we want to update Graal in the
>> JDK at this late stage. The main (only?) risk is a regression in the AOT
>> tool.
>>
>> If we don't update Graal from upstream, then the qualified exports from
>> JVMCI to jdk.internal.vm.compiler cannot be removed in JDK 9. Note that in
>> addition to updating Graal to remove the qualified exports, there would also
>> need to be changes in the relevant make files to add --add-exports options
>> when compiling Graal and jaotc as they use the dynamically exported JVMCI
>> packages.
>>
>> I have an updated hotspot patch that adapts Graal to the JVMCI API changes:
>>
>> http://cr.openjdk.java.net/~dnsimon/8177845/hotspot.02/
>>
>> Note that this patch does not include the changes removing use of JDK
>> internal API from Graal. Cherry picking those upstream Graal changes would
>> be more work than simply doing a complete update from upstream Graal.
>>
>> As I see it, there are 2 options here:
>>
>> 1. Go with the current webrev (including hotspot.02 patch) and live with the
>> qualified exports.
>> 2. Go with the current webrev (including hotspot.02 patch) and create a
>> follow up bug to update Graal from upstream, perform the relevant make file
>> changes and remove the qualified exports.
>
> I made a new webrev[1] that implements option 1.5 ;-) The changes added since
> the first webrev[2] are:
>
> - Cherry picked changes from upstream Graal that remove use of JDK internals.
>
> - The jdk.internal.vm.ci.enabled system property is set to true in
> arguments.cpp[3] iff EnableJVMCI is true
> and this property is checked in all the public methods in jdk.vm.ci.services.
>
> - The jdk.vm.ci.services package is (once again) only exported to
> jdk.internal.vm.compiler based on
> advice from the jigsaw team:
>
> "We reviewed the unqualified export jdk.vm.ci.services from
> jdk.internal.vm.ci module. This brings
> jdk.internal.vm.ci to be resolved by default that of course may resolve
> additional modules that
> provides the services that JVMCI uses. In addition. JVMCI is meant to be
> used (only) when
> -XX:+EnableJVMCI is specified but now it’s defined by default.
>
> An internal module should only have qualified exports as a design
> principle. The Lab Graal will
> have the same module name, jdk.internal.vm.compiler. The advise is to keep
> it as qualified export
> `exports jdk.vm.ci.services to jdk.internal.vm.compiler` and remove all
> other qualified exports as
> we discussed."
>
> - The jaotc launcher now needs to explicitly export JVMCI and
> jdk.internal.vm.compiler to jdk.aot[4].
> Unfortunately there needs to be one --add-exports option per qualified
> export target as combining
> them with a comma (e.g.,
> --add-exports=jdk.internal.vm.ci/jdk.vm.ci.code=jdk.internal.vm.compiler,jdk.aot)
> breaks the make process:
>
> Launcher-jdk.aot.gmk:31: *** missing separator. Stop.
> make/Main.gmk:232: recipe for target 'jdk.aot-launchers' failed.
>
> The latest webrev has been tested against upstream Graal, the closed AOT
> tests and jprt.
>
> -Doug
>
> [1] http://cr.openjdk.java.net/~dnsimon/8177845.02
> <http://cr.openjdk.java.net/~dnsimon/8177845.02>
> [2] http://cr.openjdk.java.net/~dnsimon/8177845
> <http://cr.openjdk.java.net/~dnsimon/8177845>
> [3]
> http://cr.openjdk.java.net/~dnsimon/8177845.02/hotspot/src/share/vm/runtime/arguments.cpp.udiff.html
>
> <http://cr.openjdk.java.net/~dnsimon/8177845.02/hotspot/src/share/vm/runtime/arguments.cpp.udiff.html>
> [4]
> http://cr.openjdk.java.net/~dnsimon/8177845.02/jdk/make/launcher/Launcher-jdk.aot.gmk.udiff.html
>
> <http://cr.openjdk.java.net/~dnsimon/8177845.02/jdk/make/launcher/Launcher-jdk.aot.gmk.udiff.html>
>
>>
>>> On 20 Apr 2017, at 20:50, Doug Simon <[email protected]> wrote:
>>>
>>> I've had to update the webrev again to support selection of a "null"
>>> compiler (i.e. one that raises an
>>> exception upon a compilation request) and added -Djvmci.Compiler=null to a
>>> large number of JVMCI jtreg
>>> tests to prevent Graal being selected and initialized by the JVMCI compiler
>>> auto-selection mechanism.
>>> Initializing Graal will currently fail with errors (see stack trace below)
>>> until Graal is updated to
>>> the version compatible with the JVMCI API changes.
>>>
>>> In addition to resolving the compatibility issue, explicitly selecting the
>>> "null" compiler for these
>>> tests better isolates them from parts of the runtime they are not aiming to
>>> test.
>>>
>>> org.graalvm.compiler.debug.GraalError: java.lang.ClassCastException:
>>> java.base/java.util.ImmutableCollections$MapN cannot be cast to
>>> java.base/java.util.Properties
>>> at
>>> jdk.internal.vm.compiler/org.graalvm.compiler.hotspot.HotSpotGraalCompilerFactory.getSavedProperties(HotSpotGraalCompilerFactory.java:217)
>>> at
>>> jdk.internal.vm.compiler/org.graalvm.compiler.hotspot.HotSpotGraalCompilerFactory.initializeOptions(HotSpotGraalCompilerFactory.java:138)
>>> at
>>> jdk.internal.vm.compiler/org.graalvm.compiler.hotspot.HotSpotGraalCompilerFactory.onSelection(HotSpotGraalCompilerFactory.java:95)
>>> at
>>> jdk.internal.vm.ci/jdk.vm.ci.hotspot.HotSpotJVMCICompilerConfig.getCompilerFactory(HotSpotJVMCICompilerConfig.java:104)
>>> at
>>> jdk.internal.vm.ci/jdk.vm.ci.hotspot.HotSpotJVMCIRuntime.<init>(HotSpotJVMCIRuntime.java:290)
>>> at
>>> jdk.internal.vm.ci/jdk.vm.ci.hotspot.HotSpotJVMCIRuntime.<init>(HotSpotJVMCIRuntime.java:65)
>>> at
>>> jdk.internal.vm.ci/jdk.vm.ci.hotspot.HotSpotJVMCIRuntime$DelayedInit.<clinit>(HotSpotJVMCIRuntime.java:73)
>>> at
>>> jdk.internal.vm.ci/jdk.vm.ci.hotspot.HotSpotJVMCIRuntime.runtime(HotSpotJVMCIRuntime.java:83)
>>> at jdk.internal.vm.ci/jdk.vm.ci.runtime.JVMCI.initializeRuntime(Native
>>> Method)
>>> at jdk.internal.vm.ci/jdk.vm.ci.runtime.JVMCI.<clinit>(JVMCI.java:58)
>>> at
>>> jdk.internal.vm.ci/jdk.vm.ci.hotspot.HotSpotJVMCIRuntime.runtime(HotSpotJVMCIRuntime.java:82)
>>> at
>>> jdk.internal.vm.ci/jdk.vm.ci.hotspot.HotSpotVMConfig.config(HotSpotVMConfig.java:41)
>>> at
>>> jdk.internal.vm.ci/jdk.vm.ci.hotspot.HotSpotResolvedJavaMethodImpl.getHolder(HotSpotResolvedJavaMethodImpl.java:92)
>>> at
>>> jdk.internal.vm.ci/jdk.vm.ci.hotspot.HotSpotResolvedJavaMethodImpl.fromMetaspace(HotSpotResolvedJavaMethodImpl.java:110)
>>> at
>>> jdk.internal.vm.ci/jdk.vm.ci.hotspot.CompilerToVM.asResolvedJavaMethod(Native
>>> Method)
>>> at
>>> jdk.internal.vm.ci/jdk.vm.ci.hotspot.CompilerToVMHelper.asResolvedJavaMethod(CompilerToVMHelper.java:185)
>>> at
>>> compiler.jvmci.common.CTVMUtilities.getResolvedMethod(CTVMUtilities.java:59)
>>> at
>>> compiler.jvmci.common.CTVMUtilities.getResolvedMethod(CTVMUtilities.java:64)
>>> at
>>> compiler.jvmci.compilerToVM.AllocateCompileIdTest.runSanityCorrectTest(AllocateCompileIdTest.java:125)
>>> at java.base/java.util.ArrayList.forEach(ArrayList.java:1378)
>>> at
>>> compiler.jvmci.compilerToVM.AllocateCompileIdTest.main(AllocateCompileIdTest.java:71)
>>> at
>>> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native
>>> Method)
>>> at
>>> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
>>> at
>>> java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
>>> at java.base/java.lang.reflect.Method.invoke(Method.java:563)
>>> at
>>> com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:115)
>>> at java.base/java.lang.Thread.run(Thread.java:844)
>>> Caused by: java.lang.ClassCastException:
>>> java.base/java.util.ImmutableCollections$MapN cannot be cast to
>>> java.base/java.util.Properties
>>> at
>>> jdk.internal.vm.compiler/org.graalvm.compiler.hotspot.HotSpotGraalCompilerFactory.getSavedProperties(HotSpotGraalCompilerFactory.java:215)
>>> ... 26 more
>>>
>>> -Doug
>>>
>>>> On 19 Apr 2017, at 23:26, Doug Simon <[email protected]> wrote:
>>>>
>>>> I've updated http://cr.openjdk.java.net/~dnsimon/8177845/hotspot/ with
>>>> these changes:
>>>>
>>>> 1. JVMCIServiceLocator.getProvider(Class<S>) is now protected
>>>> 2. JVMCIServiceLocator.getProviders(Class<S>) now checks JVMCIPermission
>>>> 3. Rename: jdk.vm.ci.services.internal.JDK9 ->
>>>> jdk.vm.ci.services.internal.ReflectionAccessJDK
>>>>
>>>> -Doug
>>>>
>>>>> On 19 Apr 2017, at 23:12, Doug Simon <[email protected]> wrote:
>>>>>
>>>>>>
>>>>>> On 19 Apr 2017, at 21:40, Christian Thalinger <[email protected]>
>>>>>> wrote:
>>>>>>
>>>>>>>
>>>>>>> On Apr 19, 2017, at 9:27 AM, Doug Simon <[email protected]> wrote:
>>>>>>>
>>>>>>>>
>>>>>>>> On 19 Apr 2017, at 21:04, Mandy Chung <[email protected]> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>> On Apr 19, 2017, at 11:55 AM, Christian Thalinger
>>>>>>>>> <[email protected]> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> On Apr 19, 2017, at 8:38 AM, Mandy Chung <[email protected]>
>>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>> Since jdk.internal.vm.compiler becomes an upgradeable module, it is
>>>>>>>>>> not hashed with java.base to allow it to be upgraded and there is no
>>>>>>>>>> integrity check. Such qualified export will be granted to any
>>>>>>>>>> module named jdk.internal.vm.compiler at runtime. The goal is for
>>>>>>>>>> upgradeable modules not to use any internal APIs and eliminate the
>>>>>>>>>> qualified exports.
>>>>>>>>>>
>>>>>>>>>> The main thing is that jdk.vm.ci.services API would need to be
>>>>>>>>>> guarded if it’s used by non-Graal modules.
>>>>>>>>>
>>>>>>>>> This all makes sense but where is the restriction that only
>>>>>>>>> jdk.internal.vm.compiler can use jdk.vm.ci.services?
>>>>>>>>
>>>>>>>> It's unqualified and no restriction in this change.
>>>>>>>
>>>>>>> The public methods currently in jdk.vm.ci.services are:
>>>>>>>
>>>>>>> 1. JVMCIServiceLocator.getProvider(Class<S>)
>>>>>>> 2. JVMCIServiceLocator.getProviders(Class<S>)
>>>>>>> 3. Services.initializeJVMCI()
>>>>>>> 4. Services.getSavedProperties()
>>>>>>> 5. Services.exportJVMCITo(Class<?>)
>>>>>>> 6. Services.load(Class<S>)
>>>>>>> 7. Services.loadSingle(Class<S>, boolean)
>>>>>>>
>>>>>>> 1 should be made protected. I'll update the webrev with this change.
>>>>>>
>>>>>> Good.
>>>>>>
>>>>>>>
>>>>>>> 2 should check for JVMCIPermission. I'll update the webrev with this
>>>>>>> change.
>>>>>>
>>>>>> Good.
>>>>>>
>>>>>>>
>>>>>>> 3 is harmless from a security perspective in my opinion.
>>>>>>
>>>>>> Would be good if one of Oracle’s security engineers could take a quick
>>>>>> look just to be sure.
>>>>>
>>>>> Vladimir, can you please bring this to the attention of the relevant
>>>>> engineer.
>>>>>
>>>>>>>
>>>>>>> 4 checks for JVMCIPermission.
>>>>>>
>>>>>> Ok.
>>>>>>
>>>>>>>
>>>>>>> 5, 6 and 7 will be removed in a follow bug that updates Graal from
>>>>>>> upstream (and removes its usage of these methods).
>>>>>>
>>>>>> About this, will this Graal update happen for JDK 9?
>>>>>
>>>>> Yes.
>>>>>
>>>>>> It’s awfully late in the cycle...
>>>>>
>>>>> These are jigsaw related changes and I've been told jigsaw has an FC
>>>>> exception (although I don't exactly know what that is).
>>>>>
>>>>> -Doug
