Looks good! Thank you so much David and Vladimir for all the work to cover the corner cases.
thanks, Karen > On Apr 26, 2018, at 12:12 AM, David Holmes <david.hol...@oracle.com> 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 >> -----
