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
>