(M) RFR: 8200167: Validate more special case invocations

[David Holmes]

Hi Vladimir,

On 27/04/2018 7:07 AM, Vladimir Ivanov wrote:
> David, thanks for taking care of the bug!

Thanks for all the help!

> src/hotspot/share/c1/c1_Canonicalizer.cpp
> ...
>   void Canonicalizer::do_CheckCast      (CheckCast*       x) {
> -  if (x->klass()->is_loaded()) {
> +  if (x->klass()->is_loaded() && !x->is_invokespecial_receiver_check())
> 
> It seems like it's not something specific to invokespecial, but a 
> generic problem in how interface casts are handled in C1: it's not 
> correct to eliminate the cast if obj->declared_type() is an interface. I 
> assume that's what happens in your case. FTR I'm fine with handling it 
> separately.

The above came from Tobias. If you think there is a more general issue 
here then we should file a separate bug and formulate a test case.

> test/jdk/java/lang/invoke/I4Special.jcod:
> 
> I'm curious why did you choose jcod here and not jasm? It seems jasm 
> should be a better fit to replace invokevirtual with invokespecial 
> (unless I'm missing something important in the test case).

Simply because jcod is what I have been working with in nestmates for 
classfile customizations and I don't know jasm.

> Otherwise, looks good.

Thanks again!

David

> 
> Best regards,
> Vladimir Ivanov
> 
> On 4/25/18 21:12, David Holmes wrote:
>> Revised webrev: http://cr.openjdk.java.net/~dholmes/8200167/webrev.v2/
>>
>> Karen formulated an additional test scenario invoking Object methods 
>> through an interface reference, which when turned into a new testcase 
>> demonstrated that the receiver typecheck was also missing in that case 
>> for Methodhandle's from findSpecial.
>>
>> Again Vladimir Ivanov formulated the fix for this. Thanks Vladimir!
>>
>> Summary of changes from original webrev:
>>
>> - We introduce a new version of DirectMethodHandle.preparedlambdaForm 
>> that takes the caller class as a parameter, and that class is checked 
>> for being an interface (not the method's declaring class) to trigger 
>> the switch to LF_SPECIAL_IFC. (This obviously addresses one problem 
>> with invoking Object methods - as Object is not an interface - but by 
>> itself did not fix the problem.)
>>
>> - We introduce a new LambdaForm kind, DIRECT_INVOKE_SPECIAL_IFC, which 
>> we use when dealing with LF_INVSPECIAL_IFC. (This was the key in 
>> ensuring the receiver typecheck via Special.checkReceiver remained in 
>> the generated code.)
>>
>> - In the test we:
>>    - introduce a new invokeDirect testcase for Object.toString(), but 
>> we need to do that via a modified jcod file (as javac generates an 
>> invokevirtual)
>>    - introduce the new invokeSpecialObjectMH(I2 i) call for the MH case.
>>
>> Thanks,
>> David
>>
>> On 17/04/2018 6:48 PM, David Holmes wrote:
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8200167
>>> webrev: http://cr.openjdk.java.net/~dholmes/8200167/webrev/
>>>
>>> Credit to John Rose and Vladimir Ivanov for the form of the MH fix; 
>>> and to Tobias Hartmann for the C1 fix. Their help was very much 
>>> appreciated (and needed!).
>>>
>>> tl;dr version: there are some places where badly formed interface 
>>> method calls (which are not detected by the verifier) were missing 
>>> runtime checks on the type of the receiver. Similar issues have been 
>>> fixed in the past and this was a remaining hole in relation to 
>>> invokespecial semantics and interface calls via MethodHandles. It 
>>> also turned out there was an issue in C1 that affected direct 
>>> invokespecial calls.
>>>
>>> Description of changes:
>>>
>>> - src/java.base/share/classes/java/lang/invoke/MethodTypeForm.java
>>>
>>> Added a new form LF_INVSPECIAL_IFC for invokespecial of interface 
>>> methods.
>>>
>>> - src/java.base/share/classes/java/lang/invoke/MethodHandles.java
>>>
>>> Renamed callerClass parameter to boundCallerClass, where it 
>>> originates from findBoundCallerClass. The name "callerClass" is 
>>> misleading because most of the time it is NULL!
>>>
>>> In getDirectMethodCommon, pass the actual caller class (lookupClass) 
>>> to DirectMethodHandle.make. And correct the comment about 
>>> restrictReceiver.
>>>
>>> - src/java.base/share/classes/java/lang/invoke/DirectMethodHandle.java
>>>
>>> DirectMethodHandle.make now takes a callerClass parameter (which may 
>>> be null).
>>>
>>> DirectMethodHandle make "receiver" parameter is renamed "refc" as it 
>>> is not the receiver (it's the resolved type of the method holder ie 
>>> REFC).
>>>
>>> The Special subclass now takes a "checkClass" parameter which is 
>>> either refc, or callerClass. This is what we will check the receiver 
>>> against.
>>>
>>> In preparedLambdaForm we switch from LF_INVSPECIAL to 
>>> LF_INVSPECIAL_IFC if the target method is in an interface.
>>>
>>> In makePreparedLambdaForm we augment the needsReceiverCheck test to 
>>> include LF_INVSPECIAL_IFC. (So we will not be doing a new receiver 
>>> type check on all invokespecials, just those involving interface 
>>> methods.)
>>>
>>> Added DMH.checkReceiver which is overridden by both the Special and 
>>> Interface subclasses.
>>>
>>> - src/hotspot/share/c1/c1_Canonicalizer.cpp
>>>
>>> Don't optimize away the checkcast when it is an invokespecial 
>>> receiver check.
>>>
>>> - test/jdk/java/lang/invoke/SpecialInterfaceCall.java
>>>
>>> (I've moved this test back and forth between hotspot/runtime and 
>>> j.l.invoke as it spans direct calls and MH calls. But I think on 
>>> balance it lands better here.)
>>>
>>> The test sets up direct interface method calls for which 
>>> invokespecial is generated by javac, and similarly it creates 
>>> MethodHandles with invokespecial semantics. The should-not-throw 
>>> cases are trivial. The should-throw cases involve MH wizardry.
>>>
>>> The test is broken into three parts to check the behaviour on first 
>>> use (when the method has to be initially resolved); on second use (to 
>>> test we don't use the cpcache in a way that is invalid); and again 
>>> after forcing JIT compilation of the calls.
>>>
>>> The test is run 5 times to exercise the interpreter (yes the multiple 
>>> runs internal to the test are redundant in this case, but it's much 
>>> simpler to just run it all than try to work out what needs to be 
>>> run); the three variants of using C1, plus a C2 only run.
>>>
>>> ---
>>>
>>> Testing:
>>>    - the new test of course
>>>    - hotspot/runtime/*
>>>    - hotspot/compiler/jsr292
>>>    - jdk/java/lang/invoke
>>>
>>>    - hs tiers 1 and 2
>>>    - jdk tiers 1, 2 and 3
>>>
>>> Plus some related closed tests.
>>>
>>> Thanks,
>>> David
>>> -----