Re: RFR: 8145337: [JVMCI] JVMCI initialization with SecurityManager installed fails: java.security.AccessControlException: access denied

[Doug Simon]

> On 1 Feb 2017, at 20:54, Sean Mullan <sean.mul...@oracle.com> wrote:
> 
> Couple of comments:
> 
> - jdk.vm.ci is already loaded by the boot loader so it is implicitly granted 
> AllPermission and does not need an entry in default.policy.

Thanks - I removed it.

> - all internal APIs in the jdk.vm.compiler module will now be restricted by 
> default by SecurityManager::checkPackageAccess(), so if you have any code or 
> tests running with a SecurityManager that are accessing internal APIs in the 
> jdk.vm.compiler module, you will need to grant them an appropriate 
> "accessClassInPackage" RuntimePermission in addition to any --add-exports 
> option you are using to break through encapsulation.

Vladimir, does the AOT need to run with a SecurityManager and if so, I assume 
the qualified exports from jdk.vm.compiler to jdk.aot will allow it to run 
without needed an extra policy file?

-Doug

> On 2/1/17 6:07 AM, Doug Simon wrote:
>> I’ve reworked the webrev as requested to make jdk.vm.compiler a 
>> non-upgradeable platform module, this allowing it to be mentioned in 
>> default.policy:
>> 
>> http://cr.openjdk.java.net/~dnsimon/8145337/
>> 
>> -Doug
>> 
>>> On 30 Jan 2017, at 22:53, Mandy Chung <mandy.ch...@oracle.com> wrote:
>>> 
>>>> 
>>>> On Jan 30, 2017, at 1:36 PM, Doug Simon <doug.si...@oracle.com> wrote:
>>>> 
>>>> 
>>>>> On 30 Jan 2017, at 21:55, Mandy Chung <mandy.ch...@oracle.com> wrote:
>>>>> 
>>>>> 
>>>>>> On Jan 30, 2017, at 10:38 AM, Doug Simon <doug.si...@oracle.com> wrote:
>>>>>> 
>>>>>> I’ve extended the webrev with that change - please re-review:
>>>>>> 
>>>>>> http://cr.openjdk.java.net/~dnsimon/8145337_make/webrev
>>>>>> 
>>>>> 
>>>>> +1
>>>> 
>>>> Thanks. Is that a “Reviewed”?
>>>> 
>>> 
>>> Sorry. I only noticed now that you added this to UPGRADEABLE_MODULE.   
>>> Please add it only to PLATFORM_MODULES list instead.
>>> 
>>> Making it an upgradeable module is a separate issue.  I suggest you reopen 
>>> JDK-8171448.  Specifically, since upgradeable modules are not tied with 
>>> java.base, our goal for JDK 9 is to eliminate qualified exports from JDK 
>>> modules to upgradeable modules, e.g. JDK-8170116, JDK-8166745, JDK-8161549.
>>> 
>>>> I think I should get at least one sign-off from the security team.
>>>> 
>>> 
>>> Hope Sean will review this one.  Please send an updated webrev.
>>> 
>>>> Also, since this is effectively making jdk.vm.compiler an upgradeable 
>>>> module,
>>> 
>>> No it does not.
>>> 
>>>> what’s the implication for it being a hash-checked module?
>>> 
>>> When a module M is recorded in the ModuleHashes attribute of java.base, the 
>>> runtime will check if module M resolved in the graph matches the one tied 
>>> with java.base when created at build time; if not, it will fail.  If an 
>>> upgradeable module
>>> 
>>>> It seems like these changes effectively achieve what I was requesting with 
>>>> https://bugs.openjdk.java.net/browse/JDK-8171448.
>>> 
>>> JDK-8145337 is about the security permission.  It’s better to separate this 
>>> review from JDK-8171448.
>>> 
>>> Mandy
>>> 
>>>> 
>>>> -Doug
>>>> 
>>>>> 
>>>>>> Strangely, there was no existing declaration of jdk.vm.compiler in 
>>>>>> Modules.gmk.
>>>>>> 
>>>>> 
>>>>> Default is to be defined by the application class loader.  The build will 
>>>>> find all modules from the source. There is no need to list all modules.
>>>>> 
>>>>>> BTW, I never answered your question:
>>>>>> 
>>>>>> "How does JVMCI call out to jdk.vm.compiler?  does it load classes using 
>>>>>> Class::forName with the system class loader?”
>>>>>> 
>>>>>> It uses JVMCIServiceLocator[1] which is a mechanism built on the 
>>>>>> standard ServiceLoader.
>>>>> 
>>>>> Thanks for the pointer. That confirms my understanding that loads the 
>>>>> service providers using the system class loader.
>>>>> 
>>>>> Mandy
>> 

Re: RFR: 8145337: [JVMCI] JVMCI initialization with SecurityManager installed fails: java.security.AccessControlException: access denied

[Doug Simon]

I’ve reworked the webrev as requested to make jdk.vm.compiler a non-upgradeable 
platform module, this allowing it to be mentioned in default.policy:

http://cr.openjdk.java.net/~dnsimon/8145337/

-Doug

> On 30 Jan 2017, at 22:53, Mandy Chung <mandy.ch...@oracle.com> wrote:
> 
>> 
>> On Jan 30, 2017, at 1:36 PM, Doug Simon <doug.si...@oracle.com> wrote:
>> 
>> 
>>> On 30 Jan 2017, at 21:55, Mandy Chung <mandy.ch...@oracle.com> wrote:
>>> 
>>> 
>>>> On Jan 30, 2017, at 10:38 AM, Doug Simon <doug.si...@oracle.com> wrote:
>>>> 
>>>> I’ve extended the webrev with that change - please re-review:
>>>> 
>>>> http://cr.openjdk.java.net/~dnsimon/8145337_make/webrev
>>>> 
>>> 
>>> +1
>> 
>> Thanks. Is that a “Reviewed”?
>> 
> 
> Sorry. I only noticed now that you added this to UPGRADEABLE_MODULE.   Please 
> add it only to PLATFORM_MODULES list instead.
> 
> Making it an upgradeable module is a separate issue.  I suggest you reopen 
> JDK-8171448.  Specifically, since upgradeable modules are not tied with 
> java.base, our goal for JDK 9 is to eliminate qualified exports from JDK 
> modules to upgradeable modules, e.g. JDK-8170116, JDK-8166745, JDK-8161549.
> 
>> I think I should get at least one sign-off from the security team.
>> 
> 
> Hope Sean will review this one.  Please send an updated webrev.
> 
>> Also, since this is effectively making jdk.vm.compiler an upgradeable 
>> module, 
> 
> No it does not.
> 
>> what’s the implication for it being a hash-checked module?
> 
> When a module M is recorded in the ModuleHashes attribute of java.base, the 
> runtime will check if module M resolved in the graph matches the one tied 
> with java.base when created at build time; if not, it will fail.  If an 
> upgradeable module
> 
>> It seems like these changes effectively achieve what I was requesting with 
>> https://bugs.openjdk.java.net/browse/JDK-8171448.
> 
> JDK-8145337 is about the security permission.  It’s better to separate this 
> review from JDK-8171448.
> 
> Mandy
> 
>> 
>> -Doug
>> 
>>> 
>>>> Strangely, there was no existing declaration of jdk.vm.compiler in 
>>>> Modules.gmk.
>>>> 
>>> 
>>> Default is to be defined by the application class loader.  The build will 
>>> find all modules from the source. There is no need to list all modules.
>>> 
>>>> BTW, I never answered your question:
>>>> 
>>>> "How does JVMCI call out to jdk.vm.compiler?  does it load classes using 
>>>> Class::forName with the system class loader?”
>>>> 
>>>> It uses JVMCIServiceLocator[1] which is a mechanism built on the standard 
>>>> ServiceLoader.
>>> 
>>> Thanks for the pointer. That confirms my understanding that loads the 
>>> service providers using the system class loader.
>>> 
>>> Mandy

Re: RFR: 8145337: [JVMCI] JVMCI initialization with SecurityManager installed fails: java.security.AccessControlException: access denied

[Doug Simon]

> On 30 Jan 2017, at 21:55, Mandy Chung <mandy.ch...@oracle.com> wrote:
> 
> 
>> On Jan 30, 2017, at 10:38 AM, Doug Simon <doug.si...@oracle.com> wrote:
>> 
>> I’ve extended the webrev with that change - please re-review:
>> 
>> http://cr.openjdk.java.net/~dnsimon/8145337_make/webrev
>> 
> 
> +1

Thanks. Is that a “Reviewed”?

I think I should get at least one sign-off from the security team.

Also, since this is effectively making jdk.vm.compiler an upgradeable module, 
what’s the implication for it being a hash-checked module? It seems like these 
changes effectively achieve what I was requesting with 
https://bugs.openjdk.java.net/browse/JDK-8171448.

-Doug

> 
>> Strangely, there was no existing declaration of jdk.vm.compiler in 
>> Modules.gmk.
>> 
> 
> Default is to be defined by the application class loader.  The build will 
> find all modules from the source. There is no need to list all modules.
> 
>> BTW, I never answered your question:
>> 
>> "How does JVMCI call out to jdk.vm.compiler?  does it load classes using 
>> Class::forName with the system class loader?”
>> 
>> It uses JVMCIServiceLocator[1] which is a mechanism built on the standard 
>> ServiceLoader.
> 
> Thanks for the pointer. That confirms my understanding that loads the service 
> providers using the system class loader.
> 
> Mandy

Re: RFR: 8145337: [JVMCI] JVMCI initialization with SecurityManager installed fails: java.security.AccessControlException: access denied

[Doug Simon]

> On 1 Feb 2017, at 22:19, Vladimir Kozlov <vladimir.koz...@oracle.com> wrote:
> 
> AOT tool jaotc does not run with SecurityManager. We assume it runs in secure 
> environment and it does not access any external resources.

Great.

Can I now consider this change reviewed and integrate it?

-Doug

>> On Feb 1, 2017, at 12:03 PM, Doug Simon <doug.si...@oracle.com> wrote:
>> 
>> 
>>> On 1 Feb 2017, at 20:54, Sean Mullan <sean.mul...@oracle.com> wrote:
>>> 
>>> Couple of comments:
>>> 
>>> - jdk.vm.ci is already loaded by the boot loader so it is implicitly 
>>> granted AllPermission and does not need an entry in default.policy.
>> 
>> Thanks - I removed it.
>> 
>>> - all internal APIs in the jdk.vm.compiler module will now be restricted by 
>>> default by SecurityManager::checkPackageAccess(), so if you have any code 
>>> or tests running with a SecurityManager that are accessing internal APIs in 
>>> the jdk.vm.compiler module, you will need to grant them an appropriate 
>>> "accessClassInPackage" RuntimePermission in addition to any --add-exports 
>>> option you are using to break through encapsulation.
>> 
>> Vladimir, does the AOT need to run with a SecurityManager and if so, I 
>> assume the qualified exports from jdk.vm.compiler to jdk.aot will allow it 
>> to run without needed an extra policy file?
>> 
>> -Doug
>> 
>>>> On 2/1/17 6:07 AM, Doug Simon wrote:
>>>> I’ve reworked the webrev as requested to make jdk.vm.compiler a 
>>>> non-upgradeable platform module, this allowing it to be mentioned in 
>>>> default.policy:
>>>> 
>>>> http://cr.openjdk.java.net/~dnsimon/8145337/
>>>> 
>>>> -Doug
>>>> 
>>>>>> On 30 Jan 2017, at 22:53, Mandy Chung <mandy.ch...@oracle.com> wrote:
>>>>>> 
>>>>>> 
>>>>>> On Jan 30, 2017, at 1:36 PM, Doug Simon <doug.si...@oracle.com> wrote:
>>>>>> 
>>>>>> 
>>>>>>> On 30 Jan 2017, at 21:55, Mandy Chung <mandy.ch...@oracle.com> wrote:
>>>>>>> 
>>>>>>> 
>>>>>>>> On Jan 30, 2017, at 10:38 AM, Doug Simon <doug.si...@oracle.com> wrote:
>>>>>>>> 
>>>>>>>> I’ve extended the webrev with that change - please re-review:
>>>>>>>> 
>>>>>>>> http://cr.openjdk.java.net/~dnsimon/8145337_make/webrev
>>>>>>>> 
>>>>>>> 
>>>>>>> +1
>>>>>> 
>>>>>> Thanks. Is that a “Reviewed”?
>>>>>> 
>>>>> 
>>>>> Sorry. I only noticed now that you added this to UPGRADEABLE_MODULE.   
>>>>> Please add it only to PLATFORM_MODULES list instead.
>>>>> 
>>>>> Making it an upgradeable module is a separate issue.  I suggest you 
>>>>> reopen JDK-8171448.  Specifically, since upgradeable modules are not tied 
>>>>> with java.base, our goal for JDK 9 is to eliminate qualified exports from 
>>>>> JDK modules to upgradeable modules, e.g. JDK-8170116, JDK-8166745, 
>>>>> JDK-8161549.
>>>>> 
>>>>>> I think I should get at least one sign-off from the security team.
>>>>>> 
>>>>> 
>>>>> Hope Sean will review this one.  Please send an updated webrev.
>>>>> 
>>>>>> Also, since this is effectively making jdk.vm.compiler an upgradeable 
>>>>>> module,
>>>>> 
>>>>> No it does not.
>>>>> 
>>>>>> what’s the implication for it being a hash-checked module?
>>>>> 
>>>>> When a module M is recorded in the ModuleHashes attribute of java.base, 
>>>>> the runtime will check if module M resolved in the graph matches the one 
>>>>> tied with java.base when created at build time; if not, it will fail.  If 
>>>>> an upgradeable module
>>>>> 
>>>>>> It seems like these changes effectively achieve what I was requesting 
>>>>>> with https://bugs.openjdk.java.net/browse/JDK-8171448.
>>>>> 
>>>>> JDK-8145337 is about the security permission.  It’s better to separate 
>>>>> this review from JDK-8171448.
>>>>> 
>>>>> Mandy
>>>>> 
>>>>>> 
>>>>>> -Doug
>>>>>> 
>>>>>>> 
>>>>>>>> Strangely, there was no existing declaration of jdk.vm.compiler in 
>>>>>>>> Modules.gmk.
>>>>>>>> 
>>>>>>> 
>>>>>>> Default is to be defined by the application class loader.  The build 
>>>>>>> will find all modules from the source. There is no need to list all 
>>>>>>> modules.
>>>>>>> 
>>>>>>>> BTW, I never answered your question:
>>>>>>>> 
>>>>>>>> "How does JVMCI call out to jdk.vm.compiler?  does it load classes 
>>>>>>>> using Class::forName with the system class loader?”
>>>>>>>> 
>>>>>>>> It uses JVMCIServiceLocator[1] which is a mechanism built on the 
>>>>>>>> standard ServiceLoader.
>>>>>>> 
>>>>>>> Thanks for the pointer. That confirms my understanding that loads the 
>>>>>>> service providers using the system class loader.
>>>>>>> 
>>>>>>> Mandy
>>>> 
>> 
> 

Re: Running jaotc with an external Graal

[Doug Simon]

With the current bits in jdk9/hs and graal-core, the following bootstrapping 
command works in terms of replacing Graal in the JDK:

java -server -XX:+UnlockExperimentalVMOptions 
--module-path=/Users/dsimon/hs/truffle/mxbuild/modules/com.oracle.truffle.truffle_api.jar
 
--upgrade-module-path=/Users/dsimon/hs/graal-core/mxbuild/modules/jdk.vm.compiler.jar
 --patch-module=jdk.vm.compiler=.jar -XX:+UseJVMCICompiler -XX:+BootstrapJVMCI 
-version

However, the --patch-module + --upgrade-module-path trick[1] we’re using to 
replace the version of Graal in the JDK apparently only works due to a bug that 
will be fixed at some point. From Mandy Chung:

"-—patch-module is to patch a module to replace or add content of that module 
and yes it disables the hash checking on the patched module. However, it’s not 
intended to allow it to combine with —-upgrade-module-path as you did, to 
change the module dependences."

Given all the continuing flux around jigsaw, we cannot be sure that developing 
and using GitHub Graal on JDK 9 is stable until jigsaw is stable.

-Doug

[1] 
https://bugs.openjdk.java.net/browse/JDK-8171448?focusedCommentId=14046154=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14046154

> On 16 Feb 2017, at 10:17, Andrew Haley  wrote:
> 
> On 15/02/17 21:56, Christian Thalinger wrote:
>> 
>>> On Feb 14, 2017, at 4:38 AM, Andrew Haley  wrote:
>>> 
>>> On 14/02/17 14:37, Alan Bateman wrote:
 --patch-module can be used to patch any module in the boot layer. So if 
 you are looking to override or add classes then --patch-module should work.
>>> 
>>> Aha!  Thanks,
>> 
>> Does it?
> 
> Not quite.  The problem is that the service loader finds classes in
> the Hotspot copy of Graal which don't exist in the external copy, and
> chaos ensues.
> 
> I suppose the only way to make it work is to replace the copy of Graal
> in HotSpot, but I don't think that's a trivial thing to do.
> 
> Andrew.
> 

Re: Running jaotc with an external Graal

[Doug Simon]

I don’t know how one patches a module in the middle of the module graph. That 
is, we use --patch-module and --upgrade-module-path to override the 
jdk.vm.compiler module in the JDK. I don’t know what that means for modules 
such as jdk.aot that depend on jdk.vm.compiler. Maybe someone from the AOT or 
jigsaw team can help.

-Doug

> On 14 Feb 2017, at 12:26, Andrew Haley  wrote:
> 
> Is this possible?  Seems that no matter what I do, aotc always prefers to
> use the internal version of org.graalvm.compiler which is in HotSpot.
> 
> I don't quite get why this is: I can run an external Graal using JVMCI
> with no problems.  I saw "8145337: [JVMCI] JVMCI initialization with
> SecurityManager installed fails:" which might be related, but perhaps
> it's not.
> 
> So, why is it possible to use an external Graal with JVMCI, but
> apparently not with jaotc?  And is there anything I can do to make
> progress?
> 
> Thanks,
> 
> Andrew.

Re: Problem loading Truffle service providers in Graal

[Doug Simon]

> On 26 Feb 2017, at 13:26, Doug Simon <doug.si...@oracle.com> wrote:
> 
>> 
>> On 26 Feb 2017, at 13:22, Doug Simon <doug.si...@oracle.com> wrote:
>> 
>>> 
>>> On 26 Feb 2017, at 13:08, Alan Bateman <alan.bate...@oracle.com> wrote:
>>> 
>>> On 26/02/2017 11:20, Doug Simon wrote:
>>> 
>>>> While trying to get upstream Graal working again with JDK 9, I'm having 
>>>> problems with service loading. Graal uses a LayoutFactory service defined 
>>>> by Truffle where the latter also includes a provider. The relevant parts 
>>>> of the module-info descriptors are:
>>>> 
>>>> module com.oracle.truffle.truffle_api {
>>>> 
>>>>   exports com.oracle.truffle.object;
>>>>   exports com.oracle.truffle.object.basic;
>>>> 
>>>>   uses com.oracle.truffle.api.object.LayoutFactory;
>>>>   provides com.oracle.truffle.api.object.LayoutFactory with 
>>>> com.oracle.truffle.object.basic.DefaultLayoutFactory;
>>>> 
>>>> }
>>>> 
>>>> module jdk.internal.vm.compiler {
>>>>requires transitive com.oracle.truffle.truffle_api;
>>>>   uses com.oracle.truffle.api.object.LayoutFactory;
>>>> 
>>>> }
>>>> 
>>>> However, looking up a provider in Graal[1] returns no providers. As far as 
>>>> I understand service loading with modules, this is  because 
>>>> jdk.internal.vm.compiler is loaded via the platform class loader while 
>>>> truffle is loaded via the app class loader as shown by the output of trace 
>>>> statements I added:
>>>> 
>>>> GraalTruffleRuntime.class.getClassLoader() -> 
>>>> jdk.internal.loader.ClassLoaders$PlatformClassLoader@366e2eef
>>>> LayoutFactory.class.getClassLoader() -> 
>>>> jdk.internal.loader.ClassLoaders$AppClassLoader@6df97b55
>>>> 
>>>> This ability of a platform loaded class to depend on an app loaded class 
>>>> is probably due to the soon-to-be-disabled mechanism[2] we use for 
>>>> overriding the version of Graal bundled with JDK 9.
>>>> 
>>>> Is there some command line magic I can use to make this case work now or 
>>>> do I have to wait until [2] is addressed? If the latter, how will it work 
>>>> then?
>>>> 
>>> An upgraded moduledefined to the platform loader can link to types in 
>>> modules defined to the app loader. So I wouldn't expect issues there.
>>> 
>>> Can you track down the code that uses ServiceLoader.load to load the 
>>> LayoutFactory providers?
>> 
>> The code is in the first link of my previous message:
>> 
>> https://github.com/graalvm/graal-core/blob/1efc1c543acd7ed447c59788aeabc223be13e774/graal/org.graalvm.compiler.truffle/src/org/graalvm/compiler/truffle/GraalTruffleRuntime.java#L693
>> 
>>> It might be using loadInstalled or invoking it with the platform class 
>>> loader and that would explain what you are seeing.
>> 
>> Thanks - using LayoutFactory.class.getClassLoader() works. So the general 
>> rule is to use the leaf most class loader the service client knows about?
> 
> If that's the case, does that mean I could pick up any provider on the app 
> class path? Seems a little risky. Is there some way to confine it to modules 
> the service user trusts (by virtual of being a module in its dependency 
> graph)?

Also, what if there's provider in jdk.internal.vm.compiler itself? Will it be 
loaded along with the providers on my module path?

-Doug

Re: Problem loading Truffle service providers in Graal

[Doug Simon]

> On 26 Feb 2017, at 14:44, Alan Bateman <alan.bate...@oracle.com> wrote:
> 
> 
> 
> On 26/02/2017 13:13, Doug Simon wrote:
>> :
>> Also, what if there's provider in jdk.internal.vm.compiler itself? Will it 
>> be loaded along with the providers on my module path?
>> 
> If it has `provides com.oracle.truffle.api.object.LayoutFactory` then it will 
> be located too.

I assume that's because the app class loader delegates to the platform class 
loader?

> If the provider interface is only for overriding then you could use 
> ServiceLoader.load(LayerFactory.class).findFirst().orElse(DefaultLayoutFactory.get())
>  so that it use a default/fallback implementation in your module when there 
> isn't a provider deployed on the module path.

Ok, thanks for the tip.

-Doug

Re: Problem loading Truffle service providers in Graal

[Doug Simon]

> On 26 Feb 2017, at 13:22, Doug Simon <doug.si...@oracle.com> wrote:
> 
>> 
>> On 26 Feb 2017, at 13:08, Alan Bateman <alan.bate...@oracle.com> wrote:
>> 
>> On 26/02/2017 11:20, Doug Simon wrote:
>> 
>>> While trying to get upstream Graal working again with JDK 9, I'm having 
>>> problems with service loading. Graal uses a LayoutFactory service defined 
>>> by Truffle where the latter also includes a provider. The relevant parts of 
>>> the module-info descriptors are:
>>> 
>>> module com.oracle.truffle.truffle_api {
>>> 
>>>exports com.oracle.truffle.object;
>>>exports com.oracle.truffle.object.basic;
>>> 
>>>uses com.oracle.truffle.api.object.LayoutFactory;
>>>provides com.oracle.truffle.api.object.LayoutFactory with 
>>> com.oracle.truffle.object.basic.DefaultLayoutFactory;
>>> 
>>> }
>>> 
>>> module jdk.internal.vm.compiler {
>>> requires transitive com.oracle.truffle.truffle_api;
>>>uses com.oracle.truffle.api.object.LayoutFactory;
>>> 
>>> }
>>> 
>>> However, looking up a provider in Graal[1] returns no providers. As far as 
>>> I understand service loading with modules, this is  because 
>>> jdk.internal.vm.compiler is loaded via the platform class loader while 
>>> truffle is loaded via the app class loader as shown by the output of trace 
>>> statements I added:
>>> 
>>> GraalTruffleRuntime.class.getClassLoader() -> 
>>> jdk.internal.loader.ClassLoaders$PlatformClassLoader@366e2eef
>>> LayoutFactory.class.getClassLoader() -> 
>>> jdk.internal.loader.ClassLoaders$AppClassLoader@6df97b55
>>> 
>>> This ability of a platform loaded class to depend on an app loaded class is 
>>> probably due to the soon-to-be-disabled mechanism[2] we use for overriding 
>>> the version of Graal bundled with JDK 9.
>>> 
>>> Is there some command line magic I can use to make this case work now or do 
>>> I have to wait until [2] is addressed? If the latter, how will it work then?
>>> 
>> An upgraded moduledefined to the platform loader can link to types in 
>> modules defined to the app loader. So I wouldn't expect issues there.
>> 
>> Can you track down the code that uses ServiceLoader.load to load the 
>> LayoutFactory providers?
> 
> The code is in the first link of my previous message:
> 
> https://github.com/graalvm/graal-core/blob/1efc1c543acd7ed447c59788aeabc223be13e774/graal/org.graalvm.compiler.truffle/src/org/graalvm/compiler/truffle/GraalTruffleRuntime.java#L693
> 
>> It might be using loadInstalled or invoking it with the platform class 
>> loader and that would explain what you are seeing.
> 
> Thanks - using LayoutFactory.class.getClassLoader() works. So the general 
> rule is to use the leaf most class loader the service client knows about?

If that's the case, does that mean I could pick up any provider on the app 
class path? Seems a little risky. Is there some way to confine it to modules 
the service user trusts (by virtual of being a module in its dependency graph)?

-Doug

Re: Problem loading Truffle service providers in Graal

[Doug Simon]

> On 26 Feb 2017, at 13:08, Alan Bateman <alan.bate...@oracle.com> wrote:
> 
> On 26/02/2017 11:20, Doug Simon wrote:
> 
>> While trying to get upstream Graal working again with JDK 9, I'm having 
>> problems with service loading. Graal uses a LayoutFactory service defined by 
>> Truffle where the latter also includes a provider. The relevant parts of the 
>> module-info descriptors are:
>> 
>> module com.oracle.truffle.truffle_api {
>> 
>> exports com.oracle.truffle.object;
>> exports com.oracle.truffle.object.basic;
>> 
>> uses com.oracle.truffle.api.object.LayoutFactory;
>> provides com.oracle.truffle.api.object.LayoutFactory with 
>> com.oracle.truffle.object.basic.DefaultLayoutFactory;
>> 
>> }
>> 
>> module jdk.internal.vm.compiler {
>>  requires transitive com.oracle.truffle.truffle_api;
>> uses com.oracle.truffle.api.object.LayoutFactory;
>> 
>> }
>> 
>> However, looking up a provider in Graal[1] returns no providers. As far as I 
>> understand service loading with modules, this is  because 
>> jdk.internal.vm.compiler is loaded via the platform class loader while 
>> truffle is loaded via the app class loader as shown by the output of trace 
>> statements I added:
>> 
>> GraalTruffleRuntime.class.getClassLoader() -> 
>> jdk.internal.loader.ClassLoaders$PlatformClassLoader@366e2eef
>> LayoutFactory.class.getClassLoader() -> 
>> jdk.internal.loader.ClassLoaders$AppClassLoader@6df97b55
>> 
>> This ability of a platform loaded class to depend on an app loaded class is 
>> probably due to the soon-to-be-disabled mechanism[2] we use for overriding 
>> the version of Graal bundled with JDK 9.
>> 
>> Is there some command line magic I can use to make this case work now or do 
>> I have to wait until [2] is addressed? If the latter, how will it work then?
>> 
> An upgraded moduledefined to the platform loader can link to types in modules 
> defined to the app loader. So I wouldn't expect issues there.
> 
> Can you track down the code that uses ServiceLoader.load to load the 
> LayoutFactory providers?

The code is in the first link of my previous message:

https://github.com/graalvm/graal-core/blob/1efc1c543acd7ed447c59788aeabc223be13e774/graal/org.graalvm.compiler.truffle/src/org/graalvm/compiler/truffle/GraalTruffleRuntime.java#L693

> It might be using loadInstalled or invoking it with the platform class loader 
> and that would explain what you are seeing.

Thanks - using LayoutFactory.class.getClassLoader() works. So the general rule 
is to use the leaf most class loader the service client knows about?

-Doug

Problem loading Truffle service providers in Graal

[Doug Simon]

While trying to get upstream Graal working again with JDK 9, I'm having 
problems with service loading. Graal uses a LayoutFactory service defined by 
Truffle where the latter also includes a provider. The relevant parts of the 
module-info descriptors are:

module com.oracle.truffle.truffle_api {

exports com.oracle.truffle.object;
exports com.oracle.truffle.object.basic;

uses com.oracle.truffle.api.object.LayoutFactory;
provides com.oracle.truffle.api.object.LayoutFactory with 
com.oracle.truffle.object.basic.DefaultLayoutFactory;

}

module jdk.internal.vm.compiler {

requires transitive com.oracle.truffle.truffle_api;
uses com.oracle.truffle.api.object.LayoutFactory;

}

However, looking up a provider in Graal[1] returns no providers. As far as I 
understand service loading with modules, this is  because 
jdk.internal.vm.compiler is loaded via the platform class loader while truffle 
is loaded via the app class loader as shown by the output of trace statements I 
added:

GraalTruffleRuntime.class.getClassLoader() -> 
jdk.internal.loader.ClassLoaders$PlatformClassLoader@366e2eef
LayoutFactory.class.getClassLoader() -> 
jdk.internal.loader.ClassLoaders$AppClassLoader@6df97b55

This ability of a platform loaded class to depend on an app loaded class is 
probably due to the soon-to-be-disabled mechanism[2] we use for overriding the 
version of Graal bundled with JDK 9.

Is there some command line magic I can use to make this case work now or do I 
have to wait until [2] is addressed? If the latter, how will it work then?

-Doug

[1] 
https://github.com/graalvm/graal-core/blob/1efc1c543acd7ed447c59788aeabc223be13e774/graal/org.graalvm.compiler.truffle/src/org/graalvm/compiler/truffle/GraalTruffleRuntime.java#L693
[2] http://mail.openjdk.java.net/pipermail/graal-dev/2017-February/004889.html

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

[Doug Simon]

> On 19 Apr 2017, at 16:04, Alan Bateman <alan.bate...@oracle.com> 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

[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 <doug.si...@oracle.com> wrote:
> 
> I've had to update the webrev again to support selection of a "null" compiler 
> (i.e. one that raises an
> exception upon a compilation request) and added -Djvmci.Compiler=null to a 
> large number of JVMCI jtreg
> tests to prevent Graal being selected and initialized by the JVMCI compiler 
> auto-selection mechanism.
> Initializing Graal will currently fail with errors (see stack trace below) 
> until Graal is updated to
> the version compatible with the JVMCI API changes.
> 
> In addition to resolving the compatibility issue, explicitly selecting the 
> "null" compiler for these
> tests better isolates them from parts of the runtime they are not aiming to 
> test.
> 
> org.graalvm.compiler.debug.GraalError: java.lang.ClassCastException: 
> java.base/java.util.ImmutableCollections$MapN cannot be cast to 
> java.base/java.util.Properties
>   at 
> jdk.internal.vm.compiler/org.graalvm.compiler.hotspot.HotSpotGraalCompilerFactory.getSavedProperties(HotSpotGraalCompilerFactory.java:217)
>   at 
> jdk.internal.vm.compiler/org.graalvm.compiler.hotspot.HotSpotGraalCompilerFactory.initializeOptions(HotSpotGraalCompilerFactory.java:138)
>   at 
> jdk.internal.vm.compiler/org.graalvm.compiler.hotspot.HotSpotGraalCompilerFactory.onSelection(HotSpotGraalCompilerFactory.java:95)
>   at 
> jdk.internal.vm.ci/jdk.vm.ci.hotspot.HotSpotJVMCICompilerConfig.getCompilerFactory(HotSpotJVMCICompilerConfig.java:104)
>   at 
> jdk.internal.vm.ci/jdk.vm.ci.hotspot.HotSpotJVMCIRuntime.(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(

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 <doug.si...@oracle.com> 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 <doug.si...@oracle.com> wrote:
>> 
>>> 
>>> On 19 Apr 2017, at 21:40, Christian Thalinger <cthalin...@twitter.com> 
>>> wrote:
>>> 
>>>> 
>>>> On Apr 19, 2017, at 9:27 AM, Doug Simon <doug.si...@oracle.com> wrote:
>>>> 
>>>>> 
>>>>> On 19 Apr 2017, at 21:04, Mandy Chung <mandy.ch...@oracle.com> wrote:
>>>>> 
>>>>> 
>>>>

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

[Doug Simon]

> On 19 Apr 2017, at 22:29, Vladimir Kozlov <vladimir.koz...@oracle.com> 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 <vladimir.koz...@oracle.com> 
>>> 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 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

[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 <vladimir.koz...@oracle.com> 
> 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 <vladimir.koz...@oracle.com> 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

[Doug Simon]

> On 19 Apr 2017, at 21:40, Christian Thalinger <cthalin...@twitter.com> wrote:
> 
>> 
>> On Apr 19, 2017, at 9:27 AM, Doug Simon <doug.si...@oracle.com> wrote:
>> 
>>> 
>>> On 19 Apr 2017, at 21:04, Mandy Chung <mandy.ch...@oracle.com> wrote:
>>> 
>>> 
>>>> On Apr 19, 2017, at 11:55 AM, Christian Thalinger <cthalin...@twitter.com> 
>>>> wrote:
>>>> 
>>>> 
>>>>> On Apr 19, 2017, at 8:38 AM, Mandy Chung <mandy.ch...@oracle.com> wrote:
>>>>> 
>>>>> Since jdk.internal.vm.compiler becomes an upgradeable module, it is not 
>>>>> hashed with java.base to allow it to be upgraded and there is no 
>>>>> integrity check.  Such qualified export will be granted to any module 
>>>>> named jdk.internal.vm.compiler at runtime.  The goal is for upgradeable 
>>>>> modules not to use any internal APIs and eliminate the qualified exports.
>>>>> 
>>>>> The main thing is that jdk.vm.ci.services API would need to be guarded if 
>>>>> it’s used by non-Graal modules.
>>>> 
>>>> This all makes sense but where is the restriction that only 
>>>> jdk.internal.vm.compiler can use jdk.vm.ci.services?  
>>> 
>>> It's unqualified and no restriction in this change.
>> 
>> The public methods currently in jdk.vm.ci.services are:
>> 
>> 1. JVMCIServiceLocator.getProvider(Class)
>> 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 <doug.si...@oracle.com> wrote:
> 
>> 
>> On 19 Apr 2017, at 21:40, Christian Thalinger <cthalin...@twitter.com> wrote:
>> 
>>> 
>>> On Apr 19, 2017, at 9:27 AM, Doug Simon <doug.si...@oracle.com> wrote:
>>> 
>>>> 
>>>> On 19 Apr 2017, at 21:04, Mandy Chung <mandy.ch...@oracle.com> wrote:
>>>> 
>>>> 
>>>>> On Apr 19, 2017, at 11:55 AM, Christian Thalinger 
>>>>> <cthalin...@twitter.com> wrote:
>>>>> 
>>>>> 
>>>>>> On Apr 19, 2017, at 8:38 AM, Mandy Chung <mandy.ch...@oracle.com> wrote:
>>>>>> 
>>>>>> Since jdk.internal.vm.compiler becomes an upgradeable module, it is not 
>>>>>> hashed with java.base to allow it to be upgraded and there is no 
>>>>>> integrity check.  Such qualified export will be granted to any module 
>>>>>> named jdk.internal.vm.compiler at runtime.  The goal is for upgradeable 
>>>>>> modules not to use any internal APIs and eliminate the qualified exports.
>>>>>> 
>>>>>> The main thing is that jdk.vm.ci.services API would need to be guarded 
>>>>>> if it’s used by non-Graal modules.
>>>>> 
>>>>> This all makes sense but where is the restriction that only 
>>>>> jdk.internal.vm.compiler can use jdk.vm.ci.services?  
>>>> 
>>>> It's unqualified and no restriction in this change.
>>> 
>>> The public methods currently in jdk.vm.ci.services are:
>>> 
>>> 1. JVMCIServiceLocator.getProvider(Class)
>>> 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]

(adding hotspot-compiler-dev)

> On 19 Apr 2017, at 02:02, Mandy Chung <mandy.ch...@oracle.com> wrote:
> 
> 
>> On Apr 18, 2017, at 3:13 PM, Doug Simon <doug.si...@oracle.com> 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: javac unzips class files from jars on class path into output directory

[Doug Simon]

Hi Jon,

I've attached bug.zip which should reproduce the issue (assuming jdk 9 javac is 
on your path):

unzip bug.zip
cd bug
./run.sh
find bin

The last command above should show the extra class files from 
jdk.internal.vm.ci.jar in bin.

-Doug



> On 7 Mar 2017, at 18:55, Jonathan Gibbons <jonathan.gibb...@oracle.com> wrote:
> 
> 
> 
> On 03/07/2017 08:06 AM, Doug Simon wrote:
>> To be able to develop Graal on JDK 9, we're using the `--release 8` javac 
>> option and providing jar files for API that is either not in 9 or is not 
>> exported in 9. Here is a simplified form of a javac command:
>> 
>> javac -cp jdk.internal.vm.ci.jar:jdk.unsupported_sun.misc.Unsafe.jar -d bin/ 
>> --release 8 
>> graal/org.graalvm.compiler.api.runtime/src/org/graalvm/compiler/api/runtime/*.java
>> 
>> where:
>> 
>> dsimon@kurz-3 ~/h/graal-core> ls 
>> graal/org.graalvm.compiler.api.runtime/src/org/graalvm/compiler/api/runtime/*.java
>> graal/org.graalvm.compiler.api.runtime/src/org/graalvm/compiler/api/runtime/GraalJVMCICompiler.java
>> graal/org.graalvm.compiler.api.runtime/src/org/graalvm/compiler/api/runtime/GraalRuntime.java
>> 
>> I expect 2 class files to be written to bin/. However, I see a number of 
>> files from jdk.internal.vm.ci.jar in bin:
>> 
>> dsimon@kurz-3 ~/h/graal-core> jar tf jdk.internal.vm.ci.jar | wc -l
>>  444
>> dsimon@kurz-3 ~/h/graal-core> find bin/jdk/vm/ci | wc -l
>>   55
>> 
>> I'm guessing that these are the classes in jdk.internal.vm.ci.jar referenced 
>> (transitively?) from the Graal sources.
>> 
>> Why is this happening? That is, why is javac extracting classes from a jar 
>> on the classpath and putting them in the output directory?
>> 
>> -Doug
> 
> Doug,
> 
> Can you provide a more complete test case?
> 
> -- Jon

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
   

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

[Doug Simon]

> On 19 Apr 2017, at 02:02, Mandy Chung <mandy.ch...@oracle.com> wrote:
> 
> 
>> On Apr 18, 2017, at 3:13 PM, Doug Simon <doug.si...@oracle.com> 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]

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

[Doug Simon]

> On 21 Apr 2017, at 13:46, Doug Simon <doug.si...@oracle.com> wrote:
> 
> There has been some discussion about whether we want to update Graal in the 
> JDK at this late stage. The main (only?) risk is a regression in the AOT tool.
> 
> If we don't update Graal from upstream, then the qualified exports from JVMCI 
> to jdk.internal.vm.compiler cannot be removed in JDK 9. Note that in addition 
> to updating Graal to remove the qualified exports, there would also need to 
> be changes in the relevant make files to add --add-exports options when 
> compiling Graal and jaotc as they use the dynamically exported JVMCI packages.
> 
> I have an updated hotspot patch that adapts Graal to the JVMCI API changes:
> 
> http://cr.openjdk.java.net/~dnsimon/8177845/hotspot.02/
> 
> Note that this patch does not include the changes removing use of JDK 
> internal API from Graal. Cherry picking those upstream Graal changes would be 
> more work than simply doing a complete update from upstream Graal.
> 
> As I see it, there are 2 options here:
> 
> 1. Go with the current webrev (including hotspot.02 patch) and live with the 
> qualified exports.
> 2. Go with the current webrev (including hotspot.02 patch) and create a 
> follow up bug to update Graal from upstream, perform the relevant make file 
> changes and remove the qualified exports.

I made a new webrev[1] that implements option 1.5 ;-) The changes added since 
the first webrev[2] are:

- Cherry picked changes from upstream Graal that remove use of JDK internals.

- The jdk.internal.vm.ci.enabled system property is set to true in 
arguments.cpp[3] iff EnableJVMCI is true
  and this property is checked in all the public methods in jdk.vm.ci.services.

- The jdk.vm.ci.services package is (once again) only exported to 
jdk.internal.vm.compiler based on
  advice from the jigsaw team:

  "We reviewed the unqualified export jdk.vm.ci.services from 
jdk.internal.vm.ci module. This brings
   jdk.internal.vm.ci to be resolved by default that of course may resolve 
additional modules that
   provides the services that JVMCI uses.  In addition.  JVMCI is meant to be 
used (only) when
   -XX:+EnableJVMCI is specified but now it’s defined by default.

   An internal module should only have qualified exports as a design principle. 
 The Lab Graal will
   have the same module name, jdk.internal.vm.compiler.  The advise is to keep 
it as qualified export
   `exports jdk.vm.ci.services to jdk.internal.vm.compiler` and remove all 
other qualified exports as
we discussed."

- The jaotc launcher now needs to explicitly export JVMCI and 
jdk.internal.vm.compiler to jdk.aot[4].
  Unfortunately there needs to be one --add-exports option per qualified export 
target as combining
  them with a comma (e.g., 
--add-exports=jdk.internal.vm.ci/jdk.vm.ci.code=jdk.internal.vm.compiler,jdk.aot)
  breaks the make process:

Launcher-jdk.aot.gmk:31: *** missing separator.  Stop.
make/Main.gmk:232: recipe for target 'jdk.aot-launchers' failed.

The latest webrev has been tested against upstream Graal, the closed AOT tests 
and jprt.

-Doug

[1] http://cr.openjdk.java.net/~dnsimon/8177845.02
[2] http://cr.openjdk.java.net/~dnsimon/8177845
[3] 
http://cr.openjdk.java.net/~dnsimon/8177845.02/hotspot/src/share/vm/runtime/arguments.cpp.udiff.html
[4] 
http://cr.openjdk.java.net/~dnsimon/8177845.02/jdk/make/launcher/Launcher-jdk.aot.gmk.udiff.html

> 
>> On 20 Apr 2017, at 20:50, Doug Simon <doug.si...@oracle.com> wrote:
>> 
>> I've had to update the webrev again to support selection of a "null" 
>> compiler (i.e. one that raises an
>> exception upon a compilation request) and added -Djvmci.Compiler=null to a 
>> large number of JVMCI jtreg
>> tests to prevent Graal being selected and initialized by the JVMCI compiler 
>> auto-selection mechanism.
>> Initializing Graal will currently fail with errors (see stack trace below) 
>> until Graal is updated to
>> the version compatible with the JVMCI API changes.
>> 
>> In addition to resolving the compatibility issue, explicitly selecting the 
>> "null" compiler for these
>> tests better isolates them from parts of the runtime they are not aiming to 
>> test.
>> 
>> org.graalvm.compiler.debug.GraalError: java.lang.ClassCastException: 
>> java.base/java.util.ImmutableCollections$MapN cannot be cast to 
>> java.base/java.util.Properties
>>  at 
>> jdk.internal.vm.compiler/org.graalvm.compiler.hotspot.HotSpotGraalCompilerFactory.getSavedProperties(HotSpotGraalCompilerFactory.java:217)
>>  at 
>> jdk.internal.vm.compiler/org.graalvm.compiler.hotspot.HotSpotGraalCompilerFactory.initializeOptions(HotSpotGraalCompilerFactory.java:138)
>>  at 
>> jdk.internal.vm.comp

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

[Doug Simon]

> On 27 Apr 2017, at 17:40, Alan Bateman <alan.bate...@oracle.com> 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

[Doug Simon]

> On 27 Apr 2017, at 21:15, Vladimir Kozlov <vladimir.koz...@oracle.com> wrote:
> 
> make/CompileJavaModules.gmk Please, adjust comment for jdk.aot:
> 
> ># Don't use Indy strings concatenation to have good JVMCI startup 
> >performance.
> ---
> <# Don't use Indy strings concatenation to have good JAOTC startup 
> performance.
> 
> Should we also add jdk.vm.ci.aarch64 and jdk.vm.ci.sparc exports for AOT? It 
> is not needed for JDK 9 but we will support them in a future and analyze 
> build failures is painful.
> 
> Same in make/launcher/Launcher-jdk.aot.gmk.

Good point. I made it the same set of exports as for jdk.internal.vm.compiler 
(in both locations[1][2]).

> I don't see changes in hotspot/make/CompileTools.gmk and 
> hotspot/make/gensrc/Gensrc-jdk.internal.vm.compiler.gmk (which do annotation 
> processor build):
> 
> http://hg.openjdk.java.net/jdk9/dev/hotspot/file/f1cca489e9c6/make/gensrc/Gensrc-jdk.internal.vm.compiler.gmk
> 
> I assume you don't need to change anything there. Right?

I presume not as well. These sources are compiled as Java 8 sources where 
modules are not a concept.

> Overall changes looks good to me.

Thanks for the review! I'll run the Graal and AOT tests once more and then 
integrate this change.

-Doug

[1] 
http://cr.openjdk.java.net/~dnsimon/8177845.02/jdk/make/launcher/Launcher-jdk.aot.gmk.udiff.html
[2] 
http://cr.openjdk.java.net/~dnsimon/8177845.02/make/make/CompileJavaModules.gmk.udiff.html

> On 4/27/17 7:47 AM, Doug Simon wrote:
>> 
>>> On 21 Apr 2017, at 13:46, Doug Simon <doug.si...@oracle.com> wrote:
>>> 
>>> There has been some discussion about whether we want to update Graal in the 
>>> JDK at this late stage. The main (only?) risk is a regression in the AOT 
>>> tool.
>>> 
>>> If we don't update Graal from upstream, then the qualified exports from 
>>> JVMCI to jdk.internal.vm.compiler cannot be removed in JDK 9. Note that in 
>>> addition to updating Graal to remove the qualified exports, there would 
>>> also need to be changes in the relevant make files to add --add-exports 
>>> options when compiling Graal and jaotc as they use the dynamically exported 
>>> JVMCI packages.
>>> 
>>> I have an updated hotspot patch that adapts Graal to the JVMCI API changes:
>>> 
>>> http://cr.openjdk.java.net/~dnsimon/8177845/hotspot.02/
>>> 
>>> Note that this patch does not include the changes removing use of JDK 
>>> internal API from Graal. Cherry picking those upstream Graal changes would 
>>> be more work than simply doing a complete update from upstream Graal.
>>> 
>>> As I see it, there are 2 options here:
>>> 
>>> 1. Go with the current webrev (including hotspot.02 patch) and live with 
>>> the qualified exports.
>>> 2. Go with the current webrev (including hotspot.02 patch) and create a 
>>> follow up bug to update Graal from upstream, perform the relevant make file 
>>> changes and remove the qualified exports.
>> 
>> I made a new webrev[1] that implements option 1.5 ;-) The changes added 
>> since the first webrev[2] are:
>> 
>> - Cherry picked changes from upstream Graal that remove use of JDK internals.
>> 
>> - The jdk.internal.vm.ci.enabled system property is set to true in 
>> arguments.cpp[3] iff EnableJVMCI is true
>>  and this property is checked in all the public methods in 
>> jdk.vm.ci.services.
>> 
>> - The jdk.vm.ci.services package is (once again) only exported to 
>> jdk.internal.vm.compiler based on
>>  advice from the jigsaw team:
>> 
>>  "We reviewed the unqualified export jdk.vm.ci.services from 
>> jdk.internal.vm.ci module. This brings
>>   jdk.internal.vm.ci to be resolved by default that of course may resolve 
>> additional modules that
>>   provides the services that JVMCI uses.  In addition.  JVMCI is meant to be 
>> used (only) when
>>   -XX:+EnableJVMCI is specified but now it’s defined by default.
>> 
>>   An internal module should only have qualified exports as a design 
>> principle.  The Lab Graal will
>>   have the same module name, jdk.internal.vm.compiler.  The advise is to 
>> keep it as qualified export
>>   `exports jdk.vm.ci.services to jdk.internal.vm.compiler` and remove all 
>> other qualified exports as
>>we discussed."
>> 
>> - The jaotc launcher now needs to explicitly export JVMCI and 
>> jdk.internal.vm.compiler to jdk.aot[4].
>>  Unfortunately there needs to be one --add-exports option per qualified 
>> export target as combining
>> 

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

[Doug Simon]

> On 27 Apr 2017, at 18:44, Christian Thalinger <cthalin...@twitter.com> wrote:
> 
> Thanks for doing this.  I have a hard time following what’s happening :-)
> 
> src/jdk.internal.vm.compiler/.mx.graal/suite.py
> 
> +  "jdklibraries" : {
> +"JVMCI_SERVICES" : {
> +  "path" : "lib/jvmci-services.jar",
> +  "sourcePath" : "lib/jvmci-services.src.zip",
> +  "optional" : False,
> +  "jdkStandardizedSince" : "9",
> +  "module" : "jdk.internal.vm.ci"
> +},
> +"JVMCI_API" : {
> +  "path" : "lib/jvmci/jvmci-api.jar",
> +  "sourcePath" : "lib/jvmci/jvmci-api.src.zip",
> +  "dependencies" : [
> +"JVMCI_SERVICES",
> +  ],
> +  "optional" : False,
> +  "jdkStandardizedSince" : "9",
> +  "module" : "jdk.internal.vm.ci"
> +},
> +"JVMCI_HOTSPOT" : {
> +  "path" : "lib/jvmci/jvmci-hotspot.jar",
> +  "sourcePath" : "lib/jvmci/jvmci-hotspot.src.zip",
> +  "dependencies" : [
> +"JVMCI_API",
> +  ],
> +  "optional" : False,
> +  "jdkStandardizedSince" : "9",
> +  "module" : "jdk.internal.vm.ci"
> +},
> +  },
> 
> Can you explain to me why we need this?  I don’t think these files are built 
> in 9.

This simply allows `mx eclipseinit` to work when wanting to edit the JDK Graal 
sources in Eclipse. As you state, this is completely by-passed by the JDK make 
system.

> 
> src/jdk.internal.vm.ci/share/classes/module-info.java
> 
> Why can we remove the exports to jdk.internal.vm.compiler?  Because of this 
> in JVMCIServiceLocator:
> 
> +    ReflectionAccessJDK.openJVMCITo(getClass());

Yes. And the --add-exports in CompileJavaModules.gmk[1] and 
Launcher-jdk.aot.gmk[2]. It also means these VM options are required in 
upstream Graal when running the Graal junit tests.

-Doug

[1] 
http://cr.openjdk.java.net/~dnsimon/8177845.02/make/make/CompileJavaModules.gmk.udiff.html
[2] 
http://cr.openjdk.java.net/~dnsimon/8177845.02/jdk/make/launcher/Launcher-jdk.aot.gmk.udiff.html

> 
> ?
> 
>> On Apr 27, 2017, at 7:47 AM, Doug Simon <doug.si...@oracle.com> wrote:
>> 
>>> 
>>> On 21 Apr 2017, at 13:46, Doug Simon <doug.si...@oracle.com> wrote:
>>> 
>>> There has been some discussion about whether we want to update Graal in the 
>>> JDK at this late stage. The main (only?) risk is a regression in the AOT 
>>> tool.
>>> 
>>> If we don't update Graal from upstream, then the qualified exports from 
>>> JVMCI to jdk.internal.vm.compiler cannot be removed in JDK 9. Note that in 
>>> addition to updating Graal to remove the qualified exports, there would 
>>> also need to be changes in the relevant make files to add --add-exports 
>>> options when compiling Graal and jaotc as they use the dynamically exported 
>>> JVMCI packages.
>>> 
>>> I have an updated hotspot patch that adapts Graal to the JVMCI API changes:
>>> 
>>> http://cr.openjdk.java.net/~dnsimon/8177845/hotspot.02/
>>> 
>>> Note that this patch does not include the changes removing use of JDK 
>>> internal API from Graal. Cherry picking those upstream Graal changes would 
>>> be more work than simply doing a complete update from upstream Graal.
>>> 
>>> As I see it, there are 2 options here:
>>> 
>>> 1. Go with the current webrev (including hotspot.02 patch) and live with 
>>> the qualified exports.
>>> 2. Go with the current webrev (including hotspot.02 patch) and create a 
>>> follow up bug to update Graal from upstream, perform the relevant make file 
>>> changes and remove the qualified exports.
>> 
>> I made a new webrev[1] that implements option 1.5 ;-) The changes added 
>> since the first webrev[2] are:
>> 
>> - Cherry picked changes from upstream Graal that remove use of JDK internals.
>> 
>> - The jdk.internal.vm.ci.enabled system property is set to true in 
>> arguments.cpp[3] iff EnableJVMCI is true
>>  and this property is checked in all the public methods in 
>> jdk.vm.ci.services.
>> 
>> - The jdk.vm.ci.services package is (once again) only exported to 
>> jdk.internal.vm.compiler based on
>>  advice from the jigsaw team:
>> 
>>  "We reviewed the unqualified export jdk.vm.

RFR: 8179434: test/java/lang/Class/getDeclaredField/FieldSetAccessibleTest.java fails due to JDK-8177845

[Doug Simon]

Please review this small fix for a regression introduced by the change for 
https://bugs.openjdk.java.net/browse/JDK-8177845.

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

-Doug

Re: RFR: 8179434: test/java/lang/Class/getDeclaredField/FieldSetAccessibleTest.java fails due to JDK-8177845

[Doug Simon]

> On 28 Apr 2017, at 21:11, Mandy Chung <mandy.ch...@oracle.com> wrote:
> 
> Looks good.

Thanks for the review.

-Doug

>> On Apr 28, 2017, at 12:05 PM, Doug Simon <doug.si...@oracle.com> wrote:
>> 
>> Please review this small fix for a regression introduced by the change for 
>> https://bugs.openjdk.java.net/browse/JDK-8177845.
>> 
>> http://cr.openjdk.java.net/~dnsimon/8179434/
>> https://bugs.openjdk.java.net/browse/JDK-8179434
>> 
>> -Doug
> 

RFR(XS): 8205025: [AOT] make jdk.aot module upgradeable

[Doug Simon]

In the context of JDK-8202762, we to need to make the jdk.aot module 
upgradeable. Otherwise, it is impossible to run or test the version of jdk.aot 
under development in a Graal repo: 

java 
--module-path=../sdk/mxbuild/modules/org.graalvm.graal_sdk.jar:../truffle/mxbuild/modules/com.oracle.truffle.truffle_api.jar:mxbuild/modules/jdk.internal.vm.compiler.jar
 
--upgrade-module-path=mxbuild/modules/jdk.internal.vm.compiler.jar:mxbuild/modules/jdk.internal.vm.compiler.management.jar:mxbuild/modules/jdk.aot.jar
 -m jdk.aot/jdk.tools.jaotc.Main 
Error occurred during initialization of boot layer 
java.lang.module.FindException: Hash of jdk.aot 
(55cfefcfb0ca2a8b12403c47848d2bbd54416149cfe75f5051ad77628a2764b4) differs to 
expected hash 
(e6882d3461a21ea46c52da87ef52b5850a7b1f5ae0cfd650b7f784c970aaa0ee) recorded in 
java.base 

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

-Doug

Re: RFR(XS): 8205025: [AOT] make jdk.aot module upgradeable

[Doug Simon]

> On 14 Jun 2018, at 12:03, Alan Bateman  wrote:
> 
> 
> 
> On 14/06/2018 09:09, Doug Simon wrote:
>> In the context of JDK-8202762, we to need to make the jdk.aot module 
>> upgradeable. Otherwise, it is impossible to run or test the version of 
>> jdk.aot under development in a Graal repo:
>> 
>> java 
>> --module-path=../sdk/mxbuild/modules/org.graalvm.graal_sdk.jar:../truffle/mxbuild/modules/com.oracle.truffle.truffle_api.jar:mxbuild/modules/jdk.internal.vm.compiler.jar
>>  
>> --upgrade-module-path=mxbuild/modules/jdk.internal.vm.compiler.jar:mxbuild/modules/jdk.internal.vm.compiler.management.jar:mxbuild/modules/jdk.aot.jar
>>  -m jdk.aot/jdk.tools.jaotc.Main
>> Error occurred during initialization of boot layer
>> java.lang.module.FindException: Hash of jdk.aot 
>> (55cfefcfb0ca2a8b12403c47848d2bbd54416149cfe75f5051ad77628a2764b4) differs 
>> to expected hash 
>> (e6882d3461a21ea46c52da87ef52b5850a7b1f5ae0cfd650b7f784c970aaa0ee) recorded 
>> in java.base
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8205025
>> http://cr.openjdk.java.net/~dnsimon/8205025/
>> 
> This looks okay except that there has been an attempt to keep the list of 
> modules in alphabetic order in these make files.
> 
> Do all tests pass with this change? There is at least one test in 
> test/jdk/jdk/modules/etc that is sensitive to the list of upgradeable modules.

I changed the test (thanks Mandy for pointing out exactly which one), fixed the 
alphabetic ordering of the list and am running tests now.

-Doug

RFR: 8187490: HotSpotRuntimeMBean should be moved to Graal management module

[Doug Simon]

Please review this change that removes the existing Graal service provider for 
hooking into the Platform MBean Server and makes 
jdk.internal.vm.compiler.management an upgradeable module.

Please refer to 
https://bugs.openjdk.java.net/browse/JDK-8187490?focusedCommentId=14170942=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14170942
 for discussion on the latter point.

The Graal changes that dynamically register an MBean for accessing Graal will 
be part of a subsequent Graal update.

http://cr.openjdk.java.net/~dnsimon/8187490/

-Doug

Re: RFR: 8187490: HotSpotRuntimeMBean should be moved to Graal management module

[Doug Simon]

> On 13 Apr 2018, at 07:15, David Holmes <david.hol...@oracle.com> wrote:
> 
> Hi Doug,
> 
> Not a review. :) Just wondering what HotSpotRuntimeMBean has to do with this 
> ???

These are the non-Graal code base changes needed to move the bean out of the 
jdk.internal.vm.compiler module. The rest of the changes will come in the next 
Graal update. If you'd like, I can defer pushing these changes until the Graal 
changes land on github so that the complete change can be reviewed.

-Doug

> On 13/04/2018 4:24 AM, Doug Simon wrote:
>> Please review this change that removes the existing Graal service provider 
>> for hooking into the Platform MBean Server and makes 
>> jdk.internal.vm.compiler.management an upgradeable module.
>> Please refer to 
>> https://bugs.openjdk.java.net/browse/JDK-8187490?focusedCommentId=14170942=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14170942
>>  for discussion on the latter point.
>> The Graal changes that dynamically register an MBean for accessing Graal 
>> will be part of a subsequent Graal update.
>> http://cr.openjdk.java.net/~dnsimon/8187490/
>> -Doug

Re: Provides clauses in binary module descriptor but not in source

[Doug Simon]

> On 13 Apr 2018, at 14:33, Alan Bateman <alan.bate...@oracle.com> wrote:
> 
> On 13/04/2018 13:16, Doug Simon wrote:
>> I just noticed that in the jdk.internal.vm.compiler module descriptor source 
>> there is a `uses` clause for CompilerConfigurationFactory[1] but no 
>> `provides` clause for the CoreCompilerConfigurationFactory provider[2] which 
>> is in the same module. However, `java -d jdk.internal.vm.compiler | grep 
>> Core` shows me the provider clause exists in the binary module descriptor. 
>> Is this done auto-magically by javac when building the module? If not, is it 
>> in the make files somewhere? I'm asking because there are new service 
>> providers being added in Graal.
>> 
> The build for that module is complex as it runs an annotation processor and 
> generates a module-info.java.extra (see 
> support/gensrc/jdk.internal.vm.compiler/ in the build output) that is merged 
> with the module-info.java before it is compiled. So no javac magic.

Ah yes, I'd forgotten that the @ServiceProvider annotation was run my the JDK 
make system. Thanks for the helpful reminder; it means we can simply use this 
annotation for the new service providers we're adding and everything should 
Just Work.

-Doug

Provides clauses in binary module descriptor but not in source

[Doug Simon]

I just noticed that in the jdk.internal.vm.compiler module descriptor source 
there is a `uses` clause for CompilerConfigurationFactory[1] but no `provides` 
clause for the CoreCompilerConfigurationFactory provider[2] which is in the 
same module. However, `java -d jdk.internal.vm.compiler | grep Core` shows me 
the provider clause exists in the binary module descriptor. Is this done 
auto-magically by javac when building the module? If not, is it in the make 
files somewhere? I'm asking because there are new service providers being added 
in Graal.

-Doug

[1] 
http://hg.openjdk.java.net/jdk/jdk/file/2918e1146106/src/jdk.internal.vm.compiler/share/classes/module-info.java#l36
[2] 
http://hg.openjdk.java.net/jdk/jdk/file/2918e1146106/src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/CoreCompilerConfigurationFactory.java#l30

Re: RFR: 8187490: HotSpotRuntimeMBean should be moved to Graal management module

[Doug Simon]

> On 13 Apr 2018, at 15:59, David Holmes <david.hol...@oracle.com> wrote:
> 
> On 13/04/2018 5:12 PM, Doug Simon wrote:
>>> On 13 Apr 2018, at 07:15, David Holmes <david.hol...@oracle.com> wrote:
>>> 
>>> Hi Doug,
>>> 
>>> Not a review. :) Just wondering what HotSpotRuntimeMBean has to do with 
>>> this ???
>> These are the non-Graal code base changes needed to move the bean out of the 
>> jdk.internal.vm.compiler module. The rest of the changes will come in the 
>> next Graal update. If you'd like, I can defer pushing these changes until 
>> the Graal changes land on github so that the complete change can be reviewed.
> 
> So we seem to have both HotSpotRuntimeMBean and HotspotRuntimeMBean (note 
> small 's') defined in the source code! That seems to be a bad thing to me! I 
> was wondering what this had to do with the small 's' HotspotRuntimeMBean - 
> and the answer seems to be "nothing"!

There is actually no HotSpotRuntimeMBean.java source - it's 
HotSpotGraalMBean.java. I've renamed the issue and fixed the naming in the 
description.

-Doug

>> -Doug
>>> On 13/04/2018 4:24 AM, Doug Simon wrote:
>>>> Please review this change that removes the existing Graal service provider 
>>>> for hooking into the Platform MBean Server and makes 
>>>> jdk.internal.vm.compiler.management an upgradeable module.
>>>> Please refer to 
>>>> https://bugs.openjdk.java.net/browse/JDK-8187490?focusedCommentId=14170942=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14170942
>>>>  for discussion on the latter point.
>>>> The Graal changes that dynamically register an MBean for accessing Graal 
>>>> will be part of a subsequent Graal update.
>>>> http://cr.openjdk.java.net/~dnsimon/8187490/
>>>> -Doug

Re: RFR: 8201794: [Graal] fix regressions from JDK-8187490

[Doug Simon]

I've updated the webrev with a fix for another regression caused by JDK-8187490:

--- old/test/jdk/jdk/modules/etc/UpgradeableModules.java2018-04-18 
12:57:32.0 +0200
+++ new/test/jdk/jdk/modules/etc/UpgradeableModules.java2018-04-18 
12:57:32.0 +0200
@@ -46,6 +46,7 @@
 List.of("java.compiler",
 "java.jnlp",
 "jdk.internal.vm.compiler",
+"jdk.internal.vm.compiler.management",
 "jdk.deploy",
 "jdk.javaws",
     "jdk.plugin",

-Doug

> On 18 Apr 2018, at 12:35, Doug Simon <doug.si...@oracle.com> wrote:
> 
> Right after pushing the change for JDK-8187490, I noticed that the mach5 
> build had 2 CTW test failures due to jdk.internal.vm.compiler.management now 
> being an empty module. This will be fixed in the next Graal update but the 
> failing tests should be fixed in the meantime.
> 
> http://cr.openjdk.java.net/~dnsimon/8201794/
> https://bugs.openjdk.java.net/browse/JDK-8201794
> 
> I'm running mach5 now to ensure the regressions are fixed and no other 
> regressions occur.
> 
> -Doug

Re: RFR: 8201794: [Graal] fix regressions from JDK-8187490

[Doug Simon]

> On 18 Apr 2018, at 13:23, mandy chung <mandy.ch...@oracle.com> wrote:
> 
> Looks okay.  Have you run all jdk_core tests in addition to other hotspot 
> tests.

I ran all open/test/jdk/jdk tests - is that what you mean?

-Doug

> On 4/18/18 7:00 PM, Doug Simon wrote:
>> I've updated the webrev with a fix for another regression caused by 
>> JDK-8187490:
>> 
>> --- old/test/jdk/jdk/modules/etc/UpgradeableModules.java 2018-04-18 
>> 12:57:32.0 +0200
>> +++ new/test/jdk/jdk/modules/etc/UpgradeableModules.java 2018-04-18 
>> 12:57:32.0 +0200
>> @@ -46,6 +46,7 @@
>>  List.of("java.compiler",
>>  "java.jnlp",
>>  "jdk.internal.vm.compiler",
>> +"jdk.internal.vm.compiler.management",
>>  "jdk.deploy",
>>  "jdk.javaws",
>>  "jdk.plugin",
>> 
>> -Doug
>> 
>> 
>>> On 18 Apr 2018, at 12:35, Doug Simon <doug.si...@oracle.com>
>>>  wrote:
>>> 
>>> Right after pushing the change for JDK-8187490, I noticed that the mach5 
>>> build had 2 CTW test failures due to jdk.internal.vm.compiler.management 
>>> now being an empty module. This will be fixed in the next Graal update but 
>>> the failing tests should be fixed in the meantime.
>>> 
>>> 
>>> http://cr.openjdk.java.net/~dnsimon/8201794/
>>> https://bugs.openjdk.java.net/browse/JDK-8201794
>>> 
>>> 
>>> I'm running mach5 now to ensure the regressions are fixed and no other 
>>> regressions occur.
>>> 
>>> -Doug
>>> 
> 

RFR: 8201794: [Graal] add dummy HotSpotGraalManagement class

[Doug Simon]

Right after pushing the change for JDK-8187490, I noticed that the mach5 build 
had 2 CTW test failures due to jdk.internal.vm.compiler.management now being an 
empty module. This will be fixed in the next Graal update but the failing tests 
should be fixed in the meantime.

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

I'm running mach5 now to ensure the regressions are fixed and no other 
regressions occur.

-Doug

Re: BackEnd Service Provider.

[Doug Simon]

Thanks for your valuable input Alan.

So until the VM provides an --add-provides option (which is not currently 
planned), --patch-module is the only mechanism for dynamically adding providers 
of otherwise “sealed” Graal services. We could indeed make Graal service 
loading look for providers on the class path but that would have to be off by 
default and thus require a command line option (e.g. 
-Dgraal.AllowServicesOnClassPath=true). However, that doesn’t seem like any 
less effort than having to use --patch-module.

-Doug

> On 22 Apr 2020, at 09:12, Alan Bateman  wrote:
> 
> On 21/04/2020 14:01, Doug Simon wrote:
>> Hi Gary,
>> 
>> I cannot understand why --add-reads does not work and have reached out to 
>> Alan Bateman for more insight.
>> 
> I read through the thread [1] and I don't see an obvious solution to this.
> 
> The main issue here is that org.grfstuff.compiler wants to provide an 
> implementation of org.graalvm.compiler.hotspot.HotSpotBackendFactory but that 
> service type is not in org.grfstuff.compiler and org.graalvm.compiler.hotspot 
> is not exported to org.grfstuff.compiler by any of the modules that it reads. 
> The post resolution consistency check that it trips up on is specified in the 
> Configuration API docs if you are interested.
> 
> I assume `--add-exports 
> jdk.internal.vm.compiler/org.graalvm.compiler.hotspot=org.grfstuff.compiler` 
> was added in an attempt to workaround this but that CLI option is the 
> equivalent of jdk.internal.vm.compiler invoking Module::addExports after the 
> module graph has been reified. It doesn't impact resolution or any of the 
> post resolution consistency checks at run-time. Yes, there is an unfortunate 
> difference between between compile-time and run-time in this regard but that 
> is something for another discussion.
> 
> I don't think there are any obvious workarounds. The Module API defines 
> addXXX methods to export or open packages at run-time, extend readability or 
> add a service dependences but it doesn't define addProvides. I think it came 
> up once during JDK 9 but more in the context of symmetry rather than a 
> compelling use-case. It's not clear that the issue under discussion here is 
> compelling enough, needs more thought as the scenario is a bit weird. If 
> org.grfstuff.compiler was co developed with jdk.internal.vm.compiler then it 
> could use a qualified export of course.
> 
> Initially I thought you could workaround this by moving org.grfstuff.compiler 
>  to the class path but this it's not going to work as ServiceLodaer usage in 
> Graal seems to use module layers as congtext, so it will not find 
> implementations on the class path.
> 
> -Alan
> 
> [1] https://mail.openjdk.java.net/pipermail/graal-dev/2020-April/005949.html