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

Vladimir Ivanov vladimir.x.ivanov at oracle.com
Fri May 4 07:47:07 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.
>>
>> 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.

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