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

mandy chung 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.)

OK.

Should JVMAreNestMates 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 JVMAreNestMates) 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.

That's good.

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.

Mandy

More information about the valhalla-dev mailing list