Thanks for the helpful review, Vladimir. I have incorporated all your comments and updated the webrev here:
http://cr.openjdk.java.net/~jrose/8008688/webrev.05 Detailed replies follow. On Jul 1, 2013, at 3:36 PM, Vladimir Ivanov <vladimir.x.iva...@oracle.com> wrote: > John, > > I have some minor suggestions about code style and code readability. > Otherwise, the change looks good! > > src/share/classes/java/lang/invoke/MemberName.java: > public MemberName(Method m, boolean wantSpecial) { > ... > MethodHandleNatives.init(this, m); > + if (clazz == null) { // MHN.init failed > + if (m.getDeclaringClass() == MethodHandle.class && > + isMethodHandleInvokeName(m.getName())) { > > Please, add a comment with a short description why a custom init procedure > for MH.invoke* cases is needed. Done: // The JVM did not reify this signature-polymorphic instance. // Need a special case here. // See comments on MethodHandleNatives.linkMethod. And I added several paragraphs in the javadoc for linkMethod. They cover non-reification, linker methods, appendixes, "synthetic", varargs, and more. > + /** Create a name for a signature-polymorphic invoker. */ > + static MemberName makeMethodHandleInvoke(String name, MethodType type) { > + return makeMethodHandleInvoke(name, type, MH_INVOKE_MODS | > SYNTHETIC); > > Please, add a comment why SYNTHETIC flag is necessary. /** * Create a name for a signature-polymorphic invoker. * This is a placeholder for a signature-polymorphic instance * (of MH.invokeExact, etc.) that the JVM does not reify. * See comments on {@link MethodHandleNatives#linkMethod}. */ > src/share/classes/java/lang/invoke/MethodHandleInfo.java: > src/share/classes/java/lang/invoke/MethodHandles.java: > > +import java.security.*; > > This import isn't used. Fixed. > src/share/classes/java/lang/invoke/MethodHandles.java: > > + public MethodHandleInfo revealDirect(MethodHandle target) { > ... > + byte refKind = member.getReferenceKind(); > ... > + // Check SM permissions and member access before cracking. > + try { > + //@@checkSecurityManager(defc, member, lookupClass()); > + checkSecurityManager(defc, member); > + checkAccess(member.getReferenceKind(), defc, member); > + } catch (IllegalAccessException ex) { > + throw new IllegalArgumentException(ex); > + } > > You prepare a separate refKind for MethodHandleInfo, but perform security > checks against original member.getReferenceKind(). Is it intentional? No, it's bug. Thanks for catching that. > src/share/classes/java/lang/invoke/InfoFromMemberName.java: > > 81 public <T extends Member> T reflectAs(Class<T> expected, Lookup > lookup) { > 82 if (member.isMethodHandleInvoke() && !member.isVarargs()) { > 83 // this member is an instance of a signature-polymorphic > method, which cannot be reflected > 84 throw new IllegalArgumentException("cannot reflect signature > polymorphic method"); > > Please, add a comment why (!member.isVarargs()) check is necessary. // This member is an instance of a signature-polymorphic method, which cannot be reflected // A method handle invoker can come in either of two forms: // A generic placeholder (present in the source code, and varargs) // and a signature-polymorphic instance (synthetic and not varargs). // For more information see comments on {@link MethodHandleNatives#linkMethod}. > src/share/classes/java/lang/invoke/InfoFromMemberName.java: > > 127 private void checkAccess(Member mem, Lookup lookup) throws > IllegalAccessException { > 128 byte refKind = (byte) getReferenceKind(); > 129 if (mem instanceof Method) { > 130 boolean wantSpecial = (refKind == REF_invokeSpecial); > 131 lookup.checkAccess(refKind, getDeclaringClass(), new > MemberName((Method) mem, wantSpecial)); > 132 } else if (mem instanceof Constructor) { > 133 lookup.checkAccess(refKind, getDeclaringClass(), new > MemberName((Constructor) mem)); > 134 } else if (mem instanceof Field) { > 135 boolean isSetter = (refKind == REF_putField || refKind == > REF_putStatic); > 136 lookup.checkAccess(refKind, getDeclaringClass(), new > MemberName((Field) mem, isSetter)); > 137 } > 138 } > > Can you introduce a factory method to convert a Member instance into > MemberName and call lookup.checkAccess(refKind, getDeclaringClass(), > covertToMemberName(mem)) instead? It'll considerably simplify the code and > make the intention clearer. Good idea. Done. — John > Best regards, > Vladimir Ivanov > > On 6/27/13 10:00 AM, John Rose wrote: >> This change implements the MethodHandleInfo API for cracking a direct method >> handle back into its symbolic reference components. A DMH is any >> CONSTANT_MethodHandle or any result of a Lookup.find* or Lookup.unreflect* >> API call. >> >> http://cr.openjdk.java.net/~jrose/8008688/webrev.04 >> >> Problem: >> >> JDK 8 (esp. Project Lambda) needs a stable API for "cracking" >> CONSTANT_MethodHandle constants that are involved with lambda capture sites >> (which are implemented with invokedynamic). >> >> Solution: >> >> Create, specify, implement, and test such an API. Run the API design past >> the 292 EG, the Project Lambda folks, and the Oracle internal review council >> (CCC). >> >> Testing: >> >> Regular JSR 292 regression tests, plus a new jtreg test with positive and 3 >> kinds of negative tests, in hundreds of combinations. (See below.) >> >> — John >> >> P.S. Output from RevealDirectTest.java. (It runs with and without a >> security manager.) >> >> @Test testSimple executed 107 positive tests in 446 ms >> @Test testPublicLookup/1 executed 56 positive tests in 99 ms >> @Test testPublicLookup/2 executed 80 positive tests in 551 ms >> @Test testPublicLookup/3 executed 56 positive tests in 47 ms >> @Test testPublicLookupNegative/1 executed 23/0/0 negative tests in 2 ms >> @Test testPublicLookupNegative/2 executed 0/27/0 negative tests in 4 ms >> @Test testPublicLookupNegative/3 executed 0/0/27 negative tests in 10 ms >> @Test testJavaLangClass executed 60 positive tests in 67 ms >> @Test testCallerSensitive executed 30 positive tests in 425 ms >> @Test testCallerSensitiveNegative executed 12/0/0 negative tests in 1 ms >> @Test testMethodHandleNatives executed 4 positive tests in 5 ms >> @Test testMethodHandleInvokes/1 executed 640 positive tests in 828 ms >> @Test testMethodHandleInvokes/2 executed 640 positive tests in 177 ms >> >> _______________________________________________ >> mlvm-dev mailing list >> mlvm-dev@openjdk.java.net >> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev >> _______________________________________________ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
