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

[David Holmes]

Hi Mandy,

<trimming>

On 1/02/2018 3:01 PM, mandy chung wrote:
> On 1/31/18 6:51 PM, David Holmes wrote:
>> 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/
>>> 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.
> 
> OK (I was confused with the semantics of JVM_AreNestMates).  It does 
> return true on the same class.   It would help if you can add the 
> comment describing this function in jvm.h.

Most methods in jvm.h are not documented. The assumption seems to be 
that the semantics are defined as per the JDK method that calls them - 
in this case Reflection.areNestMates. In this case, like many Reflection 
methods, that wasn't documented either (oops!) so I've added some basic 
docs:

  /**
    * Returns true if {@code currentClass} and {@code memberClass}
    * are nestmates - that is, if they have the same nesthost as
    * determined by the VM.
    */

>>
>> 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.
> 
> These optimization in VerifyAccess::areNestMates can be moved to 
> Reflection::areNestMates so that it's clearer the difference is the 
> top-level enclosing class check.

Reflection.areNestMates is a native method. Any fast-paths belong in the 
callers ie VerifyAccess.areNestMates, or Reflection.verifyMemberAccess - 
and indeed they do exist there.

If you are concerned about confusion between VerifyAccess.areNestMates 
and Reflection.areNestMates I can return to using 
VerifyAccess.isSamePackageMember - though I find that a very poor name 
to begin with and keep writing it the wrong way round. 
(isInSamePackageMember would be more accurate.)

>>> 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. ??
>>
> 
> Ah good point.  This test will fail when the javac symbols are updated 
> when it bumps to 11 (--release 10 will ensure it can only use JDK 10 
> API).   You will need to split the C class to a separate source file to 
> prepare that change.

I've reverted to -source/-target and hope that will not induce an API 
check (though will probably produce a warning). If it does do the API 
check then I'll have to factor the test classes into a separate file as 
you say.

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

Thanks,
David

> Mandy