[Nestmates] RFR: 8196320: [Nestmates] Restore the old enclosing-class based isSamePackageMember check in sun/invoke/util/VerifyAccess.java

David Holmes david.holmes at oracle.com
Thu Feb 1 02:51:07 UTC 2018


Hi Mandy,

Thanks for looking at this.

On 1/02/2018 5:13 AM, mandy chung wrote:
> On 1/30/18 8:54 PM, David Holmes wrote:
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8196320
>> webrev: http://cr.openjdk.java.net/~dholmes/8196320/webrev/
>>
>> I've restored the original enclosing-class check and the 
>> isSamePackageMember method, but renamed it to areNestMates. It will 
>> first do the real Reflection.areNestMates check, and if that fails 
>> fall-back to the pre-nestmate enclosing class check.
>>
> 
> Does core reflection work with the pre-nestmate class scenario?

No. Pre-nestmates core reflection doesn't permit private access between 
nestmates. (Though core reflection could have used the same enclosing 
class check that is in VerifyAccess.)

> Should JVM_AreNestMates handle the pre-nestmate class and return true if 
> they are the same class or same runtime package?

Not sure what you are getting at here. That function is purely for 
checking nestmate attributes and establishing nestmateship based on that 
attribute. For any pre-nestmate classfile each class is considered its 
own nesthost and a check between any two different pre-nestmate classes 
will yield false. If you pass the same class it will vacuously yield true.

Callers to Reflection.areNestmates (which calls JVM_AreNestMates) can do 
their own "optimizations" if they wish to pre-check if we are dealing 
with the same class or different packages - as the original 
isSameMemberPackage check does.

>> I've added a test under runtime/Nestmates/legacy (where I'll 
>> accumulate tests that check legacy code still works after the nestmate 
>> changes - for specific areas like this.) 
> 
> At some point we should look at where the nestmates tests should be 
> placed.  For example, this test seems to be appropriate to land in 
> test/jdk/java/lang/invoke.   Just a note to consider when integrating 
> this to jdk.

A bit OT for this review but our test structure is based around 
regression tests, not functional testing, but is now being used more and 
more for functional tests for new features. It becomes difficult when a 
feature impacts a large number of areas as there are multiple ways the 
tests could be structured.

Personally I would not want to split the nestmate tests up as they form 
a complete functional group. The basic test matrix here is:

{ method, field, constructor, static-method, static-field }
X
{ direct-access, core-reflection-access, method-handle-access, jni 
access, JVM TI access, JDI access }

so the tests for each access mechanism (some are still to be written) 
appear under the relevant member directory ie:

privateMethods/TestInvoke.java
                TestReflection.java
                TestMethodHandle.java
                ...
privateConstructors/TestInvoke.java
                TestReflection.java
                TestMethodHandle.java
                ...
privateFields/...
...

Obviously you could invert this and have instead something like:

reflect/TestPrivateMethods.java
         TestPrivateConstructors.java
         ...
invoke/TestPrivateMethods.java
         TestPrivateConstructors.java
         ...
etc

but that kind of structure is only partially in existence today, and as 
I said I'd prefer to see the tests kept together as a functional group. 
That doesn't mean hotspot/jtreg/runtime/Nestmates is necessarily the 
final landing place - but again we're not really set up for feature 
based functional test organization.

>> The test will need tweaking once we bump the version to 11 
>> (unfortunately there's no system property that reports the classfile 
>> version AFAICS).
> 
> Are you referring to this check:
> 
>    39     static boolean VMHasNestmates =
>    40         System.getProperty("java.vm.specification.version").equals("10");
> :
>    84         if (!VMHasNestmates) {
>    85             throw new Error("This test is only for JDK 11 with nestmates");
>    86         }
> 
> If so, I don't think you need this check.  This test uses Class::getNestHost
> method that will fail to compile if running on an older JDK.

True. And if compiled for 11 but run on 10 that wont work anyway. :) 
Okay I can delete this conservative check.

> Nit:
> 
>    30  * @compile -source 10 -target 10 TestPrivateLookup.java
> 
> Alternatively it can use --release 10 instead of -source and -target.

Done. Though I wasn't sure if it would try to be too clever and tell me 
Class.getNestHost() doesn't exist in JDK 10. Hmmm perhaps it still will 
as right now we still appear to be JDK 10. ??

Anyway webrev updated with those tweaks:

http://cr.openjdk.java.net/~dholmes/8196320/webrev.v1/

Thanks,
David
-----

> Mandy
> 


More information about the valhalla-dev mailing list