[core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control
This review is being spread across four groups: langtools, core-libs, hotspot and serviceability. This is the specific review thread for core-libs - webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/ See below for full details - including annotated full webrev guiding the review. The intent is to have JEP-181 targeted and integrated by the end of this month. Thanks, David ----- The nestmates project (JEP-181) introduces new classfile attributes to identify classes and interfaces in the same nest, so that the VM can perform access control based on those attributes and so allow direct private access between nestmates without requiring javac to generate synthetic accessor methods. These access control changes also extend to core reflection and the MethodHandle.Lookup contexts. Direct private calls between nestmates requires a more general calling context than is permitted by invokespecial, and so the JVMS is updated to allow, and javac updated to use, invokevirtual and invokeinterface for private class and interface method calls respectively. These changed semantics also extend to MethodHandle findXXX operations. At this time we are only concerned with static nest definitions, which map to a top-level class/interface as the nest-host and all its nested types as nest-members. Please see the JEP for further details. JEP: https://bugs.openjdk.java.net/browse/JDK-8046171 Bug: https://bugs.openjdk.java.net/browse/JDK-8010319 CSR: https://bugs.openjdk.java.net/browse/JDK-8197445 All of the specification changes have been previously been worked out by the Valhalla Project Expert Group, and the implementation reviewed by the various contributors and discussed on the valhalla-dev mailing list. Acknowledgments and contributions: Alex Buckley, Maurizio Cimadamore, Mandy Chung, Tobias Hartmann, Vladimir Ivanov, Karen Kinnear, Vladimir Kozlov, John Rose, Dan Smith, Serguei Spitsyn, Kumar Srinivasan Master webrev of all changes: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.full.v1/ Annotated master webrev index: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/jep181-webrev.html Performance: this is expected to be performance neutral in a general sense. Benchmarking and performance runs are about to start. Testing Discussion: ------------------ The testing for nestmates can be broken into four main groups: - New tests specifically related to nestmates and currently in the runtime/Nestmates directory - New tests to complement existing tests by adding in testcases not previously expressible. - For example java/lang/invoke/SpecialInterfaceCall.java tests use of invokespecial for private interface methods and performing receiver typechecks, so we add java/lang/invoke/PrivateInterfaceCall.java to do similar tests for invokeinterface. - New JVM TI tests to verify the spec changes related to nest attributes. - Existing tests significantly affected by the nestmates changes, primarily: - runtime/SelectionResolution In most cases the nestmate changes makes certain invocations that were illegal, legal (e.g. not requiring invokespecial to invoke private interface methods; allowing access to private members via reflection/Methodhandles that were previously not allowed). - Existing tests incidentally affected by the nestmate changes This includes tests of things utilising class redefinition/retransformation to alter nested types but which unintentionally alter nest relationships (which is not permitted). There are still a number of tests problem-listed with issues filed against them to have them adapted to work with nestmates. Some of these are intended to be addressed in the short-term, while some (such as the runtime/SelectionResolution test changes) may not eventuate. - https://bugs.openjdk.java.net/browse/JDK-8203033 - https://bugs.openjdk.java.net/browse/JDK-8199450 - https://bugs.openjdk.java.net/browse/JDK-8196855 - https://bugs.openjdk.java.net/browse/JDK-8194857 - https://bugs.openjdk.java.net/browse/JDK-8187655 There is also further test work still to be completed (the JNI and JDI invocation tests): - https://bugs.openjdk.java.net/browse/JDK-8191117 which will continue in parallel with the main RFR. Pre-integration Testing: - General: - Mach5: hs/jdk tier1,2 - Mach5: hs-nightly (tiers 1 -3) - Targetted - nashorn (for asm changes) - hotspot: runtime/* serviceability/* compiler/* vmTestbase/* - jdk: java/lang/invoke/* java/lang/reflect/* java/lang/instrument/* java/lang/Class/* java/lang/management/* - langtools: tools/javac tools/javap
On 15/05/2018 01:52, David Holmes wrote:
This review is being spread across four groups: langtools, core-libs, hotspot and serviceability. This is the specific review thread for core-libs - webrev:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/ The API additions mostly look good, just a few comments:
- Can the spec for Class::getNestHost be explicit that it returns "this" when the class is a primitive or array class? - Can Class::getNestMembers specify if it runs the class initializer of any classes that it loads? Less of an issue for getNestHost as the host is likely loaded already. - I suspect the @throws SecurityException in getNestMembers was copied from getNestHost as it uses "returned class" (singular). As the host and members are in the same runtime package then maybe it can be specified in terms of the host or members package? - isNestmateOf could be a bit clearer that it returns false in the event that the attributes can't be parsed. I realize it's specified in terms of getNestHost but I'm sure you see what I mean. The tests for core reflection are in test/jdk/java/lang/reflect and I'm wondering if there has been any discussion about adding at least some basic tests there? I see the tests in test/hotspot/jtreg/runtime/Nestmates/reflectionAPI but this isn't an obvious location for someone touching this code. The test in InterfaceAccessFlagsTest.java can be disabled with @Test(enabled=false), might be cleaner than commenting it out. Maybe a question for Kumar but are we planning to pull in any ASM updates for JDK 11? NestMembers extends Attribute looks okay, I'm less sure about the change to ClassReader as I don't know if there is somewhere else in ASM that has the list of attributes to always parse. The update to the access check in Reflection.java looks okay (just need to use 4-space indent, not 2). I think it's okay in VerifyAccess too but the "FIX ME" suggests there are doubts so I assume this needs more eyes to triple check. In passing, you might want to double check the javadoc links. I noticed one #getNestHost that is missing parenthesis and I'm not sure how javadoc handles that. -Alan.
Hi all, ----- Mail original -----
De: "Alan Bateman" <Alan.Bateman@oracle.com> À: "David Holmes" <david.holmes@oracle.com>, "core-libs-dev" <core-libs-dev@openjdk.java.net> Envoyé: Mardi 15 Mai 2018 15:53:44 Objet: Re: [core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control
On 15/05/2018 01:52, David Holmes wrote:
This review is being spread across four groups: langtools, core-libs, hotspot and serviceability. This is the specific review thread for core-libs - webrev:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/
[...]
Maybe a question for Kumar but are we planning to pull in any ASM updates for JDK 11? NestMembers extends Attribute looks okay, I'm less sure about the change to ClassReader as I don't know if there is somewhere else in ASM that has the list of attributes to always parse.
With my ASM hat, the current master of ASM (the release of ASM 6.2 is scheduled for the next week-end) already supports nestmates (and constant dynamic and preview feature) so i suppose that at some point in the future Kumar will merge it to the JDK. We have recently changed the way we implement features in ASM, instead of having features lingering in different branches, we now integrate them directly in the master under an experimental flag (ASM7_EXPERIMENTAL), which means for the JDK that it is no longer necessary to wait until the release of ASM 7 because it can use the experimental support of ASM 6.2. (note that experimental doesn't mean full of bugs, or half baked or anything like this, it means that the feature is not yet integrated in a released JDK). I've taking a look to the code in this patch, i've two comments, - in Attributes, it seems that the code store the bytecode slice corresponding to the attribute only to use its length as argument of the ByteVector which is like an ArrayList of byte, it grows automatically so the initial capacity is a perf optimization. Perhaps the byte array is used somewhere else ? - patching the ClassReader.accept is really a quick hack because the method accept with 3 arguments is not patched so if this method is called somewhere in the JDK it will behave as it should. [...]
-Alan.
Rémi
Hi Remi, On 17/05/2018 6:16 PM, Remi Forax wrote:
Hi all,
----- Mail original -----
De: "Alan Bateman" <Alan.Bateman@oracle.com> À: "David Holmes" <david.holmes@oracle.com>, "core-libs-dev" <core-libs-dev@openjdk.java.net> Envoyé: Mardi 15 Mai 2018 15:53:44 Objet: Re: [core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control
On 15/05/2018 01:52, David Holmes wrote:
This review is being spread across four groups: langtools, core-libs, hotspot and serviceability. This is the specific review thread for core-libs - webrev:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/
[...]
Maybe a question for Kumar but are we planning to pull in any ASM updates for JDK 11? NestMembers extends Attribute looks okay, I'm less sure about the change to ClassReader as I don't know if there is somewhere else in ASM that has the list of attributes to always parse.
With my ASM hat, the current master of ASM (the release of ASM 6.2 is scheduled for the next week-end) already supports nestmates (and constant dynamic and preview feature) so i suppose that at some point in the future Kumar will merge it to the JDK.
Unfortunately Kumar is no longer with us.
We have recently changed the way we implement features in ASM, instead of having features lingering in different branches, we now integrate them directly in the master under an experimental flag (ASM7_EXPERIMENTAL), which means for the JDK that it is no longer necessary to wait until the release of ASM 7 because it can use the experimental support of ASM 6.2. (note that experimental doesn't mean full of bugs, or half baked or anything like this, it means that the feature is not yet integrated in a released JDK).
I've taking a look to the code in this patch, i've two comments, - in Attributes, it seems that the code store the bytecode slice corresponding to the attribute only to use its length as argument of the ByteVector which is like an ArrayList of byte, it grows automatically so the initial capacity is a perf optimization. Perhaps the byte array is used somewhere else ? - patching the ClassReader.accept is really a quick hack because the method accept with 3 arguments is not patched so if this method is called somewhere in the JDK it will behave as it should.
I'll take a look at this. To be honest I don't even remember who provided those changes ... I thought you had provided feedback at some point in the past :) There's a valhalla-dev email with a link that's no longer valid: https://gitlab.ow2.org/asm/asm/tree/NEST_MATES Thanks, David
[...]
-Alan.
Rémi
----- Mail original -----
De: "David Holmes" <david.holmes@oracle.com> À: "Remi Forax" <forax@univ-mlv.fr>, "Alan Bateman" <Alan.Bateman@oracle.com> Cc: "core-libs-dev" <core-libs-dev@openjdk.java.net> Envoyé: Jeudi 17 Mai 2018 10:37:46 Objet: Re: [core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control
Hi Remi,
Hi David
On 17/05/2018 6:16 PM, Remi Forax wrote:
Hi all,
----- Mail original -----
De: "Alan Bateman" <Alan.Bateman@oracle.com> À: "David Holmes" <david.holmes@oracle.com>, "core-libs-dev" <core-libs-dev@openjdk.java.net> Envoyé: Mardi 15 Mai 2018 15:53:44 Objet: Re: [core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control
On 15/05/2018 01:52, David Holmes wrote:
This review is being spread across four groups: langtools, core-libs, hotspot and serviceability. This is the specific review thread for core-libs - webrev:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/
[...]
Maybe a question for Kumar but are we planning to pull in any ASM updates for JDK 11? NestMembers extends Attribute looks okay, I'm less sure about the change to ClassReader as I don't know if there is somewhere else in ASM that has the list of attributes to always parse.
With my ASM hat, the current master of ASM (the release of ASM 6.2 is scheduled for the next week-end) already supports nestmates (and constant dynamic and preview feature) so i suppose that at some point in the future Kumar will merge it to the JDK.
Unfortunately Kumar is no longer with us.
I will miss him.
We have recently changed the way we implement features in ASM, instead of having features lingering in different branches, we now integrate them directly in the master under an experimental flag (ASM7_EXPERIMENTAL), which means for the JDK that it is no longer necessary to wait until the release of ASM 7 because it can use the experimental support of ASM 6.2. (note that experimental doesn't mean full of bugs, or half baked or anything like this, it means that the feature is not yet integrated in a released JDK).
I've taking a look to the code in this patch, i've two comments, - in Attributes, it seems that the code store the bytecode slice corresponding to the attribute only to use its length as argument of the ByteVector which is like an ArrayList of byte, it grows automatically so the initial capacity is a perf optimization. Perhaps the byte array is used somewhere else ? - patching the ClassReader.accept is really a quick hack because the method accept with 3 arguments is not patched so if this method is called somewhere in the JDK it will behave as it should.
I'll take a look at this. To be honest I don't even remember who provided those changes ... I thought you had provided feedback at some point in the past :)
yes, i may have, i do not remember.
There's a valhalla-dev email with a link that's no longer valid:
as i said above, the support has been integrated in the master of ASM, see https://gitlab.ow2.org/asm/asm/blob/master/asm/src/main/java/org/objectweb/a... and https://gitlab.ow2.org/asm/asm/blob/master/asm/src/main/java/org/objectweb/a... the current code is fine, just not optimal, but given that when the JDK will use ASM 6.2, this code will have to be removed, i do not think you should spend some time one this.
Thanks, David
regards, Rémi
Sorry Remi I overlooked responding to this. Thanks for confirming I don't have to do anything at this time. David On 17/05/2018 8:02 PM, forax@univ-mlv.fr wrote:
----- Mail original -----
De: "David Holmes" <david.holmes@oracle.com> À: "Remi Forax" <forax@univ-mlv.fr>, "Alan Bateman" <Alan.Bateman@oracle.com> Cc: "core-libs-dev" <core-libs-dev@openjdk.java.net> Envoyé: Jeudi 17 Mai 2018 10:37:46 Objet: Re: [core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control
Hi Remi,
Hi David
On 17/05/2018 6:16 PM, Remi Forax wrote:
Hi all,
----- Mail original -----
De: "Alan Bateman" <Alan.Bateman@oracle.com> À: "David Holmes" <david.holmes@oracle.com>, "core-libs-dev" <core-libs-dev@openjdk.java.net> Envoyé: Mardi 15 Mai 2018 15:53:44 Objet: Re: [core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control
On 15/05/2018 01:52, David Holmes wrote:
This review is being spread across four groups: langtools, core-libs, hotspot and serviceability. This is the specific review thread for core-libs - webrev:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/
[...]
Maybe a question for Kumar but are we planning to pull in any ASM updates for JDK 11? NestMembers extends Attribute looks okay, I'm less sure about the change to ClassReader as I don't know if there is somewhere else in ASM that has the list of attributes to always parse.
With my ASM hat, the current master of ASM (the release of ASM 6.2 is scheduled for the next week-end) already supports nestmates (and constant dynamic and preview feature) so i suppose that at some point in the future Kumar will merge it to the JDK.
Unfortunately Kumar is no longer with us.
I will miss him.
We have recently changed the way we implement features in ASM, instead of having features lingering in different branches, we now integrate them directly in the master under an experimental flag (ASM7_EXPERIMENTAL), which means for the JDK that it is no longer necessary to wait until the release of ASM 7 because it can use the experimental support of ASM 6.2. (note that experimental doesn't mean full of bugs, or half baked or anything like this, it means that the feature is not yet integrated in a released JDK).
I've taking a look to the code in this patch, i've two comments, - in Attributes, it seems that the code store the bytecode slice corresponding to the attribute only to use its length as argument of the ByteVector which is like an ArrayList of byte, it grows automatically so the initial capacity is a perf optimization. Perhaps the byte array is used somewhere else ? - patching the ClassReader.accept is really a quick hack because the method accept with 3 arguments is not patched so if this method is called somewhere in the JDK it will behave as it should.
I'll take a look at this. To be honest I don't even remember who provided those changes ... I thought you had provided feedback at some point in the past :)
yes, i may have, i do not remember.
There's a valhalla-dev email with a link that's no longer valid:
as i said above, the support has been integrated in the master of ASM, see https://gitlab.ow2.org/asm/asm/blob/master/asm/src/main/java/org/objectweb/a... and https://gitlab.ow2.org/asm/asm/blob/master/asm/src/main/java/org/objectweb/a...
the current code is fine, just not optimal, but given that when the JDK will use ASM 6.2, this code will have to be removed, i do not think you should spend some time one this.
Thanks, David
regards, Rémi
Clarification ... On 17/05/2018 6:37 PM, David Holmes wrote:
Hi Remi,
On 17/05/2018 6:16 PM, Remi Forax wrote:
Hi all,
----- Mail original -----
De: "Alan Bateman" <Alan.Bateman@oracle.com> À: "David Holmes" <david.holmes@oracle.com>, "core-libs-dev" <core-libs-dev@openjdk.java.net> Envoyé: Mardi 15 Mai 2018 15:53:44 Objet: Re: [core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control
On 15/05/2018 01:52, David Holmes wrote:
This review is being spread across four groups: langtools, core-libs, hotspot and serviceability. This is the specific review thread for core-libs - webrev:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/
[...]
Maybe a question for Kumar but are we planning to pull in any ASM updates for JDK 11? NestMembers extends Attribute looks okay, I'm less sure about the change to ClassReader as I don't know if there is somewhere else in ASM that has the list of attributes to always parse.
With my ASM hat, the current master of ASM (the release of ASM 6.2 is scheduled for the next week-end) already supports nestmates (and constant dynamic and preview feature) so i suppose that at some point in the future Kumar will merge it to the JDK.
Unfortunately Kumar is no longer with us.
By which I simply mean he is no longer at Oracle. David -----
We have recently changed the way we implement features in ASM, instead of having features lingering in different branches, we now integrate them directly in the master under an experimental flag (ASM7_EXPERIMENTAL), which means for the JDK that it is no longer necessary to wait until the release of ASM 7 because it can use the experimental support of ASM 6.2. (note that experimental doesn't mean full of bugs, or half baked or anything like this, it means that the feature is not yet integrated in a released JDK).
I've taking a look to the code in this patch, i've two comments, - in Attributes, it seems that the code store the bytecode slice corresponding to the attribute only to use its length as argument of the ByteVector which is like an ArrayList of byte, it grows automatically so the initial capacity is a perf optimization. Perhaps the byte array is used somewhere else ? - patching the ClassReader.accept is really a quick hack because the method accept with 3 arguments is not patched so if this method is called somewhere in the JDK it will behave as it should.
I'll take a look at this. To be honest I don't even remember who provided those changes ... I thought you had provided feedback at some point in the past :) There's a valhalla-dev email with a link that's no longer valid:
https://gitlab.ow2.org/asm/asm/tree/NEST_MATES
Thanks, David
[...]
-Alan.
Rémi
----- Mail original -----
De: "David Holmes" <david.holmes@oracle.com> À: "Remi Forax" <forax@univ-mlv.fr>, "Alan Bateman" <Alan.Bateman@oracle.com> Cc: "core-libs-dev" <core-libs-dev@openjdk.java.net> Envoyé: Jeudi 17 Mai 2018 12:59:56 Objet: Re: [core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control
Clarification ...
On 17/05/2018 6:37 PM, David Holmes wrote:
Hi Remi,
On 17/05/2018 6:16 PM, Remi Forax wrote:
Hi all,
----- Mail original -----
De: "Alan Bateman" <Alan.Bateman@oracle.com> À: "David Holmes" <david.holmes@oracle.com>, "core-libs-dev" <core-libs-dev@openjdk.java.net> Envoyé: Mardi 15 Mai 2018 15:53:44 Objet: Re: [core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control
On 15/05/2018 01:52, David Holmes wrote:
This review is being spread across four groups: langtools, core-libs, hotspot and serviceability. This is the specific review thread for core-libs - webrev:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/
[...]
Maybe a question for Kumar but are we planning to pull in any ASM updates for JDK 11? NestMembers extends Attribute looks okay, I'm less sure about the change to ClassReader as I don't know if there is somewhere else in ASM that has the list of attributes to always parse.
With my ASM hat, the current master of ASM (the release of ASM 6.2 is scheduled for the next week-end) already supports nestmates (and constant dynamic and preview feature) so i suppose that at some point in the future Kumar will merge it to the JDK.
Unfortunately Kumar is no longer with us.
By which I simply mean he is no longer at Oracle.
oh, cool, sorry for this awkward moment.
David -----
Rémi
Hi Alan, Many thanks for looking at this! Let me start by saying that generally any spec changes (even non-normative) have to pass a high bar given the number of levels of review the spec, ie API, has already had. That isn't to say that no changes will be considered but I'm very wary of making last minute changes during code review, without all the parties previously involved with, and content with, the spec being party to the conversation. Such changes also affect the CSR request. So my initial position will be that of the immovable object. ;) On 15/05/2018 11:53 PM, Alan Bateman wrote:
On 15/05/2018 01:52, David Holmes wrote:
This review is being spread across four groups: langtools, core-libs, hotspot and serviceability. This is the specific review thread for core-libs - webrev:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/ The API additions mostly look good, just a few comments:
- Can the spec for Class::getNestHost be explicit that it returns "this" when the class is a primitive or array class?
This is covered by: "A class or interface that is not explicitly a member of a nest, is a member of the nest consisting only of itself, and is the nest host. Every class and interface is a member of exactly one nest." Do we really need to spell out the case for primitives and arrays? If so would it suffice to add the following: "A class or interface that is not explicitly a member of a nest *(such as primitive or array classes)*, is a member of the nest consisting only of itself, and is the nest host. Every class and interface is a member of exactly one nest."
- Can Class::getNestMembers specify if it runs the class initializer of any classes that it loads? Less of an issue for getNestHost as the host is likely loaded already.
None of these methods specifically or intentionally initialize any classes. A class will be loaded if needed but that is all. The only Class methods that refer to class initialization are the forName() variants. I don't see that the nest related methods should do any more or less in this regard than any other Class methods that may require classes to be loaded.
- I suspect the @throws SecurityException in getNestMembers was copied from getNestHost as it uses "returned class" (singular).
It refers to "If any returned class ..." and "that returned class". I don't see any problematic singular uses - can you elaborate please.
As the host and members are in the same runtime package then maybe it can be specified in terms of the host or members package?
I'm not sure how to accurately formulate that. The current wording was based on similar @throws in getEnclosingClass, as suggested by Mandy: http://mail.openjdk.java.net/pipermail/valhalla-dev/2018-March/003955.html and then refined a little.
- isNestmateOf could be a bit clearer that it returns false in the event that the attributes can't be parsed. I realize it's specified in terms of getNestHost but I'm sure you see what I mean.
Just to be clear, parsing of nest attributes occurs at class load time and must be syntactically correct else the class won't load. Validation of those attributes (i.e agreement between the purported nest-host class and the claimed nest-member class) is what is deferred until either an access check is needed, or these new Class reflection methods are called. So when getNestHost is called the current class is obviously already loaded, but the purported nest-host may need to be loaded which may encounter: - linkage-errors (NoClassDefFoundError, VerifyError, ClassfileFormatError etc) - runtime errors (OOME, stackoverflow) - disagreement between the nest-host and this class about nest membership I just noticed that there is a wording error in getNestHost() as it states: "If there is any error accessing the nest host, ... then this is returned." but in the implementation that's only true for LinkageErrors. Runtime errors get propagated. I think this is what we want (other places in MH runtime where runtime-errors were getting swallowed or converted have since been switched back to propagate them.) But I'll need to run this past the EG to check. That notwithstanding, exactly what would you propose here? The current spec is clear and concise and precise, but defers to getNestHost somewhat for details on what happens when things go wrong.
The tests for core reflection are in test/jdk/java/lang/reflect and I'm wondering if there has been any discussion about adding at least some basic tests there? I see the tests in test/hotspot/jtreg/runtime/Nestmates/reflectionAPI but this isn't an obvious location for someone touching this code.
Good catch! It was always my intent to redistribute the new nestmate tests into their component areas where applicable and appropriate - but I forgot to do that before calling for the RFR. I will move them to java/lang/reflect/Nestmates.
The test in InterfaceAccessFlagsTest.java can be disabled with @Test(enabled=false), might be cleaner than commenting it out.
Thanks - will fix.
Maybe a question for Kumar but are we planning to pull in any ASM updates for JDK 11? NestMembers extends Attribute looks okay, I'm less sure about the change to ClassReader as I don't know if there is somewhere else in ASM that has the list of attributes to always parse.
Remi already responded on this. Seems what we have now is okay but eventually it will have to be reconciled when we update from upstream ASM.
The update to the access check in Reflection.java looks okay (just need to use 4-space indent, not 2).
Will fix the indent - thanks for spotting.
I think it's okay in VerifyAccess too but the "FIX ME" suggests there are doubts so I assume this needs more eyes to triple check.
The "FIX ME" is not about the access check but the form of the assertion. The assert is verification that resolution of a private method always leads to selection of that private method: refc == defc. I used "myassert" so that this check was always enabled during testing. The "FIX ME" was to either convert to language assert statement or else remove it (having validated that it never fires). Obviously "myassert" has not fired in all the testing that I have done so either choice seems fine. Do you have a preference?
In passing, you might want to double check the javadoc links. I noticed one #getNestHost that is missing parenthesis and I'm not sure how javadoc handles that.
Should have generated a warning so I'll fix that - thanks. Thanks again for the review. David
-Alan.
On 05/21/2018 07:57 AM, David Holmes wrote:
Do we really need to spell out the case for primitives and arrays? If so would it suffice to add the following:
"A class or interface that is not explicitly a member of a nest *(such as primitive or array classes)*, is a member of the nest consisting only of itself, and is the nest host. Every class and interface is a member of exactly one nest."
That's fine, but then someone might wonder whether only primitive and array classes are not explicit members of a nest. What about: "A class or interface that is not explicitly a member of a nest *(including but not limited to primitive and array classes)*, is a member of the nest consisting only of itself, and is the nest host. Every class and interface is a member of exactly one nest." Regards, Peter
Hi Peter, On 21/05/2018 4:12 PM, Peter Levart wrote:
On 05/21/2018 07:57 AM, David Holmes wrote:
Do we really need to spell out the case for primitives and arrays? If so would it suffice to add the following:
"A class or interface that is not explicitly a member of a nest *(such as primitive or array classes)*, is a member of the nest consisting only of itself, and is the nest host. Every class and interface is a member of exactly one nest."
That's fine, but then someone might wonder whether only primitive and array classes are not explicit members of a nest.
What about:
"A class or interface that is not explicitly a member of a nest *(including but not limited to primitive and array classes)*, is a member of the nest consisting only of itself, and is the nest host. Every class and interface is a member of exactly one nest."
But "including but not limited to" is just a more verbose way of saying "such as". "such as x" introduces an example case x, hence "including x", but is not exhaustive and so covers "not limited to x". David -----
Regards, Peter
On this topic ... On 21/05/2018 4:48 PM, David Holmes wrote:
Hi Peter,
On 21/05/2018 4:12 PM, Peter Levart wrote:
On 05/21/2018 07:57 AM, David Holmes wrote:
Do we really need to spell out the case for primitives and arrays? If
May I add that getEnclosingClass() doesn't explain that primitive and array classes are considered top-level classes. Similarly getNestHost() doesn't explain that they are not explicitly nested classes. Note: it was deliberate to not phrase this in terms of top-level classes in case in the future we want to expand the notion of nestmates further (such as "all classes defined in the same compilation unit form a nest"). Thanks, David -----
so would it suffice to add the following:
"A class or interface that is not explicitly a member of a nest *(such as primitive or array classes)*, is a member of the nest consisting only of itself, and is the nest host. Every class and interface is a member of exactly one nest."
That's fine, but then someone might wonder whether only primitive and array classes are not explicit members of a nest.
What about:
"A class or interface that is not explicitly a member of a nest *(including but not limited to primitive and array classes)*, is a member of the nest consisting only of itself, and is the nest host. Every class and interface is a member of exactly one nest."
But "including but not limited to" is just a more verbose way of saying "such as". "such as x" introduces an example case x, hence "including x", but is not exhaustive and so covers "not limited to x".
David -----
Regards, Peter
On 21/05/2018 06:57, David Holmes wrote:
:
- Can Class::getNestMembers specify if it runs the class initializer of any classes that it loads? Less of an issue for getNestHost as the host is likely loaded already.
None of these methods specifically or intentionally initialize any classes. A class will be loaded if needed but that is all. The only Class methods that refer to class initialization are the forName() variants. I don't see that the nest related methods should do any more or less in this regard than any other Class methods that may require classes to be loaded. It returns an array of Class objects so it's potentially loading and linking a number of classes, a bulk Class.forName to some extent. I don't think it does any harm to make it clear that it does not run their class initializers. There are other methods where this could be clearer too but that is outside of this feature.
-Alan
On 21/05/2018 5:09 PM, Alan Bateman wrote:
On 21/05/2018 06:57, David Holmes wrote:
:
- Can Class::getNestMembers specify if it runs the class initializer of any classes that it loads? Less of an issue for getNestHost as the host is likely loaded already.
None of these methods specifically or intentionally initialize any classes. A class will be loaded if needed but that is all. The only Class methods that refer to class initialization are the forName() variants. I don't see that the nest related methods should do any more or less in this regard than any other Class methods that may require classes to be loaded. It returns an array of Class objects so it's potentially loading and linking a number of classes, a bulk Class.forName to some extent. I don't think it does any harm to make it clear that it does not run their class initializers. There are other methods where this could be clearer too but that is outside of this feature.
If we're going to raise the bar here then I would prefer to do that to all methods at a later time rather then requiring these new methods to set a new standard. getClasses(), getDeclaredClass(), getInterface() etc all return arrays of Class objects (the same ones as getNestMembers() in some cases!) without saying anything about loading versus linking versus initialization. The new API was modelled on the existing ones. Thanks, David -----
-Alan
On 21/05/2018 06:57, David Holmes wrote:
:
- I suspect the @throws SecurityException in getNestMembers was copied from getNestHost as it uses "returned class" (singular).
It refers to "If any returned class ..." and "that returned class". I don't see any problematic singular uses - can you elaborate please. I see your point but it's hard to read. An alternative would be to make it clear there is a permission check when the nest includes classes other than the nest host (rather than "current class").
BTW: Paul brought up the hazard of repeated classes. It it possible to have the nest host repeated here? If so then the implementation will do a permission check even if there are no other members. -Alan
On 21/05/2018 5:20 PM, Alan Bateman wrote:
On 21/05/2018 06:57, David Holmes wrote:
:
- I suspect the @throws SecurityException in getNestMembers was copied from getNestHost as it uses "returned class" (singular).
It refers to "If any returned class ..." and "that returned class". I don't see any problematic singular uses - can you elaborate please. I see your point but it's hard to read. An alternative would be to make it clear there is a permission check when the nest includes classes other than the nest host (rather than "current class").
But it is the current class that determines the need for the access check, so I'm not sure what you mean exactly. ?? The condition: + if (members.length > 1) { + // If we return anything other than the current class we need + // a security check is an optimization suggested by John based on the fact that otherwise we're going to hand back the object the caller already has, so there is nothing to be gained by having an additional security check (the check is to ensure we don't hand back something the caller shouldn't have access to - even then I strain to see how this is possible). Here's the original review thread: http://mail.openjdk.java.net/pipermail/valhalla-dev/2018-March/003955.html
BTW: Paul brought up the hazard of repeated classes. It it possible to have the nest host repeated here? If so then the implementation will do a permission check even if there are no other members.
I wouldn't call it a "hazard" as such. The spec allows it so that the implementation doesn't have to waste time verifying it for the 99.9999999% of well formed code. So yes it could have the host in twice and yes that would trigger the check - but would that matter? Thanks, David
-Alan
On 21/05/2018 06:57, David Holmes wrote:
:
- Can the spec for Class::getNestHost be explicit that it returns "this" when the class is a primitive or array class?
This is covered by:
"A class or interface that is not explicitly a member of a nest, is a member of the nest consisting only of itself, and is the nest host. Every class and interface is a member of exactly one nest."
Do we really need to spell out the case for primitives and arrays? If so would it suffice to add the following:
"A class or interface that is not explicitly a member of a nest *(such as primitive or array classes)*, is a member of the nest consisting only of itself, and is the nest host. Every class and interface is a member of exactly one nest." My comment was actually about the @return but expanding the method description with the above would be clearer.
BTW: In the @return it currently has " ... or this if we cannot obtain a valid nest host". Will the "we" part be re-worded? -Alan
On 21/05/2018 5:55 PM, Alan Bateman wrote:
On 21/05/2018 06:57, David Holmes wrote:
:
- Can the spec for Class::getNestHost be explicit that it returns "this" when the class is a primitive or array class?
This is covered by:
"A class or interface that is not explicitly a member of a nest, is a member of the nest consisting only of itself, and is the nest host. Every class and interface is a member of exactly one nest."
Do we really need to spell out the case for primitives and arrays? If so would it suffice to add the following:
"A class or interface that is not explicitly a member of a nest *(such as primitive or array classes)*, is a member of the nest consisting only of itself, and is the nest host. Every class and interface is a member of exactly one nest." My comment was actually about the @return but expanding the method description with the above would be clearer.
Okay I can propose that.
BTW: In the @return it currently has " ... or this if we cannot obtain a valid nest host". Will the "we" part be re-worded?
"we" hadn't planned too :) but we can do so. Thanks, David
-Alan
On 5/20/18 10:57 PM, David Holmes wrote:
- I suspect the @throws SecurityException in getNestMembers was copied from getNestHost as it uses "returned class" (singular).
It refers to "If any returned class ..." and "that returned class". I don't see any problematic singular uses - can you elaborate please.
As the host and members are in the same runtime package then maybe it can be specified in terms of the host or members package?
I'm not sure how to accurately formulate that. The current wording was based on similar @throws in getEnclosingClass, as suggested by Mandy:
http://mail.openjdk.java.net/pipermail/valhalla-dev/2018-March/003955.html
and then refined a little.
@throws SecurityException in my version suggested to refer to "the current class". I see your version referring to "the returned class" which is what Alan commented on. getNestMembers returns more than one class. What about: @throws SecurityException if this class is not in the nest of itself, and if a security manager, <i>s</i>, is present and the caller'sclass loader is not the same as or an ancestor of the nest of thisclass and invocation of {@linkSecurityManager#checkPackageAccess s.checkPackageAccess()}denies access to the package of the nest of class. The above can apply to both getNestHost and getNestMembers. The javadoc can also explicitly state that "Classes in the same nest, i.e. nest host and nest members, are in the same runtime package." Mandy
Hi Mandy, Thanks for taking another look at this! On 22/05/2018 7:47 AM, mandy chung wrote:
On 5/20/18 10:57 PM, David Holmes wrote:
- I suspect the @throws SecurityException in getNestMembers was copied from getNestHost as it uses "returned class" (singular).
It refers to "If any returned class ..." and "that returned class". I don't see any problematic singular uses - can you elaborate please.
As the host and members are in the same runtime package then maybe it can be specified in terms of the host or members package?
I'm not sure how to accurately formulate that. The current wording was based on similar @throws in getEnclosingClass, as suggested by Mandy:
http://mail.openjdk.java.net/pipermail/valhalla-dev/2018-March/003955.html
and then refined a little.
@throws SecurityException in my version suggested to refer to "the current class". I see your version referring to "the returned class" which is what Alan commented on. getNestMembers returns more than one class.
The original copied wording was simply: ! * @throws SecurityException ! * If a security manager, <i>s</i>, is present and the caller's ! * class loader is not the same as or an ancestor of the class ! * loader for the current class and invocation of {@link ! * SecurityManager#checkPackageAccess s.checkPackageAccess()} ! * denies access to the package of the current class http://cr.openjdk.java.net/~dholmes/8199309/webrev/src/java.base/share/class... then John pointed out that we don't need to do a security check if returning the current class. Hence it was updated to the present wording to exclude that case. This was discussed in the review on valhalla-dev and you gave your okay to it then: http://mail.openjdk.java.net/pipermail/valhalla-dev/2018-March/003971.html and as I responded to Alan, for getNestMembers() it doesn't say "the returned class" it says "any returned class" and "that returned class". There is no singular/plural ambiguity.
What about: @throws SecurityException if this class is not in the nest of itself,
I think you mean if the class is in a nest consisting solely of itself? But that seems convoluted to me regardless. The existing statements are extremely clear IMHO: getNestHost: "If the returned class is not the current class ..." getNestMembers: "If any returned class is not the current class ..." I don't see how they can be misinterpreted. ???
and if a security manager, <i>s</i>, is present and the caller'sclass loader is not the same as or an ancestor of the nest of thisclass and
Something not right there - you're comparing a classloader with a nest ??
invocation of {@linkSecurityManager#checkPackageAccess s.checkPackageAccess()}denies access to the package of the nest of class.
I would not want to refer to the "package of the nest".
The above can apply to both getNestHost and getNestMembers.
The javadoc can also explicitly state that "Classes in the same nest, i.e. nest host and nest members, are in the same runtime package."
How is this adjustment in getNestHost (which is the only place where we explain nests): * <p>A <em>nest</em> is a set of classes and interfaces (nestmates) that * form an access control context in which each nestmate has access to the * private members of the other nestmates. * The <em>nest host</em> is the class or interface designated to hold the list of * classes and interfaces that make up the nest, and to which each of the * other nestmates refer. +* All nestmates are implicitly defined in the same runtime package. ? Thanks, David -----
Mandy
On May 21, 2018, at 5:48 PM, David Holmes <david.holmes@oracle.com> wrote:
* <p>A <em>nest</em> is a set of classes and interfaces (nestmates) that * form an access control context in which each nestmate has access to the * private members of the other nestmates. * The <em>nest host</em> is the class or interface designated to hold the list of * classes and interfaces that make up the nest, and to which each of the * other nestmates refer. +* All nestmates are implicitly defined in the same runtime package.
FWIW I agree that this would be a fine addition. Note that this is not something the API enforces; the JVM requires, as a structural constraint, that the various nestmates all share a common package prefix. Pointing this out in the reflection API doc is useful but not strictly required; it won't affect compliance or dictate behavior not already specified (indirectly) elsewhere. So all of the above text is descriptive and not the primary normative spec. of how nestmates work; that normative spec. is in the JVMS, not javadoc. For this reason, it's reasonable and low-risk to add clarifying text like this; it doesn't change behavior, so much as help users understand it. When I've written stuff like this in the past, sometimes I make the link explicit, by saying something like "…as stated in the JVMS, this API produces…". Then we have a little classroom session about the implications of the JVMS. Not normative, but illuminating. In fact, making the spec. more user friendly is a good thing to pay attention to in the implementation review, even after is has passed the CCC and spec. expert reviews, although I would expect those earlier reviews to produce a reasonably user-friendly result. That said, such things can be added later, and I deeply sympathize with David's "immutable object" stance. Also (in a different way) with him having to field documentation RFEs that are equally applicable across pre-existing APIs. I've noticed that when we really dig into our specs. at new points, we suddenly realize that they could have been much better all along. I propose we note this and file a tracking bug, rather than wedge the improvements into new spec. only. An *implementation* code review should not, in general, reopen spec. issues that have already been reviewed. If I were David in this case, I'd feel like some of these review points are making about the same progress as a revolving door does. I don't know what the full fix is, but part of it has to be agreeing to partition the problem and work on one part at a time. Which we do, of course, as a habit, but it seems some highly interconnected features like nestmates attract expansive reviews. We could say "well, it's a fundamental JVM change" but does that mean we should also so 100x times this expansive review process for value types? If so, it's going to take longer than any of us want. And, for smaller RFEs like nestmates, it's going to be hard doing the next several. I hope the next few will get through the various review gauntlets with more ease. I'm thinking specifically of sealed classes (and maybe fields), final-means-final, lazy fields, and frozen arrays. All of these are, like nestmates, smaller features that cut across many layers of the system, and so will need multiple facets of review. If any (or all) of those happen, let's learn from this experience and do even better next time. I'm not unhappy with this experience, but want to do even better. Buy David a beer to get his viewpoint. And, thank you to everyone who has put eyeballs on these reviews; they are crucial to getting a good outcome. — John
On 5/21/18 5:48 PM, David Holmes wrote:
http://mail.openjdk.java.net/pipermail/valhalla-dev/2018-March/003971.html
and as I responded to Alan, for getNestMembers() it doesn't say "the returned class" it says "any returned class" and "that returned class". There is no singular/plural ambiguity.
Ah. I was not able to connect "any returned class" and "that returned class" in my first read. I now see "that returned class" refers to one class.
What about: @throws SecurityException if this class is not in the nest of itself,
I think you mean if the class is in a nest consisting solely of itself?
Yes.
But that seems convoluted to me regardless. The existing statements are extremely clear IMHO:
getNestHost: "If the returned class is not the current class ..."
getNestMembers: "If any returned class is not the current class ..."
I don't see how they can be misinterpreted. ???
I don't think it can be misinterpreted but just shared my thought in possible clarification in the wording (looks like not helping).
and if a security manager, <i>s</i>, is present and the caller'sclass loader is not the same as or an ancestor of the nest of thisclass and
Something not right there - you're comparing a classloader with a nest ??
grammatical error - the class loader loading the classes in the nest.
invocation of {@linkSecurityManager#checkPackageAccess s.checkPackageAccess()}denies access to the package of the nest of class.
I would not want to refer to the "package of the nest".
Right that's probably the not best.
The above can apply to both getNestHost and getNestMembers.
The javadoc can also explicitly state that "Classes in the same nest, i.e. nest host and nest members, are in the same runtime package."
How is this adjustment in getNestHost (which is the only place where we explain nests):
* <p>A <em>nest</em> is a set of classes and interfaces (nestmates) that * form an access control context in which each nestmate has access to the * private members of the other nestmates. * The <em>nest host</em> is the class or interface designated to hold the list of * classes and interfaces that make up the nest, and to which each of the * other nestmates refer. +* All nestmates are implicitly defined in the same runtime package.
This is good. Thanks. Mandy
Thanks Mandy. I'll make the addition regarding the package - as endorsed by John (thanks John!) David On 22/05/2018 11:32 AM, mandy chung wrote:
On 5/21/18 5:48 PM, David Holmes wrote:
http://mail.openjdk.java.net/pipermail/valhalla-dev/2018-March/003971.html
and as I responded to Alan, for getNestMembers() it doesn't say "the returned class" it says "any returned class" and "that returned class". There is no singular/plural ambiguity.
Ah. I was not able to connect "any returned class" and "that returned class" in my first read. I now see "that returned class" refers to one class.
What about: @throws SecurityException if this class is not in the nest of itself,
I think you mean if the class is in a nest consisting solely of itself?
Yes.
But that seems convoluted to me regardless. The existing statements are extremely clear IMHO:
getNestHost: "If the returned class is not the current class ..."
getNestMembers: "If any returned class is not the current class ..."
I don't see how they can be misinterpreted. ???
I don't think it can be misinterpreted but just shared my thought in possible clarification in the wording (looks like not helping).
and if a security manager, <i>s</i>, is present and the caller'sclass loader is not the same as or an ancestor of the nest of thisclass and
Something not right there - you're comparing a classloader with a nest ??
grammatical error - the class loader loading the classes in the nest.
invocation of {@linkSecurityManager#checkPackageAccess s.checkPackageAccess()}denies access to the package of the nest of class.
I would not want to refer to the "package of the nest".
Right that's probably the not best.
The above can apply to both getNestHost and getNestMembers.
The javadoc can also explicitly state that "Classes in the same nest, i.e. nest host and nest members, are in the same runtime package."
How is this adjustment in getNestHost (which is the only place where we explain nests):
* <p>A <em>nest</em> is a set of classes and interfaces (nestmates) that * form an access control context in which each nestmate has access to the * private members of the other nestmates. * The <em>nest host</em> is the class or interface designated to hold the list of * classes and interfaces that make up the nest, and to which each of the * other nestmates refer. +* All nestmates are implicitly defined in the same runtime package.
This is good. Thanks.
Mandy
Hi David, I reviewed: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/ Looks good in general. I have a couple other comments. I will review the new version that includes new tests in test/jdk/java/lang/reflect when it's ready. Class.java 3988 Class<?>[] members = getNestMembers0(); If the above fails with any LinkageError, what is the expected behavior if Class::getNestMembers() is called the second time? will it throw the same LinkageError? On 5/20/18 10:57 PM, David Holmes wrote:
I think it's okay in VerifyAccess too but the "FIX ME" suggests there are doubts so I assume this needs more eyes to triple check.
The "FIX ME" is not about the access check but the form of the assertion. The assert is verification that resolution of a private method always leads to selection of that private method: refc == defc. I used "myassert" so that this check was always enabled during testing. The "FIX ME" was to either convert to language assert statement or else remove it (having validated that it never fires). Obviously "myassert" has not fired in all the testing that I have done so either choice seems fine. Do you have a preference?
Since you have the assert, I would convert it to language assert that would help the readers to understand the result of such resolution. Regarding Alan's and Peter's comment on the spec clarification on primitive type and array type, it is reasonable to take a pass on all reflection APIs and make the text explicit consistently if this class is a primitive type and array type. I will file an issue to track this. Class::getModifiers does return the modifier of the component type whereas Class::getEnclosingClass returns null if this class is an array type even if the component type has an enclosing class. Clarification would help developers. Mandy
Hi Mandy, On 22/05/2018 1:23 PM, mandy chung wrote:
Hi David,
I reviewed: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/
Looks good in general. I have a couple other comments. I will review the new version that includes new tests in test/jdk/java/lang/reflect when it's ready.
Thanks! That should be out later today.
Class.java
3988 Class<?>[] members = getNestMembers0();
If the above fails with any LinkageError, what is the expected behavior if Class::getNestMembers() is called the second time? will it throw the same LinkageError?
Based on the implementation it should throw the same error. We use constant-pool resolution to locate the member classes so if resolution fails then it must continue to fail for the same reason per the JVMS rules. Are you thinking that getNestMembers() should say something about this?
On 5/20/18 10:57 PM, David Holmes wrote:
I think it's okay in VerifyAccess too but the "FIX ME" suggests there are doubts so I assume this needs more eyes to triple check.
The "FIX ME" is not about the access check but the form of the assertion. The assert is verification that resolution of a private method always leads to selection of that private method: refc == defc. I used "myassert" so that this check was always enabled during testing. The "FIX ME" was to either convert to language assert statement or else remove it (having validated that it never fires). Obviously "myassert" has not fired in all the testing that I have done so either choice seems fine. Do you have a preference?
Since you have the assert, I would convert it to language assert that would help the readers to understand the result of such resolution.
Will do.
Regarding Alan's and Peter's comment on the spec clarification on primitive type and array type, it is reasonable to take a pass on all reflection APIs and make the text explicit consistently if this class is a primitive type and array type. I will file an issue to track this. Class::getModifiers does return the modifier of the component type whereas Class::getEnclosingClass returns null if this class is an array type even if the component type has an enclosing class. Clarification would help developers.
Thanks. I will add the small clarification that Alan requested on getNestHost(). Thanks, David
Mandy
On 5/21/18 8:41 PM, David Holmes wrote:
Class.java
3988 Class<?>[] members = getNestMembers0();
If the above fails with any LinkageError, what is the expected behavior if Class::getNestMembers() is called the second time? will it throw the same LinkageError?
Based on the implementation it should throw the same error. We use constant-pool resolution to locate the member classes so if resolution fails then it must continue to fail for the same reason per the JVMS rules.
Are you thinking that getNestMembers() should say something about this?
That matches what I interpret from the spec. I just want to confirm. It'd be good to have a test to cover that.
Regarding Alan's and Peter's comment on the spec clarification on primitive type and array type, it is reasonable to take a pass on all reflection APIs and make the text explicit consistently if this class is a primitive type and array type. I will file an issue to track this. Class::getModifiers does return the modifier of the component type whereas Class::getEnclosingClass returns null if this class is an array type even if the component type has an enclosing class. Clarification would help developers.
Thanks. I will add the small clarification that Alan requested on getNestHost().
That works too. thanks Mandy
Hi Mandy, On 22/05/2018 2:14 PM, mandy chung wrote:
On 5/21/18 8:41 PM, David Holmes wrote:
Class.java
3988 Class<?>[] members = getNestMembers0();
If the above fails with any LinkageError, what is the expected behavior if Class::getNestMembers() is called the second time? will it throw the same LinkageError?
Based on the implementation it should throw the same error. We use constant-pool resolution to locate the member classes so if resolution fails then it must continue to fail for the same reason per the JVMS rules.
Are you thinking that getNestMembers() should say something about this?
That matches what I interpret from the spec. I just want to confirm.
Okay.
It'd be good to have a test to cover that.
I can add a short loop to the negative testcases for this.
Regarding Alan's and Peter's comment on the spec clarification on primitive type and array type, it is reasonable to take a pass on all reflection APIs and make the text explicit consistently if this class is a primitive type and array type. I will file an issue to track this. Class::getModifiers does return the modifier of the component type whereas Class::getEnclosingClass returns null if this class is an array type even if the component type has an enclosing class. Clarification would help developers.
Thanks. I will add the small clarification that Alan requested on getNestHost().
That works too.
Ok. Stay tuned. Thanks, David -----
thanks Mandy
HI, Nice thorough work on this, surprisingly tricky in some areas esp. MHs. Class — 3857 * <p>If there is any error accessing the nest host, or the nest host is 3858 * in any way invalid, then {@code this} is returned. I am curious under what conditions this can arise. As a caller this makes me nervous :-) Are there conditions that are worth calling it. This is related to Alan’s comment about primitive or array classes. Its clearer when looking at the implementation: primitive/array classes, linkage error, and the more general nest membership validation. 3883 * @throws SecurityException 3884 * If the returned class is not the current class, and 3885 * if a security manager, <i>s</i>, is present and the caller's 3886 * class loader is not the same as or an ancestor of the class 3887 * loader for the returned class and invocation of {@link 3888 * SecurityManager#checkPackageAccess s.checkPackageAccess()} 3889 * denies access to the package of the returned class 3890 * @since 11 3891 * @jvms 4.7.28 and 4.7.29 NestHost and NestMembers attributes 3892 */ 3893 @CallerSensitive 3894 public Class<?> getNestHost() { Maybe i need more coffee, but I am struggling to see in the implementation the checks for the case of "and the caller’s class loader is not the same as or an ancestor of the class loader for the returned class”. Is it implied that all classes in the nest have to be loaded from the same or from a common ancestor class loader? so you only need to check one class in the nest against the calling class. 3984 public Class<?>[] getNestMembers() { I still think not removing dups is a mistake as it could be a source of subtle bugs. But i doubt at this point i can persuade you or others to change it :-) 3989 // Can't actually enable this due to bootstrapping issues 3990 // assert(members.length != 1 || members[0] == this); // expected invariant from VM That's interesting and frustrating! Reflection.java — 146 // Check for nestmate access if member is private 147 if (Modifier.isPrivate(modifiers)) { 148 // assert: isSubclassof(targetClass, memberClass) 149 // Note: targetClass may be outside the nest, but that is okay 150 // as long as memberClass is in the nest. 151 boolean nestmates = areNestMates(currentClass, memberClass); 152 if (nestmates) { 153 return true; 154 } 155 } Trivially, you don’t need the local variable “nestmates”. VerifyAccess.java — 134 case PRIVATE: 135 // Rules for privates follows access rules for nestmates. 136 boolean canAccess = ((allowedModes & PRIVATE) != 0 && 137 Reflection.areNestMates(defc, lookupClass)); 138 // FIX ME: Sanity check refc == defc. Either remove or convert to 139 // plain assert before integration. 140 myassert((canAccess && refc == defc) || !canAccess); 141 return canAccess; 142 default: 143 throw new IllegalArgumentException("bad modifiers: "+Modifier.toString(mods)); 144 } 145 } 146 static void myassert(boolean cond) { 147 if (!cond) throw new Error("Assertion failed"); 148 } Do you plan to chase up the FIX ME now or later? I agree with Alan about the current location of the reflection API tests. If these tests don’t need to be hammered with various and exotic HotSpot flags i think they are better placed to be under test/jdk/java/lang/reflect. Thanks, Paul.
On May 14, 2018, at 5:52 PM, David Holmes <david.holmes@oracle.com> wrote:
This review is being spread across four groups: langtools, core-libs, hotspot and serviceability. This is the specific review thread for core-libs - webrev:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/
See below for full details - including annotated full webrev guiding the review.
The intent is to have JEP-181 targeted and integrated by the end of this month.
Thanks, David -----
The nestmates project (JEP-181) introduces new classfile attributes to identify classes and interfaces in the same nest, so that the VM can perform access control based on those attributes and so allow direct private access between nestmates without requiring javac to generate synthetic accessor methods. These access control changes also extend to core reflection and the MethodHandle.Lookup contexts.
Direct private calls between nestmates requires a more general calling context than is permitted by invokespecial, and so the JVMS is updated to allow, and javac updated to use, invokevirtual and invokeinterface for private class and interface method calls respectively. These changed semantics also extend to MethodHandle findXXX operations.
At this time we are only concerned with static nest definitions, which map to a top-level class/interface as the nest-host and all its nested types as nest-members.
Please see the JEP for further details.
JEP: https://bugs.openjdk.java.net/browse/JDK-8046171 Bug: https://bugs.openjdk.java.net/browse/JDK-8010319 CSR: https://bugs.openjdk.java.net/browse/JDK-8197445
All of the specification changes have been previously been worked out by the Valhalla Project Expert Group, and the implementation reviewed by the various contributors and discussed on the valhalla-dev mailing list.
Acknowledgments and contributions: Alex Buckley, Maurizio Cimadamore, Mandy Chung, Tobias Hartmann, Vladimir Ivanov, Karen Kinnear, Vladimir Kozlov, John Rose, Dan Smith, Serguei Spitsyn, Kumar Srinivasan
Master webrev of all changes:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.full.v1/
Annotated master webrev index:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/jep181-webrev.html
Performance: this is expected to be performance neutral in a general sense. Benchmarking and performance runs are about to start.
Testing Discussion: ------------------
The testing for nestmates can be broken into four main groups:
- New tests specifically related to nestmates and currently in the runtime/Nestmates directory
- New tests to complement existing tests by adding in testcases not previously expressible. - For example java/lang/invoke/SpecialInterfaceCall.java tests use of invokespecial for private interface methods and performing receiver typechecks, so we add java/lang/invoke/PrivateInterfaceCall.java to do similar tests for invokeinterface.
- New JVM TI tests to verify the spec changes related to nest attributes.
- Existing tests significantly affected by the nestmates changes, primarily: - runtime/SelectionResolution
In most cases the nestmate changes makes certain invocations that were illegal, legal (e.g. not requiring invokespecial to invoke private interface methods; allowing access to private members via reflection/Methodhandles that were previously not allowed).
- Existing tests incidentally affected by the nestmate changes
This includes tests of things utilising class redefinition/retransformation to alter nested types but which unintentionally alter nest relationships (which is not permitted).
There are still a number of tests problem-listed with issues filed against them to have them adapted to work with nestmates. Some of these are intended to be addressed in the short-term, while some (such as the runtime/SelectionResolution test changes) may not eventuate.
- https://bugs.openjdk.java.net/browse/JDK-8203033 - https://bugs.openjdk.java.net/browse/JDK-8199450 - https://bugs.openjdk.java.net/browse/JDK-8196855 - https://bugs.openjdk.java.net/browse/JDK-8194857 - https://bugs.openjdk.java.net/browse/JDK-8187655
There is also further test work still to be completed (the JNI and JDI invocation tests): - https://bugs.openjdk.java.net/browse/JDK-8191117 which will continue in parallel with the main RFR.
Pre-integration Testing: - General: - Mach5: hs/jdk tier1,2 - Mach5: hs-nightly (tiers 1 -3) - Targetted - nashorn (for asm changes) - hotspot: runtime/* serviceability/* compiler/* vmTestbase/* - jdk: java/lang/invoke/* java/lang/reflect/* java/lang/instrument/* java/lang/Class/* java/lang/management/* - langtools: tools/javac tools/javap
Paul, Alan, Just a quick thank you for taking a look at this so soon. I will respond to both of you as soon as practical. Thanks, David On 17/05/2018 4:00 AM, Paul Sandoz wrote:
HI,
Nice thorough work on this, surprisingly tricky in some areas esp. MHs.
Class —
3857 * <p>If there is any error accessing the nest host, or the nest host is 3858 * in any way invalid, then {@code this} is returned.
I am curious under what conditions this can arise. As a caller this makes me nervous :-) Are there conditions that are worth calling it. This is related to Alan’s comment about primitive or array classes. Its clearer when looking at the implementation: primitive/array classes, linkage error, and the more general nest membership validation.
3883 * @throws SecurityException 3884 * If the returned class is not the current class, and 3885 * if a security manager, <i>s</i>, is present and the caller's 3886 * class loader is not the same as or an ancestor of the class 3887 * loader for the returned class and invocation of {@link 3888 * SecurityManager#checkPackageAccess s.checkPackageAccess()} 3889 * denies access to the package of the returned class 3890 * @since 11 3891 * @jvms 4.7.28 and 4.7.29 NestHost and NestMembers attributes 3892 */ 3893 @CallerSensitive 3894 public Class<?> getNestHost() {
Maybe i need more coffee, but I am struggling to see in the implementation the checks for the case of "and the caller’s class loader is not the same as or an ancestor of the class loader for the returned class”. Is it implied that all classes in the nest have to be loaded from the same or from a common ancestor class loader? so you only need to check one class in the nest against the calling class.
3984 public Class<?>[] getNestMembers() {
I still think not removing dups is a mistake as it could be a source of subtle bugs. But i doubt at this point i can persuade you or others to change it :-)
3989 // Can't actually enable this due to bootstrapping issues 3990 // assert(members.length != 1 || members[0] == this); // expected invariant from VM
That's interesting and frustrating!
Reflection.java —
146 // Check for nestmate access if member is private 147 if (Modifier.isPrivate(modifiers)) { 148 // assert: isSubclassof(targetClass, memberClass) 149 // Note: targetClass may be outside the nest, but that is okay 150 // as long as memberClass is in the nest. 151 boolean nestmates = areNestMates(currentClass, memberClass); 152 if (nestmates) { 153 return true; 154 } 155 }
Trivially, you don’t need the local variable “nestmates”.
VerifyAccess.java —
134 case PRIVATE: 135 // Rules for privates follows access rules for nestmates. 136 boolean canAccess = ((allowedModes & PRIVATE) != 0 && 137 Reflection.areNestMates(defc, lookupClass)); 138 // FIX ME: Sanity check refc == defc. Either remove or convert to 139 // plain assert before integration. 140 myassert((canAccess && refc == defc) || !canAccess); 141 return canAccess; 142 default: 143 throw new IllegalArgumentException("bad modifiers: "+Modifier.toString(mods)); 144 } 145 } 146 static void myassert(boolean cond) { 147 if (!cond) throw new Error("Assertion failed"); 148 }
Do you plan to chase up the FIX ME now or later?
I agree with Alan about the current location of the reflection API tests. If these tests don’t need to be hammered with various and exotic HotSpot flags i think they are better placed to be under test/jdk/java/lang/reflect.
Thanks, Paul.
On May 14, 2018, at 5:52 PM, David Holmes <david.holmes@oracle.com> wrote:
This review is being spread across four groups: langtools, core-libs, hotspot and serviceability. This is the specific review thread for core-libs - webrev:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/
See below for full details - including annotated full webrev guiding the review.
The intent is to have JEP-181 targeted and integrated by the end of this month.
Thanks, David -----
The nestmates project (JEP-181) introduces new classfile attributes to identify classes and interfaces in the same nest, so that the VM can perform access control based on those attributes and so allow direct private access between nestmates without requiring javac to generate synthetic accessor methods. These access control changes also extend to core reflection and the MethodHandle.Lookup contexts.
Direct private calls between nestmates requires a more general calling context than is permitted by invokespecial, and so the JVMS is updated to allow, and javac updated to use, invokevirtual and invokeinterface for private class and interface method calls respectively. These changed semantics also extend to MethodHandle findXXX operations.
At this time we are only concerned with static nest definitions, which map to a top-level class/interface as the nest-host and all its nested types as nest-members.
Please see the JEP for further details.
JEP: https://bugs.openjdk.java.net/browse/JDK-8046171 Bug: https://bugs.openjdk.java.net/browse/JDK-8010319 CSR: https://bugs.openjdk.java.net/browse/JDK-8197445
All of the specification changes have been previously been worked out by the Valhalla Project Expert Group, and the implementation reviewed by the various contributors and discussed on the valhalla-dev mailing list.
Acknowledgments and contributions: Alex Buckley, Maurizio Cimadamore, Mandy Chung, Tobias Hartmann, Vladimir Ivanov, Karen Kinnear, Vladimir Kozlov, John Rose, Dan Smith, Serguei Spitsyn, Kumar Srinivasan
Master webrev of all changes:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.full.v1/
Annotated master webrev index:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/jep181-webrev.html
Performance: this is expected to be performance neutral in a general sense. Benchmarking and performance runs are about to start.
Testing Discussion: ------------------
The testing for nestmates can be broken into four main groups:
- New tests specifically related to nestmates and currently in the runtime/Nestmates directory
- New tests to complement existing tests by adding in testcases not previously expressible. - For example java/lang/invoke/SpecialInterfaceCall.java tests use of invokespecial for private interface methods and performing receiver typechecks, so we add java/lang/invoke/PrivateInterfaceCall.java to do similar tests for invokeinterface.
- New JVM TI tests to verify the spec changes related to nest attributes.
- Existing tests significantly affected by the nestmates changes, primarily: - runtime/SelectionResolution
In most cases the nestmate changes makes certain invocations that were illegal, legal (e.g. not requiring invokespecial to invoke private interface methods; allowing access to private members via reflection/Methodhandles that were previously not allowed).
- Existing tests incidentally affected by the nestmate changes
This includes tests of things utilising class redefinition/retransformation to alter nested types but which unintentionally alter nest relationships (which is not permitted).
There are still a number of tests problem-listed with issues filed against them to have them adapted to work with nestmates. Some of these are intended to be addressed in the short-term, while some (such as the runtime/SelectionResolution test changes) may not eventuate.
- https://bugs.openjdk.java.net/browse/JDK-8203033 - https://bugs.openjdk.java.net/browse/JDK-8199450 - https://bugs.openjdk.java.net/browse/JDK-8196855 - https://bugs.openjdk.java.net/browse/JDK-8194857 - https://bugs.openjdk.java.net/browse/JDK-8187655
There is also further test work still to be completed (the JNI and JDI invocation tests): - https://bugs.openjdk.java.net/browse/JDK-8191117 which will continue in parallel with the main RFR.
Pre-integration Testing: - General: - Mach5: hs/jdk tier1,2 - Mach5: hs-nightly (tiers 1 -3) - Targetted - nashorn (for asm changes) - hotspot: runtime/* serviceability/* compiler/* vmTestbase/* - jdk: java/lang/invoke/* java/lang/reflect/* java/lang/instrument/* java/lang/Class/* java/lang/management/* - langtools: tools/javac tools/javap
Hi Paul, Thanks for looking at this. On 17/05/2018 4:00 AM, Paul Sandoz wrote:
HI,
Nice thorough work on this, surprisingly tricky in some areas esp. MHs.
Yeah the fundamental access control part was simple. The invokeinterface semantics for private method invocations via MH got a bit tricky. :)
Class —
3857 * <p>If there is any error accessing the nest host, or the nest host is 3858 * in any way invalid, then {@code this} is returned.
I am curious under what conditions this can arise. As a caller this makes me nervous :-) Are there conditions that are worth calling it. This is related to Alan’s comment about primitive or array classes. Its clearer when looking at the implementation: primitive/array classes, linkage error, and the more general nest membership validation.
In a well-formed program (all classfiles generated at the same time by the same compiler from the same source) the only way it can happen is if the classfile was omitted from the jar or other distribution mechanism. Otherwise it can only happen through direct crafting of classfiles (or deliberate mixing of classfiles created from different sources). So you shouldn't be nervous. ;-)
3883 * @throws SecurityException 3884 * If the returned class is not the current class, and 3885 * if a security manager, <i>s</i>, is present and the caller's 3886 * class loader is not the same as or an ancestor of the class 3887 * loader for the returned class and invocation of {@link 3888 * SecurityManager#checkPackageAccess s.checkPackageAccess()} 3889 * denies access to the package of the returned class 3890 * @since 11 3891 * @jvms 4.7.28 and 4.7.29 NestHost and NestMembers attributes 3892 */ 3893 @CallerSensitive 3894 public Class<?> getNestHost() {
Maybe i need more coffee, but I am struggling to see in the implementation the checks for the case of "and the caller’s class loader is not the same as or an ancestor of the class loader for the returned class”. Is it implied that all classes in the nest have to be loaded from the same or from a common ancestor class loader? so you only need to check one class in the nest against the calling class.
Yes. All members of the nest must be in the same run-time package, and hence the same class-loader and hence subject to the same package-access check. So the current/target class can be used in that check.
3984 public Class<?>[] getNestMembers() {
I still think not removing dups is a mistake as it could be a source of subtle bugs. But i doubt at this point i can persuade you or others to change it :-)
Unlikely. :) Again well-formed programs just won't encounter this and it would penalize all well-formed programs.
3989 // Can't actually enable this due to bootstrapping issues 3990 // assert(members.length != 1 || members[0] == this); // expected invariant from VM
That's interesting and frustrating!
Not completely surprising though.
Reflection.java —
146 // Check for nestmate access if member is private 147 if (Modifier.isPrivate(modifiers)) { 148 // assert: isSubclassof(targetClass, memberClass) 149 // Note: targetClass may be outside the nest, but that is okay 150 // as long as memberClass is in the nest. 151 boolean nestmates = areNestMates(currentClass, memberClass); 152 if (nestmates) { 153 return true; 154 } 155 }
Trivially, you don’t need the local variable “nestmates”.
Right - a leftover from when I inserted debugging statements. Thanks.
VerifyAccess.java —
134 case PRIVATE: 135 // Rules for privates follows access rules for nestmates. 136 boolean canAccess = ((allowedModes & PRIVATE) != 0 && 137 Reflection.areNestMates(defc, lookupClass)); 138 // FIX ME: Sanity check refc == defc. Either remove or convert to 139 // plain assert before integration. 140 myassert((canAccess && refc == defc) || !canAccess); 141 return canAccess; 142 default: 143 throw new IllegalArgumentException("bad modifiers: "+Modifier.toString(mods)); 144 } 145 } 146 static void myassert(boolean cond) { 147 if (!cond) throw new Error("Assertion failed"); 148 }
Do you plan to chase up the FIX ME now or later?
Now. Please see response to Alan's email.
I agree with Alan about the current location of the reflection API tests. If these tests don’t need to be hammered with various and exotic HotSpot flags i think they are better placed to be under test/jdk/java/lang/reflect.
Agreed - again please see response to Alan's email. Thanks again for the review! David
Thanks, Paul.
On May 14, 2018, at 5:52 PM, David Holmes <david.holmes@oracle.com> wrote:
This review is being spread across four groups: langtools, core-libs, hotspot and serviceability. This is the specific review thread for core-libs - webrev:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/
See below for full details - including annotated full webrev guiding the review.
The intent is to have JEP-181 targeted and integrated by the end of this month.
Thanks, David -----
The nestmates project (JEP-181) introduces new classfile attributes to identify classes and interfaces in the same nest, so that the VM can perform access control based on those attributes and so allow direct private access between nestmates without requiring javac to generate synthetic accessor methods. These access control changes also extend to core reflection and the MethodHandle.Lookup contexts.
Direct private calls between nestmates requires a more general calling context than is permitted by invokespecial, and so the JVMS is updated to allow, and javac updated to use, invokevirtual and invokeinterface for private class and interface method calls respectively. These changed semantics also extend to MethodHandle findXXX operations.
At this time we are only concerned with static nest definitions, which map to a top-level class/interface as the nest-host and all its nested types as nest-members.
Please see the JEP for further details.
JEP: https://bugs.openjdk.java.net/browse/JDK-8046171 Bug: https://bugs.openjdk.java.net/browse/JDK-8010319 CSR: https://bugs.openjdk.java.net/browse/JDK-8197445
All of the specification changes have been previously been worked out by the Valhalla Project Expert Group, and the implementation reviewed by the various contributors and discussed on the valhalla-dev mailing list.
Acknowledgments and contributions: Alex Buckley, Maurizio Cimadamore, Mandy Chung, Tobias Hartmann, Vladimir Ivanov, Karen Kinnear, Vladimir Kozlov, John Rose, Dan Smith, Serguei Spitsyn, Kumar Srinivasan
Master webrev of all changes:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.full.v1/
Annotated master webrev index:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/jep181-webrev.html
Performance: this is expected to be performance neutral in a general sense. Benchmarking and performance runs are about to start.
Testing Discussion: ------------------
The testing for nestmates can be broken into four main groups:
- New tests specifically related to nestmates and currently in the runtime/Nestmates directory
- New tests to complement existing tests by adding in testcases not previously expressible. - For example java/lang/invoke/SpecialInterfaceCall.java tests use of invokespecial for private interface methods and performing receiver typechecks, so we add java/lang/invoke/PrivateInterfaceCall.java to do similar tests for invokeinterface.
- New JVM TI tests to verify the spec changes related to nest attributes.
- Existing tests significantly affected by the nestmates changes, primarily: - runtime/SelectionResolution
In most cases the nestmate changes makes certain invocations that were illegal, legal (e.g. not requiring invokespecial to invoke private interface methods; allowing access to private members via reflection/Methodhandles that were previously not allowed).
- Existing tests incidentally affected by the nestmate changes
This includes tests of things utilising class redefinition/retransformation to alter nested types but which unintentionally alter nest relationships (which is not permitted).
There are still a number of tests problem-listed with issues filed against them to have them adapted to work with nestmates. Some of these are intended to be addressed in the short-term, while some (such as the runtime/SelectionResolution test changes) may not eventuate.
- https://bugs.openjdk.java.net/browse/JDK-8203033 - https://bugs.openjdk.java.net/browse/JDK-8199450 - https://bugs.openjdk.java.net/browse/JDK-8196855 - https://bugs.openjdk.java.net/browse/JDK-8194857 - https://bugs.openjdk.java.net/browse/JDK-8187655
There is also further test work still to be completed (the JNI and JDI invocation tests): - https://bugs.openjdk.java.net/browse/JDK-8191117 which will continue in parallel with the main RFR.
Pre-integration Testing: - General: - Mach5: hs/jdk tier1,2 - Mach5: hs-nightly (tiers 1 -3) - Targetted - nashorn (for asm changes) - hotspot: runtime/* serviceability/* compiler/* vmTestbase/* - jdk: java/lang/invoke/* java/lang/reflect/* java/lang/instrument/* java/lang/Class/* java/lang/management/* - langtools: tools/javac tools/javap
On May 20, 2018, at 11:32 PM, David Holmes <david.holmes@oracle.com> wrote:
3984 public Class<?>[] getNestMembers() { I still think not removing dups is a mistake as it could be a source of subtle bugs. But i doubt at this point i can persuade you or others to change it :-)
Unlikely. :) Again well-formed programs just won't encounter this and it would penalize all well-formed programs.
Although those well-formed programs may need to check for dups themselves because they don’t want to rely in implementation details (and they are not aware of the probability of implementations deviating). Here’s a thought: did you consider caching the nest members in the ReflectionData class? that may be worth doing regardless of dups. Paul.
On 5/21/18 9:39 AM, Paul Sandoz wrote:
On May 20, 2018, at 11:32 PM, David Holmes <david.holmes@oracle.com> wrote:
3984 public Class<?>[] getNestMembers() { I still think not removing dups is a mistake as it could be a source of subtle bugs. But i doubt at this point i can persuade you or others to change it :-) Unlikely. :) Again well-formed programs just won't encounter this and it would penalize all well-formed programs.
Although those well-formed programs may need to check for dups themselves because they don’t want to rely in implementation details (and they are not aware of the probability of implementations deviating).
Here’s a thought: did you consider caching the nest members in the ReflectionData class? that may be worth doing regardless of dups.
I think that is a good idea and no change to the VM performance in return the nest members. I would cache the nest host in ReflectionData as well if it caches the nest members. Mandy
Hi Paul, On 22/05/2018 2:39 AM, Paul Sandoz wrote:
On May 20, 2018, at 11:32 PM, David Holmes <david.holmes@oracle.com> wrote:
3984 public Class<?>[] getNestMembers() { I still think not removing dups is a mistake as it could be a source of subtle bugs. But i doubt at this point i can persuade you or others to change it :-)
Unlikely. :) Again well-formed programs just won't encounter this and it would penalize all well-formed programs.
Although those well-formed programs may need to check for dups themselves because they don’t want to rely in implementation details (and they are not aware of the probability of implementations deviating).
I'm quite concerned about your level of concern with "dups". This just shouldn't be an issue. While the spec allows for dups javac will never produce them - and file a bug on it if it ever does! Similarly for other compilers - there is no reason to generate duplicate entries. Looking through the JVMS and the defined classfile attributes it seems to me that the annotations[] of RuntimeVisibleAnnotations (et al) doesn't preclude duplicates either. And the bootstrap_methods[] of the BootstrapMethodsAttribute. Also look at the parameters[] of the MethodParametersAttribute**. Do you agree? ** Which even has an explicit note this is not something a JVM implementation has to check.
Here’s a thought: did you consider caching the nest members in the ReflectionData class? that may be worth doing regardless of dups.
No that was not considered. Caching, as you know, is a space-time trade off and we have no data to use to determine whether caching would be of any benefit here. To me that would be a future RFE regardless. (And I don't expect these introspective nest methods to be used much in any case.) Thanks, David -----
Paul.
Hi David,
On May 21, 2018, at 5:05 PM, David Holmes <david.holmes@oracle.com> wrote:
Hi Paul,
On 22/05/2018 2:39 AM, Paul Sandoz wrote:
On May 20, 2018, at 11:32 PM, David Holmes <david.holmes@oracle.com> wrote:
3984 public Class<?>[] getNestMembers() { I still think not removing dups is a mistake as it could be a source of subtle bugs. But i doubt at this point i can persuade you or others to change it :-)
Unlikely. :) Again well-formed programs just won't encounter this and it would penalize all well-formed programs.
Although those well-formed programs may need to check for dups themselves because they don’t want to rely in implementation details (and they are not aware of the probability of implementations deviating).
I'm quite concerned about your level of concern with "dups". This just shouldn't be an issue. While the spec allows for dups javac will never produce them - and file a bug on it if it ever does! Similarly for other compilers - there is no reason to generate duplicate entries.
Perhaps i am obsessing a little too much, i thought there might be a slight window of opportunity while other related reviews are progressing :-) but i don’t want to block things for 11. My concern, placing my library/API designer hat on, is the specification is saying something very clear and yet on the other hand it's as if we are saying “oh you can ignore that, the specification does not matter, it will never happen in practice”. It feels like the JVM world is intruding too much into the reflection world (see below).
Looking through the JVMS and the defined classfile attributes it seems to me that the annotations[] of RuntimeVisibleAnnotations (et al) doesn't preclude duplicates either. And the bootstrap_methods[] of the BootstrapMethodsAttribute. Also look at the parameters[] of the MethodParametersAttribute**. Do you agree?
** Which even has an explicit note this is not something a JVM implementation has to check.
I don’t dispute there may be duplicate information in class file bytes, nor am i suggesting the class file bytes for nestmates be changed, nor that the verifier get involved. However, the reflection API is not a direct reflection of those class file bytes. It provides a runtime view of a class and often performs its own computation from and validation of information in the class file bytes. Not all information in the class file bytes is directly accessible via the reflection API, such as BootstrapMethodsAttribute, or MethodParametersAttribute, the latter of which AFAICT is exposed via the Reflection API indirectly via a Parameter[] array returned by Method.getParameters(). I am less sure about RuntimeVisibleAnnotations but there is quite a lot of processing performed by the Java reflection code before annotations reach the hands of the developer, and a quick look at some code shows the use of maps keyed by annotation class to the annotation value. And see for example here in AnnotationParser: if (AnnotationType.getInstance(klass).retention() == RetentionPolicy.RUNTIME && result.put(klass, a) != null) { throw new AnnotationFormatError( "Duplicate annotation for class: "+klass+": " + a); http://hg.openjdk.java.net/jdk/jdk/file/95ba3a1dc2b2/src/java.base/share/cla...
Here’s a thought: did you consider caching the nest members in the ReflectionData class? that may be worth doing regardless of dups.
No that was not considered. Caching, as you know, is a space-time trade off and we have no data to use to determine whether caching would be of any benefit here. To me that would be a future RFE regardless. (And I don't expect these introspective nest methods to be used much in any case.)
Yes, agreed, caching can be possible future work. Paul,
Hi Paul, On 22/05/2018 2:15 PM, Paul Sandoz wrote:
Hi David,
On May 21, 2018, at 5:05 PM, David Holmes <david.holmes@oracle.com> wrote:
Hi Paul,
On 22/05/2018 2:39 AM, Paul Sandoz wrote:
On May 20, 2018, at 11:32 PM, David Holmes <david.holmes@oracle.com> wrote:
3984 public Class<?>[] getNestMembers() { I still think not removing dups is a mistake as it could be a source of subtle bugs. But i doubt at this point i can persuade you or others to change it :-)
Unlikely. :) Again well-formed programs just won't encounter this and it would penalize all well-formed programs.
Although those well-formed programs may need to check for dups themselves because they don’t want to rely in implementation details (and they are not aware of the probability of implementations deviating).
I'm quite concerned about your level of concern with "dups". This just shouldn't be an issue. While the spec allows for dups javac will never produce them - and file a bug on it if it ever does! Similarly for other compilers - there is no reason to generate duplicate entries.
Perhaps i am obsessing a little too much, i thought there might be a slight window of opportunity while other related reviews are progressing :-) but i don’t want to block things for 11.
My concern, placing my library/API designer hat on, is the specification is saying something very clear and yet on the other hand it's as if we are saying “oh you can ignore that, the specification does not matter, it will never happen in practice”. It feels like the JVM world is intruding too much into the reflection world (see below).
Looking through the JVMS and the defined classfile attributes it seems to me that the annotations[] of RuntimeVisibleAnnotations (et al) doesn't preclude duplicates either. And the bootstrap_methods[] of the BootstrapMethodsAttribute. Also look at the parameters[] of the MethodParametersAttribute**. Do you agree?
** Which even has an explicit note this is not something a JVM implementation has to check.
I don’t dispute there may be duplicate information in class file bytes, nor am i suggesting the class file bytes for nestmates be changed, nor that the verifier get involved. However, the reflection API is not a direct reflection of those class file bytes. It provides a runtime view of a class and often performs its own computation from and validation of information in the class file bytes.
Not all information in the class file bytes is directly accessible via the reflection API, such as BootstrapMethodsAttribute, or MethodParametersAttribute, the latter of which AFAICT is exposed via the Reflection API indirectly via a Parameter[] array returned by Method.getParameters().
I am less sure about RuntimeVisibleAnnotations but there is quite a lot of processing performed by the Java reflection code before annotations reach the hands of the developer, and a quick look at some code shows the use of maps keyed by annotation class to the annotation value. And see for example here in AnnotationParser:
if (AnnotationType.getInstance(klass).retention() == RetentionPolicy.RUNTIME && result.put(klass, a) != null) { throw new AnnotationFormatError( "Duplicate annotation for class: "+klass+": " + a);
http://hg.openjdk.java.net/jdk/jdk/file/95ba3a1dc2b2/src/java.base/share/cla...
Okay I acknowledge your point here about VM view versus Reflection view. It would be easier if we were returning the members in a duplicate-detecting data structure rather than an array (though placing the host at zeroth element complicates that). The sheer effort involved in detecting and removing duplicates from an array is what made me shy away from pushing for that. You can see the EG discussion from here: http://mail.openjdk.java.net/pipermail/valhalla-spec-experts/2017-December/0... If you really think this is worth re-opening it would probably be expedient to discuss with John so you both end up on the same page, then let me know the outcome. Though I'll also note that we can strengthen the current implementation at any time and just update the @implNote. Thanks, David -----
Here’s a thought: did you consider caching the nest members in the ReflectionData class? that may be worth doing regardless of dups.
No that was not considered. Caching, as you know, is a space-time trade off and we have no data to use to determine whether caching would be of any benefit here. To me that would be a future RFE regardless. (And I don't expect these introspective nest methods to be used much in any case.)
Yes, agreed, caching can be possible future work.
Paul,
Hi David, Thank you for your patience, i struggled to explain my point. How about we proceed as is, and as you suggest, i can discuss more with John when he is back from vacation. I think we will have time to revisit if necessary. My preference would be to store the classes from the class file bytes in a Set, cache in ReflectionData, then obtain the “cloned” array from that cached Set. We can use Set.of, which is efficient for small sizes, for the case of one element we could even erase the cached value to Object. Thanks, Paul.
On May 21, 2018, at 10:50 PM, David Holmes <david.holmes@oracle.com> wrote:
Hi Paul,
On 22/05/2018 2:15 PM, Paul Sandoz wrote:
Hi David,
On May 21, 2018, at 5:05 PM, David Holmes <david.holmes@oracle.com> wrote:
Hi Paul,
On 22/05/2018 2:39 AM, Paul Sandoz wrote:
On May 20, 2018, at 11:32 PM, David Holmes <david.holmes@oracle.com> wrote:
3984 public Class<?>[] getNestMembers() { I still think not removing dups is a mistake as it could be a source of subtle bugs. But i doubt at this point i can persuade you or others to change it :-)
Unlikely. :) Again well-formed programs just won't encounter this and it would penalize all well-formed programs.
Although those well-formed programs may need to check for dups themselves because they don’t want to rely in implementation details (and they are not aware of the probability of implementations deviating).
I'm quite concerned about your level of concern with "dups". This just shouldn't be an issue. While the spec allows for dups javac will never produce them - and file a bug on it if it ever does! Similarly for other compilers - there is no reason to generate duplicate entries.
Perhaps i am obsessing a little too much, i thought there might be a slight window of opportunity while other related reviews are progressing :-) but i don’t want to block things for 11. My concern, placing my library/API designer hat on, is the specification is saying something very clear and yet on the other hand it's as if we are saying “oh you can ignore that, the specification does not matter, it will never happen in practice”. It feels like the JVM world is intruding too much into the reflection world (see below).
Looking through the JVMS and the defined classfile attributes it seems to me that the annotations[] of RuntimeVisibleAnnotations (et al) doesn't preclude duplicates either. And the bootstrap_methods[] of the BootstrapMethodsAttribute. Also look at the parameters[] of the MethodParametersAttribute**. Do you agree?
** Which even has an explicit note this is not something a JVM implementation has to check.
I don’t dispute there may be duplicate information in class file bytes, nor am i suggesting the class file bytes for nestmates be changed, nor that the verifier get involved. However, the reflection API is not a direct reflection of those class file bytes. It provides a runtime view of a class and often performs its own computation from and validation of information in the class file bytes. Not all information in the class file bytes is directly accessible via the reflection API, such as BootstrapMethodsAttribute, or MethodParametersAttribute, the latter of which AFAICT is exposed via the Reflection API indirectly via a Parameter[] array returned by Method.getParameters(). I am less sure about RuntimeVisibleAnnotations but there is quite a lot of processing performed by the Java reflection code before annotations reach the hands of the developer, and a quick look at some code shows the use of maps keyed by annotation class to the annotation value. And see for example here in AnnotationParser: if (AnnotationType.getInstance(klass).retention() == RetentionPolicy.RUNTIME && result.put(klass, a) != null) { throw new AnnotationFormatError( "Duplicate annotation for class: "+klass+": " + a); http://hg.openjdk.java.net/jdk/jdk/file/95ba3a1dc2b2/src/java.base/share/cla...
Okay I acknowledge your point here about VM view versus Reflection view. It would be easier if we were returning the members in a duplicate-detecting data structure rather than an array (though placing the host at zeroth element complicates that). The sheer effort involved in detecting and removing duplicates from an array is what made me shy away from pushing for that. You can see the EG discussion from here:
http://mail.openjdk.java.net/pipermail/valhalla-spec-experts/2017-December/0...
If you really think this is worth re-opening it would probably be expedient to discuss with John so you both end up on the same page, then let me know the outcome.
Though I'll also note that we can strengthen the current implementation at any time and just update the @implNote.
Thanks, David -----
Here’s a thought: did you consider caching the nest members in the ReflectionData class? that may be worth doing regardless of dups.
No that was not considered. Caching, as you know, is a space-time trade off and we have no data to use to determine whether caching would be of any benefit here. To me that would be a future RFE regardless. (And I don't expect these introspective nest methods to be used much in any case.)
Yes, agreed, caching can be possible future work. Paul,
Hi Paul, On 23/05/2018 2:05 AM, Paul Sandoz wrote:
Hi David,
Thank you for your patience, i struggled to explain my point.
How about we proceed as is, and as you suggest, i can discuss more with John when he is back from vacation. I think we will have time to revisit if necessary.
That sounds good to me. As I noted this would not involve any spec change so there is no concern with making an implementation change later.
My preference would be to store the classes from the class file bytes in a Set, cache in ReflectionData, then obtain the “cloned” array from that cached Set. We can use Set.of, which is efficient for small sizes, for the case of one element we could even erase the cached value to Object.
The only glitch there is you then need to find the nest-host and swap it into the zeroth element. Thanks, David
Thanks, Paul.
On May 21, 2018, at 10:50 PM, David Holmes <david.holmes@oracle.com> wrote:
Hi Paul,
On 22/05/2018 2:15 PM, Paul Sandoz wrote:
Hi David,
On May 21, 2018, at 5:05 PM, David Holmes <david.holmes@oracle.com> wrote:
Hi Paul,
On 22/05/2018 2:39 AM, Paul Sandoz wrote:
On May 20, 2018, at 11:32 PM, David Holmes <david.holmes@oracle.com> wrote:
> 3984 public Class<?>[] getNestMembers() { > I still think not removing dups is a mistake as it could be a source of subtle bugs. But i doubt at this point i can persuade you or others to change it :-)
Unlikely. :) Again well-formed programs just won't encounter this and it would penalize all well-formed programs.
Although those well-formed programs may need to check for dups themselves because they don’t want to rely in implementation details (and they are not aware of the probability of implementations deviating).
I'm quite concerned about your level of concern with "dups". This just shouldn't be an issue. While the spec allows for dups javac will never produce them - and file a bug on it if it ever does! Similarly for other compilers - there is no reason to generate duplicate entries.
Perhaps i am obsessing a little too much, i thought there might be a slight window of opportunity while other related reviews are progressing :-) but i don’t want to block things for 11. My concern, placing my library/API designer hat on, is the specification is saying something very clear and yet on the other hand it's as if we are saying “oh you can ignore that, the specification does not matter, it will never happen in practice”. It feels like the JVM world is intruding too much into the reflection world (see below).
Looking through the JVMS and the defined classfile attributes it seems to me that the annotations[] of RuntimeVisibleAnnotations (et al) doesn't preclude duplicates either. And the bootstrap_methods[] of the BootstrapMethodsAttribute. Also look at the parameters[] of the MethodParametersAttribute**. Do you agree?
** Which even has an explicit note this is not something a JVM implementation has to check.
I don’t dispute there may be duplicate information in class file bytes, nor am i suggesting the class file bytes for nestmates be changed, nor that the verifier get involved. However, the reflection API is not a direct reflection of those class file bytes. It provides a runtime view of a class and often performs its own computation from and validation of information in the class file bytes. Not all information in the class file bytes is directly accessible via the reflection API, such as BootstrapMethodsAttribute, or MethodParametersAttribute, the latter of which AFAICT is exposed via the Reflection API indirectly via a Parameter[] array returned by Method.getParameters(). I am less sure about RuntimeVisibleAnnotations but there is quite a lot of processing performed by the Java reflection code before annotations reach the hands of the developer, and a quick look at some code shows the use of maps keyed by annotation class to the annotation value. And see for example here in AnnotationParser: if (AnnotationType.getInstance(klass).retention() == RetentionPolicy.RUNTIME && result.put(klass, a) != null) { throw new AnnotationFormatError( "Duplicate annotation for class: "+klass+": " + a); http://hg.openjdk.java.net/jdk/jdk/file/95ba3a1dc2b2/src/java.base/share/cla...
Okay I acknowledge your point here about VM view versus Reflection view. It would be easier if we were returning the members in a duplicate-detecting data structure rather than an array (though placing the host at zeroth element complicates that). The sheer effort involved in detecting and removing duplicates from an array is what made me shy away from pushing for that. You can see the EG discussion from here:
http://mail.openjdk.java.net/pipermail/valhalla-spec-experts/2017-December/0...
If you really think this is worth re-opening it would probably be expedient to discuss with John so you both end up on the same page, then let me know the outcome.
Though I'll also note that we can strengthen the current implementation at any time and just update the @implNote.
Thanks, David -----
Here’s a thought: did you consider caching the nest members in the ReflectionData class? that may be worth doing regardless of dups.
No that was not considered. Caching, as you know, is a space-time trade off and we have no data to use to determine whether caching would be of any benefit here. To me that would be a future RFE regardless. (And I don't expect these introspective nest methods to be used much in any case.)
Yes, agreed, caching can be possible future work. Paul,
On 22/05/2018 01:05, David Holmes wrote:
:
Here’s a thought: did you consider caching the nest members in the ReflectionData class? that may be worth doing regardless of dups.
No that was not considered. Caching, as you know, is a space-time trade off and we have no data to use to determine whether caching would be of any benefit here. To me that would be a future RFE regardless. (And I don't expect these introspective nest methods to be used much in any case.) I think this is the first time that an access check in core reflection (and java.lang.invoke too?) has to call into the VM. It's the private member case so it might not be an issue. If it turns out to be an issue then it can be re-examined.
-Alan
Hi Alan, On 22/05/2018 4:19 PM, Alan Bateman wrote:
On 22/05/2018 01:05, David Holmes wrote:
:
Here’s a thought: did you consider caching the nest members in the ReflectionData class? that may be worth doing regardless of dups.
No that was not considered. Caching, as you know, is a space-time trade off and we have no data to use to determine whether caching would be of any benefit here. To me that would be a future RFE regardless. (And I don't expect these introspective nest methods to be used much in any case.) I think this is the first time that an access check in core reflection (and java.lang.invoke too?) has to call into the VM. It's the private member case so it might not be an issue. If it turns out to be an issue then it can be re-examined.
I hadn't connected the caching with the access check, but yes lets re-examine if it is an issue. Thanks, David
-Alan
On 05/22/18 02:05, David Holmes wrote:
Although those well-formed programs may need to check for dups themselves because they don’t want to rely in implementation details (and they are not aware of the probability of implementations deviating).
I'm quite concerned about your level of concern with "dups". This just shouldn't be an issue. While the spec allows for dups javac will never produce them - and file a bug on it if it ever does! Similarly for other compilers - there is no reason to generate duplicate entries.
Javac compiler can misbehave (have bugs) in various ways and produce misbehaving programs, but that doesn't mean that runtime libraries should try all possible ways to work around any imagined compiler misbehavior. I think this is a situation where assert would be appropriate. Unfortunately asserts in j.l.Class are not "allowed" or what? Would it work if assert was issued in some other class? For example in ReflectionData (if it is going to be used for caching anyway)? Regards, Peter
Hi Peter, On 22/05/2018 6:36 PM, Peter Levart wrote:
On 05/22/18 02:05, David Holmes wrote:
Although those well-formed programs may need to check for dups themselves because they don’t want to rely in implementation details (and they are not aware of the probability of implementations deviating).
I'm quite concerned about your level of concern with "dups". This just shouldn't be an issue. While the spec allows for dups javac will never produce them - and file a bug on it if it ever does! Similarly for other compilers - there is no reason to generate duplicate entries.
Javac compiler can misbehave (have bugs) in various ways and produce misbehaving programs, but that doesn't mean that runtime libraries should try all possible ways to work around any imagined compiler misbehavior. I think this is a situation where assert would be appropriate. Unfortunately asserts in j.l.Class are not "allowed" or
Yeah no asserts in Class itself.
what? Would it work if assert was issued in some other class? For example in ReflectionData (if it is going to be used for caching anyway)?
If we were to cache in the future then yes it could be a place to check for and/or filter duplicates. Cheers, David
Regards, Peter
Here are the updates so far in response to all the review comments. Incremental webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v2-incr/ Full webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v2/ Change summary: src/java.base/share/classes/java/lang/Class.java - getNesthost: - change "any error" -> "any linkage error" as runtime errors will propagate. [This needs ratifying by EG] - add clarification that primitive and array classes are not explicitly members of any nest and so form singleton nests - add clarification that all nestmates are in the same package - re-word @return text to exclude the "royal 'we'" - fix javadoc cross references --- Moved reflection API tests from test/hotspot/jtreg/runtime/Nestmates/reflectionAPI/ to test/jdk/java/lang/reflect/Nestmates/ --- java/lang/reflect/Nestmates/TestReflectionAPI.java Run tests twice to show that failure reasons remain the same. --- test/jdk/jdk/lambda/vm/InterfaceAccessFlagsTest.java Disable test via annotation rather than commenting out. --- src/java.base/share/classes/jdk/internal/reflect/Reflection.java - Fix indent for nestmate access check. - Remove unnecessary local variable --- src/java.base/share/classes/sun/invoke/util/VerifyAccess.java - Replace myassert with proper assert --- Thanks, David On 15/05/2018 10:52 AM, David Holmes wrote:
This review is being spread across four groups: langtools, core-libs, hotspot and serviceability. This is the specific review thread for core-libs - webrev:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/
See below for full details - including annotated full webrev guiding the review.
The intent is to have JEP-181 targeted and integrated by the end of this month.
Thanks, David -----
The nestmates project (JEP-181) introduces new classfile attributes to identify classes and interfaces in the same nest, so that the VM can perform access control based on those attributes and so allow direct private access between nestmates without requiring javac to generate synthetic accessor methods. These access control changes also extend to core reflection and the MethodHandle.Lookup contexts.
Direct private calls between nestmates requires a more general calling context than is permitted by invokespecial, and so the JVMS is updated to allow, and javac updated to use, invokevirtual and invokeinterface for private class and interface method calls respectively. These changed semantics also extend to MethodHandle findXXX operations.
At this time we are only concerned with static nest definitions, which map to a top-level class/interface as the nest-host and all its nested types as nest-members.
Please see the JEP for further details.
JEP: https://bugs.openjdk.java.net/browse/JDK-8046171 Bug: https://bugs.openjdk.java.net/browse/JDK-8010319 CSR: https://bugs.openjdk.java.net/browse/JDK-8197445
All of the specification changes have been previously been worked out by the Valhalla Project Expert Group, and the implementation reviewed by the various contributors and discussed on the valhalla-dev mailing list.
Acknowledgments and contributions: Alex Buckley, Maurizio Cimadamore, Mandy Chung, Tobias Hartmann, Vladimir Ivanov, Karen Kinnear, Vladimir Kozlov, John Rose, Dan Smith, Serguei Spitsyn, Kumar Srinivasan
Master webrev of all changes:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.full.v1/
Annotated master webrev index:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/jep181-webrev.html
Performance: this is expected to be performance neutral in a general sense. Benchmarking and performance runs are about to start.
Testing Discussion: ------------------
The testing for nestmates can be broken into four main groups:
- New tests specifically related to nestmates and currently in the runtime/Nestmates directory
- New tests to complement existing tests by adding in testcases not previously expressible. - For example java/lang/invoke/SpecialInterfaceCall.java tests use of invokespecial for private interface methods and performing receiver typechecks, so we add java/lang/invoke/PrivateInterfaceCall.java to do similar tests for invokeinterface.
- New JVM TI tests to verify the spec changes related to nest attributes.
- Existing tests significantly affected by the nestmates changes, primarily: - runtime/SelectionResolution
In most cases the nestmate changes makes certain invocations that were illegal, legal (e.g. not requiring invokespecial to invoke private interface methods; allowing access to private members via reflection/Methodhandles that were previously not allowed).
- Existing tests incidentally affected by the nestmate changes
This includes tests of things utilising class redefinition/retransformation to alter nested types but which unintentionally alter nest relationships (which is not permitted).
There are still a number of tests problem-listed with issues filed against them to have them adapted to work with nestmates. Some of these are intended to be addressed in the short-term, while some (such as the runtime/SelectionResolution test changes) may not eventuate.
- https://bugs.openjdk.java.net/browse/JDK-8203033 - https://bugs.openjdk.java.net/browse/JDK-8199450 - https://bugs.openjdk.java.net/browse/JDK-8196855 - https://bugs.openjdk.java.net/browse/JDK-8194857 - https://bugs.openjdk.java.net/browse/JDK-8187655
There is also further test work still to be completed (the JNI and JDI invocation tests): - https://bugs.openjdk.java.net/browse/JDK-8191117 which will continue in parallel with the main RFR.
Pre-integration Testing: - General: - Mach5: hs/jdk tier1,2 - Mach5: hs-nightly (tiers 1 -3) - Targetted - nashorn (for asm changes) - hotspot: runtime/* serviceability/* compiler/* vmTestbase/* - jdk: java/lang/invoke/* java/lang/reflect/* java/lang/instrument/* java/lang/Class/* java/lang/management/* - langtools: tools/javac tools/javap
On 5/22/18 3:15 AM, David Holmes wrote:
Here are the updates so far in response to all the review comments.
Incremental webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v2-incr/
Full webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v2/
thanks for the update. Looks good in general. SampleNest.java 33 static List<Class<?>> _nestedTypes = new LinkedList<Class<?>>(); This can use the diamond expression i.e. new LinkedList<>(). TestReflectionAPI.java Thanks for covering many test cases. 263 Class<?>[] memberSet = 264 new HashSet<Class<?>>(Arrays.asList(members)).toArray(new Class<?>[0]); It can be simplified using Stream: Class<?>[] memberSet = Arrays.stream(members).toArray(Class<?>[]::new); Mandy
Hi Mandy, On 23/05/2018 10:56 AM, mandy chung wrote:
On 5/22/18 3:15 AM, David Holmes wrote:
Here are the updates so far in response to all the review comments.
Incremental webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v2-incr/
Full webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v2/
thanks for the update. Looks good in general.
Thanks for continuing to work through it!
SampleNest.java
33 static List<Class<?>> _nestedTypes = new LinkedList<Class<?>>();
This can use the diamond expression i.e. new LinkedList<>().
Ok.
TestReflectionAPI.java
Thanks for covering many test cases.
263 Class<?>[] memberSet = 264 new HashSet<Class<?>>(Arrays.asList(members)).toArray(new Class<?>[0]);
It can be simplified using Stream:
Class<?>[] memberSet = Arrays.stream(members).toArray(Class<?>[]::new);
I'm not very stream literate :) Will make the suggested changes for v3 (once I've processed hotspot v2 stuff). Thanks, David
Mandy
On 5/22/18 6:08 PM, David Holmes wrote:
Will make the suggested changes for v3 (once I've processed hotspot v2 stuff).
Unless you need to send a new webrev for other's comments, the test update I suggest don't need a new webrev. You can fix it in your local repo before you push. Mandy
Here are the further minor updates so far in response to all the review comments. Incremental corelibs webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v3-incr/ Full corelibs webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v3/ Change summary: src/java.base/share/classes/jdk/internal/reflect/Reflection.java - remove inaccurate pseudo-assertion comment test/jdk/java/lang/reflect/Nestmates/SampleNest.java - code cleanup: <> operator test/jdk/java/lang/reflect/Nestmates/TestReflectionAPI.java - code cleanup: streamify duplicate removals test/jdk/java/lang/invoke/PrivateInterfaceCall.java - use consistent @bug number Thanks, David On 22/05/2018 8:15 PM, David Holmes wrote:
Here are the updates so far in response to all the review comments.
Incremental webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v2-incr/
Full webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v2/
Change summary:
src/java.base/share/classes/java/lang/Class.java - getNesthost: - change "any error" -> "any linkage error" as runtime errors will propagate. [This needs ratifying by EG] - add clarification that primitive and array classes are not explicitly members of any nest and so form singleton nests - add clarification that all nestmates are in the same package - re-word @return text to exclude the "royal 'we'" - fix javadoc cross references
---
Moved reflection API tests from test/hotspot/jtreg/runtime/Nestmates/reflectionAPI/ to test/jdk/java/lang/reflect/Nestmates/
---
java/lang/reflect/Nestmates/TestReflectionAPI.java
Run tests twice to show that failure reasons remain the same.
---
test/jdk/jdk/lambda/vm/InterfaceAccessFlagsTest.java
Disable test via annotation rather than commenting out.
---
src/java.base/share/classes/jdk/internal/reflect/Reflection.java
- Fix indent for nestmate access check. - Remove unnecessary local variable
---
src/java.base/share/classes/sun/invoke/util/VerifyAccess.java
- Replace myassert with proper assert
---
Thanks, David
On 15/05/2018 10:52 AM, David Holmes wrote:
This review is being spread across four groups: langtools, core-libs, hotspot and serviceability. This is the specific review thread for core-libs - webrev:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/
See below for full details - including annotated full webrev guiding the review.
The intent is to have JEP-181 targeted and integrated by the end of this month.
Thanks, David -----
The nestmates project (JEP-181) introduces new classfile attributes to identify classes and interfaces in the same nest, so that the VM can perform access control based on those attributes and so allow direct private access between nestmates without requiring javac to generate synthetic accessor methods. These access control changes also extend to core reflection and the MethodHandle.Lookup contexts.
Direct private calls between nestmates requires a more general calling context than is permitted by invokespecial, and so the JVMS is updated to allow, and javac updated to use, invokevirtual and invokeinterface for private class and interface method calls respectively. These changed semantics also extend to MethodHandle findXXX operations.
At this time we are only concerned with static nest definitions, which map to a top-level class/interface as the nest-host and all its nested types as nest-members.
Please see the JEP for further details.
JEP: https://bugs.openjdk.java.net/browse/JDK-8046171 Bug: https://bugs.openjdk.java.net/browse/JDK-8010319 CSR: https://bugs.openjdk.java.net/browse/JDK-8197445
All of the specification changes have been previously been worked out by the Valhalla Project Expert Group, and the implementation reviewed by the various contributors and discussed on the valhalla-dev mailing list.
Acknowledgments and contributions: Alex Buckley, Maurizio Cimadamore, Mandy Chung, Tobias Hartmann, Vladimir Ivanov, Karen Kinnear, Vladimir Kozlov, John Rose, Dan Smith, Serguei Spitsyn, Kumar Srinivasan
Master webrev of all changes:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.full.v1/
Annotated master webrev index:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/jep181-webrev.html
Performance: this is expected to be performance neutral in a general sense. Benchmarking and performance runs are about to start.
Testing Discussion: ------------------
The testing for nestmates can be broken into four main groups:
- New tests specifically related to nestmates and currently in the runtime/Nestmates directory
- New tests to complement existing tests by adding in testcases not previously expressible. - For example java/lang/invoke/SpecialInterfaceCall.java tests use of invokespecial for private interface methods and performing receiver typechecks, so we add java/lang/invoke/PrivateInterfaceCall.java to do similar tests for invokeinterface.
- New JVM TI tests to verify the spec changes related to nest attributes.
- Existing tests significantly affected by the nestmates changes, primarily: - runtime/SelectionResolution
In most cases the nestmate changes makes certain invocations that were illegal, legal (e.g. not requiring invokespecial to invoke private interface methods; allowing access to private members via reflection/Methodhandles that were previously not allowed).
- Existing tests incidentally affected by the nestmate changes
This includes tests of things utilising class redefinition/retransformation to alter nested types but which unintentionally alter nest relationships (which is not permitted).
There are still a number of tests problem-listed with issues filed against them to have them adapted to work with nestmates. Some of these are intended to be addressed in the short-term, while some (such as the runtime/SelectionResolution test changes) may not eventuate.
- https://bugs.openjdk.java.net/browse/JDK-8203033 - https://bugs.openjdk.java.net/browse/JDK-8199450 - https://bugs.openjdk.java.net/browse/JDK-8196855 - https://bugs.openjdk.java.net/browse/JDK-8194857 - https://bugs.openjdk.java.net/browse/JDK-8187655
There is also further test work still to be completed (the JNI and JDI invocation tests): - https://bugs.openjdk.java.net/browse/JDK-8191117 which will continue in parallel with the main RFR.
Pre-integration Testing: - General: - Mach5: hs/jdk tier1,2 - Mach5: hs-nightly (tiers 1 -3) - Targetted - nashorn (for asm changes) - hotspot: runtime/* serviceability/* compiler/* vmTestbase/* - jdk: java/lang/invoke/* java/lang/reflect/* java/lang/instrument/* java/lang/Class/* java/lang/management/* - langtools: tools/javac tools/javap
Looks good. Mandy On 5/24/18 10:52 PM, David Holmes wrote:
Here are the further minor updates so far in response to all the review comments.
Incremental corelibs webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v3-incr/
Full corelibs webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v3/
Change summary:
src/java.base/share/classes/jdk/internal/reflect/Reflection.java - remove inaccurate pseudo-assertion comment
test/jdk/java/lang/reflect/Nestmates/SampleNest.java - code cleanup: <> operator
test/jdk/java/lang/reflect/Nestmates/TestReflectionAPI.java - code cleanup: streamify duplicate removals
test/jdk/java/lang/invoke/PrivateInterfaceCall.java - use consistent @bug number
Thanks, David
Thanks Mandy! David On 26/05/2018 7:13 AM, mandy chung wrote:
Looks good.
Mandy
On 5/24/18 10:52 PM, David Holmes wrote:
Here are the further minor updates so far in response to all the review comments.
Incremental corelibs webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v3-incr/
Full corelibs webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v3/
Change summary:
src/java.base/share/classes/jdk/internal/reflect/Reflection.java - remove inaccurate pseudo-assertion comment
test/jdk/java/lang/reflect/Nestmates/SampleNest.java - code cleanup: <> operator
test/jdk/java/lang/reflect/Nestmates/TestReflectionAPI.java - code cleanup: streamify duplicate removals
test/jdk/java/lang/invoke/PrivateInterfaceCall.java - use consistent @bug number
Thanks, David
Here is one further minor update from the CSR discussions: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v5-incr/s... Thanks, David On 25/05/2018 3:52 PM, David Holmes wrote:
Here are the further minor updates so far in response to all the review comments.
Incremental corelibs webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v3-incr/
Full corelibs webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v3/
Change summary:
src/java.base/share/classes/jdk/internal/reflect/Reflection.java - remove inaccurate pseudo-assertion comment
test/jdk/java/lang/reflect/Nestmates/SampleNest.java - code cleanup: <> operator
test/jdk/java/lang/reflect/Nestmates/TestReflectionAPI.java - code cleanup: streamify duplicate removals
test/jdk/java/lang/invoke/PrivateInterfaceCall.java - use consistent @bug number
Thanks, David
On 22/05/2018 8:15 PM, David Holmes wrote:
Here are the updates so far in response to all the review comments.
Incremental webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v2-incr/
Full webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v2/
Change summary:
src/java.base/share/classes/java/lang/Class.java - getNesthost: - change "any error" -> "any linkage error" as runtime errors will propagate. [This needs ratifying by EG] - add clarification that primitive and array classes are not explicitly members of any nest and so form singleton nests - add clarification that all nestmates are in the same package - re-word @return text to exclude the "royal 'we'" - fix javadoc cross references
---
Moved reflection API tests from test/hotspot/jtreg/runtime/Nestmates/reflectionAPI/ to test/jdk/java/lang/reflect/Nestmates/
---
java/lang/reflect/Nestmates/TestReflectionAPI.java
Run tests twice to show that failure reasons remain the same.
---
test/jdk/jdk/lambda/vm/InterfaceAccessFlagsTest.java
Disable test via annotation rather than commenting out.
---
src/java.base/share/classes/jdk/internal/reflect/Reflection.java
- Fix indent for nestmate access check. - Remove unnecessary local variable
---
src/java.base/share/classes/sun/invoke/util/VerifyAccess.java
- Replace myassert with proper assert
---
Thanks, David
On 15/05/2018 10:52 AM, David Holmes wrote:
This review is being spread across four groups: langtools, core-libs, hotspot and serviceability. This is the specific review thread for core-libs - webrev:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/
See below for full details - including annotated full webrev guiding the review.
The intent is to have JEP-181 targeted and integrated by the end of this month.
Thanks, David -----
The nestmates project (JEP-181) introduces new classfile attributes to identify classes and interfaces in the same nest, so that the VM can perform access control based on those attributes and so allow direct private access between nestmates without requiring javac to generate synthetic accessor methods. These access control changes also extend to core reflection and the MethodHandle.Lookup contexts.
Direct private calls between nestmates requires a more general calling context than is permitted by invokespecial, and so the JVMS is updated to allow, and javac updated to use, invokevirtual and invokeinterface for private class and interface method calls respectively. These changed semantics also extend to MethodHandle findXXX operations.
At this time we are only concerned with static nest definitions, which map to a top-level class/interface as the nest-host and all its nested types as nest-members.
Please see the JEP for further details.
JEP: https://bugs.openjdk.java.net/browse/JDK-8046171 Bug: https://bugs.openjdk.java.net/browse/JDK-8010319 CSR: https://bugs.openjdk.java.net/browse/JDK-8197445
All of the specification changes have been previously been worked out by the Valhalla Project Expert Group, and the implementation reviewed by the various contributors and discussed on the valhalla-dev mailing list.
Acknowledgments and contributions: Alex Buckley, Maurizio Cimadamore, Mandy Chung, Tobias Hartmann, Vladimir Ivanov, Karen Kinnear, Vladimir Kozlov, John Rose, Dan Smith, Serguei Spitsyn, Kumar Srinivasan
Master webrev of all changes:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.full.v1/
Annotated master webrev index:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/jep181-webrev.html
Performance: this is expected to be performance neutral in a general sense. Benchmarking and performance runs are about to start.
Testing Discussion: ------------------
The testing for nestmates can be broken into four main groups:
- New tests specifically related to nestmates and currently in the runtime/Nestmates directory
- New tests to complement existing tests by adding in testcases not previously expressible. - For example java/lang/invoke/SpecialInterfaceCall.java tests use of invokespecial for private interface methods and performing receiver typechecks, so we add java/lang/invoke/PrivateInterfaceCall.java to do similar tests for invokeinterface.
- New JVM TI tests to verify the spec changes related to nest attributes.
- Existing tests significantly affected by the nestmates changes, primarily: - runtime/SelectionResolution
In most cases the nestmate changes makes certain invocations that were illegal, legal (e.g. not requiring invokespecial to invoke private interface methods; allowing access to private members via reflection/Methodhandles that were previously not allowed).
- Existing tests incidentally affected by the nestmate changes
This includes tests of things utilising class redefinition/retransformation to alter nested types but which unintentionally alter nest relationships (which is not permitted).
There are still a number of tests problem-listed with issues filed against them to have them adapted to work with nestmates. Some of these are intended to be addressed in the short-term, while some (such as the runtime/SelectionResolution test changes) may not eventuate.
- https://bugs.openjdk.java.net/browse/JDK-8203033 - https://bugs.openjdk.java.net/browse/JDK-8199450 - https://bugs.openjdk.java.net/browse/JDK-8196855 - https://bugs.openjdk.java.net/browse/JDK-8194857 - https://bugs.openjdk.java.net/browse/JDK-8187655
There is also further test work still to be completed (the JNI and JDI invocation tests): - https://bugs.openjdk.java.net/browse/JDK-8191117 which will continue in parallel with the main RFR.
Pre-integration Testing: - General: - Mach5: hs/jdk tier1,2 - Mach5: hs-nightly (tiers 1 -3) - Targetted - nashorn (for asm changes) - hotspot: runtime/* serviceability/* compiler/* vmTestbase/* - jdk: java/lang/invoke/* java/lang/reflect/* java/lang/instrument/* java/lang/Class/* java/lang/management/* - langtools: tools/javac tools/javap
On 6/11/18 10:16 PM, David Holmes wrote:
Here is one further minor update from the CSR discussions:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v5-incr/s...
"This implementation" is fine, as used in many @implNote. Any reason why it has to be changed to "reference implementation"? Mandy
On 6/11/2018 10:38 PM, mandy chung wrote:
On 6/11/18 10:16 PM, David Holmes wrote:
Here is one further minor update from the CSR discussions:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v5-incr/s...
"This implementation" is fine, as used in many @implNote. Any reason why it has to be changed to "reference implementation"?
To summarize the concern there, the phrase "This implementation..." when used elsewhere has a different meaning. Often the phrasing "This implementation..." or the more commonly used "The default implementation..." is used for text that is part of the contract of a method that can be overridden; that is, used to separate out the contract that is independent of which class or interface provides the implementation from the contract of a particular implementation. One example from an API I work on occurs for the method javax.lang.model.element.ElementVisitor.visitModule. The default method defined in an interface states "The default implementation visits a ModuleElement by calling visitUnknown..." and then various visitor classes define their own behavior for this method while still being able to @inheritDoc the general "visit a module element" contract of the visitModule method. However, java.lang.Class is final so for a particular JDK version there is only one implementation of the method in question. In that context "This implementation" doesn't mean "the implementation in this particular class or interface as opposed to the implementation in an a more specific subtype" it means "the implemetnation for the final method used in a particular JDK release." I think using the term "This implementation" in the latter context is misleading so I suggested the alternative wording David sent out for review. HTH, -Joe
Thanks for the explanation, Joe. I see the subtle difference on these terminologies. I'm okay with this patch. I like the other option better to remove @apiNote since the spec states that the duplicates may not be removed. Mandy On 6/12/18 5:49 PM, Joseph D. Darcy wrote:
On 6/11/2018 10:38 PM, mandy chung wrote:
On 6/11/18 10:16 PM, David Holmes wrote:
Here is one further minor update from the CSR discussions:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v5-incr/s...
"This implementation" is fine, as used in many @implNote. Any reason why it has to be changed to "reference implementation"?
To summarize the concern there, the phrase "This implementation..." when used elsewhere has a different meaning.
Often the phrasing "This implementation..." or the more commonly used "The default implementation..." is used for text that is part of the contract of a method that can be overridden; that is, used to separate out the contract that is independent of which class or interface provides the implementation from the contract of a particular implementation.
One example from an API I work on occurs for the method javax.lang.model.element.ElementVisitor.visitModule. The default method defined in an interface states "The default implementation visits a ModuleElement by calling visitUnknown..." and then various visitor classes define their own behavior for this method while still being able to @inheritDoc the general "visit a module element" contract of the visitModule method.
However, java.lang.Class is final so for a particular JDK version there is only one implementation of the method in question. In that context "This implementation" doesn't mean "the implementation in this particular class or interface as opposed to the implementation in an a more specific subtype" it means "the implemetnation for the final method used in a particular JDK release."
I think using the term "This implementation" in the latter context is misleading so I suggested the alternative wording David sent out for review.
HTH,
-Joe
Discussions on the CSR request have led to further changes to the documentation involving nests and the new nest-related method. There are no semantic changes here just clearer explanations. Incremental webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v7-incr/ (don't worry if you don't see a v6, it didn't really exist). Full webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.full.v7/ Specdiffs updated in place at: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/specs/ Summary of changes: - The definition of a nest etc is moved to the class-level javadoc of java.lang.Class, along with some other edits provided by Alex Buckley to pave the way for future updates - The nest-related methods are written in a more clear and consistent way Thanks, David ----- On 12/06/2018 3:16 PM, David Holmes wrote:
Here is one further minor update from the CSR discussions:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v5-incr/s...
Thanks, David
On 25/05/2018 3:52 PM, David Holmes wrote:
Here are the further minor updates so far in response to all the review comments.
Incremental corelibs webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v3-incr/
Full corelibs webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v3/
Change summary:
src/java.base/share/classes/jdk/internal/reflect/Reflection.java - remove inaccurate pseudo-assertion comment
test/jdk/java/lang/reflect/Nestmates/SampleNest.java - code cleanup: <> operator
test/jdk/java/lang/reflect/Nestmates/TestReflectionAPI.java - code cleanup: streamify duplicate removals
test/jdk/java/lang/invoke/PrivateInterfaceCall.java - use consistent @bug number
Thanks, David
On 22/05/2018 8:15 PM, David Holmes wrote:
Here are the updates so far in response to all the review comments.
Incremental webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v2-incr/
Full webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v2/
Change summary:
src/java.base/share/classes/java/lang/Class.java - getNesthost: - change "any error" -> "any linkage error" as runtime errors will propagate. [This needs ratifying by EG] - add clarification that primitive and array classes are not explicitly members of any nest and so form singleton nests - add clarification that all nestmates are in the same package - re-word @return text to exclude the "royal 'we'" - fix javadoc cross references
---
Moved reflection API tests from test/hotspot/jtreg/runtime/Nestmates/reflectionAPI/ to test/jdk/java/lang/reflect/Nestmates/
---
java/lang/reflect/Nestmates/TestReflectionAPI.java
Run tests twice to show that failure reasons remain the same.
---
test/jdk/jdk/lambda/vm/InterfaceAccessFlagsTest.java
Disable test via annotation rather than commenting out.
---
src/java.base/share/classes/jdk/internal/reflect/Reflection.java
- Fix indent for nestmate access check. - Remove unnecessary local variable
---
src/java.base/share/classes/sun/invoke/util/VerifyAccess.java
- Replace myassert with proper assert
---
Thanks, David
On 15/05/2018 10:52 AM, David Holmes wrote:
This review is being spread across four groups: langtools, core-libs, hotspot and serviceability. This is the specific review thread for core-libs - webrev:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/
See below for full details - including annotated full webrev guiding the review.
The intent is to have JEP-181 targeted and integrated by the end of this month.
Thanks, David -----
The nestmates project (JEP-181) introduces new classfile attributes to identify classes and interfaces in the same nest, so that the VM can perform access control based on those attributes and so allow direct private access between nestmates without requiring javac to generate synthetic accessor methods. These access control changes also extend to core reflection and the MethodHandle.Lookup contexts.
Direct private calls between nestmates requires a more general calling context than is permitted by invokespecial, and so the JVMS is updated to allow, and javac updated to use, invokevirtual and invokeinterface for private class and interface method calls respectively. These changed semantics also extend to MethodHandle findXXX operations.
At this time we are only concerned with static nest definitions, which map to a top-level class/interface as the nest-host and all its nested types as nest-members.
Please see the JEP for further details.
JEP: https://bugs.openjdk.java.net/browse/JDK-8046171 Bug: https://bugs.openjdk.java.net/browse/JDK-8010319 CSR: https://bugs.openjdk.java.net/browse/JDK-8197445
All of the specification changes have been previously been worked out by the Valhalla Project Expert Group, and the implementation reviewed by the various contributors and discussed on the valhalla-dev mailing list.
Acknowledgments and contributions: Alex Buckley, Maurizio Cimadamore, Mandy Chung, Tobias Hartmann, Vladimir Ivanov, Karen Kinnear, Vladimir Kozlov, John Rose, Dan Smith, Serguei Spitsyn, Kumar Srinivasan
Master webrev of all changes:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.full.v1/
Annotated master webrev index:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/jep181-webrev.html
Performance: this is expected to be performance neutral in a general sense. Benchmarking and performance runs are about to start.
Testing Discussion: ------------------
The testing for nestmates can be broken into four main groups:
- New tests specifically related to nestmates and currently in the runtime/Nestmates directory
- New tests to complement existing tests by adding in testcases not previously expressible. - For example java/lang/invoke/SpecialInterfaceCall.java tests use of invokespecial for private interface methods and performing receiver typechecks, so we add java/lang/invoke/PrivateInterfaceCall.java to do similar tests for invokeinterface.
- New JVM TI tests to verify the spec changes related to nest attributes.
- Existing tests significantly affected by the nestmates changes, primarily: - runtime/SelectionResolution
In most cases the nestmate changes makes certain invocations that were illegal, legal (e.g. not requiring invokespecial to invoke private interface methods; allowing access to private members via reflection/Methodhandles that were previously not allowed).
- Existing tests incidentally affected by the nestmate changes
This includes tests of things utilising class redefinition/retransformation to alter nested types but which unintentionally alter nest relationships (which is not permitted).
There are still a number of tests problem-listed with issues filed against them to have them adapted to work with nestmates. Some of these are intended to be addressed in the short-term, while some (such as the runtime/SelectionResolution test changes) may not eventuate.
- https://bugs.openjdk.java.net/browse/JDK-8203033 - https://bugs.openjdk.java.net/browse/JDK-8199450 - https://bugs.openjdk.java.net/browse/JDK-8196855 - https://bugs.openjdk.java.net/browse/JDK-8194857 - https://bugs.openjdk.java.net/browse/JDK-8187655
There is also further test work still to be completed (the JNI and JDI invocation tests): - https://bugs.openjdk.java.net/browse/JDK-8191117 which will continue in parallel with the main RFR.
Pre-integration Testing: - General: - Mach5: hs/jdk tier1,2 - Mach5: hs-nightly (tiers 1 -3) - Targetted - nashorn (for asm changes) - hotspot: runtime/* serviceability/* compiler/* vmTestbase/* - jdk: java/lang/invoke/* java/lang/reflect/* java/lang/instrument/* java/lang/Class/* java/lang/management/* - langtools: tools/javac tools/javap
Sorry another update is imminent ... stay tuned. David On 19/06/2018 2:41 PM, David Holmes wrote:
Discussions on the CSR request have led to further changes to the documentation involving nests and the new nest-related method. There are no semantic changes here just clearer explanations.
Incremental webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v7-incr/
(don't worry if you don't see a v6, it didn't really exist).
Full webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.full.v7/
Specdiffs updated in place at: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/specs/
Summary of changes:
- The definition of a nest etc is moved to the class-level javadoc of java.lang.Class, along with some other edits provided by Alex Buckley to pave the way for future updates - The nest-related methods are written in a more clear and consistent way
Thanks, David -----
On 12/06/2018 3:16 PM, David Holmes wrote:
Here is one further minor update from the CSR discussions:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v5-incr/s...
Thanks, David
On 25/05/2018 3:52 PM, David Holmes wrote:
Here are the further minor updates so far in response to all the review comments.
Incremental corelibs webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v3-incr/
Full corelibs webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v3/
Change summary:
src/java.base/share/classes/jdk/internal/reflect/Reflection.java - remove inaccurate pseudo-assertion comment
test/jdk/java/lang/reflect/Nestmates/SampleNest.java - code cleanup: <> operator
test/jdk/java/lang/reflect/Nestmates/TestReflectionAPI.java - code cleanup: streamify duplicate removals
test/jdk/java/lang/invoke/PrivateInterfaceCall.java - use consistent @bug number
Thanks, David
On 22/05/2018 8:15 PM, David Holmes wrote:
Here are the updates so far in response to all the review comments.
Incremental webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v2-incr/
Full webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v2/
Change summary:
src/java.base/share/classes/java/lang/Class.java - getNesthost: - change "any error" -> "any linkage error" as runtime errors will propagate. [This needs ratifying by EG] - add clarification that primitive and array classes are not explicitly members of any nest and so form singleton nests - add clarification that all nestmates are in the same package - re-word @return text to exclude the "royal 'we'" - fix javadoc cross references
---
Moved reflection API tests from test/hotspot/jtreg/runtime/Nestmates/reflectionAPI/ to test/jdk/java/lang/reflect/Nestmates/
---
java/lang/reflect/Nestmates/TestReflectionAPI.java
Run tests twice to show that failure reasons remain the same.
---
test/jdk/jdk/lambda/vm/InterfaceAccessFlagsTest.java
Disable test via annotation rather than commenting out.
---
src/java.base/share/classes/jdk/internal/reflect/Reflection.java
- Fix indent for nestmate access check. - Remove unnecessary local variable
---
src/java.base/share/classes/sun/invoke/util/VerifyAccess.java
- Replace myassert with proper assert
---
Thanks, David
On 15/05/2018 10:52 AM, David Holmes wrote:
This review is being spread across four groups: langtools, core-libs, hotspot and serviceability. This is the specific review thread for core-libs - webrev:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/
See below for full details - including annotated full webrev guiding the review.
The intent is to have JEP-181 targeted and integrated by the end of this month.
Thanks, David -----
The nestmates project (JEP-181) introduces new classfile attributes to identify classes and interfaces in the same nest, so that the VM can perform access control based on those attributes and so allow direct private access between nestmates without requiring javac to generate synthetic accessor methods. These access control changes also extend to core reflection and the MethodHandle.Lookup contexts.
Direct private calls between nestmates requires a more general calling context than is permitted by invokespecial, and so the JVMS is updated to allow, and javac updated to use, invokevirtual and invokeinterface for private class and interface method calls respectively. These changed semantics also extend to MethodHandle findXXX operations.
At this time we are only concerned with static nest definitions, which map to a top-level class/interface as the nest-host and all its nested types as nest-members.
Please see the JEP for further details.
JEP: https://bugs.openjdk.java.net/browse/JDK-8046171 Bug: https://bugs.openjdk.java.net/browse/JDK-8010319 CSR: https://bugs.openjdk.java.net/browse/JDK-8197445
All of the specification changes have been previously been worked out by the Valhalla Project Expert Group, and the implementation reviewed by the various contributors and discussed on the valhalla-dev mailing list.
Acknowledgments and contributions: Alex Buckley, Maurizio Cimadamore, Mandy Chung, Tobias Hartmann, Vladimir Ivanov, Karen Kinnear, Vladimir Kozlov, John Rose, Dan Smith, Serguei Spitsyn, Kumar Srinivasan
Master webrev of all changes:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.full.v1/
Annotated master webrev index:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/jep181-webrev.html
Performance: this is expected to be performance neutral in a general sense. Benchmarking and performance runs are about to start.
Testing Discussion: ------------------
The testing for nestmates can be broken into four main groups:
- New tests specifically related to nestmates and currently in the runtime/Nestmates directory
- New tests to complement existing tests by adding in testcases not previously expressible. - For example java/lang/invoke/SpecialInterfaceCall.java tests use of invokespecial for private interface methods and performing receiver typechecks, so we add java/lang/invoke/PrivateInterfaceCall.java to do similar tests for invokeinterface.
- New JVM TI tests to verify the spec changes related to nest attributes.
- Existing tests significantly affected by the nestmates changes, primarily: - runtime/SelectionResolution
In most cases the nestmate changes makes certain invocations that were illegal, legal (e.g. not requiring invokespecial to invoke private interface methods; allowing access to private members via reflection/Methodhandles that were previously not allowed).
- Existing tests incidentally affected by the nestmate changes
This includes tests of things utilising class redefinition/retransformation to alter nested types but which unintentionally alter nest relationships (which is not permitted).
There are still a number of tests problem-listed with issues filed against them to have them adapted to work with nestmates. Some of these are intended to be addressed in the short-term, while some (such as the runtime/SelectionResolution test changes) may not eventuate.
- https://bugs.openjdk.java.net/browse/JDK-8203033 - https://bugs.openjdk.java.net/browse/JDK-8199450 - https://bugs.openjdk.java.net/browse/JDK-8196855 - https://bugs.openjdk.java.net/browse/JDK-8194857 - https://bugs.openjdk.java.net/browse/JDK-8187655
There is also further test work still to be completed (the JNI and JDI invocation tests): - https://bugs.openjdk.java.net/browse/JDK-8191117 which will continue in parallel with the main RFR.
Pre-integration Testing: - General: - Mach5: hs/jdk tier1,2 - Mach5: hs-nightly (tiers 1 -3) - Targetted - nashorn (for asm changes) - hotspot: runtime/* serviceability/* compiler/* vmTestbase/* - jdk: java/lang/invoke/* java/lang/reflect/* java/lang/instrument/* java/lang/Class/* java/lang/management/* - langtools: tools/javac tools/javap
Some further adjustments to getNestMembers() was made. Everything updated in place. Thanks, David On 20/06/2018 9:30 AM, David Holmes wrote:
Sorry another update is imminent ... stay tuned.
David
On 19/06/2018 2:41 PM, David Holmes wrote:
Discussions on the CSR request have led to further changes to the documentation involving nests and the new nest-related method. There are no semantic changes here just clearer explanations.
Incremental webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v7-incr/
(don't worry if you don't see a v6, it didn't really exist).
Full webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.full.v7/
Specdiffs updated in place at: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/specs/
Summary of changes:
- The definition of a nest etc is moved to the class-level javadoc of java.lang.Class, along with some other edits provided by Alex Buckley to pave the way for future updates - The nest-related methods are written in a more clear and consistent way
Thanks, David -----
On 12/06/2018 3:16 PM, David Holmes wrote:
Here is one further minor update from the CSR discussions:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v5-incr/s...
Thanks, David
On 25/05/2018 3:52 PM, David Holmes wrote:
Here are the further minor updates so far in response to all the review comments.
Incremental corelibs webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v3-incr/
Full corelibs webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v3/
Change summary:
src/java.base/share/classes/jdk/internal/reflect/Reflection.java - remove inaccurate pseudo-assertion comment
test/jdk/java/lang/reflect/Nestmates/SampleNest.java - code cleanup: <> operator
test/jdk/java/lang/reflect/Nestmates/TestReflectionAPI.java - code cleanup: streamify duplicate removals
test/jdk/java/lang/invoke/PrivateInterfaceCall.java - use consistent @bug number
Thanks, David
On 22/05/2018 8:15 PM, David Holmes wrote:
Here are the updates so far in response to all the review comments.
Incremental webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v2-incr/
Full webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v2/
Change summary:
src/java.base/share/classes/java/lang/Class.java - getNesthost: - change "any error" -> "any linkage error" as runtime errors will propagate. [This needs ratifying by EG] - add clarification that primitive and array classes are not explicitly members of any nest and so form singleton nests - add clarification that all nestmates are in the same package - re-word @return text to exclude the "royal 'we'" - fix javadoc cross references
---
Moved reflection API tests from test/hotspot/jtreg/runtime/Nestmates/reflectionAPI/ to test/jdk/java/lang/reflect/Nestmates/
---
java/lang/reflect/Nestmates/TestReflectionAPI.java
Run tests twice to show that failure reasons remain the same.
---
test/jdk/jdk/lambda/vm/InterfaceAccessFlagsTest.java
Disable test via annotation rather than commenting out.
---
src/java.base/share/classes/jdk/internal/reflect/Reflection.java
- Fix indent for nestmate access check. - Remove unnecessary local variable
---
src/java.base/share/classes/sun/invoke/util/VerifyAccess.java
- Replace myassert with proper assert
---
Thanks, David
On 15/05/2018 10:52 AM, David Holmes wrote:
This review is being spread across four groups: langtools, core-libs, hotspot and serviceability. This is the specific review thread for core-libs - webrev:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/
See below for full details - including annotated full webrev guiding the review.
The intent is to have JEP-181 targeted and integrated by the end of this month.
Thanks, David -----
The nestmates project (JEP-181) introduces new classfile attributes to identify classes and interfaces in the same nest, so that the VM can perform access control based on those attributes and so allow direct private access between nestmates without requiring javac to generate synthetic accessor methods. These access control changes also extend to core reflection and the MethodHandle.Lookup contexts.
Direct private calls between nestmates requires a more general calling context than is permitted by invokespecial, and so the JVMS is updated to allow, and javac updated to use, invokevirtual and invokeinterface for private class and interface method calls respectively. These changed semantics also extend to MethodHandle findXXX operations.
At this time we are only concerned with static nest definitions, which map to a top-level class/interface as the nest-host and all its nested types as nest-members.
Please see the JEP for further details.
JEP: https://bugs.openjdk.java.net/browse/JDK-8046171 Bug: https://bugs.openjdk.java.net/browse/JDK-8010319 CSR: https://bugs.openjdk.java.net/browse/JDK-8197445
All of the specification changes have been previously been worked out by the Valhalla Project Expert Group, and the implementation reviewed by the various contributors and discussed on the valhalla-dev mailing list.
Acknowledgments and contributions: Alex Buckley, Maurizio Cimadamore, Mandy Chung, Tobias Hartmann, Vladimir Ivanov, Karen Kinnear, Vladimir Kozlov, John Rose, Dan Smith, Serguei Spitsyn, Kumar Srinivasan
Master webrev of all changes:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.full.v1/
Annotated master webrev index:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/jep181-webrev.html
Performance: this is expected to be performance neutral in a general sense. Benchmarking and performance runs are about to start.
Testing Discussion: ------------------
The testing for nestmates can be broken into four main groups:
- New tests specifically related to nestmates and currently in the runtime/Nestmates directory
- New tests to complement existing tests by adding in testcases not previously expressible. - For example java/lang/invoke/SpecialInterfaceCall.java tests use of invokespecial for private interface methods and performing receiver typechecks, so we add java/lang/invoke/PrivateInterfaceCall.java to do similar tests for invokeinterface.
- New JVM TI tests to verify the spec changes related to nest attributes.
- Existing tests significantly affected by the nestmates changes, primarily: - runtime/SelectionResolution
In most cases the nestmate changes makes certain invocations that were illegal, legal (e.g. not requiring invokespecial to invoke private interface methods; allowing access to private members via reflection/Methodhandles that were previously not allowed).
- Existing tests incidentally affected by the nestmate changes
This includes tests of things utilising class redefinition/retransformation to alter nested types but which unintentionally alter nest relationships (which is not permitted).
There are still a number of tests problem-listed with issues filed against them to have them adapted to work with nestmates. Some of these are intended to be addressed in the short-term, while some (such as the runtime/SelectionResolution test changes) may not eventuate.
- https://bugs.openjdk.java.net/browse/JDK-8203033 - https://bugs.openjdk.java.net/browse/JDK-8199450 - https://bugs.openjdk.java.net/browse/JDK-8196855 - https://bugs.openjdk.java.net/browse/JDK-8194857 - https://bugs.openjdk.java.net/browse/JDK-8187655
There is also further test work still to be completed (the JNI and JDI invocation tests): - https://bugs.openjdk.java.net/browse/JDK-8191117 which will continue in parallel with the main RFR.
Pre-integration Testing: - General: - Mach5: hs/jdk tier1,2 - Mach5: hs-nightly (tiers 1 -3) - Targetted - nashorn (for asm changes) - hotspot: runtime/* serviceability/* compiler/* vmTestbase/* - jdk: java/lang/invoke/* java/lang/reflect/* java/lang/instrument/* java/lang/Class/* java/lang/management/* - langtools: tools/javac tools/javap
The javadoc update looks good to me. Mandy On 6/19/18 9:56 PM, David Holmes wrote:
Some further adjustments to getNestMembers() was made. Everything updated in place.
Thanks, David
On 20/06/2018 9:30 AM, David Holmes wrote:
Sorry another update is imminent ... stay tuned.
David
On 19/06/2018 2:41 PM, David Holmes wrote:
Discussions on the CSR request have led to further changes to the documentation involving nests and the new nest-related method. There are no semantic changes here just clearer explanations.
Incremental webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v7-incr/
(don't worry if you don't see a v6, it didn't really exist).
Full webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.full.v7/
Specdiffs updated in place at: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/specs/
Summary of changes:
- The definition of a nest etc is moved to the class-level javadoc of java.lang.Class, along with some other edits provided by Alex Buckley to pave the way for future updates - The nest-related methods are written in a more clear and consistent way
Thanks, David
Thanks Mandy! David On 20/06/2018 3:25 PM, mandy chung wrote:
The javadoc update looks good to me.
Mandy
On 6/19/18 9:56 PM, David Holmes wrote:
Some further adjustments to getNestMembers() was made. Everything updated in place.
Thanks, David
On 20/06/2018 9:30 AM, David Holmes wrote:
Sorry another update is imminent ... stay tuned.
David
On 19/06/2018 2:41 PM, David Holmes wrote:
Discussions on the CSR request have led to further changes to the documentation involving nests and the new nest-related method. There are no semantic changes here just clearer explanations.
Incremental webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v7-incr/
(don't worry if you don't see a v6, it didn't really exist).
Full webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.full.v7/
Specdiffs updated in place at: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/specs/
Summary of changes:
- The definition of a nest etc is moved to the class-level javadoc of java.lang.Class, along with some other edits provided by Alex Buckley to pave the way for future updates - The nest-related methods are written in a more clear and consistent way
Thanks, David
On Jun 19, 2018, at 10:25 PM, mandy chung <mandy.chung@oracle.com> wrote:
The javadoc update looks good to me.
Same here, +1 Paul.
Good; I like the care to distinguish "nested" classes (using the word "enclosing") from the newer concept of "nests" and "nestmates", as well as the careful framing of how stuff bubbles up from the classfile. (Kudos to Alex!) — John On Jun 19, 2018, at 9:56 PM, David Holmes <david.holmes@oracle.com> wrote:
Some further adjustments to getNestMembers() was made. Everything updated in place.
Thanks, David
On 20/06/2018 9:30 AM, David Holmes wrote:
Sorry another update is imminent ... stay tuned. David On 19/06/2018 2:41 PM, David Holmes wrote:
Discussions on the CSR request have led to further changes to the documentation involving nests and the new nest-related method. There are no semantic changes here just clearer explanations.
Incremental webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v7-incr/ <http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v7-incr/>
Thanks John! On 21/06/2018 6:15 AM, John Rose wrote:
Good; I like the care to distinguish "nested" classes (using the word "enclosing") from the newer concept of "nests" and "nestmates", as well as the careful framing of how stuff bubbles up from the classfile.
(Kudos to Alex!)
Yes this is much improved. David
— John
On Jun 19, 2018, at 9:56 PM, David Holmes <david.holmes@oracle.com <mailto:david.holmes@oracle.com>> wrote:
Some further adjustments to getNestMembers() was made. Everything updated in place.
Thanks, David
On 20/06/2018 9:30 AM, David Holmes wrote:
Sorry another update is imminent ... stay tuned. David On 19/06/2018 2:41 PM, David Holmes wrote:
Discussions on the CSR request have led to further changes to the documentation involving nests and the new nest-related method. There are no semantic changes here just clearer explanations.
Incremental webrev:http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v7-incr/
Looks good. Can you send the updates to the valhalla spec experts please? We told them this was coming, and minor changes for clarification. thanks, Karen
On Jun 19, 2018, at 12:41 AM, David Holmes <david.holmes@oracle.com> wrote:
Discussions on the CSR request have led to further changes to the documentation involving nests and the new nest-related method. There are no semantic changes here just clearer explanations.
Incremental webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v7-incr/
(don't worry if you don't see a v6, it didn't really exist).
Full webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.full.v7/
Specdiffs updated in place at: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/specs/
Summary of changes:
- The definition of a nest etc is moved to the class-level javadoc of java.lang.Class, along with some other edits provided by Alex Buckley to pave the way for future updates - The nest-related methods are written in a more clear and consistent way
Thanks, David -----
On 12/06/2018 3:16 PM, David Holmes wrote:
Here is one further minor update from the CSR discussions: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v5-incr/s... Thanks, David On 25/05/2018 3:52 PM, David Holmes wrote:
Here are the further minor updates so far in response to all the review comments.
Incremental corelibs webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v3-incr/
Full corelibs webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v3/
Change summary:
src/java.base/share/classes/jdk/internal/reflect/Reflection.java - remove inaccurate pseudo-assertion comment
test/jdk/java/lang/reflect/Nestmates/SampleNest.java - code cleanup: <> operator
test/jdk/java/lang/reflect/Nestmates/TestReflectionAPI.java - code cleanup: streamify duplicate removals
test/jdk/java/lang/invoke/PrivateInterfaceCall.java - use consistent @bug number
Thanks, David
On 22/05/2018 8:15 PM, David Holmes wrote:
Here are the updates so far in response to all the review comments.
Incremental webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v2-incr/
Full webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v2/
Change summary:
src/java.base/share/classes/java/lang/Class.java - getNesthost: - change "any error" -> "any linkage error" as runtime errors will propagate. [This needs ratifying by EG] - add clarification that primitive and array classes are not explicitly members of any nest and so form singleton nests - add clarification that all nestmates are in the same package - re-word @return text to exclude the "royal 'we'" - fix javadoc cross references
---
Moved reflection API tests from test/hotspot/jtreg/runtime/Nestmates/reflectionAPI/ to test/jdk/java/lang/reflect/Nestmates/
---
java/lang/reflect/Nestmates/TestReflectionAPI.java
Run tests twice to show that failure reasons remain the same.
---
test/jdk/jdk/lambda/vm/InterfaceAccessFlagsTest.java
Disable test via annotation rather than commenting out.
---
src/java.base/share/classes/jdk/internal/reflect/Reflection.java
- Fix indent for nestmate access check. - Remove unnecessary local variable
---
src/java.base/share/classes/sun/invoke/util/VerifyAccess.java
- Replace myassert with proper assert
---
Thanks, David
On 15/05/2018 10:52 AM, David Holmes wrote:
This review is being spread across four groups: langtools, core-libs, hotspot and serviceability. This is the specific review thread for core-libs - webrev:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/
See below for full details - including annotated full webrev guiding the review.
The intent is to have JEP-181 targeted and integrated by the end of this month.
Thanks, David -----
The nestmates project (JEP-181) introduces new classfile attributes to identify classes and interfaces in the same nest, so that the VM can perform access control based on those attributes and so allow direct private access between nestmates without requiring javac to generate synthetic accessor methods. These access control changes also extend to core reflection and the MethodHandle.Lookup contexts.
Direct private calls between nestmates requires a more general calling context than is permitted by invokespecial, and so the JVMS is updated to allow, and javac updated to use, invokevirtual and invokeinterface for private class and interface method calls respectively. These changed semantics also extend to MethodHandle findXXX operations.
At this time we are only concerned with static nest definitions, which map to a top-level class/interface as the nest-host and all its nested types as nest-members.
Please see the JEP for further details.
JEP: https://bugs.openjdk.java.net/browse/JDK-8046171 Bug: https://bugs.openjdk.java.net/browse/JDK-8010319 CSR: https://bugs.openjdk.java.net/browse/JDK-8197445
All of the specification changes have been previously been worked out by the Valhalla Project Expert Group, and the implementation reviewed by the various contributors and discussed on the valhalla-dev mailing list.
Acknowledgments and contributions: Alex Buckley, Maurizio Cimadamore, Mandy Chung, Tobias Hartmann, Vladimir Ivanov, Karen Kinnear, Vladimir Kozlov, John Rose, Dan Smith, Serguei Spitsyn, Kumar Srinivasan
Master webrev of all changes:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.full.v1/
Annotated master webrev index:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/jep181-webrev.html
Performance: this is expected to be performance neutral in a general sense. Benchmarking and performance runs are about to start.
Testing Discussion: ------------------
The testing for nestmates can be broken into four main groups:
- New tests specifically related to nestmates and currently in the runtime/Nestmates directory
- New tests to complement existing tests by adding in testcases not previously expressible. - For example java/lang/invoke/SpecialInterfaceCall.java tests use of invokespecial for private interface methods and performing receiver typechecks, so we add java/lang/invoke/PrivateInterfaceCall.java to do similar tests for invokeinterface.
- New JVM TI tests to verify the spec changes related to nest attributes.
- Existing tests significantly affected by the nestmates changes, primarily: - runtime/SelectionResolution
In most cases the nestmate changes makes certain invocations that were illegal, legal (e.g. not requiring invokespecial to invoke private interface methods; allowing access to private members via reflection/Methodhandles that were previously not allowed).
- Existing tests incidentally affected by the nestmate changes
This includes tests of things utilising class redefinition/retransformation to alter nested types but which unintentionally alter nest relationships (which is not permitted).
There are still a number of tests problem-listed with issues filed against them to have them adapted to work with nestmates. Some of these are intended to be addressed in the short-term, while some (such as the runtime/SelectionResolution test changes) may not eventuate.
- https://bugs.openjdk.java.net/browse/JDK-8203033 - https://bugs.openjdk.java.net/browse/JDK-8199450 - https://bugs.openjdk.java.net/browse/JDK-8196855 - https://bugs.openjdk.java.net/browse/JDK-8194857 - https://bugs.openjdk.java.net/browse/JDK-8187655
There is also further test work still to be completed (the JNI and JDI invocation tests): - https://bugs.openjdk.java.net/browse/JDK-8191117 which will continue in parallel with the main RFR.
Pre-integration Testing: - General: - Mach5: hs/jdk tier1,2 - Mach5: hs-nightly (tiers 1 -3) - Targetted - nashorn (for asm changes) - hotspot: runtime/* serviceability/* compiler/* vmTestbase/* - jdk: java/lang/invoke/* java/lang/reflect/* java/lang/instrument/* java/lang/Class/* java/lang/management/* - langtools: tools/javac tools/javap
Hi Karen, On 21/06/2018 7:42 AM, Karen Kinnear wrote:
Looks good.
Thanks. Unfortunately there were a few more minor edits "overnight". Everything updated in place (apologies as I wanted to include a simple patch but overwrote things before realising I now had no means to do so.) Summary: - Class-level nest definition: use nestmates instead of members to avoid potential confusion/conflict with reference to "private members" "A _nest_ is a set of classes and interfaces... . The classes and interfaces are known as _nestmates_. One nestmate acts as the _nest host_, and enumerates the other nestmates which belong to the nest; each of them in turn records it as the nest host. ... - getNestHost - replace record withenumerate when referring to nest members (members record their nest host; nest hosts enumerate their members) -getNestMembers - replace @jvms ref with @see getNestHost
Can you send the updates to the valhalla spec experts please? We told them this was coming, and minor changes for clarification.
I already did: http://mail.openjdk.java.net/pipermail/valhalla-spec-experts/2018-June/00071... Thanks, David
thanks, Karen
On Jun 19, 2018, at 12:41 AM, David Holmes <david.holmes@oracle.com> wrote:
Discussions on the CSR request have led to further changes to the documentation involving nests and the new nest-related method. There are no semantic changes here just clearer explanations.
Incremental webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v7-incr/
(don't worry if you don't see a v6, it didn't really exist).
Full webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.full.v7/
Specdiffs updated in place at: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/specs/
Summary of changes:
- The definition of a nest etc is moved to the class-level javadoc of java.lang.Class, along with some other edits provided by Alex Buckley to pave the way for future updates - The nest-related methods are written in a more clear and consistent way
Thanks, David -----
On 12/06/2018 3:16 PM, David Holmes wrote:
Here is one further minor update from the CSR discussions: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v5-incr/s... Thanks, David On 25/05/2018 3:52 PM, David Holmes wrote:
Here are the further minor updates so far in response to all the review comments.
Incremental corelibs webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v3-incr/
Full corelibs webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v3/
Change summary:
src/java.base/share/classes/jdk/internal/reflect/Reflection.java - remove inaccurate pseudo-assertion comment
test/jdk/java/lang/reflect/Nestmates/SampleNest.java - code cleanup: <> operator
test/jdk/java/lang/reflect/Nestmates/TestReflectionAPI.java - code cleanup: streamify duplicate removals
test/jdk/java/lang/invoke/PrivateInterfaceCall.java - use consistent @bug number
Thanks, David
On 22/05/2018 8:15 PM, David Holmes wrote:
Here are the updates so far in response to all the review comments.
Incremental webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v2-incr/
Full webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v2/
Change summary:
src/java.base/share/classes/java/lang/Class.java - getNesthost: - change "any error" -> "any linkage error" as runtime errors will propagate. [This needs ratifying by EG] - add clarification that primitive and array classes are not explicitly members of any nest and so form singleton nests - add clarification that all nestmates are in the same package - re-word @return text to exclude the "royal 'we'" - fix javadoc cross references
---
Moved reflection API tests from test/hotspot/jtreg/runtime/Nestmates/reflectionAPI/ to test/jdk/java/lang/reflect/Nestmates/
---
java/lang/reflect/Nestmates/TestReflectionAPI.java
Run tests twice to show that failure reasons remain the same.
---
test/jdk/jdk/lambda/vm/InterfaceAccessFlagsTest.java
Disable test via annotation rather than commenting out.
---
src/java.base/share/classes/jdk/internal/reflect/Reflection.java
- Fix indent for nestmate access check. - Remove unnecessary local variable
---
src/java.base/share/classes/sun/invoke/util/VerifyAccess.java
- Replace myassert with proper assert
---
Thanks, David
On 15/05/2018 10:52 AM, David Holmes wrote:
This review is being spread across four groups: langtools, core-libs, hotspot and serviceability. This is the specific review thread for core-libs - webrev:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/
See below for full details - including annotated full webrev guiding the review.
The intent is to have JEP-181 targeted and integrated by the end of this month.
Thanks, David -----
The nestmates project (JEP-181) introduces new classfile attributes to identify classes and interfaces in the same nest, so that the VM can perform access control based on those attributes and so allow direct private access between nestmates without requiring javac to generate synthetic accessor methods. These access control changes also extend to core reflection and the MethodHandle.Lookup contexts.
Direct private calls between nestmates requires a more general calling context than is permitted by invokespecial, and so the JVMS is updated to allow, and javac updated to use, invokevirtual and invokeinterface for private class and interface method calls respectively. These changed semantics also extend to MethodHandle findXXX operations.
At this time we are only concerned with static nest definitions, which map to a top-level class/interface as the nest-host and all its nested types as nest-members.
Please see the JEP for further details.
JEP: https://bugs.openjdk.java.net/browse/JDK-8046171 Bug: https://bugs.openjdk.java.net/browse/JDK-8010319 CSR: https://bugs.openjdk.java.net/browse/JDK-8197445
All of the specification changes have been previously been worked out by the Valhalla Project Expert Group, and the implementation reviewed by the various contributors and discussed on the valhalla-dev mailing list.
Acknowledgments and contributions: Alex Buckley, Maurizio Cimadamore, Mandy Chung, Tobias Hartmann, Vladimir Ivanov, Karen Kinnear, Vladimir Kozlov, John Rose, Dan Smith, Serguei Spitsyn, Kumar Srinivasan
Master webrev of all changes:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.full.v1/
Annotated master webrev index:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/jep181-webrev.html
Performance: this is expected to be performance neutral in a general sense. Benchmarking and performance runs are about to start.
Testing Discussion: ------------------
The testing for nestmates can be broken into four main groups:
- New tests specifically related to nestmates and currently in the runtime/Nestmates directory
- New tests to complement existing tests by adding in testcases not previously expressible. - For example java/lang/invoke/SpecialInterfaceCall.java tests use of invokespecial for private interface methods and performing receiver typechecks, so we add java/lang/invoke/PrivateInterfaceCall.java to do similar tests for invokeinterface.
- New JVM TI tests to verify the spec changes related to nest attributes.
- Existing tests significantly affected by the nestmates changes, primarily: - runtime/SelectionResolution
In most cases the nestmate changes makes certain invocations that were illegal, legal (e.g. not requiring invokespecial to invoke private interface methods; allowing access to private members via reflection/Methodhandles that were previously not allowed).
- Existing tests incidentally affected by the nestmate changes
This includes tests of things utilising class redefinition/retransformation to alter nested types but which unintentionally alter nest relationships (which is not permitted).
There are still a number of tests problem-listed with issues filed against them to have them adapted to work with nestmates. Some of these are intended to be addressed in the short-term, while some (such as the runtime/SelectionResolution test changes) may not eventuate.
- https://bugs.openjdk.java.net/browse/JDK-8203033 - https://bugs.openjdk.java.net/browse/JDK-8199450 - https://bugs.openjdk.java.net/browse/JDK-8196855 - https://bugs.openjdk.java.net/browse/JDK-8194857 - https://bugs.openjdk.java.net/browse/JDK-8187655
There is also further test work still to be completed (the JNI and JDI invocation tests): - https://bugs.openjdk.java.net/browse/JDK-8191117 which will continue in parallel with the main RFR.
Pre-integration Testing: - General: - Mach5: hs/jdk tier1,2 - Mach5: hs-nightly (tiers 1 -3) - Targetted - nashorn (for asm changes) - hotspot: runtime/* serviceability/* compiler/* vmTestbase/* - jdk: java/lang/invoke/* java/lang/reflect/* java/lang/instrument/* java/lang/Class/* java/lang/management/* - langtools: tools/javac tools/javap
Hi David, On 5/24/2018 10:52 PM, David Holmes wrote:
Here are the further minor updates so far in response to all the review comments.
Incremental corelibs webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v3-incr/
Full corelibs webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v3/
In Class.java, 3990 Class<?>[] members = getNestMembers0(); 3991 // Can't actually enable this due to bootstrapping issues 3992 // assert(members.length != 1 || members[0] == this); // expected invariant from VM can these checks be enabled unconditionally without using the assert facility, throwing AssertionError or some other kind of error? Thanks, -Joe
On 13/06/2018 4:08 PM, joe darcy wrote:
Hi David,
On 5/24/2018 10:52 PM, David Holmes wrote:
Here are the further minor updates so far in response to all the review comments.
Incremental corelibs webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v3-incr/
Full corelibs webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v3/
In Class.java,
3990 Class<?>[] members = getNestMembers0(); 3991 // Can't actually enable this due to bootstrapping issues 3992 // assert(members.length != 1 || members[0] == this); // expected invariant from VM
can these checks be enabled unconditionally without using the assert facility, throwing AssertionError or some other kind of error?
Of course - but we don't want to pay the price of always checking something that would indicate an error on the VM side. There's an equivalent assertion on the VM side. Thanks, David
Thanks,
-Joe
Hi David, On 05/15/2018 02:52 AM, David Holmes wrote:
Master webrev of all changes:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.full.v1/
I skimmed over reflection API changes. In jl.Class: 3911 // returning a different class requires a security check 3912 SecurityManager sm = System.getSecurityManager(); 3913 if (sm != null) { 3914 checkPackageAccess(sm, 3915 ClassLoader.getClassLoader(Reflection.getCallerClass()), true); 3916 } ...so here the "different" class is expected to be in the same package as "this" class. Is this invariant enforced in VM so it need not be checked here? 3871 * @apiNote The source language compiler is responsible for deciding which classes 3872 * and interfaces are nestmates, by generating the appropriate attributes 3873 * (JVMS 4.7.28 and 4.7.29) in the class file format (JVMS 4). 3874 * For example, the {@code javac} compiler 3875 * places a top-level class or interface into a nest with all of its direct, 3876 * and indirect, {@linkplain #getDeclaredClasses() nested classes and interfaces} 3877 * (JLS 8). 3878 * The top-level {@linkplain #getEnclosingClass() enclosing class or interface} 3879 * is designated as the nest host. Should the text warn about not relying on this implementation detail to extract knowledge about nested/enclosing classes? Users should keep using getDeclaredClasses()/getEnclosingClass() for that purpose as nestmates may in future be used for other things too. OTOH, if users want to do an equivalent of private access check (on behalf of real caller and callee - for example in some kind of language runtime), it would be better they use the nestmate API... in j.l.r.AccessibleMember: 49 * <p> Java language access control prevents use of private members *outside* 50 * *their top-level class;* package access members outside their package; protected members 51 * outside their package or subclasses; and public members outside their 52 * module unless they are declared in an {@link Module#isExported(String,Module) 53 * exported} package and the user {@link Module#canRead reads} their module. Could this be interpreted also as "private members can only be accessed from the top-level class"? I know that "nest" is not a Java language term, so it can not be used in the context of Java language access control. (If speaking of Java language, then even previous version of this text was wrong - if it was right, then it wouldn't have to be changed at all). So what about the following: "Java language access control prevents use of private members outside the set of classes composed of top-level class and its transitive closure of nested classes". Or would this be to lengthy and hard to understand? in j.l.r.Reflection: 140 // At this point we know that currentClass can access memberClass. 141 142 if (Modifier.isPublic(modifiers)) { 143 return true; 144 } 145 146 // Check for nestmate access if member is private 147 if (Modifier.isPrivate(modifiers)) { 148 // assert: isSubclassof(targetClass, memberClass) although just in a function of explaining the following comment, I think the correct assert is // assert targetClass == null || isSubclassof(targetClass, memberClass) as for static members, targetClass is null. 149 // Note: targetClass may be outside the nest, but that is okay 150 // as long as memberClass is in the nest. 151 boolean nestmates = areNestMates(currentClass, memberClass); 152 if (nestmates) { 153 return true; 154 } 155 } I'm wondering about the frequency of reflective accesses of various kinds of members. I imagine that most frequent are accesses of public members, others are less so (I'm not counting accesses made via .setAccessible(true) modified Field/Method/Constructor objects as they are bypassing this Reflection.verifyMemberAccess method). But among others I imagine reflective access to private members is the least frequent as there's no practical reason to use reflection inside and among classes that are most intimately logically coupled and "know" each other's internals at compile time. So it would make sense to check for private access at the end, after protected and package-private checks (that's how existing logic was structured). So how about putting the nestmate access logic just before the last if (!successSoFar) { return false; }: // Check for nestmate access if member is private if (!successSoFar && Modifier.isPrivate(modifiers)) { // assert: targetClass == null || isSubclassof(targetClass, memberClass) // Note: targetClass may be outside the nest, but that is okay // as long as memberClass is in the nest. successSoFar = areNestMates(currentClass, memberClass); } if (!successSoFar) { return false; } This would not penalize access to package-private and protected members with areNestMates() JNI calls and maybe caching will not be needed at all. Regards, Peter
On 05/22/2018 12:36 PM, Peter Levart wrote:
This would not penalize access to package-private and protected members with areNestMates() JNI calls and maybe caching will not be needed at all.
But if caching may need to be performed, then the simplest form of nestmate access check caching would be the caching of getNestHost() result inside the Class object. This need not be in the ReflectiveData object, but directly in the Class object as the invariant is that all members of the nest share the same package and consequently the same defining ClassLoader - there would be no ClassLoader leak(s) possible by keeping a hard reference to the cached nest-host Class object in each Class... Regards, Peter
On 05/22/2018 12:36 PM, Peter Levart wrote:
So how about putting the nestmate access logic just before the last if (!successSoFar) { return false; }:
// Check for nestmate access if member is private if (!successSoFar && Modifier.isPrivate(modifiers)) { // assert: targetClass == null || isSubclassof(targetClass, memberClass) // Note: targetClass may be outside the nest, but that is okay // as long as memberClass is in the nest. successSoFar = areNestMates(currentClass, memberClass); }
if (!successSoFar) { return false; }
This would not penalize access to package-private and protected members with areNestMates() JNI calls and maybe caching will not be needed at all.
Ah, just leave it as is. The if (Modifier.isPrivate(modifiers)) already makes sure that areNestMates is called only for private members. Peter
On 5/22/18 3:36 AM, Peter Levart wrote:
In jl.Class:
3911 // returning a different class requires a security check 3912 SecurityManager sm = System.getSecurityManager(); 3913 if (sm != null) { 3914 checkPackageAccess(sm, 3915 ClassLoader.getClassLoader(Reflection.getCallerClass()), true); 3916 }
...so here the "different" class is expected to be in the same package as "this" class. Is this invariant enforced in VM so it need not be checked here?
This permission check is to prevent leaking out nest host/members that are not accessible to any caller even it may get a hold of this class in package p. Mandy
Hi Peter, Thanks for looking at this. I'm glad I didn't try to respond last night before your follow up emails came through :) On 22/05/2018 8:36 PM, Peter Levart wrote:
Hi David,
On 05/15/2018 02:52 AM, David Holmes wrote:
Master webrev of all changes:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.full.v1/
I skimmed over reflection API changes.
In jl.Class:
3911 // returning a different class requires a security check 3912 SecurityManager sm = System.getSecurityManager(); 3913 if (sm != null) { 3914 checkPackageAccess(sm, 3915 ClassLoader.getClassLoader(Reflection.getCallerClass()), true); 3916 }
...so here the "different" class is expected to be in the same package as "this" class. Is this invariant enforced in VM so it need not be checked here?
Yes. All nestmates have to be in the same runtime package, and this is enforced in the VM. As Mandy explained this covers the case where you manage to get hold of an implementation object (potentially from deep inside some other package - perhaps module?) and call getClass() on it (which is allowed) and then try to examine it's nest. If the nest-host/member being returned is not the current class, then you should perform the SM check. (As you already have the current class it doesn't matter if it is returned.)
3871 * @apiNote The source language compiler is responsible for deciding which classes 3872 * and interfaces are nestmates, by generating the appropriate attributes 3873 * (JVMS 4.7.28 and 4.7.29) in the class file format (JVMS 4). 3874 * For example, the {@code javac} compiler 3875 * places a top-level class or interface into a nest with all of its direct, 3876 * and indirect, {@linkplain #getDeclaredClasses() nested classes and interfaces} 3877 * (JLS 8). 3878 * The top-level {@linkplain #getEnclosingClass() enclosing class or interface} 3879 * is designated as the nest host.
Should the text warn about not relying on this implementation detail to extract knowledge about nested/enclosing classes? Users should keep using getDeclaredClasses()/getEnclosingClass() for that purpose as nestmates may in future be used for other things too. OTOH, if users want to do an equivalent of private access check (on behalf of real caller and callee - for example in some kind of language runtime), it would be better they use the nestmate API...
I think the fact this is an apiNote that explicitly calls out how the javac compiler maps from the language concept of nested types to the VM concept of nests, should be enough to guide the programmer here. As you note there is no guarantee that the nest-host is the top-level class. But beyond that the existing API's work quite differently in terms of exploring the actual structure of nested types, whereas the nestmate API is a "flat world". It is possible (likely) that in the future VM nests will be used for additional things - not all of which need be observable via a Java specific API. That will be determined later. For a language runtime that wants to know before hand whether a direct private access will work, then yes isNestmate() is what they would use.
in j.l.r.AccessibleMember:
49 * <p> Java language access control prevents use of private members *outside* 50 * *their top-level class;* package access members outside their package; protected members 51 * outside their package or subclasses; and public members outside their 52 * module unless they are declared in an {@link Module#isExported(String,Module) 53 * exported} package and the user {@link Module#canRead reads} their module.
Could this be interpreted also as "private members can only be accessed from the top-level class"? I know that "nest" is not a Java language term, so it can not be used in the context of Java language access control. (If speaking of Java language, then even previous version of this text was wrong - if it was right, then it wouldn't have to be changed at all). So what about the following:
"Java language access control prevents use of private members outside the set of classes composed of top-level class and its transitive closure of nested classes".
Or would this be to lengthy and hard to understand?
Yes. :) This was a minor correction to what was, as you noted, already an inaccurate statement: "Java language access control prevents use of private members outside their class." That's obviously not true. As per JLS 6.6.1: "Otherwise, the member or constructor is declared private, and access is permitted if and only if it occurs within the body of the top level type (§7.6) that encloses the declaration of the member or constructor." So "class" was changed to "top-level class" to match the JLS.
in j.l.r.Reflection:
140 // At this point we know that currentClass can access memberClass. 141 142 if (Modifier.isPublic(modifiers)) { 143 return true; 144 } 145 146 // Check for nestmate access if member is private 147 if (Modifier.isPrivate(modifiers)) { 148 // assert: isSubclassof(targetClass, memberClass)
although just in a function of explaining the following comment, I think the correct assert is
// assert targetClass == null || isSubclassof(targetClass, memberClass)
as for static members, targetClass is null.
Yes. I think I'll just delete the psuedo-assert though because I think it's only true if the nestmate access is true - plus it was more a mental note to myself when working on this.
149 // Note: targetClass may be outside the nest, but that is okay 150 // as long as memberClass is in the nest. 151 boolean nestmates = areNestMates(currentClass, memberClass); 152 if (nestmates) { 153 return true; 154 } 155 }
I'm wondering about the frequency of reflective accesses of various kinds of members. I imagine that most frequent are accesses of public members, others are less so (I'm not counting accesses made via .setAccessible(true) modified Field/Method/Constructor objects as they are bypassing this Reflection.verifyMemberAccess method). But among others I imagine reflective access to private members is the least frequent as there's no practical reason to use reflection inside and among classes that are most intimately logically coupled and "know" each other's internals at compile time. So it would make sense to check for private access at the end, after protected and package-private checks (that's how existing logic was structured).
So how about putting the nestmate access logic just before the last if (!successSoFar) { return false; }:
// Check for nestmate access if member is private if (!successSoFar && Modifier.isPrivate(modifiers)) { // assert: targetClass == null || isSubclassof(targetClass, memberClass) // Note: targetClass may be outside the nest, but that is okay // as long as memberClass is in the nest. successSoFar = areNestMates(currentClass, memberClass); }
if (!successSoFar) { return false; }
This would not penalize access to package-private and protected members with areNestMates() JNI calls and maybe caching will not be needed at all.
As per your later email the check is guarded by the isPrivate check so we avoid the JNI call. Also I deliberately placed the check where it is (in contrast to VM code where it is almost last) to avoid the whole "successSoFar" mess. :) My view was that if this indeed turned out to impact performance then we could revisit the placement of it. Thanks, David
Regards, Peter
participants (11)
-
Alan Bateman
-
David Holmes
-
forax＠univ-mlv.fr
-
joe darcy
-
John Rose
-
Joseph D. Darcy
-
Karen Kinnear
-
mandy chung
-
Paul Sandoz
-
Peter Levart
-
Remi Forax