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

David Holmes david.holmes at oracle.com
Mon May 7 01:50:47 UTC 2018


I've worked out the difference in the tests. The jasm version used the 
wrong REFC: Object instead of the interface type.

Fixing assertions and updating tests.

David

On 7/05/2018 9:05 AM, David Holmes wrote:
> Hi Karen,
> 
> First, outside of nestmates I've filed a bug (8202686) and send out a 
> RFR to add the missing testcase for final Object methods to the test for 
> 8200167. It doesn't show any issues of course.
> 
> Next I've taken the additional testcases and moved them into the 
> PrivateInterfaceCall test - adapted for invokeinterface - which should 
> cover the test you wrote below ... however ...
> 
> On 5/05/2018 8:04 AM, David Holmes wrote:
>> 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!
> 
> Yes this overlooked that final Object methods can also follow this path. 
> It is fixed by simply extending the assert to include "or is a final 
> Object method".
> 
> This was triggered by both a direct call attempt for a final Object 
> method and a MH invocation of same.
> 
>>>
>>> 2. ConstantPoolCacheEntry::set_direct_or_vtable_call
>>>    invokeinterface asserts is_private
> 
> None of my testing hit this assertion failure. Yet your test (which I 
> essentially copied) does. This is very puzzling.
> 
> Further if I suppress that assert then I hit:
> 
> #  Internal Error 
> (/export/users/dh198349/valhalla/repos/valhalla-dev/open/src/hotspot/share/oops/cpCache.cpp:276), 
> pid=8649, tid=8650
> #  Error: assert(invoke_code == Bytecodes::_invokevirtual || 
> (method->is_private() && invoke_code == Bytecodes::_invokeinterface)) 
> failed
> 
> This is all easily fixed, but the test scenarios need more 
> investigation. Not only does your direct invocation test trigger the 
> above assertions where mine does not; my test fails due to:
> 
> IncompatibleClassChangeError: Found class java.lang.Object, but 
> interface was expected
> 
> but yours does not! The only difference I can see is that your test has 
> the call in a class, whereas mine has it in an interface.
> 
> Thanks,
> David
> 
> 
> 
>>> 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