RFR(L): 8008688: Make MethodHandleInfo public

[John Rose]

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.ivanov at 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 at openjdk.java.net
>> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
>>