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

David Holmes david.holmes at oracle.com
Fri May 4 07:24:23 UTC 2018


On 4/05/2018 5:10 PM, David Holmes wrote:
> On 4/05/2018 5:04 PM, Vladimir Ivanov wrote:
>>
>>>> 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.
> 
> I'm not sure I do now. I think we must always have a callerClass context 
> when we hit this code. Can you see where a null may come from? So far 
> testing has not produced any failures relating to a null callerClass. If 
> we did ever get a null we'd be missing the receiver <: caller check, and 
> that would be a bug requiring us to change the code to pass in the caller.

Just realized this is inconsistent with the immediately following:

if (callerClass != null) {
    checkClass = callerClass;  // potentially strengthen to caller class
}

So I'll have to change one of them. But I'd still prefer that the 
callerClass can not be null.

Cheers,
David

> Thanks,
> David
> 
>> 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
>>>>>>


More information about the valhalla-dev mailing list