RFR(L): 8008688: Make MethodHandleInfo public
John Rose
john.r.rose at oracle.com
Tue Jul 2 21:27:37 PDT 2013
Thanks, Chris.
-- John (on my iPhone)
On Jul 2, 2013, at 9:17 PM, Christian Thalinger <christian.thalinger at oracle.com> wrote:
> 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
>
> _______________________________________________
> mlvm-dev mailing list
> mlvm-dev at openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
More information about the mlvm-dev
mailing list