> On 27 Apr 2017, at 18:44, Christian Thalinger <cthalin...@twitter.com> wrote:
>
> 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.
This simply allows `mx eclipseinit` to work when wanting to edit the JDK Graal
sources in Eclipse. As you state, this is completely by-passed by the JDK make
system.
>
> 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());
Yes. And the --add-exports in CompileJavaModules.gmk[1] and
Launcher-jdk.aot.gmk[2]. It also means these VM options are required in
upstream Graal when running the Graal junit tests.
-Doug
[1]
http://cr.openjdk.java.net/~dnsimon/8177845.02/make/make/CompileJavaModules.gmk.udiff.html
[2]
http://cr.openjdk.java.net/~dnsimon/8177845.02/jdk/make/launcher/Launcher-jdk.aot.gmk.udiff.html
>
> ?
>
>> 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
>> [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
>
