[Nestmates] RFR: 8196320: [Nestmates] Restore the old enclosing-class based isSamePackageMember check in sun/invoke/util/VerifyAccess.java
mandy.chung at oracle.com
Thu Feb 1 05:01:29 UTC 2018
On 1/31/18 6:51 PM, David Holmes wrote:
> 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.
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.
> 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.
>>> 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.
> 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.
No issue with that as it's convenient to keep them together for now.
The test organization can be decided when it's prepared for integration.
> True. And if compiled for 11 but run on 10 that wont work anyway. :)
> Okay I can delete this conservative check.
>> 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.
More information about the valhalla-dev