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