[Nestmates] RFR (S): 8197915: [Nestmates] Implement receiver typecheck for private invokeinterface use
David Holmes
david.holmes at oracle.com
Fri May 4 07:10:33 UTC 2018
On 4/05/2018 5:04 PM, Vladimir Ivanov wrote:
>
>>> http://cr.openjdk.java.net/~dholmes/8197915/webrev.v2/
>
> src/java.base/share/classes/java/lang/invoke/DirectMethodHandle.java
>
> + LambdaForm lform = preparedLambdaForm(member,
> callerClass.isInterface());
>
> You need to handle "callerClass == null" case as well.
I'm not sure I do now. I think we must always have a callerClass context
when we hit this code. Can you see where a null may come from? So far
testing has not produced any failures relating to a null callerClass. If
we did ever get a null we'd be missing the receiver <: caller check, and
that would be a bug requiring us to change the code to pass in the caller.
Thanks,
David
> Best regards,
> Vladimir Ivanov
>
>>>
>>> Changes:
>>>
>>> - DirectMethodHandles.java: new simple and direct approach to dealing
>>> with LF_SPECIAL_IFC
>>
>> I like how java.lang.invoke part shapes out!
>>
>> Maybe rename adaptToSpecialIfc to needsReceiverCheck? That's what
>> confused me in the first version: though it's an interface call (which
>> always require receiver check against REFC), new checks only referred
>> to LF_INVSPECIAL (since invocation mode is a direct call).
>>
>>> - New regression test for the final virtual call from an interface
>>> bug introduced by 8200167.
>>>
>>> If necessary/desirable I can fix that part in mainline separately. So
>>> far no tests (including jck/API/java/lang) seem to tickle it.
>>
>> Or file a bug. I have some ideas how to improve relevant code and make
>> LF construction cleaner.
>>
>> Best regards,
>> Vladimir Ivanov
>>
>>
>>> Thanks,
>>> David
>>>
>>> On 4/05/2018 11:41 AM, David Holmes wrote:
>>>> Hi Karen,
>>>>
>>>> On 4/05/2018 6:39 AM, Karen Kinnear wrote:
>>>>> David,
>>>>>
>>>>> Really delighted to see you near the end of the major functional
>>>>> changes!
>>>>
>>>> Thanks for taking a look so quickly!
>>>>
>>>>> A couple minor comments, and then a question please:
>>>>>
>>>>> 1. MethodHandles.java
>>>>
>>>> DirectMethodHandle.java :)
>>>>
>>>>> 174 different “to” -> different “from” ?
>>>>
>>>> Changed. That's my UK upbringing :)
>>>>
>>>> https://en.oxforddictionaries.com/usage/different-from-than-or-to
>>>>
>>>>> 2. methodHandles.cpp
>>>>> 300-301
>>>>> Thank you for the comment.
>>>>> Might it also be worth adding that direct call is used by:
>>>>> invoke static, invokespecial, invokeinterface:local private,
>>>>> invoke virtual:vfinal and private methods
>>>>> (or are you concerned about getting out of sync if this changes?)
>>>>
>>>> It is not used by invokestatic. I'm not 100% sure of all the exact
>>>> cases where an invokeinterface/invokevirtual becomes a direct call,
>>>> so didn't want to say anything inaccurate. But the comment as it
>>>> stands is awkward so I've expanded it:
>>>>
>>>> // "special" reflects that this is a direct call, not that it
>>>> // necessarily originates from an invokespecial. We can also do
>>>> // direct calls for private and/or final non-static methods.
>>>>
>>>>> 3. DirectMethodHandle.java - this was subtle!
>>>>
>>>> More than you realise ;-)
>>>>
>>>>> I believe this is correct assuming that:
>>>>> CallerClass is always and only set for invokespecial. Is this
>>>>> accurate? Could you possibly add a comment?
>>>>
>>>> That's an excellent question and one that should have been asked
>>>> before 8200167 was finalized. :( The short answer is "no" -
>>>> callerClass can be non-null for any of the invocation modes. And yes
>>>> the current mainline code is broken - seems there is a gap in the
>>>> existing test coverage as we never call a final method from an
>>>> interface method. If we do we get:
>>>>
>>>> Exception in thread "main" java.lang.InternalError: Should only be
>>>> invoked on a subclass
>>>> at
>>>> java.base/java.lang.invoke.DirectMethodHandle.checkReceiver(DirectMethodHandle.java:441)
>>>>
>>>>
>>>> <sigh>
>>>>
>>>> We only look at callerClass when dealing with LF_INVSPECIAL, which
>>>> in mainline means we either have an invokespecial or an
>>>> invokevirtual. For invokespecial this is fine of course. But the
>>>> invokevirtual case was never encountered and so slipped by in error.
>>>> With nestmates we also add invokeinterface to the mix - which is
>>>> fine because if it is an invokeinterface then we want the check
>>>> regardless. It doesn't matter if the check is enabled because of the
>>>> (incidental) callerClass.isInterface check, or the explicit
>>>> m.getDeclaringClass().isInterface(). But the logic is messy and far
>>>> from clear and not correct by construction. So I will completely
>>>> redo it in a simpler and more direct/explicit way.
>>>>
>>>> BTW another red-herring: the !m.isStatic() part of the condition was
>>>> not needed. I was tracking down two failure modes before finalizing
>>>> this. The first was a problem with a static interface method - fixed
>>>> by the !m.isStatic(). The second was caused by missing parentheses
>>>> in the overall condition - which once fixed precluded the static
>>>> case, so the first fix was not needed (as we never use LF_INVSPECIAL
>>>> with statics). If only I'd tackled them in the reverse order.
>>>>
>>>> I'll post an updated webrev later today once I've re-tested lots of
>>>> things.
>>>>
>>>>> - agree with the theory that invokevirtual will never find a
>>>>> private interface method (and ACC_FINAL is illegal for interfaces)
>>>>
>>>> Yes. More specifically as we're dealing with MH semantics:
>>>> findVirtual for an interface method yields a MH with invokeInterface
>>>> "kind", not one with invokeVirtual "kind".
>>>>
>>>> public MethodHandle findVirtual(Class<?> refc, String name,
>>>> MethodType type) throws NoSuchMethodException, IllegalAccessException {
>>>> ...
>>>> byte refKind = (refc.isInterface() ? REF_invokeInterface :
>>>> REF_invokeVirtual);
>>>> ...
>>>> }
>>>>
>>>>
>>>>> 4. Test - I still need to study this
>>>>> I have been writing down test cases to make sure we don’t test
>>>>> cases we don’t want to, and I
>>>>> need to double-check you have them covered. Will do that tomorrow.
>>>>
>>>> The testing is all "positive" in the sense that it ensures a
>>>> receiver subtype check is in place when it "must be". In fact it
>>>> must always be the case the receiver has a type that has the method
>>>> being invoked. We were just missing a few cases that verified that
>>>> (and some stronger conditions: ie receiver <: caller for
>>>> invokespecial semantics).
>>>>
>>>> If you want to test that we don't insert the new explicit checks in
>>>> cases where they are not needed, then I don't know how to do that -
>>>> other than by adding tracing and running the test case and not
>>>> seeing checkReceiver being called.
>>>>
>>>> That said, once I've reworked the logic it will be blindingly
>>>> obvious when the new explicit check is being added.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>
>>>>> thanks,
>>>>> Karen
>>>>>
>>>>>> On May 3, 2018, at 6:21 AM, David Holmes <david.holmes at oracle.com>
>>>>>> wrote:
>>>>>>
>>>>>> bug id: https://bugs.openjdk.java.net/browse/JDK-8197915
>>>>>> webrev: http://cr.openjdk.java.net/~dholmes/8197915/webrev/
>>>>>>
>>>>>> JDK-8174962 implemented receiver typechecks for invokeinterface
>>>>>> within the interpreter (templateTable), compilers and for
>>>>>> MethodHandles. In nestmates invokeinterface can now be used for
>>>>>> private interface methods - which result in direct calls. So we
>>>>>> need to extend the receiver subtype checks to cover the new cases.
>>>>>>
>>>>>> Summary of changes:
>>>>>>
>>>>>> - src/hotspot/cpu/<cpu>/templateTable_<cpu>.cpp
>>>>>>
>>>>>> In the templateTable the 8174962 checks come after the private
>>>>>> interface method invocation logic ("vfinal") we already had in
>>>>>> place for the nestmate changes, and they rely on itable
>>>>>> information that doesn't exist for private methods. So we insert a
>>>>>> direct subtype check.
>>>>>>
>>>>>> I've provided code for all CPU's but only x86 and sparc have been
>>>>>> tested. I'll be soliciting aid on the other ports before nestmates
>>>>>> goes to mainline later this month.
>>>>>>
>>>>>> - src/hotspot/share/oops/cpCache.cpp
>>>>>>
>>>>>> We have to pass the interface klass* so it's available for the
>>>>>> typecheck.
>>>>>>
>>>>>> - src/hotspot/share/oops/klassVtable.cpp
>>>>>>
>>>>>> Updated a comment that's no longer accurate.
>>>>>>
>>>>>> - src/hotspot/share/opto/doCall.cpp
>>>>>>
>>>>>> This code was provided by Vladimir Ivanov (thank you!) and expands
>>>>>> the existing "invokespecial" support for receiver typechecks in
>>>>>> C2, to "invokeinterface" as well.
>>>>>>
>>>>>> Aside: no changes were needed for C1. It's seems all the receiver
>>>>>> typechecks for C1 are being handled at a higher level (through
>>>>>> linkResolver and/or cpCache logic).
>>>>>>
>>>>>> - src/hotspot/share/prims/methodHandles.cpp
>>>>>>
>>>>>> Comment clarifying JVM_REF_invokeSpecial doesn't necessarily mean
>>>>>> it relates to an actual "invokespecial" - it is used for all
>>>>>> direct calls.
>>>>>>
>>>>>> -
>>>>>> src/java.base/share/classes/java/lang/invoke/DirectMethodHandle.java
>>>>>>
>>>>>> Add clarifying comments regarding how "kind" can vary if a direct
>>>>>> call is involved.
>>>>>>
>>>>>> Expand the condition to switch from LF_INVSPECIAL to
>>>>>> LF_INVSPECIAL_IFC (which adds the additional receiver typecheck)
>>>>>> to account for the invokeinterface case.
>>>>>>
>>>>>> - test/jdk/java/lang/invoke/PrivateInterfaceCall.java
>>>>>>
>>>>>> New test for invokeinterface semantics that mirrors the existing
>>>>>> SpecialInterfaceCall test for invokespecial.
>>>>>>
>>>>>> This is the last of the significant functional changes for nestmates.
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>
More information about the valhalla-dev
mailing list