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

[David Holmes]

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
>