On Jan 15, 2014, at 11:41 AM, Vladimir Ivanov <vladimir.x.iva...@oracle.com> wrote:
> Chris, > > Thanks for looking into this. See my answers inline. > > Best regards, > Vladimir Ivanov > > On 1/15/14 9:51 PM, Christian Thalinger wrote: >> [I’m resending something I sent earlier today to Vladimir directly.] >> >> On Jan 15, 2014, at 7:31 AM, Vladimir Ivanov <vladimir.x.iva...@oracle.com> >> wrote: >> >>> http://cr.openjdk.java.net/~vlivanov/8031502/webrev.00/ >>> https://bugs.openjdk.java.net/browse/JDK-8031502 >>> >>> InvokeBytecodeGenerator can produce incorrect bytecode for a LambdaForm >>> when invoking a method from Object declared in an interface. >>> >>> The problem is the following: >>> (1) java.lang.CharSequence interface declares abstract method "String >>> toString()"; >>> >>> (2) after 8014013 fix, VM resolves >>> CharSequence::toString()/invokeInterface to >>> CharSequence::toString()/invokeVirtual; >> >> Without having looked at the changes of 8014013, why did the invoke type >> change? Is it an optimization that was added? >> > > After 8014013, LinkResolver::resolve_interface_call returns virtual > (_call_kind = CallInfo::vtable_call), instead of interface method and > MethodHandles::init_method_MemberName uses it as is (without any fine > tuning, which was done before). > > It's caused by the following: > - LinkResolver::linktime_resolve_interface_method returns > CharSequence::toString method, but it has vtable index, instead of > itable index; > > - LinkResolver::runtime_resolve_interface_method looks at resolved > method and sets _call_kind to vtable_call, since resolved method doesn't > have itable index. > > - then MethodHandles::init_method_MemberName looks at CallInfo passed > in and sets the reference kind flag to JVM_REF_invokeVirtual. > > That's how the conversion from invokeInterface to invokeVirtual occurs. > > I did a quick debugging session with pre-8014013 hotspot to check how it > worked before, but don't remember all the details now. > >>> >>> (3) during LambdaForm compilation, CharSequence is considered >>> statically invocable (see >>> InvokeBytecodeGenerator::isStaticallyInvocable) and invokevirtual for >>> CharSequence::toString() is issued, which is wrong (invokevirtual throws >>> ICCE if it references an interface); >>> >>> The fix is straightforward: during LambdaForm compilation, switch back >>> from invokevirtual to invokeinterface instruction when invoking a method >>> on an interface. >> >> I find this risky. Right now MemberName is only used in a couple places in >> java.lang.invoke but in the future it might be used for other things (e.g. >> java.lang.reflect). The information MemberName contains should be correct >> and usable without changing. Otherwise all users have to patch the >> information the same way as you do in your patch. >> >> I would like to see the VM passing correct information (whatever the >> definition of correct is here). >> > > It's an interesting question what kind of correctness is required for > MemberName and I don't know the answer. Looking through the code, I got > an impression MemberName isn't designed to provide information to be > used "as is" for bytecode generation, because, at least: > - MemberName::referenceKindIsConsistent already expects > (clazz.isInterface() && refKind == REF_invokeVirtual && > isObjectPublicMethod()) case; > > - InvokeBytecodeGenerator::emitStaticInvoke already has a special > case for REF_invokeSpecial referenceKind, converting it to > REF_invokeVirtual; John would know the answer. Given this change should go into JDK 8 I think we should push for now. If we can come up with a better way to handle these situations we can push another change for 9 or 8u20. > > Best regards, > Vladimir Ivanov > _______________________________________________ > mlvm-dev mailing list > mlvm-...@openjdk.java.net > http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
