Thanks, Chris. -- John (on my iPhone)
On Jul 2, 2013, at 9:17 PM, Christian Thalinger <christian.thalin...@oracle.com> wrote: > 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 _______________________________________________ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
