> On Apr 27, 2017, at 11:51 AM, Doug Simon <doug.si...@oracle.com> wrote:
>
>>
>> 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.
Got it.
>
>>
>> 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.
Ok, thanks.
>
> -Doug
>
> [1]
> http://cr.openjdk.java.net/~dnsimon/8177845.02/make/make/CompileJavaModules.gmk.udiff.html
>
> <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
>
> <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
