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

[Vladimir Ivanov]

David, thanks for taking care of the bug!

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.

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

Otherwise, looks good.


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