[Nestmates] RFR (S): 8197915: [Nestmates] Implement receiver typecheck for private invokeinterface use

Fri May 4 07:04:50 UTC 2018

>> http://cr.openjdk.java.net/~dholmes/8197915/webrev.v2/

src/java.base/share/classes/java/lang/invoke/DirectMethodHandle.java

+                    LambdaForm lform = preparedLambdaForm(member, 
callerClass.isInterface());

You need to handle "callerClass == null" case as well.

Best regards,
Vladimir Ivanov

>>
>> Changes:
>>
>> - DirectMethodHandles.java: new simple and direct approach to dealing 
>> with LF_SPECIAL_IFC
> 
> I like how java.lang.invoke part shapes out!
> 
> Maybe rename adaptToSpecialIfc to needsReceiverCheck? That's what 
> confused me in the first version: though it's an interface call (which 
> always require receiver check against REFC), new checks only referred to 
>   LF_INVSPECIAL (since invocation mode is a direct call).
> 
>> - New regression test for the final virtual call from an interface bug 
>> introduced by 8200167.
>>
>> If necessary/desirable I can fix that part in mainline separately. So 
>> far no tests (including jck/API/java/lang) seem to tickle it.
> 
> Or file a bug. I have some ideas how to improve relevant code and make 
> LF construction cleaner.
> 
> Best regards,
> Vladimir Ivanov
> 
> 
>> Thanks,
>> David
>>
>> On 4/05/2018 11:41 AM, David Holmes wrote:
>>> Hi Karen,
>>>
>>> On 4/05/2018 6:39 AM, Karen Kinnear wrote:
>>>> David,
>>>>
>>>> Really delighted to see you near the end of the major functional 
>>>> changes!
>>>
>>> Thanks for taking a look so quickly!
>>>
>>>> A couple minor comments, and then a question please:
>>>>
>>>> 1. MethodHandles.java
>>>
>>> DirectMethodHandle.java :)
>>>
>>>>   174 different “to” -> different “from” ?
>>>
>>> Changed. That's my UK upbringing :)
>>>
>>> https://en.oxforddictionaries.com/usage/different-from-than-or-to
>>>
>>>> 2. methodHandles.cpp
>>>>    300-301
>>>>    Thank you for the comment.
>>>>    Might it also be worth adding that direct call is used by:
>>>>      invoke static, invokespecial, invokeinterface:local private, 
>>>> invoke virtual:vfinal and private methods
>>>>    (or are you concerned about getting out of sync if this changes?)
>>>
>>> It is not used by invokestatic. I'm not 100% sure of all the exact 
>>> cases where an invokeinterface/invokevirtual becomes a direct call, 
>>> so didn't want to say anything inaccurate. But the comment as it 
>>> stands is awkward so I've expanded it:
>>>
>>>        // "special" reflects that this is a direct call, not that it
>>>        // necessarily originates from an invokespecial. We can also do
>>>        // direct calls for private and/or final non-static methods.
>>>
>>>> 3. DirectMethodHandle.java - this was subtle!
>>>
>>> More than you realise ;-)
>>>
>>>> I believe this is correct assuming that:
>>>>    CallerClass is always and only set for invokespecial. Is this 
>>>> accurate? Could you possibly add a comment?
>>>
>>> That's an excellent question and one that should have been asked 
>>> before 8200167 was finalized. :(  The short answer is "no" - 
>>> callerClass can be non-null for any of the invocation modes. And yes 
>>> the current mainline code is broken - seems there is a gap in the 
>>> existing test coverage as we never call a final method from an 
>>> interface method. If we do we get:
>>>
>>> Exception in thread "main" java.lang.InternalError: Should only be 
>>> invoked on a subclass
>>>          at 
>>> java.base/java.lang.invoke.DirectMethodHandle.checkReceiver(DirectMethodHandle.java:441) 
>>>
>>>
>>> <sigh>
>>>
>>> We only look at callerClass when dealing with LF_INVSPECIAL, which in 
>>> mainline means we either have an invokespecial or an invokevirtual. 
>>> For invokespecial this is fine of course. But the invokevirtual case 
>>> was never encountered and so slipped by in error. With nestmates we 
>>> also add invokeinterface to the mix - which is fine because if it is 
>>> an invokeinterface then we want the check regardless. It doesn't 
>>> matter if the check is enabled because of the (incidental) 
>>> callerClass.isInterface check, or the explicit 
>>> m.getDeclaringClass().isInterface(). But the logic is messy and far 
>>> from clear and not correct by construction. So I will completely redo 
>>> it in a simpler and more direct/explicit way.
>>>
>>> BTW another red-herring: the !m.isStatic() part of the condition was 
>>> not needed. I was tracking down two failure modes before finalizing 
>>> this. The first was a problem with a static interface method - fixed 
>>> by the !m.isStatic(). The second was caused by missing parentheses in 
>>> the overall condition - which once fixed precluded the static case, 
>>> so the first fix was not needed (as we never use LF_INVSPECIAL with 
>>> statics). If only I'd tackled them in the reverse order.
>>>
>>> I'll post an updated webrev later today once I've re-tested lots of 
>>> things.
>>>
>>>>     - agree with the theory that invokevirtual will never find a 
>>>> private interface method (and ACC_FINAL is illegal for interfaces)
>>>
>>> Yes. More specifically as we're dealing with MH semantics: 
>>> findVirtual for an interface method yields a MH with invokeInterface 
>>> "kind", not one with invokeVirtual "kind".
>>>
>>> public MethodHandle findVirtual(Class<?> refc, String name, 
>>> MethodType type) throws NoSuchMethodException, IllegalAccessException {
>>> ...
>>>      byte refKind = (refc.isInterface() ? REF_invokeInterface : 
>>> REF_invokeVirtual);
>>> ...
>>> }
>>>
>>>
>>>> 4. Test - I still need to study this
>>>> I have been writing down test cases to make sure we don’t test cases 
>>>> we don’t want to, and I
>>>> need to double-check you have them covered. Will do that tomorrow.
>>>
>>> The testing is all "positive" in the sense that it ensures a receiver 
>>> subtype check is in place when it "must be". In fact it must always 
>>> be the case the receiver has a type that has the method being 
>>> invoked. We were just missing a few cases that verified that (and 
>>> some stronger conditions: ie receiver <: caller for invokespecial 
>>> semantics).
>>>
>>> If you want to test that we don't insert the new explicit checks in 
>>> cases where they are not needed, then I don't know how to do that - 
>>> other than by adding tracing and running the test case and not seeing 
>>> checkReceiver being called.
>>>
>>> That said, once I've reworked the logic it will be blindingly obvious 
>>> when the new explicit check is being added.
>>>
>>> Thanks,
>>> David
>>>
>>>
>>>> thanks,
>>>> Karen
>>>>
>>>>> On May 3, 2018, at 6:21 AM, David Holmes <david.holmes at oracle.com> 
>>>>> wrote:
>>>>>
>>>>> bug id: https://bugs.openjdk.java.net/browse/JDK-8197915
>>>>> webrev: http://cr.openjdk.java.net/~dholmes/8197915/webrev/
>>>>>
>>>>> JDK-8174962 implemented receiver typechecks for invokeinterface 
>>>>> within the interpreter (templateTable), compilers and for 
>>>>> MethodHandles. In nestmates invokeinterface can now be used for 
>>>>> private interface methods - which result in direct calls. So we 
>>>>> need to extend the receiver subtype checks to cover the new cases.
>>>>>
>>>>> Summary of changes:
>>>>>
>>>>> - src/hotspot/cpu/<cpu>/templateTable_<cpu>.cpp
>>>>>
>>>>> In the templateTable the 8174962 checks come after the private 
>>>>> interface method invocation logic ("vfinal") we already had in 
>>>>> place for the nestmate changes, and they rely on itable information 
>>>>> that doesn't exist for private methods. So we insert a direct 
>>>>> subtype check.
>>>>>
>>>>> I've provided code for all CPU's but only x86 and sparc have been 
>>>>> tested. I'll be soliciting aid on the other ports before nestmates 
>>>>> goes to mainline later this month.
>>>>>
>>>>> -  src/hotspot/share/oops/cpCache.cpp
>>>>>
>>>>> We have to pass the interface klass* so it's available for the 
>>>>> typecheck.
>>>>>
>>>>> -  src/hotspot/share/oops/klassVtable.cpp
>>>>>
>>>>> Updated a comment that's no longer accurate.
>>>>>
>>>>> - src/hotspot/share/opto/doCall.cpp
>>>>>
>>>>> This code was provided by Vladimir Ivanov (thank you!) and expands 
>>>>> the existing "invokespecial" support for receiver typechecks in C2, 
>>>>> to "invokeinterface" as well.
>>>>>
>>>>> Aside: no changes were needed for C1. It's seems all the receiver 
>>>>> typechecks for C1 are being handled at a higher level (through 
>>>>> linkResolver and/or cpCache logic).
>>>>>
>>>>> - src/hotspot/share/prims/methodHandles.cpp
>>>>>
>>>>> Comment clarifying JVM_REF_invokeSpecial doesn't necessarily mean 
>>>>> it relates to an actual "invokespecial" - it is used for all direct 
>>>>> calls.
>>>>>
>>>>> - src/java.base/share/classes/java/lang/invoke/DirectMethodHandle.java
>>>>>
>>>>> Add clarifying comments regarding how "kind" can vary if a direct 
>>>>> call is involved.
>>>>>
>>>>> Expand the condition to switch from LF_INVSPECIAL to 
>>>>> LF_INVSPECIAL_IFC (which adds the additional receiver typecheck) to 
>>>>> account for the invokeinterface case.
>>>>>
>>>>> -  test/jdk/java/lang/invoke/PrivateInterfaceCall.java
>>>>>
>>>>> New test for invokeinterface semantics that mirrors the existing 
>>>>> SpecialInterfaceCall test for invokespecial.
>>>>>
>>>>> This is the last of the significant functional changes for nestmates.
>>>>>
>>>>> Thanks,
>>>>> David
>>>>