Re: RFR: 8177845: Need a mechanism to load Graal

[Mandy Chung]

> On Apr 27, 2017, at 2:30 PM, Doug Simon  wrote:
> 
> 
>> On 27 Apr 2017, at 23:24, Mandy Chung  wrote:
>> 
>> 
>>> On Apr 27, 2017, at 7:47 AM, Doug Simon  wrote:
>>> 
>>> [1] http://cr.openjdk.java.net/~dnsimon/8177845.02
>> 
>> I reviewed the top repo and jdk repo change. For hotspot change,
>> I reviewed jdk.vm.ci.services/* and jdk.internal.vm.ci module-info.java.
>> 
>> make/make/CompileJavaModules.gmk
>> jdk/make/launcher/Launcher-jdk.aot.gmk
>> Reading the original jdk.internal.vm.ci module-info.java [1],
>> jdk.aot only accesses a small set of packages.  e.g. it does not
>> access jdk.vm.ci.aarch64, jdk.vm.ci.code.stack, jdk.vm.ci.common, etc.
> 
> I know but I agree with Vladimir that there's no harm in exporting everything 
> to jdk.aot to simplify continued development of this tool as support for 
> other platforms is added. As I learnt putting this patch together, missing 
> these exports when they're actually needed leads to very confusing compiler 
> error messages.

Indeed the compiler error message is very confusing and ought to be improved 
(JDK-8179359).

I have no issue to export these packages to jdk.aot, as you agree to do.

Mandy

Re: RFR: 8177845: Need a mechanism to load Graal

[Mandy Chung]

> On Apr 27, 2017, at 7:47 AM, Doug Simon  wrote:
> 
> [1] http://cr.openjdk.java.net/~dnsimon/8177845.02

I reviewed the top repo and jdk repo change. For hotspot change,
I reviewed jdk.vm.ci.services/* and jdk.internal.vm.ci module-info.java.

make/make/CompileJavaModules.gmk
jdk/make/launcher/Launcher-jdk.aot.gmk
  Reading the original jdk.internal.vm.ci module-info.java [1],
  jdk.aot only accesses a small set of packages.  e.g. it does not
  access jdk.vm.ci.aarch64, jdk.vm.ci.code.stack, jdk.vm.ci.common, etc.

 115 jdk.internal.vm.compiler \

This should be removed from PLATFORM_MODULES list.  $PLATFORM_MODULES 
already includes $UPGRADEABLE_MODULES.

Otherwise, looks good to me.

There are jdk tests verifying upgradeable modules and qualified exports.
I suggest to run through JPRT -testset core options to catch any test failure.

Mandy
[1] 
http://cr.openjdk.java.net/~dnsimon/8177845.02/hotspot/src/jdk.internal.vm.ci/share/classes/module-info.java.sdiff.html

 

Re: RFR: 8177845: Need a mechanism to load Graal

[Doug Simon]

> On 27 Apr 2017, at 21:15, Vladimir Kozlov  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  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] 
>> 

Re: RFR: 8177845: Need a mechanism to load Graal

[Christian Thalinger]

> On Apr 27, 2017, at 11:51 AM, Doug Simon  wrote:
> 
>> 
>> On 27 Apr 2017, at 18:44, Christian Thalinger  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
>  
> 
> [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  wrote:
>>> 
 
 On 21 Apr 2017, at 13:46, Doug Simon  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
>>>  

Re: RFR: 8177845: Need a mechanism to load Graal

[Doug Simon]

> On 27 Apr 2017, at 18:44, Christian Thalinger  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  wrote:
>> 
>>> 
>>> On 21 Apr 2017, at 13:46, Doug Simon  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 

Re: RFR: 8177845: Need a mechanism to load Graal

[Christian Thalinger]

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  wrote:
> 
>> 
>> On 21 Apr 2017, at 13:46, Doug Simon  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
>  
> 

Re: RFR: 8177845: Need a mechanism to load Graal

[Doug Simon]

> On 27 Apr 2017, at 17:40, Alan Bateman  wrote:
> 
> On 27/04/2017 15:47, Doug Simon wrote:
> 
>> :
>> 
>> - 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.
>> 
> `--add-exports` is accumulative so what you have is okay.

Yes, it seems to work so I'm going to leave it as is.

> Alternatively the build has CommaList to convert space separated lists into 
> comma separated lists so I would expect this should work:
> 
> --add-exports=jdk.internal.vm.ci/jdk.vm.ci.amd64=$(call CommaList, 
> jdk.internal.vm.compiler  jdk.aot)

Thanks. Good to know in the future.

-Doug

Re: RFR: 8177845: Need a mechanism to load Graal

[Alan Bateman]

On 27/04/2017 15:47, Doug Simon wrote:


:

- 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.

`--add-exports` is accumulative so what you have is okay. Alternatively 
the build has CommaList to convert space separated lists into comma 
separated lists so I would expect this should work:


--add-exports=jdk.internal.vm.ci/jdk.vm.ci.amd64=$(call CommaList, 
jdk.internal.vm.compiler  jdk.aot)


-Alan

Re: RFR: 8177845: Need a mechanism to load Graal

[Doug Simon]

> On 21 Apr 2017, at 13:46, Doug Simon  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  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 
>> 

Re: RFR: 8177845: Need a mechanism to load Graal

[Doug Simon]

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've tested the current webrev (including hotspot.02 patch) against both 
upstream Graal and with jprt. 

-Doug

> On 20 Apr 2017, at 20:50, Doug Simon  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.(HotSpotJVMCIRuntime.java:290)
>   at 
> jdk.internal.vm.ci/jdk.vm.ci.hotspot.HotSpotJVMCIRuntime.(HotSpotJVMCIRuntime.java:65)
>   at 
> jdk.internal.vm.ci/jdk.vm.ci.hotspot.HotSpotJVMCIRuntime$DelayedInit.(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.(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)
>   

Re: RFR: 8177845: Need a mechanism to load Graal

[Doug Simon]

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.(HotSpotJVMCIRuntime.java:290)
at 
jdk.internal.vm.ci/jdk.vm.ci.hotspot.HotSpotJVMCIRuntime.(HotSpotJVMCIRuntime.java:65)
at 
jdk.internal.vm.ci/jdk.vm.ci.hotspot.HotSpotJVMCIRuntime$DelayedInit.(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.(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  wrote:
> 
> I've updated http://cr.openjdk.java.net/~dnsimon/8177845/hotspot/ with these 
> changes:
> 
> 1. JVMCIServiceLocator.getProvider(Class) is now protected
> 2. JVMCIServiceLocator.getProviders(Class) 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  wrote:
>> 
>>> 
>>> On 19 Apr 2017, at 21:40, Christian Thalinger  
>>> wrote:
>>> 
 
 On Apr 19, 2017, at 9:27 AM, Doug Simon  wrote:
 
> 
> On 19 Apr 2017, at 21:04, Mandy Chung  wrote:
> 
> 
>> On Apr 19, 2017, at 11:55 AM, Christian Thalinger 
>>  wrote:
>> 
>> 
>>> On Apr 19, 2017, at 8:38 AM, Mandy Chung  wrote:
>>> 
>>> Since jdk.internal.vm.compiler becomes an 

Re: RFR: 8177845: Need a mechanism to load Graal

[Doug Simon]

> On 19 Apr 2017, at 22:29, Vladimir Kozlov  wrote:
> 
> ReflectionAccessJDK ? Based on your comment in the file.
> 
> Vladimir
> 
> On 4/19/17 1:02 PM, Doug Simon wrote:
>> Sure - how about good old Util? ;-) I'm open to other suggestions.
>> 
>> Sent from my iPhone
>> 
>>> On Apr 19, 2017, at 9:46 PM, Vladimir Kozlov  
>>> wrote:
>>> 
>>> Hi Doug,
>>> 
>>> Can you consider using other name and not JDK9 for new JVMCI class? It will 
>>> be used in JDK 10 too:
>>> 
>>> jdk.vm.ci.services.internal.JDK9;
>>> 
>>> Thanks,
>>> Vladimir
>>> 
 On 4/18/17 3:13 PM, Doug Simon wrote:
 Please review these changes that make jdk.internal.vm.compiler an 
 upgradable compiler.
 The primary requirement for this is to remove all usage of JDK internals 
 from Graal.
 While this most involves changes in Graal, there remain a few capabilities 
 that must
 be exposed via JVMCI. Namely:
 
 1. Graal needs a copy of jdk.internal.misc.VM.savedProps so that it can 
 Graal initialize
  can use system properties that are guaranteed not to have been modified 
 by application
  code (Graal initialization is lazy and may occur after application code 
 has been run).
 
 2. Truffle needs to be able to trigger JVMCI initialization so that it can 
 select the Graal
  optimized implementation of the Truffle runtime.
 
 These capabilities have been added to jdk.vm.ci.services.Services. A 
 comment has
 also been added to this class to denote the current methods to be removed
 in a subsequent bug to update the JDK copy of Graal with the upstream 
 version which
 no longer uses the methods. The methods destined for removal are:
 
 exportJVMCITo(Class requestor)
 load(Class service)
 loadSingle(Class service, boolean required)
 
 The first one is no longer needed as JVMCI exports itself to Graal service 
 providers
 it loads via private API and Graal re-exports[1] JVMCI to any module the 
 extends Graal.
 The latter 2 are no longer required as Graal uses the standard 
 ServiceLoader. This API
 is only useful for JVMCI-8 where a hidden JVMCI class loader is used.
 
 These changes have been tested against upstream Graal. To make the Graal 
 tests pass, 2 extra
 minor changes were required:
 
 1. In JDK-8177663, HotSpotMemoryAccessProviderImpl::checkRead was added 
 and throws IllegalArgumentException
  on all error paths except one which throws AssertionError. The latter was 
 a mistake and has been
  fixed to also throw IllegalArgumentException.
 2. An invalid assertion has been removed from 
 HotSpotResolvedObjectTypeImpl::isDefinitelyResolvedWithRespectTo.
  Up to JDK 8, it was true that all classes in java.* packages were loaded 
 by the boot class loader.
  This is no longer true in JDK 9 with classes like java.sql being loaded 
 by platform class loader.
 
 As hinted above, a subsequent bug will be required to pull in the latest 
 upstream Graal and
 remove use of JDK internal API from the jdk.aot module. The qualified 
 exports to
 jdk.vm.internal.compiler and jdk.aot can then be removed.
 
 -Doug
 
 https://bugs.openjdk.java.net/browse/JDK-8177845
 http://cr.openjdk.java.net/~dnsimon/8177845/
 
 [1] 
 http://openjdk.java.net/projects/jigsaw/spec/issues/#IndirectQualifiedReflectiveAccess
 
 
 
>> 

Re: RFR: 8177845: Need a mechanism to load Graal

[Doug Simon]

> On 20 Apr 2017, at 00:31, Vladimir Kozlov  wrote:
> 
> Doug,
> 
> Can you point (link) particular code which needs to be reviewed? And what 
> security issues could be?

I believe Chris had possible concerns with Services.initializeJVMCI() (at 
http://cr.openjdk.java.net/~dnsimon/8177845/hotspot/src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.services/src/jdk/vm/ci/services/Services.java.udiff.html).
 As previously stated, I can't see any security issues with this method which 
is why it doesn't check JVMCIPermission.

-Doug

> 
> Thanks,
> Vladimir
> 
> On 4/19/17 2:12 PM, Doug Simon wrote:
>>> 3. Services.initializeJVMCI()
> 
 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.
>> 

Re: RFR: 8177845: Need a mechanism to load Graal

[Vladimir Kozlov]

Doug,

Can you point (link) particular code which needs to be reviewed? And 
what security issues could be?


Thanks,
Vladimir

On 4/19/17 2:12 PM, Doug Simon wrote:

3. Services.initializeJVMCI()



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.

Re: RFR: 8177845: Need a mechanism to load Graal

[Vladimir Kozlov]

Hotspot changes looks good to me.

Thanks,
Vladimir

On 4/19/17 2:26 PM, Doug Simon wrote:

I've updated http://cr.openjdk.java.net/~dnsimon/8177845/hotspot/ with these 
changes:

1. JVMCIServiceLocator.getProvider(Class) is now protected
2. JVMCIServiceLocator.getProviders(Class) 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  wrote:



On 19 Apr 2017, at 21:40, Christian Thalinger  wrote:



On Apr 19, 2017, at 9:27 AM, Doug Simon  wrote:



On 19 Apr 2017, at 21:04, Mandy Chung  wrote:



On Apr 19, 2017, at 11:55 AM, Christian Thalinger  
wrote:



On Apr 19, 2017, at 8:38 AM, Mandy Chung  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)
2. JVMCIServiceLocator.getProviders(Class)
3. Services.initializeJVMCI()
4. Services.getSavedProperties()
5. Services.exportJVMCITo(Class)
6. Services.load(Class)
7. Services.loadSingle(Class, 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

Re: RFR: 8177845: Need a mechanism to load Graal

[Doug Simon]

I've updated http://cr.openjdk.java.net/~dnsimon/8177845/hotspot/ with these 
changes:

1. JVMCIServiceLocator.getProvider(Class) is now protected
2. JVMCIServiceLocator.getProviders(Class) 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  wrote:
> 
>> 
>> On 19 Apr 2017, at 21:40, Christian Thalinger  wrote:
>> 
>>> 
>>> On Apr 19, 2017, at 9:27 AM, Doug Simon  wrote:
>>> 
 
 On 19 Apr 2017, at 21:04, Mandy Chung  wrote:
 
 
> On Apr 19, 2017, at 11:55 AM, Christian Thalinger 
>  wrote:
> 
> 
>> On Apr 19, 2017, at 8:38 AM, Mandy Chung  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)
>>> 2. JVMCIServiceLocator.getProviders(Class)
>>> 3. Services.initializeJVMCI()
>>> 4. Services.getSavedProperties()
>>> 5. Services.exportJVMCITo(Class)
>>> 6. Services.load(Class)
>>> 7. Services.loadSingle(Class, 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

Re: RFR: 8177845: Need a mechanism to load Graal

[Doug Simon]

> On 19 Apr 2017, at 21:40, Christian Thalinger  wrote:
> 
>> 
>> On Apr 19, 2017, at 9:27 AM, Doug Simon  wrote:
>> 
>>> 
>>> On 19 Apr 2017, at 21:04, Mandy Chung  wrote:
>>> 
>>> 
 On Apr 19, 2017, at 11:55 AM, Christian Thalinger  
 wrote:
 
 
> On Apr 19, 2017, at 8:38 AM, Mandy Chung  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)
>> 2. JVMCIServiceLocator.getProviders(Class)
>> 3. Services.initializeJVMCI()
>> 4. Services.getSavedProperties()
>> 5. Services.exportJVMCITo(Class)
>> 6. Services.load(Class)
>> 7. Services.loadSingle(Class, 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

Re: RFR: 8177845: Need a mechanism to load Graal

[Vladimir Kozlov]

ReflectionAccessJDK ? Based on your comment in the file.

Vladimir

On 4/19/17 1:02 PM, Doug Simon wrote:

Sure - how about good old Util? ;-) I'm open to other suggestions.

Sent from my iPhone


On Apr 19, 2017, at 9:46 PM, Vladimir Kozlov  wrote:

Hi Doug,

Can you consider using other name and not JDK9 for new JVMCI class? It will be 
used in JDK 10 too:

jdk.vm.ci.services.internal.JDK9;

Thanks,
Vladimir


On 4/18/17 3:13 PM, Doug Simon wrote:
Please review these changes that make jdk.internal.vm.compiler an upgradable 
compiler.
The primary requirement for this is to remove all usage of JDK internals from 
Graal.
While this most involves changes in Graal, there remain a few capabilities that 
must
be exposed via JVMCI. Namely:

1. Graal needs a copy of jdk.internal.misc.VM.savedProps so that it can Graal 
initialize
  can use system properties that are guaranteed not to have been modified by 
application
  code (Graal initialization is lazy and may occur after application code has 
been run).

2. Truffle needs to be able to trigger JVMCI initialization so that it can 
select the Graal
  optimized implementation of the Truffle runtime.

These capabilities have been added to jdk.vm.ci.services.Services. A comment has
also been added to this class to denote the current methods to be removed
in a subsequent bug to update the JDK copy of Graal with the upstream version 
which
no longer uses the methods. The methods destined for removal are:

exportJVMCITo(Class requestor)
load(Class service)
loadSingle(Class service, boolean required)

The first one is no longer needed as JVMCI exports itself to Graal service 
providers
it loads via private API and Graal re-exports[1] JVMCI to any module the 
extends Graal.
The latter 2 are no longer required as Graal uses the standard ServiceLoader. 
This API
is only useful for JVMCI-8 where a hidden JVMCI class loader is used.

These changes have been tested against upstream Graal. To make the Graal tests 
pass, 2 extra
minor changes were required:

1. In JDK-8177663, HotSpotMemoryAccessProviderImpl::checkRead was added and 
throws IllegalArgumentException
  on all error paths except one which throws AssertionError. The latter was a 
mistake and has been
  fixed to also throw IllegalArgumentException.
2. An invalid assertion has been removed from 
HotSpotResolvedObjectTypeImpl::isDefinitelyResolvedWithRespectTo.
  Up to JDK 8, it was true that all classes in java.* packages were loaded by 
the boot class loader.
  This is no longer true in JDK 9 with classes like java.sql being loaded by 
platform class loader.

As hinted above, a subsequent bug will be required to pull in the latest 
upstream Graal and
remove use of JDK internal API from the jdk.aot module. The qualified exports to
jdk.vm.internal.compiler and jdk.aot can then be removed.

-Doug

https://bugs.openjdk.java.net/browse/JDK-8177845
http://cr.openjdk.java.net/~dnsimon/8177845/

[1] 
http://openjdk.java.net/projects/jigsaw/spec/issues/#IndirectQualifiedReflectiveAccess

Re: RFR: 8177845: Need a mechanism to load Graal

[Doug Simon]

Sure - how about good old Util? ;-) I'm open to other suggestions.

Sent from my iPhone

> On Apr 19, 2017, at 9:46 PM, Vladimir Kozlov  
> wrote:
> 
> Hi Doug,
> 
> Can you consider using other name and not JDK9 for new JVMCI class? It will 
> be used in JDK 10 too:
> 
> jdk.vm.ci.services.internal.JDK9;
> 
> Thanks,
> Vladimir
> 
>> On 4/18/17 3:13 PM, Doug Simon wrote:
>> Please review these changes that make jdk.internal.vm.compiler an upgradable 
>> compiler.
>> The primary requirement for this is to remove all usage of JDK internals 
>> from Graal.
>> While this most involves changes in Graal, there remain a few capabilities 
>> that must
>> be exposed via JVMCI. Namely:
>> 
>> 1. Graal needs a copy of jdk.internal.misc.VM.savedProps so that it can 
>> Graal initialize
>>   can use system properties that are guaranteed not to have been modified by 
>> application
>>   code (Graal initialization is lazy and may occur after application code 
>> has been run).
>> 
>> 2. Truffle needs to be able to trigger JVMCI initialization so that it can 
>> select the Graal
>>   optimized implementation of the Truffle runtime.
>> 
>> These capabilities have been added to jdk.vm.ci.services.Services. A comment 
>> has
>> also been added to this class to denote the current methods to be removed
>> in a subsequent bug to update the JDK copy of Graal with the upstream 
>> version which
>> no longer uses the methods. The methods destined for removal are:
>> 
>> exportJVMCITo(Class requestor)
>> load(Class service)
>> loadSingle(Class service, boolean required)
>> 
>> The first one is no longer needed as JVMCI exports itself to Graal service 
>> providers
>> it loads via private API and Graal re-exports[1] JVMCI to any module the 
>> extends Graal.
>> The latter 2 are no longer required as Graal uses the standard 
>> ServiceLoader. This API
>> is only useful for JVMCI-8 where a hidden JVMCI class loader is used.
>> 
>> These changes have been tested against upstream Graal. To make the Graal 
>> tests pass, 2 extra
>> minor changes were required:
>> 
>> 1. In JDK-8177663, HotSpotMemoryAccessProviderImpl::checkRead was added and 
>> throws IllegalArgumentException
>>   on all error paths except one which throws AssertionError. The latter was 
>> a mistake and has been
>>   fixed to also throw IllegalArgumentException.
>> 2. An invalid assertion has been removed from 
>> HotSpotResolvedObjectTypeImpl::isDefinitelyResolvedWithRespectTo.
>>   Up to JDK 8, it was true that all classes in java.* packages were loaded 
>> by the boot class loader.
>>   This is no longer true in JDK 9 with classes like java.sql being loaded by 
>> platform class loader.
>> 
>> As hinted above, a subsequent bug will be required to pull in the latest 
>> upstream Graal and
>> remove use of JDK internal API from the jdk.aot module. The qualified 
>> exports to
>> jdk.vm.internal.compiler and jdk.aot can then be removed.
>> 
>> -Doug
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8177845
>> http://cr.openjdk.java.net/~dnsimon/8177845/
>> 
>> [1] 
>> http://openjdk.java.net/projects/jigsaw/spec/issues/#IndirectQualifiedReflectiveAccess
>> 
>> 
>> 

Re: RFR: 8177845: Need a mechanism to load Graal

[Vladimir Kozlov]

Hi Doug,

Can you consider using other name and not JDK9 for new JVMCI class? It 
will be used in JDK 10 too:


jdk.vm.ci.services.internal.JDK9;

Thanks,
Vladimir

On 4/18/17 3:13 PM, Doug Simon wrote:

Please review these changes that make jdk.internal.vm.compiler an upgradable 
compiler.
The primary requirement for this is to remove all usage of JDK internals from 
Graal.
While this most involves changes in Graal, there remain a few capabilities that 
must
be exposed via JVMCI. Namely:

1. Graal needs a copy of jdk.internal.misc.VM.savedProps so that it can Graal 
initialize
   can use system properties that are guaranteed not to have been modified by 
application
   code (Graal initialization is lazy and may occur after application code has 
been run).

2. Truffle needs to be able to trigger JVMCI initialization so that it can 
select the Graal
   optimized implementation of the Truffle runtime.

These capabilities have been added to jdk.vm.ci.services.Services. A comment has
also been added to this class to denote the current methods to be removed
in a subsequent bug to update the JDK copy of Graal with the upstream version 
which
no longer uses the methods. The methods destined for removal are:

exportJVMCITo(Class requestor)
load(Class service)
loadSingle(Class service, boolean required)

The first one is no longer needed as JVMCI exports itself to Graal service 
providers
it loads via private API and Graal re-exports[1] JVMCI to any module the 
extends Graal.
The latter 2 are no longer required as Graal uses the standard ServiceLoader. 
This API
is only useful for JVMCI-8 where a hidden JVMCI class loader is used.

These changes have been tested against upstream Graal. To make the Graal tests 
pass, 2 extra
minor changes were required:

1. In JDK-8177663, HotSpotMemoryAccessProviderImpl::checkRead was added and 
throws IllegalArgumentException
   on all error paths except one which throws AssertionError. The latter was a 
mistake and has been
   fixed to also throw IllegalArgumentException.
2. An invalid assertion has been removed from 
HotSpotResolvedObjectTypeImpl::isDefinitelyResolvedWithRespectTo.
   Up to JDK 8, it was true that all classes in java.* packages were loaded by 
the boot class loader.
   This is no longer true in JDK 9 with classes like java.sql being loaded by 
platform class loader.

As hinted above, a subsequent bug will be required to pull in the latest 
upstream Graal and
remove use of JDK internal API from the jdk.aot module. The qualified exports to
jdk.vm.internal.compiler and jdk.aot can then be removed.

-Doug

https://bugs.openjdk.java.net/browse/JDK-8177845
http://cr.openjdk.java.net/~dnsimon/8177845/

[1] 
http://openjdk.java.net/projects/jigsaw/spec/issues/#IndirectQualifiedReflectiveAccess

Re: RFR: 8177845: Need a mechanism to load Graal

[Christian Thalinger]

> On Apr 19, 2017, at 9:27 AM, Doug Simon  wrote:
> 
>> 
>> On 19 Apr 2017, at 21:04, Mandy Chung  wrote:
>> 
>> 
>>> On Apr 19, 2017, at 11:55 AM, Christian Thalinger  
>>> wrote:
>>> 
>>> 
 On Apr 19, 2017, at 8:38 AM, Mandy Chung  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)
> 2. JVMCIServiceLocator.getProviders(Class)
> 3. Services.initializeJVMCI()
> 4. Services.getSavedProperties()
> 5. Services.exportJVMCITo(Class)
> 6. Services.load(Class)
> 7. Services.loadSingle(Class, 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.

> 
> 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?  It’s awfully late in the 
cycle...

> 
> Does this address the security concerns?

Mostly, yes.  Thanks.

> 
> -Doug

Re: RFR: 8177845: Need a mechanism to load Graal

[Mandy Chung]

> On Apr 19, 2017, at 12:07 PM, Doug Simon  wrote:
> 
> 
>> On 19 Apr 2017, at 20:01, Mandy Chung  wrote:
>> 
>> We should catch this issue and detect if VM.getSavedProperty is called when 
>> init level < 1 during early startup.
> 
> What are you proposing? Some extra testing of VM startup to see if this is an 
> issue? If so, can you please suggest what testing is to be performed.

This is an existing issue, not related to your change.  I will create JBS issue 
to track it.

What you have is fine.

Mandy

Re: RFR: 8177845: Need a mechanism to load Graal

[Doug Simon]

> On 19 Apr 2017, at 20:01, Mandy Chung  wrote:
> 
>> 
>> On Apr 19, 2017, at 8:03 AM, Peter Levart  wrote:
>> 
>> Hi Alan,
>> 
>> On 04/19/2017 03:41 PM, Alan Bateman wrote:
>>> On 19/04/2017 08:37, Peter Levart wrote:
>>> 
 :
 
 Note that Properties class is thread-safe, while HashMap isn't. But I 
 think this should not be a problem here, as during initPhase1(), as far as 
 I know, no threads apart from main thread are started yet (is this true?), 
 so anyone accessing getSavedProperty(ies) will either be the main thread 
 itself or a thread started afterwards, so there is a happens-before 
 relationship.
>>> The ReferenceHandler and Finalizer threads are started early, before 
>>> initPhase1. However, both should immediately block and in the case of the 
>>> Finalizer, await the completion of initPhase1 before polling.
>>> 
>>> -Alan
>> 
>> Just out of curiosity, what guarantees are there that no code before 
>> VM.saveAndRemoveProperties() ever executes Integer.valueOf(int) method for 
>> example? Note that this call is implicitly hidden in autoboxing of int(s)...
>> 
>> If this happens, then class initialization of Integer.IntegerCache may fail, 
>> because it is using VM.getSavedProperty():
>> 
>>   public static String getSavedProperty(String key) {
>>   if (savedProps.isEmpty())
>>   throw new IllegalStateException("Should be non-empty if 
>> initialized");
>> 
>>   return savedProps.getProperty(key);
>>   }
>> 
>> Hm
> 
> We should catch this issue and detect if VM.getSavedProperty is called when 
> init level < 1 during early startup.

What are you proposing? Some extra testing of VM startup to see if this is an 
issue? If so, can you please suggest what testing is to be performed.

-Doug

Re: RFR: 8177845: Need a mechanism to load Graal

[Mandy Chung]

> On Apr 19, 2017, at 11:55 AM, Christian Thalinger  
> wrote:
> 
> 
>> On Apr 19, 2017, at 8:38 AM, Mandy Chung  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.

Mandy

Re: RFR: 8177845: Need a mechanism to load Graal

[Christian Thalinger]

> On Apr 19, 2017, at 8:38 AM, Mandy Chung  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?  After all, this is a 
security issue.

> 
> Mandy
> 
>> On Apr 19, 2017, at 11:24 AM, Christian Thalinger  
>> wrote:
>> 
>> I lost track a bit but why are we exporting jdk.vm.ci.services to the world 
>> now?
>> 
>> module jdk.internal.vm.ci {
>> -exports jdk.vm.ci.services to jdk.internal.vm.compiler;
>> +exports jdk.vm.ci.services;
>> 
>>> On Apr 18, 2017, at 9:16 PM, Doug Simon  wrote:
>>> 
>>> (adding hotspot-compiler-dev)
>>> 
>>> Please review these changes that make jdk.internal.vm.compiler an 
>>> upgradable compiler.
>>> The primary requirement for this is to remove all usage of JDK internals 
>>> from Graal.
>>> While this most involves changes in Graal, there remain a few capabilities 
>>> that must
>>> be exposed via JVMCI. Namely:
>>> 
>>> 1. Graal needs a copy of jdk.internal.misc.VM.savedProps so that it can 
>>> Graal initialize
>>> can use system properties that are guaranteed not to have been modified by 
>>> application
>>> code (Graal initialization is lazy and may occur after application code has 
>>> been run).
>>> 
>>> 2. Truffle needs to be able to trigger JVMCI initialization so that it can 
>>> select the Graal
>>> optimized implementation of the Truffle runtime.
>>> 
>>> These capabilities have been added to jdk.vm.ci.services.Services. A 
>>> comment has
>>> also been added to this class to denote the current methods to be removed
>>> in a subsequent bug to update the JDK copy of Graal with the upstream 
>>> version which
>>> no longer uses the methods. The methods destined for removal are:
>>> 
>>> exportJVMCITo(Class requestor)
>>> load(Class service)
>>> loadSingle(Class service, boolean required)
>>> 
>>> The first one is no longer needed as JVMCI exports itself to Graal service 
>>> providers
>>> it loads via private API and Graal re-exports[1] JVMCI to any module the 
>>> extends Graal.
>>> The latter 2 are no longer required as Graal uses the standard 
>>> ServiceLoader. This API
>>> is only useful for JVMCI-8 where a hidden JVMCI class loader is used.
>>> 
>>> These changes have been tested against upstream Graal. To make the Graal 
>>> tests pass, 2 extra
>>> minor changes were required:
>>> 
>>> 1. In JDK-8177673, HotSpotMemoryAccessProviderImpl::checkRead was added and 
>>> throws IllegalArgumentException
>>> on all error paths except one which throws AssertionError. The latter was a 
>>> mistake and has been
>>> fixed to also throw IllegalArgumentException.
>>> 2. An invalid assertion has been removed from 
>>> HotSpotResolvedObjectTypeImpl::isDefinitelyResolvedWithRespectTo.
>>> Up to JDK 8, it was true that all classes in java.* packages were loaded by 
>>> the boot class loader.
>>> This is no longer true in JDK 9 with classes like java.sql being loaded by 
>>> platform class loader.
>>> 
>>> As hinted above, a subsequent bug will be required to pull in the latest 
>>> upstream Graal and
>>> remove use of JDK internal API from the jdk.aot module. The qualified 
>>> exports to
>>> jdk.vm.internal.compiler and jdk.aot can then be removed.
>>> 
>>> -Doug
>>> 
>>> https://bugs.openjdk.java.net/browse/JDK-8177845
>>> http://cr.openjdk.java.net/~dnsimon/8177845/
>>> 
>>> [1] 
>>> http://openjdk.java.net/projects/jigsaw/spec/issues/#IndirectQualifiedReflectiveAccess
>>> 
>>> 
>>> 
>> 
> 

Re: RFR: 8177845: Need a mechanism to load Graal

[Mandy Chung]

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.

Mandy

> On Apr 19, 2017, at 11:24 AM, Christian Thalinger  
> wrote:
> 
> I lost track a bit but why are we exporting jdk.vm.ci.services to the world 
> now?
> 
> module jdk.internal.vm.ci {
> -exports jdk.vm.ci.services to jdk.internal.vm.compiler;
> +exports jdk.vm.ci.services;
> 
>> On Apr 18, 2017, at 9:16 PM, Doug Simon  wrote:
>> 
>> (adding hotspot-compiler-dev)
>> 
>> Please review these changes that make jdk.internal.vm.compiler an upgradable 
>> compiler.
>> The primary requirement for this is to remove all usage of JDK internals 
>> from Graal.
>> While this most involves changes in Graal, there remain a few capabilities 
>> that must
>> be exposed via JVMCI. Namely:
>> 
>> 1. Graal needs a copy of jdk.internal.misc.VM.savedProps so that it can 
>> Graal initialize
>> can use system properties that are guaranteed not to have been modified by 
>> application
>> code (Graal initialization is lazy and may occur after application code has 
>> been run).
>> 
>> 2. Truffle needs to be able to trigger JVMCI initialization so that it can 
>> select the Graal
>> optimized implementation of the Truffle runtime.
>> 
>> These capabilities have been added to jdk.vm.ci.services.Services. A comment 
>> has
>> also been added to this class to denote the current methods to be removed
>> in a subsequent bug to update the JDK copy of Graal with the upstream 
>> version which
>> no longer uses the methods. The methods destined for removal are:
>> 
>> exportJVMCITo(Class requestor)
>> load(Class service)
>> loadSingle(Class service, boolean required)
>> 
>> The first one is no longer needed as JVMCI exports itself to Graal service 
>> providers
>> it loads via private API and Graal re-exports[1] JVMCI to any module the 
>> extends Graal.
>> The latter 2 are no longer required as Graal uses the standard 
>> ServiceLoader. This API
>> is only useful for JVMCI-8 where a hidden JVMCI class loader is used.
>> 
>> These changes have been tested against upstream Graal. To make the Graal 
>> tests pass, 2 extra
>> minor changes were required:
>> 
>> 1. In JDK-8177673, HotSpotMemoryAccessProviderImpl::checkRead was added and 
>> throws IllegalArgumentException
>> on all error paths except one which throws AssertionError. The latter was a 
>> mistake and has been
>> fixed to also throw IllegalArgumentException.
>> 2. An invalid assertion has been removed from 
>> HotSpotResolvedObjectTypeImpl::isDefinitelyResolvedWithRespectTo.
>> Up to JDK 8, it was true that all classes in java.* packages were loaded by 
>> the boot class loader.
>> This is no longer true in JDK 9 with classes like java.sql being loaded by 
>> platform class loader.
>> 
>> As hinted above, a subsequent bug will be required to pull in the latest 
>> upstream Graal and
>> remove use of JDK internal API from the jdk.aot module. The qualified 
>> exports to
>> jdk.vm.internal.compiler and jdk.aot can then be removed.
>> 
>> -Doug
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8177845
>> http://cr.openjdk.java.net/~dnsimon/8177845/
>> 
>> [1] 
>> http://openjdk.java.net/projects/jigsaw/spec/issues/#IndirectQualifiedReflectiveAccess
>> 
>> 
>> 
> 

Re: RFR: 8177845: Need a mechanism to load Graal

[Christian Thalinger]

I lost track a bit but why are we exporting jdk.vm.ci.services to the world now?

 module jdk.internal.vm.ci {
-exports jdk.vm.ci.services to jdk.internal.vm.compiler;
+exports jdk.vm.ci.services;

> On Apr 18, 2017, at 9:16 PM, Doug Simon  wrote:
> 
> (adding hotspot-compiler-dev)
> 
> Please review these changes that make jdk.internal.vm.compiler an upgradable 
> compiler.
> The primary requirement for this is to remove all usage of JDK internals from 
> Graal.
> While this most involves changes in Graal, there remain a few capabilities 
> that must
> be exposed via JVMCI. Namely:
> 
> 1. Graal needs a copy of jdk.internal.misc.VM.savedProps so that it can Graal 
> initialize
>  can use system properties that are guaranteed not to have been modified by 
> application
>  code (Graal initialization is lazy and may occur after application code has 
> been run).
> 
> 2. Truffle needs to be able to trigger JVMCI initialization so that it can 
> select the Graal
>  optimized implementation of the Truffle runtime.
> 
> These capabilities have been added to jdk.vm.ci.services.Services. A comment 
> has
> also been added to this class to denote the current methods to be removed
> in a subsequent bug to update the JDK copy of Graal with the upstream version 
> which
> no longer uses the methods. The methods destined for removal are:
> 
> exportJVMCITo(Class requestor)
> load(Class service)
> loadSingle(Class service, boolean required)
> 
> The first one is no longer needed as JVMCI exports itself to Graal service 
> providers
> it loads via private API and Graal re-exports[1] JVMCI to any module the 
> extends Graal.
> The latter 2 are no longer required as Graal uses the standard ServiceLoader. 
> This API
> is only useful for JVMCI-8 where a hidden JVMCI class loader is used.
> 
> These changes have been tested against upstream Graal. To make the Graal 
> tests pass, 2 extra
> minor changes were required:
> 
> 1. In JDK-8177673, HotSpotMemoryAccessProviderImpl::checkRead was added and 
> throws IllegalArgumentException
>  on all error paths except one which throws AssertionError. The latter was a 
> mistake and has been
>  fixed to also throw IllegalArgumentException.
> 2. An invalid assertion has been removed from 
> HotSpotResolvedObjectTypeImpl::isDefinitelyResolvedWithRespectTo.
>  Up to JDK 8, it was true that all classes in java.* packages were loaded by 
> the boot class loader.
>  This is no longer true in JDK 9 with classes like java.sql being loaded by 
> platform class loader.
> 
> As hinted above, a subsequent bug will be required to pull in the latest 
> upstream Graal and
> remove use of JDK internal API from the jdk.aot module. The qualified exports 
> to
> jdk.vm.internal.compiler and jdk.aot can then be removed.
> 
> -Doug
> 
> https://bugs.openjdk.java.net/browse/JDK-8177845
> http://cr.openjdk.java.net/~dnsimon/8177845/
> 
> [1] 
> http://openjdk.java.net/projects/jigsaw/spec/issues/#IndirectQualifiedReflectiveAccess
> 
> 
> 

Re: RFR: 8177845: Need a mechanism to load Graal

[Mandy Chung]

> On Apr 19, 2017, at 8:03 AM, Peter Levart  wrote:
> 
> Hi Alan,
> 
> On 04/19/2017 03:41 PM, Alan Bateman wrote:
>> On 19/04/2017 08:37, Peter Levart wrote:
>> 
>>> :
>>> 
>>> Note that Properties class is thread-safe, while HashMap isn't. But I think 
>>> this should not be a problem here, as during initPhase1(), as far as I 
>>> know, no threads apart from main thread are started yet (is this true?), so 
>>> anyone accessing getSavedProperty(ies) will either be the main thread 
>>> itself or a thread started afterwards, so there is a happens-before 
>>> relationship.
>> The ReferenceHandler and Finalizer threads are started early, before 
>> initPhase1. However, both should immediately block and in the case of the 
>> Finalizer, await the completion of initPhase1 before polling.
>> 
>> -Alan
> 
> Just out of curiosity, what guarantees are there that no code before 
> VM.saveAndRemoveProperties() ever executes Integer.valueOf(int) method for 
> example? Note that this call is implicitly hidden in autoboxing of int(s)...
> 
> If this happens, then class initialization of Integer.IntegerCache may fail, 
> because it is using VM.getSavedProperty():
> 
>public static String getSavedProperty(String key) {
>if (savedProps.isEmpty())
>throw new IllegalStateException("Should be non-empty if 
> initialized");
> 
>return savedProps.getProperty(key);
>}
> 
> Hm

We should catch this issue and detect if VM.getSavedProperty is called when 
init level < 1 during early startup.

Mandy

Re: RFR: 8177845: Need a mechanism to load Graal

[Doug Simon]

> On 19 Apr 2017, at 16:04, Alan Bateman  wrote:
> 
> On 18/04/2017 23:13, Doug Simon wrote:
> 
>> :
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8177845
>> http://cr.openjdk.java.net/~dnsimon/8177845/
>> 
> I'm happy to see jdk.internal.vm.compiler changing to be an upgradable module 
> and the patching foo going away.

Yes, I'm delighted to see the command line required for using upstream Graal 
shrinking!

> If I read the changes correctly then I can extend JVMCIServiceLocator and the 
> construction of my sub-class will open up all packages in jdk.internal.vm.ci 
> to me. It is there any to tie this to -XX:+EnableJVMCI?

We could but I'm not sure what it would buy you. The service lookup only 
originates from the JVMCI runtime and the initialization of JVMCI already 
checks EnableJVMCI[1]

-Doug

[1] 
http://hg.openjdk.java.net/jdk9/dev/hotspot/file/4368832d1991/src/share/vm/jvmci/jvmciRuntime.cpp#l634

Re: RFR: 8177845: Need a mechanism to load Graal

[Alan Bateman]

On 18/04/2017 23:13, Doug Simon wrote:


:

https://bugs.openjdk.java.net/browse/JDK-8177845
http://cr.openjdk.java.net/~dnsimon/8177845/

I'm happy to see jdk.internal.vm.compiler changing to be an upgradable 
module and the patching foo going away.


If I read the changes correctly then I can extend JVMCIServiceLocator 
and the construction of my sub-class will open up all packages in 
jdk.internal.vm.ci to me. It is there any to tie this to -XX:+EnableJVMCI?


-Alan

Re: RFR: 8177845: Need a mechanism to load Graal

[Alan Bateman]

On 19/04/2017 08:37, Peter Levart wrote:


:

Note that Properties class is thread-safe, while HashMap isn't. But I 
think this should not be a problem here, as during initPhase1(), as 
far as I know, no threads apart from main thread are started yet (is 
this true?), so anyone accessing getSavedProperty(ies) will either be 
the main thread itself or a thread started afterwards, so there is a 
happens-before relationship.
The ReferenceHandler and Finalizer threads are started early, before 
initPhase1. However, both should immediately block and in the case of 
the Finalizer, await the completion of initPhase1 before polling.


-Alan

Re: RFR: 8177845: Need a mechanism to load Graal

[Peter Levart]

On 04/19/2017 11:49 AM, Peter Levart wrote:
I was not aware of the new Map.ofEntries method. Nice to see more 
space efficient map implementations being added to the JDK.


Admittedly, I used this method in a way not envisioned by the author. 
Maybe there's a reason there is no Map.copyOf(Map) method there, which 
would make this even simpler. If there was one, it would be too easy 
to (mis)use it instead of Collections.unmodifiableMap(Map), albeit 
with a slightly different semantics, and force re-hashing-copying of 
big maps where there is no need to do that. But it would be a pretty 
nice replacement for the following idiom:


Collections.unmodifiableMap(new HashMap<>(someMap)) 


Sometimes I wish that besides public, protected, "package-private" and 
private, Java also had an "expert protected" access modifier ;-)


Regards, Peter

Re: RFR: 8177845: Need a mechanism to load Graal

[Peter Levart]

Hi Simon,

On 04/19/2017 11:25 AM, Doug Simon wrote:

Hi Peter,

All of your suggestions look good. I've updated 
http://cr.openjdk.java.net/~dnsimon/8177845/jdk/src/java.base/share/classes/jdk/internal/misc/VM.java.udiff.html
 to include them (please check I didn't make any copy errors in the process).


Looks good.



I was not aware of the new Map.ofEntries method. Nice to see more space 
efficient map implementations being added to the JDK.


Admittedly, I used this method in a way not envisioned by the author. 
Maybe there's a reason there is no Map.copyOf(Map) method there, which 
would make this even simpler. If there was one, it would be too easy to 
(mis)use it instead of Collections.unmodifiableMap(Map), albeit with a 
slightly different semantics, and force re-hashing-copying of big maps 
where there is no need to do that. But it would be a pretty nice 
replacement for the following idiom:


Collections.unmodifiableMap(new HashMap<>(someMap))

Regards, Peter



Thanks!

-Doug


On 19 Apr 2017, at 10:12, Peter Levart  wrote:



On 04/19/2017 09:42 AM, Peter Levart wrote:


On 04/19/2017 09:37 AM, Peter Levart wrote:


http://cr.openjdk.java.net/~plevart/jdk9-dev/8177845_VM.getSavedProperties/webrev.01/

Also, while we are at it, the following javadocs in the getSavedProperty() do 
not apply any more:

138  * It accesses a private copy of the system properties so
139  * that user's locking of the system properties object will not
140  * cause the library to deadlock.

In JDK 9, Properties class does not use locking any more on the Properties 
instance for get()/getProperty() methods...

Regards, Peter


I also noticed the following comment:

// TODO: the Property Management needs to be refactored and
// the appropriate prop keys need to be accessible to the
// calling classes to avoid duplication of keys.
private static Map savedProps;

...which is not entirely true. Neither keys nor values are duplicated (they are 
just referenced in the new copy of the Properties/Map object). What is 
duplicated is an excessive amount of internal objects, such as array slots and 
Map.Entry objects. If this is a concern, then we could use the new immutable 
Map implementation that is available in JDK 9, so the following lines in my 
webrev:

181 @SuppressWarnings("unchecked")
182 Map sp = new HashMap<>((Map)props);
183 // only main thread is running at this time, so savedProps and
184 // its content will be correctly published to threads started later
185 savedProps = Collections.unmodifiableMap(sp);

Could be changed into:

@SuppressWarnings("unchecked")
Map sp =
Map.ofEntries(props.entrySet().toArray(new Map.Entry[0]));
// only main thread is running at this time, so savedProps
// will be correctly published to threads started later
savedProps = sp;

...to save some excessive space (the implementation is a linear-probe hashtable 
which keeps keys and values directly in an array without wrapping them with  
Map.Entry objects).

Regards, Peter

Re: RFR: 8177845: Need a mechanism to load Graal

[Doug Simon]

Hi Peter,

All of your suggestions look good. I've updated 
http://cr.openjdk.java.net/~dnsimon/8177845/jdk/src/java.base/share/classes/jdk/internal/misc/VM.java.udiff.html
 to include them (please check I didn't make any copy errors in the process).

I was not aware of the new Map.ofEntries method. Nice to see more space 
efficient map implementations being added to the JDK.

Thanks!

-Doug

> On 19 Apr 2017, at 10:12, Peter Levart  wrote:
> 
> 
> 
> On 04/19/2017 09:42 AM, Peter Levart wrote:
>> 
>> 
>> On 04/19/2017 09:37 AM, Peter Levart wrote:
>>> 
>>> 
>>> http://cr.openjdk.java.net/~plevart/jdk9-dev/8177845_VM.getSavedProperties/webrev.01/
>>>  
>> 
>> Also, while we are at it, the following javadocs in the getSavedProperty() 
>> do not apply any more:
>> 
>> 138  * It accesses a private copy of the system properties so
>> 139  * that user's locking of the system properties object will not
>> 140  * cause the library to deadlock.
>> 
>> In JDK 9, Properties class does not use locking any more on the Properties 
>> instance for get()/getProperty() methods...
>> 
>> Regards, Peter
>> 
> 
> I also noticed the following comment:
> 
>// TODO: the Property Management needs to be refactored and
>// the appropriate prop keys need to be accessible to the
>// calling classes to avoid duplication of keys.
>private static Map savedProps;
> 
> ...which is not entirely true. Neither keys nor values are duplicated (they 
> are just referenced in the new copy of the Properties/Map object). What is 
> duplicated is an excessive amount of internal objects, such as array slots 
> and Map.Entry objects. If this is a concern, then we could use the new 
> immutable Map implementation that is available in JDK 9, so the following 
> lines in my webrev:
> 
> 181 @SuppressWarnings("unchecked")
> 182 Map sp = new HashMap<>((Map)props);
> 183 // only main thread is running at this time, so savedProps and
> 184 // its content will be correctly published to threads started 
> later
> 185 savedProps = Collections.unmodifiableMap(sp);
> 
> Could be changed into:
> 
>@SuppressWarnings("unchecked")
>Map sp =
>Map.ofEntries(props.entrySet().toArray(new Map.Entry[0]));
>// only main thread is running at this time, so savedProps
>// will be correctly published to threads started later
>savedProps = sp;
> 
> ...to save some excessive space (the implementation is a linear-probe 
> hashtable which keeps keys and values directly in an array without wrapping 
> them with  Map.Entry objects).
> 
> Regards, Peter
> 

Re: RFR: 8177845: Need a mechanism to load Graal

[Peter Levart]

On 04/19/2017 09:42 AM, Peter Levart wrote:



On 04/19/2017 09:37 AM, Peter Levart wrote:



http://cr.openjdk.java.net/~plevart/jdk9-dev/8177845_VM.getSavedProperties/webrev.01/ 



Also, while we are at it, the following javadocs in the 
getSavedProperty() do not apply any more:


 138  * It accesses a private copy of the system properties so
 139  * that user's locking of the system properties object will not
 140  * cause the library to deadlock.

In JDK 9, Properties class does not use locking any more on the 
Properties instance for get()/getProperty() methods...


Regards, Peter



I also noticed the following comment:

// TODO: the Property Management needs to be refactored and
// the appropriate prop keys need to be accessible to the
// calling classes to avoid duplication of keys.
private static Map savedProps;

...which is not entirely true. Neither keys nor values are duplicated 
(they are just referenced in the new copy of the Properties/Map object). 
What is duplicated is an excessive amount of internal objects, such as 
array slots and Map.Entry objects. If this is a concern, then we could 
use the new immutable Map implementation that is available in JDK 9, so 
the following lines in my webrev:


 181 @SuppressWarnings("unchecked")
 182 Map sp = new HashMap<>((Map)props);
 183 // only main thread is running at this time, so savedProps and
 184 // its content will be correctly published to threads 
started later

 185 savedProps = Collections.unmodifiableMap(sp);

Could be changed into:

@SuppressWarnings("unchecked")
Map sp =
Map.ofEntries(props.entrySet().toArray(new Map.Entry[0]));
// only main thread is running at this time, so savedProps
// will be correctly published to threads started later
savedProps = sp;

...to save some excessive space (the implementation is a linear-probe 
hashtable which keeps keys and values directly in an array without 
wrapping them with  Map.Entry objects).


Regards, Peter

Re: RFR: 8177845: Need a mechanism to load Graal

[Peter Levart]

On 04/19/2017 09:37 AM, Peter Levart wrote:



http://cr.openjdk.java.net/~plevart/jdk9-dev/8177845_VM.getSavedProperties/webrev.01/ 



Also, while we are at it, the following javadocs in the 
getSavedProperty() do not apply any more:


 138  * It accesses a private copy of the system properties so
 139  * that user's locking of the system properties object will not
 140  * cause the library to deadlock.

In JDK 9, Properties class does not use locking any more on the 
Properties instance for get()/getProperty() methods...


Regards, Peter

Re: RFR: 8177845: Need a mechanism to load Graal

[Doug Simon]

(adding hotspot-compiler-dev)

> On 19 Apr 2017, at 02:02, Mandy Chung  wrote:
> 
> 
>> On Apr 18, 2017, at 3:13 PM, Doug Simon  wrote:
>> 
>> Please review these changes that make jdk.internal.vm.compiler an upgradable 
>> compiler.
>> :
>> http://cr.openjdk.java.net/~dnsimon/8177845/
> 
> Thanks for making this change.  This would simplify the way to replace JDK’s 
> graal with the lab graal.  A couple of comments:
> 
> jdk/internal/misc/VM.java
> 
> 161  * Note that the saved system properties do not include
> 162  * the ones set by sun.misc.Version.init().
> 
> sun.misc.Version is no longer present in JDK 9.  Renamed to 
> java.lang.VersionProps

Sorry, the webrev was out of date. I've updated it and also fixed the comment 
for VM::getSavedProperty.

> jdk/vm/ci/services/Services.java
> 67 Class.forName("jdk.vm.ci.runtime.JVMCI”);
> 
> JVMCI class is local in jdk.internal.vm.ci module.  An alternative may be
> to provide a static initialize method rather than Class::forName.

JVMCI is broken into fine grained "projects" and the jdk.vm.ci.runtime[1] 
project depends on the jdk.vm.ci.services project[2] so I cannot make a direct 
reference without introducing a circular dependency. I don't expect the 
reflection cost to be significant in practice.

> jdk/vm/ci/hotspot/HotSpotJVMCIRuntime.java
> 211 for (HotSpotJVMCIBackendFactory factory : 
> ServiceLoader.load(HotSpotJVMCIBackendFactory.class)) {
> 
> This uses the thread context class loader to load providers.  Services::load 
> uses 
> the system class loader to load providers.  I suspect you want this to use 
> the 
> system class loader here too.
> 
> jdk/vm/ci/services/JVMCIServiceLocator.java
> 78 for (JVMCIServiceLocator access : 
> ServiceLoader.load(JVMCIServiceLocator.class)) {
> 
> Same comment as above. I think you want to use system class loader to load 
> providers.

Yes, I think you're right. I've updated the webrev with that change.

Thanks for the review.

-Doug

[1] 
http://hg.openjdk.java.net/jdk9/dev/hotspot/file/560d7aa083a2/.mx.jvmci/suite.py#l94
[2] 
http://hg.openjdk.java.net/jdk9/dev/hotspot/file/560d7aa083a2/.mx.jvmci/suite.py#l45

RFR: 8177845: Need a mechanism to load Graal

[Doug Simon]

(adding hotspot-compiler-dev)

Please review these changes that make jdk.internal.vm.compiler an upgradable 
compiler.
The primary requirement for this is to remove all usage of JDK internals from 
Graal.
While this most involves changes in Graal, there remain a few capabilities that 
must
be exposed via JVMCI. Namely:

1. Graal needs a copy of jdk.internal.misc.VM.savedProps so that it can Graal 
initialize
  can use system properties that are guaranteed not to have been modified by 
application
  code (Graal initialization is lazy and may occur after application code has 
been run).

2. Truffle needs to be able to trigger JVMCI initialization so that it can 
select the Graal
  optimized implementation of the Truffle runtime.

These capabilities have been added to jdk.vm.ci.services.Services. A comment has
also been added to this class to denote the current methods to be removed
in a subsequent bug to update the JDK copy of Graal with the upstream version 
which
no longer uses the methods. The methods destined for removal are:

exportJVMCITo(Class requestor)
load(Class service)
loadSingle(Class service, boolean required)

The first one is no longer needed as JVMCI exports itself to Graal service 
providers
it loads via private API and Graal re-exports[1] JVMCI to any module the 
extends Graal.
The latter 2 are no longer required as Graal uses the standard ServiceLoader. 
This API
is only useful for JVMCI-8 where a hidden JVMCI class loader is used.

These changes have been tested against upstream Graal. To make the Graal tests 
pass, 2 extra
minor changes were required:

1. In JDK-8177673, HotSpotMemoryAccessProviderImpl::checkRead was added and 
throws IllegalArgumentException
  on all error paths except one which throws AssertionError. The latter was a 
mistake and has been
  fixed to also throw IllegalArgumentException.
2. An invalid assertion has been removed from 
HotSpotResolvedObjectTypeImpl::isDefinitelyResolvedWithRespectTo.
  Up to JDK 8, it was true that all classes in java.* packages were loaded by 
the boot class loader.
  This is no longer true in JDK 9 with classes like java.sql being loaded by 
platform class loader.

As hinted above, a subsequent bug will be required to pull in the latest 
upstream Graal and
remove use of JDK internal API from the jdk.aot module. The qualified exports to
jdk.vm.internal.compiler and jdk.aot can then be removed.

-Doug

https://bugs.openjdk.java.net/browse/JDK-8177845
http://cr.openjdk.java.net/~dnsimon/8177845/

[1] 
http://openjdk.java.net/projects/jigsaw/spec/issues/#IndirectQualifiedReflectiveAccess

Re: RFR: 8177845: Need a mechanism to load Graal

[Doug Simon]

> On 19 Apr 2017, at 02:02, Mandy Chung  wrote:
> 
> 
>> On Apr 18, 2017, at 3:13 PM, Doug Simon  wrote:
>> 
>> Please review these changes that make jdk.internal.vm.compiler an upgradable 
>> compiler.
>> :
>> http://cr.openjdk.java.net/~dnsimon/8177845/
> 
> Thanks for making this change.  This would simplify the way to replace JDK’s 
> graal with the lab graal.  A couple of comments:
> 
> jdk/internal/misc/VM.java
> 
> 161  * Note that the saved system properties do not include
> 162  * the ones set by sun.misc.Version.init().
> 
> sun.misc.Version is no longer present in JDK 9.  Renamed to 
> java.lang.VersionProps

Sorry, the webrev was out of date. I've updated it and also fixed the comment 
for VM::getSavedProperty.

> jdk/vm/ci/services/Services.java
>  67 Class.forName("jdk.vm.ci.runtime.JVMCI”);
> 
> JVMCI class is local in jdk.internal.vm.ci module.  An alternative may be
> to provide a static initialize method rather than Class::forName.

JVMCI is broken into fine grained "projects" and the jdk.vm.ci.runtime[1] 
project depends on the jdk.vm.ci.services project[2] so I cannot make a direct 
reference without introducing a circular dependency. I don't expect the 
reflection cost to be significant in practice.

> jdk/vm/ci/hotspot/HotSpotJVMCIRuntime.java
> 211 for (HotSpotJVMCIBackendFactory factory : 
> ServiceLoader.load(HotSpotJVMCIBackendFactory.class)) {
> 
> This uses the thread context class loader to load providers.  Services::load 
> uses 
> the system class loader to load providers.  I suspect you want this to use 
> the 
> system class loader here too.
> 
> jdk/vm/ci/services/JVMCIServiceLocator.java
>  78 for (JVMCIServiceLocator access : 
> ServiceLoader.load(JVMCIServiceLocator.class)) {
> 
> Same comment as above. I think you want to use system class loader to load 
> providers.

Yes, I think you're right. I've updated the webrev with that change.

Thanks for the review.

-Doug

[1] 
http://hg.openjdk.java.net/jdk9/dev/hotspot/file/560d7aa083a2/.mx.jvmci/suite.py#l94
[2] 
http://hg.openjdk.java.net/jdk9/dev/hotspot/file/560d7aa083a2/.mx.jvmci/suite.py#l45

Re: RFR: 8177845: Need a mechanism to load Graal

[Mandy Chung]

> On Apr 18, 2017, at 3:13 PM, Doug Simon  wrote:
> 
> Please review these changes that make jdk.internal.vm.compiler an upgradable 
> compiler.
> :
> http://cr.openjdk.java.net/~dnsimon/8177845/

Thanks for making this change.  This would simplify the way to replace JDK’s 
graal with the lab graal.  A couple of comments:

jdk/internal/misc/VM.java

 161  * Note that the saved system properties do not include
 162  * the ones set by sun.misc.Version.init().

sun.misc.Version is no longer present in JDK 9.  Renamed to 
java.lang.VersionProps

jdk/vm/ci/services/Services.java
  67 Class.forName("jdk.vm.ci.runtime.JVMCI”);

JVMCI class is local in jdk.internal.vm.ci module.  An alternative may be
to provide a static initialize method rather than Class::forName.

jdk/vm/ci/hotspot/HotSpotJVMCIRuntime.java
 211 for (HotSpotJVMCIBackendFactory factory : 
ServiceLoader.load(HotSpotJVMCIBackendFactory.class)) {

This uses the thread context class loader to load providers.  Services::load 
uses 
the system class loader to load providers.  I suspect you want this to use the 
system class loader here too.

jdk/vm/ci/services/JVMCIServiceLocator.java
  78 for (JVMCIServiceLocator access : 
ServiceLoader.load(JVMCIServiceLocator.class)) {

Same comment as above. I think you want to use system class loader to load 
providers.

Mandy

RFR: 8177845: Need a mechanism to load Graal

[Doug Simon]

Please review these changes that make jdk.internal.vm.compiler an upgradable 
compiler.
The primary requirement for this is to remove all usage of JDK internals from 
Graal.
While this most involves changes in Graal, there remain a few capabilities that 
must
be exposed via JVMCI. Namely:

1. Graal needs a copy of jdk.internal.misc.VM.savedProps so that it can Graal 
initialize
   can use system properties that are guaranteed not to have been modified by 
application
   code (Graal initialization is lazy and may occur after application code has 
been run).

2. Truffle needs to be able to trigger JVMCI initialization so that it can 
select the Graal
   optimized implementation of the Truffle runtime.

These capabilities have been added to jdk.vm.ci.services.Services. A comment has
also been added to this class to denote the current methods to be removed
in a subsequent bug to update the JDK copy of Graal with the upstream version 
which
no longer uses the methods. The methods destined for removal are:

exportJVMCITo(Class requestor)
load(Class service)
loadSingle(Class service, boolean required)

The first one is no longer needed as JVMCI exports itself to Graal service 
providers
it loads via private API and Graal re-exports[1] JVMCI to any module the 
extends Graal.
The latter 2 are no longer required as Graal uses the standard ServiceLoader. 
This API
is only useful for JVMCI-8 where a hidden JVMCI class loader is used.

These changes have been tested against upstream Graal. To make the Graal tests 
pass, 2 extra
minor changes were required:

1. In JDK-8177663, HotSpotMemoryAccessProviderImpl::checkRead was added and 
throws IllegalArgumentException
   on all error paths except one which throws AssertionError. The latter was a 
mistake and has been
   fixed to also throw IllegalArgumentException.
2. An invalid assertion has been removed from 
HotSpotResolvedObjectTypeImpl::isDefinitelyResolvedWithRespectTo.
   Up to JDK 8, it was true that all classes in java.* packages were loaded by 
the boot class loader.
   This is no longer true in JDK 9 with classes like java.sql being loaded by 
platform class loader.

As hinted above, a subsequent bug will be required to pull in the latest 
upstream Graal and
remove use of JDK internal API from the jdk.aot module. The qualified exports to
jdk.vm.internal.compiler and jdk.aot can then be removed.

-Doug

https://bugs.openjdk.java.net/browse/JDK-8177845
http://cr.openjdk.java.net/~dnsimon/8177845/

[1] 
http://openjdk.java.net/projects/jigsaw/spec/issues/#IndirectQualifiedReflectiveAccess