HI,
Nice thorough work on this, surprisingly tricky in some areas esp. MHs.
Class
—
3857 * <p>If there is any error accessing the nest host, or the nest host
is
3858 * in any way invalid, then {@code this} is returned.
I am curious under what conditions this can arise. As a caller this makes me
nervous :-) Are there conditions that are worth calling it. This is related to
Alan’s comment about primitive or array classes. Its clearer when looking at
the implementation: primitive/array classes, linkage error, and the more
general nest membership validation.
3883 * @throws SecurityException
3884 * If the returned class is not the current class, and
3885 * if a security manager, <i>s</i>, is present and the caller's
3886 * class loader is not the same as or an ancestor of the class
3887 * loader for the returned class and invocation of {@link
3888 * SecurityManager#checkPackageAccess s.checkPackageAccess()}
3889 * denies access to the package of the returned class
3890 * @since 11
3891 * @jvms 4.7.28 and 4.7.29 NestHost and NestMembers attributes
3892 */
3893 @CallerSensitive
3894 public Class<?> getNestHost() {
Maybe i need more coffee, but I am struggling to see in the implementation the
checks for the case of "and the caller’s class loader is not the same as or an
ancestor of the class loader for the returned class”. Is it implied that all
classes in the nest have to be loaded from the same or from a common ancestor
class loader? so you only need to check one class in the nest against the
calling class.
3984 public Class<?>[] getNestMembers() {
I still think not removing dups is a mistake as it could be a source of subtle
bugs. But i doubt at this point i can persuade you or others to change it :-)
3989 // Can't actually enable this due to bootstrapping issues
3990 // assert(members.length != 1 || members[0] == this); // expected
invariant from VM
That's interesting and frustrating!
Reflection.java
—
146 // Check for nestmate access if member is private
147 if (Modifier.isPrivate(modifiers)) {
148 // assert: isSubclassof(targetClass, memberClass)
149 // Note: targetClass may be outside the nest, but that is okay
150 // as long as memberClass is in the nest.
151 boolean nestmates = areNestMates(currentClass, memberClass);
152 if (nestmates) {
153 return true;
154 }
155 }
Trivially, you don’t need the local variable “nestmates”.
VerifyAccess.java
—
134 case PRIVATE:
135 // Rules for privates follows access rules for nestmates.
136 boolean canAccess = ((allowedModes & PRIVATE) != 0 &&
137 Reflection.areNestMates(defc,
lookupClass));
138 // FIX ME: Sanity check refc == defc. Either remove or convert
to
139 // plain assert before integration.
140 myassert((canAccess && refc == defc) || !canAccess);
141 return canAccess;
142 default:
143 throw new IllegalArgumentException("bad modifiers:
"+Modifier.toString(mods));
144 }
145 }
146 static void myassert(boolean cond) {
147 if (!cond) throw new Error("Assertion failed");
148 }
Do you plan to chase up the FIX ME now or later?
I agree with Alan about the current location of the reflection API tests. If
these tests don’t need to be hammered with various and exotic HotSpot flags i
think they are better placed to be under test/jdk/java/lang/reflect.
Thanks,
Paul.
> On May 14, 2018, at 5:52 PM, David Holmes <david.hol...@oracle.com> wrote:
>
> This review is being spread across four groups: langtools, core-libs, hotspot
> and serviceability. This is the specific review thread for core-libs - webrev:
>
> http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/
>
> See below for full details - including annotated full webrev guiding the
> review.
>
> The intent is to have JEP-181 targeted and integrated by the end of this
> month.
>
> Thanks,
> David
> -----
>
> The nestmates project (JEP-181) introduces new classfile attributes to
> identify classes and interfaces in the same nest, so that the VM can perform
> access control based on those attributes and so allow direct private access
> between nestmates without requiring javac to generate synthetic accessor
> methods. These access control changes also extend to core reflection and the
> MethodHandle.Lookup contexts.
>
> Direct private calls between nestmates requires a more general calling
> context than is permitted by invokespecial, and so the JVMS is updated to
> allow, and javac updated to use, invokevirtual and invokeinterface for
> private class and interface method calls respectively. These changed
> semantics also extend to MethodHandle findXXX operations.
>
> At this time we are only concerned with static nest definitions, which map to
> a top-level class/interface as the nest-host and all its nested types as
> nest-members.
>
> Please see the JEP for further details.
>
> JEP: https://bugs.openjdk.java.net/browse/JDK-8046171
> Bug: https://bugs.openjdk.java.net/browse/JDK-8010319
> CSR: https://bugs.openjdk.java.net/browse/JDK-8197445
>
> All of the specification changes have been previously been worked out by the
> Valhalla Project Expert Group, and the implementation reviewed by the various
> contributors and discussed on the valhalla-dev mailing list.
>
> Acknowledgments and contributions: Alex Buckley, Maurizio Cimadamore, Mandy
> Chung, Tobias Hartmann, Vladimir Ivanov, Karen Kinnear, Vladimir Kozlov, John
> Rose, Dan Smith, Serguei Spitsyn, Kumar Srinivasan
>
> Master webrev of all changes:
>
> http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.full.v1/
>
> Annotated master webrev index:
>
> http://cr.openjdk.java.net/~dholmes/8010319-JEP181/jep181-webrev.html
>
> Performance: this is expected to be performance neutral in a general sense.
> Benchmarking and performance runs are about to start.
>
> Testing Discussion:
> ------------------
>
> The testing for nestmates can be broken into four main groups:
>
> - New tests specifically related to nestmates and currently in the
> runtime/Nestmates directory
>
> - New tests to complement existing tests by adding in testcases not
> previously expressible.
> - For example java/lang/invoke/SpecialInterfaceCall.java tests use of
> invokespecial for private interface methods and performing receiver
> typechecks, so we add java/lang/invoke/PrivateInterfaceCall.java to do
> similar tests for invokeinterface.
>
> - New JVM TI tests to verify the spec changes related to nest attributes.
>
> - Existing tests significantly affected by the nestmates changes, primarily:
> - runtime/SelectionResolution
>
> In most cases the nestmate changes makes certain invocations that were
> illegal, legal (e.g. not requiring invokespecial to invoke private interface
> methods; allowing access to private members via reflection/Methodhandles that
> were previously not allowed).
>
> - Existing tests incidentally affected by the nestmate changes
>
> This includes tests of things utilising class redefinition/retransformation
> to alter nested types but which unintentionally alter nest relationships
> (which is not permitted).
>
> There are still a number of tests problem-listed with issues filed against
> them to have them adapted to work with nestmates. Some of these are intended
> to be addressed in the short-term, while some (such as the
> runtime/SelectionResolution test changes) may not eventuate.
>
> - https://bugs.openjdk.java.net/browse/JDK-8203033
> - https://bugs.openjdk.java.net/browse/JDK-8199450
> - https://bugs.openjdk.java.net/browse/JDK-8196855
> - https://bugs.openjdk.java.net/browse/JDK-8194857
> - https://bugs.openjdk.java.net/browse/JDK-8187655
>
> There is also further test work still to be completed (the JNI and JDI
> invocation tests):
> - https://bugs.openjdk.java.net/browse/JDK-8191117
> which will continue in parallel with the main RFR.
>
> Pre-integration Testing:
> - General:
> - Mach5: hs/jdk tier1,2
> - Mach5: hs-nightly (tiers 1 -3)
> - Targetted
> - nashorn (for asm changes)
> - hotspot: runtime/*
> serviceability/*
> compiler/*
> vmTestbase/*
> - jdk: java/lang/invoke/*
> java/lang/reflect/*
> java/lang/instrument/*
> java/lang/Class/*
> java/lang/management/*
> - langtools: tools/javac
> tools/javap
>
