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

Fri May 4 08:10:13 UTC 2018

> 
> 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
>>>>>>>>>