Couldn't find any obvious problems. Looks good. -- Chris
On Jul 2, 2013, at 6:26 PM, John Rose <john.r.r...@oracle.com> wrote: > 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 _______________________________________________ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
