[Nestmates] RFR (S): 8197915: [Nestmates] Implement receiver typecheck for private invokeinterface use
David Holmes
david.holmes at oracle.com
Tue May 8 05:51:27 UTC 2018
Hi Karen,
On 8/05/2018 7:29 AM, Karen Kinnear wrote:
> David,
>
> Looks good!
Thanks. Alas I took a slightly wrong turn way back when which needs to
be addressed - see below. Updated webrev:
http://cr.openjdk.java.net/~dholmes/8197915/webrev.v5/
> Many thanks for the update and the explanations. I had missed that DirectMethodHandle.make() refKind reflects
> the original Lookup.findVirtual/findSpecial distinction - that is just what we needed. Thanks for the clarification.
> Now I understand this fix and really like it.
>
> Totally support following up with bugs/rfes that are not nestmate specific.
>
> Could you possibly modify a couple of comments in the code to make this a bit clearer?
>
> DirectMethodHandle.java
>
> 1. line 96
> I don’t know what “// we always adapt ‘special’ when dealing with interfaces”
> I think what you mean is that we always need to perform a dynamic check
> that the receiver is a subtype of the reference class for interfaces
The comment was trying to explain why we pass true to
preparedLambdaForm, and followed on from the comment in the
REF_invokeSpecial case. I've changed it to:
// for interfaces we always adapt to get the receiver
// check inserted (only if the MemberName kind is
// REF_invokeSpecial of course)
> 2. Relative to line 84: it would help me to have a comment that refKind is relative to the
> FindVirtual(Interface or Class) or findSpecial original bytecode-equivalent request.
Inserted:
// refKind reflects the original type of lookup via findSpecial or
// findVirtual etc.
>
> 2. lines 176-179
> Perhaps replace with:
> // MemberName.getReferenceKind represents the JVM optimized form of the call
> // as distinct from the ‘kind’ passed to DMH.make which represents the original
> // bytecode-equivalent request.Specifically private/final methods that use a direct call
> // have MemberName.getReferenceKind adapted to REF_invokeSpecial, even though
> // the actual invocation mode as represented by the ‘kind’ passed to DMH.make, which
> // may be invokevirtual or invokeinterface
Done (with minor editing).
> 3. cpCache.cpp line 192
> Thank you for the assertion change.
> Is line 192 always the case? I think it is sometimes interface klass* and sometimes Object klass*
Right. This is where I went off track a little with my changes earlier
on. The key point is that this code was never intended for processing
any Object methods, but only private interface methods. All Object
methods and any non-private interface methods (which can happen if an
interface redeclares a method also declared in Object!) were supposed to
follow the "else" and be treated exactly the same as they were before
the nestmate changes. But I unintentionally pulled final Object methods
into the private interface code - hence the assertion problem and the
line 192 ambiguity. So that's been fixed.
Of course that highlighted the same kind of mistake in another area -
the templateTable. I inserted my private interface method "vfinal" check
before the special Object method handling code. But of course that also
causes final Object methods to follow my interface logic - which is
wrong (and triggers a SEGV/NPE because the 'interface' klass* is not set
in f2 in that case). So I needed to reorder the vfinal logic with the
forced_virtual_shift logic.
Re-tested:
jdk/java/lang/invoke
hotspot/runtime/Nestmates
hotspot/runtime/SelectionResolution
hotspot/compiler/jsr232
mach5: jdk-tier1,jdk-tier2,jdk-tier3,hs-tier1,hs-tier2,builds-tier1
Thanks,
David
> thank you for the additional tests,
> Karen
>
>> On May 7, 2018, at 2:43 AM, David Holmes <david.holmes at oracle.com> wrote:
>>
>> Updated webrev:
>>
>> http://cr.openjdk.java.net/~dholmes/8197915/webrev.v4/
>>
>> Fixed the too-strict assertions in linkResolver and ConstantPoolCacheEntry::set_direct_or_vtable_call. Adjusted the logic in ConstantPoolCacheEntry::set_direct_or_vtable_call to match the updated assertion.
>>
>> Updated the test with additional test cases (related to additional ones being added for 8200167 under 8202686).
>>
>> Two test forms are commented out as they fail (ie they don't throw the expected exceptions). These failings are due to non-nestmate-specific omissions in the cpCache and MH code. As such they will need to be addressed at a later time. Bugs will be filed once a few logistical issues have been resolved.
>>
>> Thanks,
>> David
>>
>> On 7/05/2018 11:50 AM, David Holmes wrote:
>>> I've worked out the difference in the tests. The jasm version used the wrong REFC: Object instead of the interface type.
>>> Fixing assertions and updating tests.
>>> David
>>> On 7/05/2018 9:05 AM, David Holmes wrote:
>>>> Hi Karen,
>>>>
>>>> First, outside of nestmates I've filed a bug (8202686) and send out a RFR to add the missing testcase for final Object methods to the test for 8200167. It doesn't show any issues of course.
>>>>
>>>> Next I've taken the additional testcases and moved them into the PrivateInterfaceCall test - adapted for invokeinterface - which should cover the test you wrote below ... however ...
>>>>
>>>> On 5/05/2018 8:04 AM, David Holmes wrote:
>>>>> Hi Karen,
>>>>>
>>>>> On 5/05/2018 5:54 AM, Karen Kinnear wrote:
>>>>>> David,
>>>>>>
>>>>>> Putting together a wiki to describe how I think this works with cpCache and with MethodHandles. Not
>>>>>> yet done …
>>>>>>
>>>>>> In the process of testing cases - I found a couple of assertions in the nestmate repo that are not accurate:
>>>>>>
>>>>>> 1. linkResolver.cpp:
>>>>>> # assert(resolved_method()->is_private()) failed: Should only have non-virtual invokeinterface for private methods!
>>>>
>>>> Yes this overlooked that final Object methods can also follow this path. It is fixed by simply extending the assert to include "or is a final Object method".
>>>>
>>>> This was triggered by both a direct call attempt for a final Object method and a MH invocation of same.
>>>>
>>>>>>
>>>>>> 2. ConstantPoolCacheEntry::set_direct_or_vtable_call
>>>>>> invokeinterface asserts is_private
>>>>
>>>> None of my testing hit this assertion failure. Yet your test (which I essentially copied) does. This is very puzzling.
>>>>
>>>> Further if I suppress that assert then I hit:
>>>>
>>>> # Internal Error (/export/users/dh198349/valhalla/repos/valhalla-dev/open/src/hotspot/share/oops/cpCache.cpp:276), pid=8649, tid=8650
>>>> # Error: assert(invoke_code == Bytecodes::_invokevirtual || (method->is_private() && invoke_code == Bytecodes::_invokeinterface)) failed
>>>>
>>>> This is all easily fixed, but the test scenarios need more investigation. Not only does your direct invocation test trigger the above assertions where mine does not; my test fails due to:
>>>>
>>>> IncompatibleClassChangeError: Found class java.lang.Object, but interface was expected
>>>>
>>>> but yours does not! The only difference I can see is that your test has the call in a class, whereas mine has it in an interface.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>
>>>>
>>>>>> I wrote a small test (sorry - I patched the bytecodes to do this quickly) which has
>>>>>> invokeinterface I.getClass() // javac put invokevirtual when I tried to get it to generate that
>>>>>
>>>>> Great catch! Another variant of the "invoking object methods via invokeinterface" problem - the final method case. The asserts in principle need to weaken to "or is an Object method".
>>>>>
>>>>>> This isn’t the methodHandles, this is just the straight bytecodes - but it is part of the decision tree of are we using Ref_invokeSpecial.
>>>>>
>>>>> Not sure how the bytecode issue relates at all to the MH logic? But of course we have to try and construct a MH version of the direct invoke as well.
>>>>>
>>>>> Will tackle this Monday.
>>>>>
>>>>> Many thanks,
>>>>> David
>>>>>
>>>>>>
>>>>>> I attached the test - it is built for jdk10 so I could test before and after. If you recompile Test.java it will need repatching.
>>>>>>
>>>>>> Since getClass is final, it also goes through the direct_call route for invokeinterface.
>>>>>>
>>>>>> thanks,
>>>>>> Karen
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>> On May 4, 2018, at 4:22 AM, David Holmes <david.holmes at oracle.com <mailto:david.holmes at oracle.com>> wrote:
>>>>>>>
>>>>>>> Thanks Vladimir!
>>>>>>>
>>>>>>> David
>>>>>>>
>>>>>>> On 4/05/2018 6:10 PM, Vladimir Ivanov wrote:
>>>>>>>>>
>>>>>>>>> Ok. webrev updated to v3:
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~dholmes/8197915/webrev.v3/
>>>>>>>> Looks good!
>>>>>>>> Best regards,
>>>>>>>> Vladimir Ivanov
>>>>>>>>>
>>>>>>>>> New code:
>>>>>>>>>
>>>>>>>>> // if caller is an interface we need to adapt to get the
>>>>>>>>> // receiver check inserted
>>>>>>>>> if (callerClass == null) {
>>>>>>>>> throw new InternalError("callerClass must not be null for REF_invokeSpecial");
>>>>>>>>> }
>>>>>>>>> LambdaForm lform = preparedLambdaForm(member, callerClass.isInterface());
>>>>>>>>> return new Special(mtype, lform, member, callerClass);
>>>>>>>>>
>>>>>>>>> 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