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 <doug.si...@oracle.com> wrote: > >> >> On 21 Apr 2017, at 13:46, Doug Simon <doug.si...@oracle.com> 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 <doug.si...@oracle.com> 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 <doug.si...@oracle.com> 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 <doug.si...@oracle.com> wrote: >>>>> >>>>>> >>>>>> On 19 Apr 2017, at 21:40, Christian Thalinger <cthalin...@twitter.com> >>>>>> wrote: >>>>>> >>>>>>> >>>>>>> On Apr 19, 2017, at 9:27 AM, Doug Simon <doug.si...@oracle.com> wrote: >>>>>>> >>>>>>>> >>>>>>>> On 19 Apr 2017, at 21:04, Mandy Chung <mandy.ch...@oracle.com> wrote: >>>>>>>> >>>>>>>> >>>>>>>>> On Apr 19, 2017, at 11:55 AM, Christian Thalinger >>>>>>>>> <cthalin...@twitter.com> wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>>> On Apr 19, 2017, at 8:38 AM, Mandy Chung <mandy.ch...@oracle.com> >>>>>>>>>> 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