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.

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?

Overall changes looks good to me.

thanks,
Vladimir

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




Reply via email to