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

[David Holmes]

Updated webrev:

http://cr.openjdk.java.net/~dholmes/8197915/webrev.v2/

Changes:

- DirectMethodHandles.java: new simple and direct approach to dealing 
with LF_SPECIAL_IFC
- 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.

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