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

David Holmes david.holmes at oracle.com
Fri May 4 08:06:11 UTC 2018


On 4/05/2018 5:47 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
>> }
> 
> Yes, that's what initially draw my attention.
> 
>>
>> So I'll have to change one of them. But I'd still prefer that the 
>> callerClass can not be null.
> 
> I agree with you that DMH.make() should not observe both 
> m.getReferenceKind() == REF_invokeSpecial and callerClass == null at the 
> same time.
> 
> If you want to rely on "callerClass != null" for now, then add an 
> explicit check and throw InternalError instead of NPE.

Ok. webrev updated to v3:

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

New code:

     // if caller is an interface we need to adapt to get the
     // receiver check inserted
     if (callerClass == null) {
        throw new InternalError("callerClass must not be null for 
REF_invokeSpecial");
     }
     LambdaForm lform = preparedLambdaForm(member, 
callerClass.isInterface());
     return new Special(mtype, lform, member, callerClass);

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