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

David Holmes david.holmes at oracle.com
Fri May 4 22:04:55 UTC 2018


Hi Karen,

On 5/05/2018 5:54 AM, Karen Kinnear wrote:
> David,
> 
> Putting together a wiki to describe how I think this works with cpCache 
> and with MethodHandles. Not
> yet done …
> 
> In the process of testing cases - I found a couple of assertions in the 
> nestmate repo that are not accurate:
> 
> 1. linkResolver.cpp:
> #  assert(resolved_method()->is_private()) failed: Should only have 
> non-virtual invokeinterface for private methods!
> 
> 2. ConstantPoolCacheEntry::set_direct_or_vtable_call
>    invokeinterface asserts is_private
> 
> I wrote a small test (sorry - I patched the bytecodes to do this 
> quickly) which has
> invokeinterface I.getClass()  // javac put invokevirtual when I tried to 
> get it to generate that

Great catch! Another variant of the "invoking object methods via 
invokeinterface" problem - the final method case. The asserts in 
principle need to weaken to "or is an Object method".

> This isn’t the methodHandles, this is just the straight bytecodes - but 
> it is part of the decision tree of are we using Ref_invokeSpecial.

Not sure how the bytecode issue relates at all to the MH logic? But of 
course we have to try and construct a MH version of the direct invoke as 
well.

Will tackle this Monday.

Many thanks,
David

> 
> I attached the test - it is built for jdk10 so I could test before and 
> after. If you recompile Test.java it will need repatching.
> 
> Since getClass is final, it also goes through the direct_call route for 
> invokeinterface.
> 
> thanks,
> Karen
> 
> 
> 
> 
>> On May 4, 2018, at 4:22 AM, David Holmes <david.holmes at oracle.com 
>> <mailto:david.holmes at oracle.com>> wrote:
>>
>> 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