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

David Holmes david.holmes at oracle.com
Tue May 8 22:26:50 UTC 2018


Hi Karen,

On 9/05/2018 7:58 AM, Karen Kinnear wrote:
> David,
> 
> Wow. Thank you again for tracking this down. And for the updated 
> comments - this is really challenging logic.

Indeed!

> Couple of questions please.
> 
> 1. cpCache.cpp
> Could you possibly walk through the CharSequence declares toString behavior?

All I can say is that processing CharSequence.toString ends up in 
set_direct_or_vtable_call with invokeinterface. In mainline that takes 
the change_to_virtual path. I discovered this when I tried to add overly 
strong assertions to the code paths.

> 2. DirectMethodHandle.java
> Thank you for the clearer comments.
> 
>     case REF_invokeInterface: {
> 98 // for interfaces we always adapt to get the receiver
> 99 // check inserted (only if the MemberName kind is
> 100 // REF_invokeSpecial of course)
> 101 LambdaForm lform = preparedLambdaForm(member, true);
>   102                     return new Interface(mtype, lform, member, refc);
> 
> You lost me on this one -
> Where is the runtime check for Lookup.findVirtual(Interface, …) receiver 
> is subtype of REFC?
> Why does that work for e.g. a public default method that does not 
> optimize to “direct_call” -> REF_invokeSpecial
> but it does not work for a private interface method that does optimize 
> to “direct_call” -> REF_invokeSpecial.
> I actually expected you to need this for both cases ?

We do. My comment is misleading as it only refers to the "true" argument 
being passed to preparedLambdaForm. I'll revise as:

   // for interfaces we always need the receiver typecheck,
   // so we always pass 'true' to ensure we adapt if needed
   // to include the REF_invokeSpecial case
   LambdaForm lform = preparedLambdaForm(member, true);

Does that help?

To be clear, the existing logic (in mainline) in makePreparedLambdaForm has:

  boolean needsReceiverCheck = (which == LF_INVINTERFACE);

which triggers inclusion of the receiver typecheck by calling 
Interface.checkReceiver. That covers the non-direct-call case. But that 
was missing the direct-call case for private interface methods where we 
had switched to LF_INVSPECIAL, and which we now switch to 
LF_INVSPECIAL_IFC. The above logic is now:

  boolean needsReceiverCheck = (which == LF_INVINTERFACE ||
                                which == LF_INVSPECIAL_IFC);

so that enables the missing checkReceiver call. So now both non-direct 
and direct calls are covered.

Hope that clarifies.

Thanks,
David
-----

> thanks,
> Karen
> 
> 
>> On May 8, 2018, at 1:51 AM, David Holmes <david.holmes at oracle.com 
>> <mailto:david.holmes at oracle.com>> wrote:
>>
>> Hi Karen,
>>
>> On 8/05/2018 7:29 AM, Karen Kinnear wrote:
>>> David,
>>> Looks good!
>>
>> Thanks. Alas I took a slightly wrong turn way back when which needs to 
>> be addressed - see below. Updated webrev:
>>
>> http://cr.openjdk.java.net/~dholmes/8197915/webrev.v5/
>>
>>> Many thanks for the update and the explanations.  I had missed that 
>>> DirectMethodHandle.make() refKind reflects
>>> the original Lookup.findVirtual/findSpecial distinction - that is 
>>> just what we needed. Thanks for the clarification.
>>> Now I understand this fix and really like it.
>>> Totally support following up with bugs/rfes that are not nestmate 
>>> specific.
>>> Could you possibly modify a couple of comments in the code to make 
>>> this a bit clearer?
>>> DirectMethodHandle.java
>>> 1. line 96
>>> I don’t know what “// we always adapt ‘special’ when dealing with 
>>> interfaces”
>>> I think what you mean is that we always need to perform a dynamic check
>>> that the receiver is a subtype of the reference class for interfaces
>>
>> The comment was trying to explain why we pass true to 
>> preparedLambdaForm, and followed on from the comment in the 
>> REF_invokeSpecial case. I've changed it to:
>>
>>  // for interfaces we always adapt to get the receiver
>>  // check inserted (only if the MemberName kind is
>>  // REF_invokeSpecial of course)
>>
>>> 2. Relative to line 84: it would help me to have a comment that 
>>> refKind is relative to the
>>> FindVirtual(Interface or Class) or findSpecial original 
>>> bytecode-equivalent request.
>>
>> Inserted:
>>
>>  // refKind reflects the original type of lookup via findSpecial or
>>  // findVirtual etc.
>>
>>> 2. lines 176-179
>>> Perhaps replace with:
>>> // MemberName.getReferenceKind represents the JVM optimized form of 
>>> the call
>>> // as distinct from the ‘kind’ passed to DMH.make which represents 
>>> the original
>>> // bytecode-equivalent request.Specifically private/final methods 
>>> that use a direct call
>>> // have MemberName.getReferenceKind adapted to REF_invokeSpecial, 
>>> even though
>>> // the actual invocation mode as represented by the ‘kind’ passed to 
>>> DMH.make, which
>>> // may be invokevirtual or invokeinterface
>>
>> Done (with minor editing).
>>
>>> 3. cpCache.cpp line 192
>>> Thank you for the assertion change.
>>> Is line 192 always the case? I think it is sometimes interface klass* 
>>> and sometimes Object klass*
>>
>> Right. This is where I went off track a little with my changes earlier 
>> on. The key point is that this code was never intended for processing 
>> any Object methods, but only private interface methods. All Object 
>> methods and any non-private interface methods (which can happen if an 
>> interface redeclares a method also declared in Object!) were supposed 
>> to follow the "else" and be treated exactly the same as they were 
>> before the nestmate changes. But I unintentionally pulled final Object 
>> methods into the private interface code - hence the assertion problem 
>> and the line 192 ambiguity. So that's been fixed.
>>
>> Of course that highlighted the same kind of mistake in another area - 
>> the templateTable. I inserted my private interface method "vfinal" 
>> check before the special Object method handling code. But of course 
>> that also causes final Object methods to follow my interface logic - 
>> which is wrong (and triggers a SEGV/NPE because the 'interface' klass* 
>> is not set in f2 in that case). So I needed to reorder the vfinal 
>> logic with the forced_virtual_shift logic.
>>
>> Re-tested:
>>  jdk/java/lang/invoke
>>  hotspot/runtime/Nestmates
>>  hotspot/runtime/SelectionResolution
>>  hotspot/compiler/jsr232
>>
>>  mach5: jdk-tier1,jdk-tier2,jdk-tier3,hs-tier1,hs-tier2,builds-tier1
>>
>> Thanks,
>> David
>>
>>> thank you for the additional tests,
>>> Karen
>>>> On May 7, 2018, at 2:43 AM, David Holmes <david.holmes at oracle.com> 
>>>> wrote:
>>>>
>>>> Updated webrev:
>>>>
>>>> http://cr.openjdk.java.net/~dholmes/8197915/webrev.v4/
>>>>
>>>> Fixed the too-strict assertions in linkResolver and 
>>>> ConstantPoolCacheEntry::set_direct_or_vtable_call. Adjusted the 
>>>> logic in ConstantPoolCacheEntry::set_direct_or_vtable_call to match 
>>>> the updated assertion.
>>>>
>>>> Updated the test with additional test cases (related to additional 
>>>> ones being added for 8200167 under 8202686).
>>>>
>>>> Two test forms are commented out as they fail (ie they don't throw 
>>>> the expected exceptions). These failings are due to 
>>>> non-nestmate-specific omissions in the cpCache and MH code. As such 
>>>> they will need to be addressed at a later time. Bugs will be filed 
>>>> once a few logistical issues have been resolved.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>> On 7/05/2018 11:50 AM, David Holmes wrote:
>>>>> 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