Re: RFR: 8177845: Need a mechanism to load Graal
> On 19 Apr 2017, at 02:02, Mandy Chung wrote:
>
>
>> On Apr 18, 2017, at 3:13 PM, Doug Simon wrote:
>>
>> Please review these changes that make jdk.internal.vm.compiler an upgradable
>> compiler.
>> :
>> http://cr.openjdk.java.net/~dnsimon/8177845/
>
> Thanks for making this change. This would simplify the way to replace JDK’s
> graal with the lab graal. A couple of comments:
>
> jdk/internal/misc/VM.java
>
> 161 * Note that the saved system properties do not include
> 162 * the ones set by sun.misc.Version.init().
>
> sun.misc.Version is no longer present in JDK 9. Renamed to
> java.lang.VersionProps
Sorry, the webrev was out of date. I've updated it and also fixed the comment
for VM::getSavedProperty.
> jdk/vm/ci/services/Services.java
> 67 Class.forName("jdk.vm.ci.runtime.JVMCI”);
>
> JVMCI class is local in jdk.internal.vm.ci module. An alternative may be
> to provide a static initialize method rather than Class::forName.
JVMCI is broken into fine grained "projects" and the jdk.vm.ci.runtime[1]
project depends on the jdk.vm.ci.services project[2] so I cannot make a direct
reference without introducing a circular dependency. I don't expect the
reflection cost to be significant in practice.
> jdk/vm/ci/hotspot/HotSpotJVMCIRuntime.java
> 211 for (HotSpotJVMCIBackendFactory factory :
> ServiceLoader.load(HotSpotJVMCIBackendFactory.class)) {
>
> This uses the thread context class loader to load providers. Services::load
> uses
> the system class loader to load providers. I suspect you want this to use
> the
> system class loader here too.
>
> jdk/vm/ci/services/JVMCIServiceLocator.java
> 78 for (JVMCIServiceLocator access :
> ServiceLoader.load(JVMCIServiceLocator.class)) {
>
> Same comment as above. I think you want to use system class loader to load
> providers.
Yes, I think you're right. I've updated the webrev with that change.
Thanks for the review.
-Doug
[1]
http://hg.openjdk.java.net/jdk9/dev/hotspot/file/560d7aa083a2/.mx.jvmci/suite.py#l94
[2]
http://hg.openjdk.java.net/jdk9/dev/hotspot/file/560d7aa083a2/.mx.jvmci/suite.py#l45
RFR: 8177845: Need a mechanism to load Graal
(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
(adding hotspot-compiler-dev)
> On 19 Apr 2017, at 02:02, Mandy Chung wrote:
>
>
>> On Apr 18, 2017, at 3:13 PM, Doug Simon wrote:
>>
>> Please review these changes that make jdk.internal.vm.compiler an upgradable
>> compiler.
>> :
>> http://cr.openjdk.java.net/~dnsimon/8177845/
>
> Thanks for making this change. This would simplify the way to replace JDK’s
> graal with the lab graal. A couple of comments:
>
> jdk/internal/misc/VM.java
>
> 161 * Note that the saved system properties do not include
> 162 * the ones set by sun.misc.Version.init().
>
> sun.misc.Version is no longer present in JDK 9. Renamed to
> java.lang.VersionProps
Sorry, the webrev was out of date. I've updated it and also fixed the comment
for VM::getSavedProperty.
> jdk/vm/ci/services/Services.java
> 67 Class.forName("jdk.vm.ci.runtime.JVMCI”);
>
> JVMCI class is local in jdk.internal.vm.ci module. An alternative may be
> to provide a static initialize method rather than Class::forName.
JVMCI is broken into fine grained "projects" and the jdk.vm.ci.runtime[1]
project depends on the jdk.vm.ci.services project[2] so I cannot make a direct
reference without introducing a circular dependency. I don't expect the
reflection cost to be significant in practice.
> jdk/vm/ci/hotspot/HotSpotJVMCIRuntime.java
> 211 for (HotSpotJVMCIBackendFactory factory :
> ServiceLoader.load(HotSpotJVMCIBackendFactory.class)) {
>
> This uses the thread context class loader to load providers. Services::load
> uses
> the system class loader to load providers. I suspect you want this to use
> the
> system class loader here too.
>
> jdk/vm/ci/services/JVMCIServiceLocator.java
> 78 for (JVMCIServiceLocator access :
> ServiceLoader.load(JVMCIServiceLocator.class)) {
>
> Same comment as above. I think you want to use system class loader to load
> providers.
Yes, I think you're right. I've updated the webrev with that change.
Thanks for the review.
-Doug
[1]
http://hg.openjdk.java.net/jdk9/dev/hotspot/file/560d7aa083a2/.mx.jvmci/suite.py#l94
[2]
http://hg.openjdk.java.net/jdk9/dev/hotspot/file/560d7aa083a2/.mx.jvmci/suite.py#l45
Re: RFR: 8177845: Need a mechanism to load Graal
Hi Doug, Mandy,
Just a minor comment on the VM class changes...
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.
If this is true, then we could wrap the copy of saved properties with an
unmodifiable map view before setting the savedProps field so that
getSavedProperties() could always return the same instance. Like for
example in the following alternative change:
http://cr.openjdk.java.net/~plevart/jdk9-dev/8177845_VM.getSavedProperties/webrev.01/
What do you think?
Regards, Peter
On 04/19/2017 02:02 AM, Mandy Chung wrote:
On Apr 18, 2017, at 3:13 PM, Doug Simon wrote:
Please review these changes that make jdk.internal.vm.compiler an upgradable
compiler.
:
http://cr.openjdk.java.net/~dnsimon/8177845/
Thanks for making this change. This would simplify the way to replace JDK’s
graal with the lab graal. A couple of comments:
jdk/internal/misc/VM.java
161 * Note that the saved system properties do not include
162 * the ones set by sun.misc.Version.init().
sun.misc.Version is no longer present in JDK 9. Renamed to
java.lang.VersionProps
jdk/vm/ci/services/Services.java
67 Class.forName("jdk.vm.ci.runtime.JVMCI”);
JVMCI class is local in jdk.internal.vm.ci module. An alternative may be
to provide a static initialize method rather than Class::forName.
jdk/vm/ci/hotspot/HotSpotJVMCIRuntime.java
211 for (HotSpotJVMCIBackendFactory factory :
ServiceLoader.load(HotSpotJVMCIBackendFactory.class)) {
This uses the thread context class loader to load providers. Services::load
uses
the system class loader to load providers. I suspect you want this to use the
system class loader here too.
jdk/vm/ci/services/JVMCIServiceLocator.java
78 for (JVMCIServiceLocator access :
ServiceLoader.load(JVMCIServiceLocator.class)) {
Same comment as above. I think you want to use system class loader to load
providers.
Mandy
Re: RFR: 8177845: Need a mechanism to load Graal
On 04/19/2017 09:37 AM, Peter Levart wrote: http://cr.openjdk.java.net/~plevart/jdk9-dev/8177845_VM.getSavedProperties/webrev.01/ Also, while we are at it, the following javadocs in the getSavedProperty() do not apply any more: 138 * It accesses a private copy of the system properties so 139 * that user's locking of the system properties object will not 140 * cause the library to deadlock. In JDK 9, Properties class does not use locking any more on the Properties instance for get()/getProperty() methods... Regards, Peter
Re: RFR: 8177845: Need a mechanism to load Graal
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
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
Hi Simon,
On 04/19/2017 11:25 AM, Doug Simon wrote:
Hi Peter,
All of your suggestions look good. I've updated
http://cr.openjdk.java.net/~dnsimon/8177845/jdk/src/java.base/share/classes/jdk/internal/misc/VM.java.udiff.html
to include them (please check I didn't make any copy errors in the process).
Looks good.
I was not aware of the new Map.ofEntries method. Nice to see more space
efficient map implementations being added to the JDK.
Admittedly, I used this method in a way not envisioned by the author.
Maybe there's a reason there is no Map.copyOf(Map) method there, which
would make this even simpler. If there was one, it would be too easy to
(mis)use it instead of Collections.unmodifiableMap(Map), albeit with a
slightly different semantics, and force re-hashing-copying of big maps
where there is no need to do that. But it would be a pretty nice
replacement for the following idiom:
Collections.unmodifiableMap(new HashMap<>(someMap))
Regards, Peter
Thanks!
-Doug
On 19 Apr 2017, at 10:12, Peter Levart wrote:
On 04/19/2017 09:42 AM, Peter Levart wrote:
On 04/19/2017 09:37 AM, Peter Levart wrote:
http://cr.openjdk.java.net/~plevart/jdk9-dev/8177845_VM.getSavedProperties/webrev.01/
Also, while we are at it, the following javadocs in the getSavedProperty() do
not apply any more:
138 * It accesses a private copy of the system properties so
139 * that user's locking of the system properties object will not
140 * cause the library to deadlock.
In JDK 9, Properties class does not use locking any more on the Properties
instance for get()/getProperty() methods...
Regards, Peter
I also noticed the following comment:
// TODO: the Property Management needs to be refactored and
// the appropriate prop keys need to be accessible to the
// calling classes to avoid duplication of keys.
private static Map savedProps;
...which is not entirely true. Neither keys nor values are duplicated (they are
just referenced in the new copy of the Properties/Map object). What is
duplicated is an excessive amount of internal objects, such as array slots and
Map.Entry objects. If this is a concern, then we could use the new immutable
Map implementation that is available in JDK 9, so the following lines in my
webrev:
181 @SuppressWarnings("unchecked")
182 Map sp = new HashMap<>((Map)props);
183 // only main thread is running at this time, so savedProps and
184 // its content will be correctly published to threads started later
185 savedProps = Collections.unmodifiableMap(sp);
Could be changed into:
@SuppressWarnings("unchecked")
Map sp =
Map.ofEntries(props.entrySet().toArray(new Map.Entry[0]));
// only main thread is running at this time, so savedProps
// will be correctly published to threads started later
savedProps = sp;
...to save some excessive space (the implementation is a linear-probe hashtable
which keeps keys and values directly in an array without wrapping them with
Map.Entry objects).
Regards, Peter
Re: RFR: 8177845: Need a mechanism to load Graal
On 04/19/2017 11:49 AM, Peter Levart wrote: I was not aware of the new Map.ofEntries method. Nice to see more space efficient map implementations being added to the JDK. Admittedly, I used this method in a way not envisioned by the author. Maybe there's a reason there is no Map.copyOf(Map) method there, which would make this even simpler. If there was one, it would be too easy to (mis)use it instead of Collections.unmodifiableMap(Map), albeit with a slightly different semantics, and force re-hashing-copying of big maps where there is no need to do that. But it would be a pretty nice replacement for the following idiom: Collections.unmodifiableMap(new HashMap<>(someMap)) Sometimes I wish that besides public, protected, "package-private" and private, Java also had an "expert protected" access modifier ;-) Regards, Peter
Re: RFR: 8177845: Need a mechanism to load Graal
On 19/04/2017 08:37, Peter Levart wrote: : Note that Properties class is thread-safe, while HashMap isn't. But I think this should not be a problem here, as during initPhase1(), as far as I know, no threads apart from main thread are started yet (is this true?), so anyone accessing getSavedProperty(ies) will either be the main thread itself or a thread started afterwards, so there is a happens-before relationship. The ReferenceHandler and Finalizer threads are started early, before initPhase1. However, both should immediately block and in the case of the Finalizer, await the completion of initPhase1 before polling. -Alan
Re: RFR: 8177845: Need a mechanism to load Graal
On 18/04/2017 23:13, Doug Simon wrote: : https://bugs.openjdk.java.net/browse/JDK-8177845 http://cr.openjdk.java.net/~dnsimon/8177845/ I'm happy to see jdk.internal.vm.compiler changing to be an upgradable module and the patching foo going away. If I read the changes correctly then I can extend JVMCIServiceLocator and the construction of my sub-class will open up all packages in jdk.internal.vm.ci to me. It is there any to tie this to -XX:+EnableJVMCI? -Alan
Re: RFR: 8177845: Need a mechanism to load Graal
> On 19 Apr 2017, at 16:04, Alan Bateman wrote: > > On 18/04/2017 23:13, Doug Simon wrote: > >> : >> >> https://bugs.openjdk.java.net/browse/JDK-8177845 >> http://cr.openjdk.java.net/~dnsimon/8177845/ >> > I'm happy to see jdk.internal.vm.compiler changing to be an upgradable module > and the patching foo going away. Yes, I'm delighted to see the command line required for using upstream Graal shrinking! > If I read the changes correctly then I can extend JVMCIServiceLocator and the > construction of my sub-class will open up all packages in jdk.internal.vm.ci > to me. It is there any to tie this to -XX:+EnableJVMCI? We could but I'm not sure what it would buy you. The service lookup only originates from the JVMCI runtime and the initialization of JVMCI already checks EnableJVMCI[1] -Doug [1] http://hg.openjdk.java.net/jdk9/dev/hotspot/file/4368832d1991/src/share/vm/jvmci/jvmciRuntime.cpp#l634
Re: Issue with JavaFX and Jigsaw
Actually, since the API doc for Module#isExported(String) [1] specifies that it also returns true if the package is opened unconditionally, do you think I need to mention it in the JavaFX docs as a parenthetical comment, or it is sufficient to link to Module#isExported(String)? -- Kevin [1] http://download.java.net/java/jdk9/docs/api/java/lang/Module.html#isExported-java.lang.String- Kevin Rushforth wrote: Alex Buckley wrote: OK, so for the JDK 9 docs, "unconditionally exported (or unconditionally opened)" would be clear. I was originally thinking to require unconditional export, but since it will also be fine for an application to open the package unconditionally, I will make this change and document either as being sufficient. Thanks. -- Kevin
Re: RFR: 8177845: Need a mechanism to load Graal
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
Peter
Re: RFR: 8177845: Need a mechanism to load Graal
> On Apr 19, 2017, at 8:03 AM, Peter Levart wrote:
>
> Hi Alan,
>
> On 04/19/2017 03:41 PM, Alan Bateman wrote:
>> On 19/04/2017 08:37, Peter Levart wrote:
>>
>>> :
>>>
>>> Note that Properties class is thread-safe, while HashMap isn't. But I think
>>> this should not be a problem here, as during initPhase1(), as far as I
>>> know, no threads apart from main thread are started yet (is this true?), so
>>> anyone accessing getSavedProperty(ies) will either be the main thread
>>> itself or a thread started afterwards, so there is a happens-before
>>> relationship.
>> The ReferenceHandler and Finalizer threads are started early, before
>> initPhase1. However, both should immediately block and in the case of the
>> Finalizer, await the completion of initPhase1 before polling.
>>
>> -Alan
>
> Just out of curiosity, what guarantees are there that no code before
> VM.saveAndRemoveProperties() ever executes Integer.valueOf(int) method for
> example? Note that this call is implicitly hidden in autoboxing of int(s)...
>
> If this happens, then class initialization of Integer.IntegerCache may fail,
> because it is using VM.getSavedProperty():
>
>public static String getSavedProperty(String key) {
>if (savedProps.isEmpty())
>throw new IllegalStateException("Should be non-empty if
> initialized");
>
>return savedProps.getProperty(key);
>}
>
> Hm
We should catch this issue and detect if VM.getSavedProperty is called when
init level < 1 during early startup.
Mandy
Re: RFR: 8177845: Need a mechanism to load Graal
I lost track a bit but why are we exporting jdk.vm.ci.services to the world now?
module jdk.internal.vm.ci {
-exports jdk.vm.ci.services to jdk.internal.vm.compiler;
+exports jdk.vm.ci.services;
> On Apr 18, 2017, at 9:16 PM, Doug Simon wrote:
>
> (adding hotspot-compiler-dev)
>
> Please review these changes that make jdk.internal.vm.compiler an upgradable
> compiler.
> The primary requirement for this is to remove all usage of JDK internals from
> Graal.
> While this most involves changes in Graal, there remain a few capabilities
> that must
> be exposed via JVMCI. Namely:
>
> 1. Graal needs a copy of jdk.internal.misc.VM.savedProps so that it can Graal
> initialize
> can use system properties that are guaranteed not to have been modified by
> application
> code (Graal initialization is lazy and may occur after application code has
> been run).
>
> 2. Truffle needs to be able to trigger JVMCI initialization so that it can
> select the Graal
> optimized implementation of the Truffle runtime.
>
> These capabilities have been added to jdk.vm.ci.services.Services. A comment
> has
> also been added to this class to denote the current methods to be removed
> in a subsequent bug to update the JDK copy of Graal with the upstream version
> which
> no longer uses the methods. The methods destined for removal are:
>
> exportJVMCITo(Class requestor)
> load(Class service)
> loadSingle(Class service, boolean required)
>
> The first one is no longer needed as JVMCI exports itself to Graal service
> providers
> it loads via private API and Graal re-exports[1] JVMCI to any module the
> extends Graal.
> The latter 2 are no longer required as Graal uses the standard ServiceLoader.
> This API
> is only useful for JVMCI-8 where a hidden JVMCI class loader is used.
>
> These changes have been tested against upstream Graal. To make the Graal
> tests pass, 2 extra
> minor changes were required:
>
> 1. In JDK-8177673, HotSpotMemoryAccessProviderImpl::checkRead was added and
> throws IllegalArgumentException
> on all error paths except one which throws AssertionError. The latter was a
> mistake and has been
> fixed to also throw IllegalArgumentException.
> 2. An invalid assertion has been removed from
> HotSpotResolvedObjectTypeImpl::isDefinitelyResolvedWithRespectTo.
> Up to JDK 8, it was true that all classes in java.* packages were loaded by
> the boot class loader.
> This is no longer true in JDK 9 with classes like java.sql being loaded by
> platform class loader.
>
> As hinted above, a subsequent bug will be required to pull in the latest
> upstream Graal and
> remove use of JDK internal API from the jdk.aot module. The qualified exports
> to
> jdk.vm.internal.compiler and jdk.aot can then be removed.
>
> -Doug
>
> https://bugs.openjdk.java.net/browse/JDK-8177845
> http://cr.openjdk.java.net/~dnsimon/8177845/
>
> [1]
> http://openjdk.java.net/projects/jigsaw/spec/issues/#IndirectQualifiedReflectiveAccess
>
>
>
Re: RFR: 8177845: Need a mechanism to load Graal
Since jdk.internal.vm.compiler becomes an upgradeable module, it is not hashed
with java.base to allow it to be upgraded and there is no integrity check.
Such qualified export will be granted to any module named
jdk.internal.vm.compiler at runtime. The goal is for upgradeable modules not
to use any internal APIs and eliminate the qualified exports.
The main thing is that jdk.vm.ci.services API would need to be guarded if it’s
used by non-Graal modules.
Mandy
> On Apr 19, 2017, at 11:24 AM, Christian Thalinger
> wrote:
>
> I lost track a bit but why are we exporting jdk.vm.ci.services to the world
> now?
>
> module jdk.internal.vm.ci {
> -exports jdk.vm.ci.services to jdk.internal.vm.compiler;
> +exports jdk.vm.ci.services;
>
>> On Apr 18, 2017, at 9:16 PM, Doug Simon wrote:
>>
>> (adding hotspot-compiler-dev)
>>
>> Please review these changes that make jdk.internal.vm.compiler an upgradable
>> compiler.
>> The primary requirement for this is to remove all usage of JDK internals
>> from Graal.
>> While this most involves changes in Graal, there remain a few capabilities
>> that must
>> be exposed via JVMCI. Namely:
>>
>> 1. Graal needs a copy of jdk.internal.misc.VM.savedProps so that it can
>> Graal initialize
>> can use system properties that are guaranteed not to have been modified by
>> application
>> code (Graal initialization is lazy and may occur after application code has
>> been run).
>>
>> 2. Truffle needs to be able to trigger JVMCI initialization so that it can
>> select the Graal
>> optimized implementation of the Truffle runtime.
>>
>> These capabilities have been added to jdk.vm.ci.services.Services. A comment
>> has
>> also been added to this class to denote the current methods to be removed
>> in a subsequent bug to update the JDK copy of Graal with the upstream
>> version which
>> no longer uses the methods. The methods destined for removal are:
>>
>> exportJVMCITo(Class requestor)
>> load(Class service)
>> loadSingle(Class service, boolean required)
>>
>> The first one is no longer needed as JVMCI exports itself to Graal service
>> providers
>> it loads via private API and Graal re-exports[1] JVMCI to any module the
>> extends Graal.
>> The latter 2 are no longer required as Graal uses the standard
>> ServiceLoader. This API
>> is only useful for JVMCI-8 where a hidden JVMCI class loader is used.
>>
>> These changes have been tested against upstream Graal. To make the Graal
>> tests pass, 2 extra
>> minor changes were required:
>>
>> 1. In JDK-8177673, HotSpotMemoryAccessProviderImpl::checkRead was added and
>> throws IllegalArgumentException
>> on all error paths except one which throws AssertionError. The latter was a
>> mistake and has been
>> fixed to also throw IllegalArgumentException.
>> 2. An invalid assertion has been removed from
>> HotSpotResolvedObjectTypeImpl::isDefinitelyResolvedWithRespectTo.
>> Up to JDK 8, it was true that all classes in java.* packages were loaded by
>> the boot class loader.
>> This is no longer true in JDK 9 with classes like java.sql being loaded by
>> platform class loader.
>>
>> As hinted above, a subsequent bug will be required to pull in the latest
>> upstream Graal and
>> remove use of JDK internal API from the jdk.aot module. The qualified
>> exports to
>> jdk.vm.internal.compiler and jdk.aot can then be removed.
>>
>> -Doug
>>
>> https://bugs.openjdk.java.net/browse/JDK-8177845
>> http://cr.openjdk.java.net/~dnsimon/8177845/
>>
>> [1]
>> http://openjdk.java.net/projects/jigsaw/spec/issues/#IndirectQualifiedReflectiveAccess
>>
>>
>>
>
Re: RFR: 8177845: Need a mechanism to load Graal
> On Apr 19, 2017, at 8:38 AM, Mandy Chung wrote:
>
> Since jdk.internal.vm.compiler becomes an upgradeable module, it is not
> hashed with java.base to allow it to be upgraded and there is no integrity
> check. Such qualified export will be granted to any module named
> jdk.internal.vm.compiler at runtime. The goal is for upgradeable modules not
> to use any internal APIs and eliminate the qualified exports.
>
> The main thing is that jdk.vm.ci.services API would need to be guarded if
> it’s used by non-Graal modules.
This all makes sense but where is the restriction that only
jdk.internal.vm.compiler can use jdk.vm.ci.services? After all, this is a
security issue.
>
> Mandy
>
>> On Apr 19, 2017, at 11:24 AM, Christian Thalinger
>> wrote:
>>
>> I lost track a bit but why are we exporting jdk.vm.ci.services to the world
>> now?
>>
>> module jdk.internal.vm.ci {
>> -exports jdk.vm.ci.services to jdk.internal.vm.compiler;
>> +exports jdk.vm.ci.services;
>>
>>> On Apr 18, 2017, at 9:16 PM, Doug Simon wrote:
>>>
>>> (adding hotspot-compiler-dev)
>>>
>>> Please review these changes that make jdk.internal.vm.compiler an
>>> upgradable compiler.
>>> The primary requirement for this is to remove all usage of JDK internals
>>> from Graal.
>>> While this most involves changes in Graal, there remain a few capabilities
>>> that must
>>> be exposed via JVMCI. Namely:
>>>
>>> 1. Graal needs a copy of jdk.internal.misc.VM.savedProps so that it can
>>> Graal initialize
>>> can use system properties that are guaranteed not to have been modified by
>>> application
>>> code (Graal initialization is lazy and may occur after application code has
>>> been run).
>>>
>>> 2. Truffle needs to be able to trigger JVMCI initialization so that it can
>>> select the Graal
>>> optimized implementation of the Truffle runtime.
>>>
>>> These capabilities have been added to jdk.vm.ci.services.Services. A
>>> comment has
>>> also been added to this class to denote the current methods to be removed
>>> in a subsequent bug to update the JDK copy of Graal with the upstream
>>> version which
>>> no longer uses the methods. The methods destined for removal are:
>>>
>>> exportJVMCITo(Class requestor)
>>> load(Class service)
>>> loadSingle(Class service, boolean required)
>>>
>>> The first one is no longer needed as JVMCI exports itself to Graal service
>>> providers
>>> it loads via private API and Graal re-exports[1] JVMCI to any module the
>>> extends Graal.
>>> The latter 2 are no longer required as Graal uses the standard
>>> ServiceLoader. This API
>>> is only useful for JVMCI-8 where a hidden JVMCI class loader is used.
>>>
>>> These changes have been tested against upstream Graal. To make the Graal
>>> tests pass, 2 extra
>>> minor changes were required:
>>>
>>> 1. In JDK-8177673, HotSpotMemoryAccessProviderImpl::checkRead was added and
>>> throws IllegalArgumentException
>>> on all error paths except one which throws AssertionError. The latter was a
>>> mistake and has been
>>> fixed to also throw IllegalArgumentException.
>>> 2. An invalid assertion has been removed from
>>> HotSpotResolvedObjectTypeImpl::isDefinitelyResolvedWithRespectTo.
>>> Up to JDK 8, it was true that all classes in java.* packages were loaded by
>>> the boot class loader.
>>> This is no longer true in JDK 9 with classes like java.sql being loaded by
>>> platform class loader.
>>>
>>> As hinted above, a subsequent bug will be required to pull in the latest
>>> upstream Graal and
>>> remove use of JDK internal API from the jdk.aot module. The qualified
>>> exports to
>>> jdk.vm.internal.compiler and jdk.aot can then be removed.
>>>
>>> -Doug
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8177845
>>> http://cr.openjdk.java.net/~dnsimon/8177845/
>>>
>>> [1]
>>> http://openjdk.java.net/projects/jigsaw/spec/issues/#IndirectQualifiedReflectiveAccess
>>>
>>>
>>>
>>
>
Re: RFR: 8177845: Need a mechanism to load Graal
> On Apr 19, 2017, at 11:55 AM, Christian Thalinger > wrote: > > >> On Apr 19, 2017, at 8:38 AM, Mandy Chung wrote: >> >> Since jdk.internal.vm.compiler becomes an upgradeable module, it is not >> hashed with java.base to allow it to be upgraded and there is no integrity >> check. Such qualified export will be granted to any module named >> jdk.internal.vm.compiler at runtime. The goal is for upgradeable modules >> not to use any internal APIs and eliminate the qualified exports. >> >> The main thing is that jdk.vm.ci.services API would need to be guarded if >> it’s used by non-Graal modules. > > This all makes sense but where is the restriction that only > jdk.internal.vm.compiler can use jdk.vm.ci.services? It's unqualified and no restriction in this change. Mandy
Re: RFR: 8177845: Need a mechanism to load Graal
> 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
> On Apr 19, 2017, at 12:07 PM, Doug Simon wrote: > > >> On 19 Apr 2017, at 20:01, Mandy Chung wrote: >> >> We should catch this issue and detect if VM.getSavedProperty is called when >> init level < 1 during early startup. > > What are you proposing? Some extra testing of VM startup to see if this is an > issue? If so, can you please suggest what testing is to be performed. This is an existing issue, not related to your change. I will create JBS issue to track it. What you have is fine. Mandy
Re: RFR: 8177845: Need a mechanism to load Graal
> On 19 Apr 2017, at 21:04, Mandy Chung wrote: > > >> On Apr 19, 2017, at 11:55 AM, Christian Thalinger >> wrote: >> >> >>> On Apr 19, 2017, at 8:38 AM, Mandy Chung wrote: >>> >>> Since jdk.internal.vm.compiler becomes an upgradeable module, it is not >>> hashed with java.base to allow it to be upgraded and there is no integrity >>> check. Such qualified export will be granted to any module named >>> jdk.internal.vm.compiler at runtime. The goal is for upgradeable modules >>> not to use any internal APIs and eliminate the qualified exports. >>> >>> The main thing is that jdk.vm.ci.services API would need to be guarded if >>> it’s used by non-Graal modules. >> >> This all makes sense but where is the restriction that only >> jdk.internal.vm.compiler can use jdk.vm.ci.services? > > It's unqualified and no restriction in this change. The public methods currently in jdk.vm.ci.services are: 1. JVMCIServiceLocator.getProvider(Class) 2. JVMCIServiceLocator.getProviders(Class) 3. Services.initializeJVMCI() 4. Services.getSavedProperties() 5. Services.exportJVMCITo(Class) 6. Services.load(Class) 7. Services.loadSingle(Class, boolean) 1 should be made protected. I'll update the webrev with this change. 2 should check for JVMCIPermission. I'll update the webrev with this change. 3 is harmless from a security perspective in my opinion. 4 checks for JVMCIPermission. 5, 6 and 7 will be removed in a follow bug that updates Graal from upstream (and removes its usage of these methods). Does this address the security concerns? -Doug
Re: RFR: 8177845: Need a mechanism to load Graal
> On Apr 19, 2017, at 9:27 AM, Doug Simon wrote: > >> >> On 19 Apr 2017, at 21:04, Mandy Chung wrote: >> >> >>> On Apr 19, 2017, at 11:55 AM, Christian Thalinger >>> wrote: >>> >>> On Apr 19, 2017, at 8:38 AM, Mandy Chung wrote: Since jdk.internal.vm.compiler becomes an upgradeable module, it is not hashed with java.base to allow it to be upgraded and there is no integrity check. Such qualified export will be granted to any module named jdk.internal.vm.compiler at runtime. The goal is for upgradeable modules not to use any internal APIs and eliminate the qualified exports. The main thing is that jdk.vm.ci.services API would need to be guarded if it’s used by non-Graal modules. >>> >>> This all makes sense but where is the restriction that only >>> jdk.internal.vm.compiler can use jdk.vm.ci.services? >> >> It's unqualified and no restriction in this change. > > The public methods currently in jdk.vm.ci.services are: > > 1. JVMCIServiceLocator.getProvider(Class) > 2. JVMCIServiceLocator.getProviders(Class) > 3. Services.initializeJVMCI() > 4. Services.getSavedProperties() > 5. Services.exportJVMCITo(Class) > 6. Services.load(Class) > 7. Services.loadSingle(Class, boolean) > > 1 should be made protected. I'll update the webrev with this change. Good. > > 2 should check for JVMCIPermission. I'll update the webrev with this change. Good. > > 3 is harmless from a security perspective in my opinion. Would be good if one of Oracle’s security engineers could take a quick look just to be sure. > > 4 checks for JVMCIPermission. Ok. > > 5, 6 and 7 will be removed in a follow bug that updates Graal from upstream > (and removes its usage of these methods). About this, will this Graal update happen for JDK 9? It’s awfully late in the cycle... > > Does this address the security concerns? Mostly, yes. Thanks. > > -Doug
Re: RFR: 8177845: Need a mechanism to load Graal
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
Sure - how about good old Util? ;-) I'm open to other suggestions. Sent from my iPhone > On Apr 19, 2017, at 9:46 PM, Vladimir Kozlov > wrote: > > Hi Doug, > > Can you consider using other name and not JDK9 for new JVMCI class? It will > be used in JDK 10 too: > > jdk.vm.ci.services.internal.JDK9; > > Thanks, > Vladimir > >> On 4/18/17 3:13 PM, Doug Simon wrote: >> Please review these changes that make jdk.internal.vm.compiler an upgradable >> compiler. >> The primary requirement for this is to remove all usage of JDK internals >> from Graal. >> While this most involves changes in Graal, there remain a few capabilities >> that must >> be exposed via JVMCI. Namely: >> >> 1. Graal needs a copy of jdk.internal.misc.VM.savedProps so that it can >> Graal initialize >> can use system properties that are guaranteed not to have been modified by >> application >> code (Graal initialization is lazy and may occur after application code >> has been run). >> >> 2. Truffle needs to be able to trigger JVMCI initialization so that it can >> select the Graal >> optimized implementation of the Truffle runtime. >> >> These capabilities have been added to jdk.vm.ci.services.Services. A comment >> has >> also been added to this class to denote the current methods to be removed >> in a subsequent bug to update the JDK copy of Graal with the upstream >> version which >> no longer uses the methods. The methods destined for removal are: >> >> exportJVMCITo(Class requestor) >> load(Class service) >> loadSingle(Class service, boolean required) >> >> The first one is no longer needed as JVMCI exports itself to Graal service >> providers >> it loads via private API and Graal re-exports[1] JVMCI to any module the >> extends Graal. >> The latter 2 are no longer required as Graal uses the standard >> ServiceLoader. This API >> is only useful for JVMCI-8 where a hidden JVMCI class loader is used. >> >> These changes have been tested against upstream Graal. To make the Graal >> tests pass, 2 extra >> minor changes were required: >> >> 1. In JDK-8177663, HotSpotMemoryAccessProviderImpl::checkRead was added and >> throws IllegalArgumentException >> on all error paths except one which throws AssertionError. The latter was >> a mistake and has been >> fixed to also throw IllegalArgumentException. >> 2. An invalid assertion has been removed from >> HotSpotResolvedObjectTypeImpl::isDefinitelyResolvedWithRespectTo. >> Up to JDK 8, it was true that all classes in java.* packages were loaded >> by the boot class loader. >> This is no longer true in JDK 9 with classes like java.sql being loaded by >> platform class loader. >> >> As hinted above, a subsequent bug will be required to pull in the latest >> upstream Graal and >> remove use of JDK internal API from the jdk.aot module. The qualified >> exports to >> jdk.vm.internal.compiler and jdk.aot can then be removed. >> >> -Doug >> >> https://bugs.openjdk.java.net/browse/JDK-8177845 >> http://cr.openjdk.java.net/~dnsimon/8177845/ >> >> [1] >> http://openjdk.java.net/projects/jigsaw/spec/issues/#IndirectQualifiedReflectiveAccess >> >> >>
Re: RFR: 8177845: Need a mechanism to load Graal
ReflectionAccessJDK ? Based on your comment in the file. Vladimir On 4/19/17 1:02 PM, Doug Simon wrote: Sure - how about good old Util? ;-) I'm open to other suggestions. Sent from my iPhone On Apr 19, 2017, at 9:46 PM, Vladimir Kozlov wrote: Hi Doug, Can you consider using other name and not JDK9 for new JVMCI class? It will be used in JDK 10 too: jdk.vm.ci.services.internal.JDK9; Thanks, Vladimir On 4/18/17 3:13 PM, Doug Simon wrote: Please review these changes that make jdk.internal.vm.compiler an upgradable compiler. The primary requirement for this is to remove all usage of JDK internals from Graal. While this most involves changes in Graal, there remain a few capabilities that must be exposed via JVMCI. Namely: 1. Graal needs a copy of jdk.internal.misc.VM.savedProps so that it can Graal initialize can use system properties that are guaranteed not to have been modified by application code (Graal initialization is lazy and may occur after application code has been run). 2. Truffle needs to be able to trigger JVMCI initialization so that it can select the Graal optimized implementation of the Truffle runtime. These capabilities have been added to jdk.vm.ci.services.Services. A comment has also been added to this class to denote the current methods to be removed in a subsequent bug to update the JDK copy of Graal with the upstream version which no longer uses the methods. The methods destined for removal are: exportJVMCITo(Class requestor) load(Class service) loadSingle(Class service, boolean required) The first one is no longer needed as JVMCI exports itself to Graal service providers it loads via private API and Graal re-exports[1] JVMCI to any module the extends Graal. The latter 2 are no longer required as Graal uses the standard ServiceLoader. This API is only useful for JVMCI-8 where a hidden JVMCI class loader is used. These changes have been tested against upstream Graal. To make the Graal tests pass, 2 extra minor changes were required: 1. In JDK-8177663, HotSpotMemoryAccessProviderImpl::checkRead was added and throws IllegalArgumentException on all error paths except one which throws AssertionError. The latter was a mistake and has been fixed to also throw IllegalArgumentException. 2. An invalid assertion has been removed from HotSpotResolvedObjectTypeImpl::isDefinitelyResolvedWithRespectTo. Up to JDK 8, it was true that all classes in java.* packages were loaded by the boot class loader. This is no longer true in JDK 9 with classes like java.sql being loaded by platform class loader. As hinted above, a subsequent bug will be required to pull in the latest upstream Graal and remove use of JDK internal API from the jdk.aot module. The qualified exports to jdk.vm.internal.compiler and jdk.aot can then be removed. -Doug https://bugs.openjdk.java.net/browse/JDK-8177845 http://cr.openjdk.java.net/~dnsimon/8177845/ [1] http://openjdk.java.net/projects/jigsaw/spec/issues/#IndirectQualifiedReflectiveAccess
Re: RFR: 8177845: Need a mechanism to load Graal
> On 19 Apr 2017, at 21:40, Christian Thalinger wrote: > >> >> On Apr 19, 2017, at 9:27 AM, Doug Simon wrote: >> >>> >>> On 19 Apr 2017, at 21:04, Mandy Chung wrote: >>> >>> On Apr 19, 2017, at 11:55 AM, Christian Thalinger wrote: > On Apr 19, 2017, at 8:38 AM, Mandy Chung wrote: > > Since jdk.internal.vm.compiler becomes an upgradeable module, it is not > hashed with java.base to allow it to be upgraded and there is no > integrity check. Such qualified export will be granted to any module > named jdk.internal.vm.compiler at runtime. The goal is for upgradeable > modules not to use any internal APIs and eliminate the qualified exports. > > The main thing is that jdk.vm.ci.services API would need to be guarded if > it’s used by non-Graal modules. This all makes sense but where is the restriction that only jdk.internal.vm.compiler can use jdk.vm.ci.services? >>> >>> It's unqualified and no restriction in this change. >> >> The public methods currently in jdk.vm.ci.services are: >> >> 1. JVMCIServiceLocator.getProvider(Class) >> 2. JVMCIServiceLocator.getProviders(Class) >> 3. Services.initializeJVMCI() >> 4. Services.getSavedProperties() >> 5. Services.exportJVMCITo(Class) >> 6. Services.load(Class) >> 7. Services.loadSingle(Class, boolean) >> >> 1 should be made protected. I'll update the webrev with this change. > > Good. > >> >> 2 should check for JVMCIPermission. I'll update the webrev with this change. > > Good. > >> >> 3 is harmless from a security perspective in my opinion. > > Would be good if one of Oracle’s security engineers could take a quick look > just to be sure. Vladimir, can you please bring this to the attention of the relevant engineer. >> >> 4 checks for JVMCIPermission. > > Ok. > >> >> 5, 6 and 7 will be removed in a follow bug that updates Graal from upstream >> (and removes its usage of these methods). > > About this, will this Graal update happen for JDK 9? Yes. > It’s awfully late in the cycle... These are jigsaw related changes and I've been told jigsaw has an FC exception (although I don't exactly know what that is). -Doug
Re: RFR: 8177845: Need a mechanism to load Graal
I've updated http://cr.openjdk.java.net/~dnsimon/8177845/hotspot/ with these changes: 1. JVMCIServiceLocator.getProvider(Class) is now protected 2. JVMCIServiceLocator.getProviders(Class) now checks JVMCIPermission 3. Rename: jdk.vm.ci.services.internal.JDK9 -> jdk.vm.ci.services.internal.ReflectionAccessJDK -Doug > On 19 Apr 2017, at 23:12, Doug Simon wrote: > >> >> On 19 Apr 2017, at 21:40, Christian Thalinger wrote: >> >>> >>> On Apr 19, 2017, at 9:27 AM, Doug Simon wrote: >>> On 19 Apr 2017, at 21:04, Mandy Chung wrote: > On Apr 19, 2017, at 11:55 AM, Christian Thalinger > wrote: > > >> On Apr 19, 2017, at 8:38 AM, Mandy Chung wrote: >> >> Since jdk.internal.vm.compiler becomes an upgradeable module, it is not >> hashed with java.base to allow it to be upgraded and there is no >> integrity check. Such qualified export will be granted to any module >> named jdk.internal.vm.compiler at runtime. The goal is for upgradeable >> modules not to use any internal APIs and eliminate the qualified exports. >> >> The main thing is that jdk.vm.ci.services API would need to be guarded >> if it’s used by non-Graal modules. > > This all makes sense but where is the restriction that only > jdk.internal.vm.compiler can use jdk.vm.ci.services? It's unqualified and no restriction in this change. >>> >>> The public methods currently in jdk.vm.ci.services are: >>> >>> 1. JVMCIServiceLocator.getProvider(Class) >>> 2. JVMCIServiceLocator.getProviders(Class) >>> 3. Services.initializeJVMCI() >>> 4. Services.getSavedProperties() >>> 5. Services.exportJVMCITo(Class) >>> 6. Services.load(Class) >>> 7. Services.loadSingle(Class, boolean) >>> >>> 1 should be made protected. I'll update the webrev with this change. >> >> Good. >> >>> >>> 2 should check for JVMCIPermission. I'll update the webrev with this change. >> >> Good. >> >>> >>> 3 is harmless from a security perspective in my opinion. >> >> Would be good if one of Oracle’s security engineers could take a quick look >> just to be sure. > > Vladimir, can you please bring this to the attention of the relevant engineer. > >>> >>> 4 checks for JVMCIPermission. >> >> Ok. >> >>> >>> 5, 6 and 7 will be removed in a follow bug that updates Graal from upstream >>> (and removes its usage of these methods). >> >> About this, will this Graal update happen for JDK 9? > > Yes. > >> It’s awfully late in the cycle... > > These are jigsaw related changes and I've been told jigsaw has an FC > exception (although I don't exactly know what that is). > > -Doug
Re: RFR: 8177845: Need a mechanism to load Graal
Hotspot changes looks good to me. Thanks, Vladimir On 4/19/17 2:26 PM, Doug Simon wrote: I've updated http://cr.openjdk.java.net/~dnsimon/8177845/hotspot/ with these changes: 1. JVMCIServiceLocator.getProvider(Class) is now protected 2. JVMCIServiceLocator.getProviders(Class) now checks JVMCIPermission 3. Rename: jdk.vm.ci.services.internal.JDK9 -> jdk.vm.ci.services.internal.ReflectionAccessJDK -Doug On 19 Apr 2017, at 23:12, Doug Simon wrote: On 19 Apr 2017, at 21:40, Christian Thalinger wrote: On Apr 19, 2017, at 9:27 AM, Doug Simon wrote: On 19 Apr 2017, at 21:04, Mandy Chung wrote: On Apr 19, 2017, at 11:55 AM, Christian Thalinger wrote: On Apr 19, 2017, at 8:38 AM, Mandy Chung wrote: Since jdk.internal.vm.compiler becomes an upgradeable module, it is not hashed with java.base to allow it to be upgraded and there is no integrity check. Such qualified export will be granted to any module named jdk.internal.vm.compiler at runtime. The goal is for upgradeable modules not to use any internal APIs and eliminate the qualified exports. The main thing is that jdk.vm.ci.services API would need to be guarded if it’s used by non-Graal modules. This all makes sense but where is the restriction that only jdk.internal.vm.compiler can use jdk.vm.ci.services? It's unqualified and no restriction in this change. The public methods currently in jdk.vm.ci.services are: 1. JVMCIServiceLocator.getProvider(Class) 2. JVMCIServiceLocator.getProviders(Class) 3. Services.initializeJVMCI() 4. Services.getSavedProperties() 5. Services.exportJVMCITo(Class) 6. Services.load(Class) 7. Services.loadSingle(Class, boolean) 1 should be made protected. I'll update the webrev with this change. Good. 2 should check for JVMCIPermission. I'll update the webrev with this change. Good. 3 is harmless from a security perspective in my opinion. Would be good if one of Oracle’s security engineers could take a quick look just to be sure. Vladimir, can you please bring this to the attention of the relevant engineer. 4 checks for JVMCIPermission. Ok. 5, 6 and 7 will be removed in a follow bug that updates Graal from upstream (and removes its usage of these methods). About this, will this Graal update happen for JDK 9? Yes. It’s awfully late in the cycle... These are jigsaw related changes and I've been told jigsaw has an FC exception (although I don't exactly know what that is). -Doug
Re: RFR: 8177845: Need a mechanism to load Graal
Doug, Can you point (link) particular code which needs to be reviewed? And what security issues could be? Thanks, Vladimir On 4/19/17 2:12 PM, Doug Simon wrote: 3. Services.initializeJVMCI() 3 is harmless from a security perspective in my opinion. Would be good if one of Oracle’s security engineers could take a quick look just to be sure. Vladimir, can you please bring this to the attention of the relevant engineer.
Re: setAccessible() alternative with Jigsaw - feedback on Lookup
So I did some hacking and tried to make Weld use MethodHandles.Lookup and here is a bit of a feedback for you. But first of all, thanks for your quick advice on how to approach this. Alright, I should say that I _somehow_ managed to make it work in simple cases (SE container for most part). I haven't tried my dirty solution in anything EE, which is where problems will definitely pop-up (as they always do when you say "EAR"). ATM it is pretty impossible to try Weld 3 (will support jdk 9, still not final, CDI 2.0 impl) with any AS as none support it ATM (will try to port it to Wildfly 11). To sum up how well Lookup worked for me, let me shed some light on how Weld works (simplified ofc) and therefore what I needed to achieve: In short, we scan CP for classes which are to become beans. Such classes are the processed and eventually get a proxy created for them (given they have the right scope). This proxy is a piece of generated bytecode which we need to register via CL, or now via Lookup, using defineClass method. We respect the packages - proxy lands in the same package as the original class (with notable exceptions, see below). All the above has to be done during bootstrap - you cannot dynamically add/remove beans once you got application running. So, how did Lookup work for us? 1) privateLookupIn + drop private mode This was the way to go, as the bean classes can be anywhere (in classes not openly accessible to us, especially if we consider modules). BTW I am not sure about the purpose of the private mode as you always need to drop it to be able to use lookup. 2) Lookup approach carries along the need to pass the reference to the base lookup class at all times. This is kind of weird because in some (not-so-rare) cases, we need to create "artificial packages" in which we place proxy classes. For instance when we create a proxy for Interger, Map, Principal,... Ofc we cannot place in into java.* packages, so we create our own. For this to work with Lookup, we need to have that package created ahead of time and have a reference to some "dummy" useless class inside to be able to do the lookup. Or is there a way to define a whole new (open by default) package where we could place it using Lookup? Having the "dummy" package/class just to use Lookup is lame and very error prone (I can think of several ways to break the proxy creation even now). 3) All in all the changes carried along a lot of complexity compared to plain old ClassLoader.defineClass We need to create A LOT of Lookups (might present performance overhead, haven't tested yet). Extra care needs to be given to the packages for Lookup not to blow up while we in fact don't really care where the proxy lands as long as it is usable. Another nasty thing is that the code of course needs to work with both, JDK 9 and 8. While it isn't impossible, it will add a not-so-nice reflection magic layer to the mix. Regards Matej - Original Message - > From: "Alan Bateman" > To: "Matej Novotny" , jigsaw-dev@openjdk.java.net > Cc: "Martin Kouba" > Sent: Friday, March 31, 2017 4:28:34 PM > Subject: Re: setAccessible() alternative with Jigsaw > > On 31/03/2017 14:46, Matej Novotny wrote: > > > Hello, > > > > I work on Weld, context dependency injection framework. > > Long story short - we need to generate proxies for classes - bytecode which > > we then "register" with the class loader using > > java.lang.ClassLoader#defineClass. > > > > Obviously, for this you need reflections - to load java.lang.ClassLoader, > > then to load the method itself, and most importantly, to make the method > > accessible cause it's `protected`. > > In JDK 9, this blows up as soon as you try to make the method accessible > > (invocation of setAccessible). > > > > Fair enough, but what is the "legitimate" alternative? > > I know I can --add-opens / --add-opens / --permit-illegal-access > > But all those just bypass the checks and don't really solve it. I am > > looking for an intended way to do such stuff. > > I am pretty sure there are many frameworks which need to do this in one way > > or the other. > > > > So far I have found workarounds which involve using `sun.misc.Unsafe` > > because there (for some reason) you are allowed to invoke setAccessible(). > > Is this the official intended backdoor? Because it sure does not look any > > safer/cleaner solution than the original one... > > > As Claes pointed out, look at Lookup.defineClass. It shouldn't be too > hard to imagine a future update of the CDI spec where there a select > variant that includes a Lookup to the target class with the @Inject > annotation. When using Weld outside of a container then I think there is > main code that obtains the WeldContainer and selects the types, that > might be a suitable place to extend to provide the Lookup to the library. > > MethodHandles.privateLookupIn might also be useful to know about, esp. > starting out when you don't want to make API changes
