RFR(L): 8008688: Make MethodHandleInfo public

Tue Jul 2 21:17:23 PDT 2013

Couldn't find any obvious problems.  Looks good.

-- Chris

On Jul 2, 2013, at 6:26 PM, John Rose <john.r.rose at 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.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
>>> 
> 
> _______________________________________________
> mlvm-dev mailing list
> mlvm-dev at openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev