> On 27 Apr 2017, at 21:15, Vladimir Kozlov <vladimir.koz...@oracle.com> wrote: > > make/CompileJavaModules.gmk Please, adjust comment for jdk.aot: > > ># Don't use Indy strings concatenation to have good JVMCI startup > >performance. > --- > <# Don't use Indy strings concatenation to have good JAOTC startup > performance. > > Should we also add jdk.vm.ci.aarch64 and jdk.vm.ci.sparc exports for AOT? It > is not needed for JDK 9 but we will support them in a future and analyze > build failures is painful. > > Same in make/launcher/Launcher-jdk.aot.gmk.
Good point. I made it the same set of exports as for jdk.internal.vm.compiler (in both locations[1][2]). > I don't see changes in hotspot/make/CompileTools.gmk and > hotspot/make/gensrc/Gensrc-jdk.internal.vm.compiler.gmk (which do annotation > processor build): > > http://hg.openjdk.java.net/jdk9/dev/hotspot/file/f1cca489e9c6/make/gensrc/Gensrc-jdk.internal.vm.compiler.gmk > > I assume you don't need to change anything there. Right? I presume not as well. These sources are compiled as Java 8 sources where modules are not a concept. > Overall changes looks good to me. Thanks for the review! I'll run the Graal and AOT tests once more and then integrate this change. -Doug [1] http://cr.openjdk.java.net/~dnsimon/8177845.02/jdk/make/launcher/Launcher-jdk.aot.gmk.udiff.html [2] http://cr.openjdk.java.net/~dnsimon/8177845.02/make/make/CompileJavaModules.gmk.udiff.html > On 4/27/17 7:47 AM, Doug Simon 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 >> [2] 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 >> [4] >> 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 >>>>> >>>> >>> >>