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

David Holmes david.holmes at oracle.com
Fri May 4 08:22:59 UTC 2018


Thanks Vladimir!

David

On 4/05/2018 6:10 PM, Vladimir Ivanov wrote:
>>
>> Ok. webrev updated to v3:
>>
>> http://cr.openjdk.java.net/~dholmes/8197915/webrev.v3/
> 
> Looks good!
> 
> Best regards,
> Vladimir Ivanov
> 
>>
>> 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