[Nestmates] RFR (S): 8197915: [Nestmates] Implement receiver typecheck for private invokeinterface use
David Holmes
david.holmes at oracle.com
Wed May 9 23:02:07 UTC 2018
Thanks Karen, I'll get this pushed today.
David
On 10/05/2018 7:29 AM, Karen Kinnear wrote:
> David,
>> On May 8, 2018, at 6:26 PM, David Holmes <david.holmes at oracle.com
>> <mailto:david.holmes at oracle.com>> wrote:
>>
>> Hi Karen,
>>
>> On 9/05/2018 7:58 AM, Karen Kinnear wrote:
>>> David,
>>> Wow. Thank you again for tracking this down. And for the updated
>>> comments - this is really challenging logic.
>>
>> Indeed!
>>
>>> Couple of questions please.
>>> 1. cpCache.cpp
>>> Could you possibly walk through the CharSequence declares toString
>>> behavior?
>>
>> All I can say is that processing CharSequence.toString ends up in
>> set_direct_or_vtable_call with invokeinterface. In mainline that takes
>> the change_to_virtual path. I discovered this when I tried to add
>> overly strong assertions to the code paths.
> Thanks. Been wondering about that one for years.
> Calling LinkResolver for invokeinterface, we discover that
> CharSequence.toString has a vtable index because the public abstract
> toString()
> method overrides a java.lang.Object method and therefore needs to be
> treated relative to a vtable index rather than ani table index.
> A given method only gets one or the other.
>
>>
>>> 2. DirectMethodHandle.java
>>> Thank you for the clearer comments.
>>> case REF_invokeInterface: {
>>> 98 // for interfaces we always adapt to get the receiver
>>> 99 // check inserted (only if the MemberName kind is
>>> 100 // REF_invokeSpecial of course)
>>> 101 LambdaForm lform = preparedLambdaForm(member, true);
>>> 102 return new Interface(mtype, lform, member,
>>> refc);
>>> You lost me on this one -
>>> Where is the runtime check for Lookup.findVirtual(Interface, …)
>>> receiver is subtype of REFC?
>>> Why does that work for e.g. a public default method that does not
>>> optimize to “direct_call” -> REF_invokeSpecial
>>> but it does not work for a private interface method that does
>>> optimize to “direct_call” -> REF_invokeSpecial.
>>> I actually expected you to need this for both cases ?
>>
>> We do. My comment is misleading as it only refers to the "true"
>> argument being passed to preparedLambdaForm. I'll revise as:
>>
>> // for interfaces we always need the receiver typecheck,
>> // so we always pass 'true' to ensure we adapt if needed
>> // to include the REF_invokeSpecial case
>> LambdaForm lform = preparedLambdaForm(member, true);
>>
>> Does that help?
> Many thanks - I don’t need to see that again.
>>
>> To be clear, the existing logic (in mainline) in
>> makePreparedLambdaForm has:
>>
>> boolean needsReceiverCheck = (which == LF_INVINTERFACE);
>>
>> which triggers inclusion of the receiver typecheck by calling
>> Interface.checkReceiver. That covers the non-direct-call case. But
>> that was missing the direct-call case for private interface methods
>> where we had switched to LF_INVSPECIAL, and which we now switch to
>> LF_INVSPECIAL_IFC. The above logic is now:
>>
>> boolean needsReceiverCheck = (which == LF_INVINTERFACE ||
>> which == LF_INVSPECIAL_IFC);
>>
>> so that enables the missing checkReceiver call. So now both non-direct
>> and direct calls are covered.
>>
>> Hope that clarifies.
> Yes - that fits what I had thought. Thank you for fixing the comment.
> The code logic looks correct.
>
> thanks,
> Karen
>>
>> Thanks,
>> David
>> -----
>>
>>> thanks,
>>> Karen
>>>> On May 8, 2018, at 1:51 AM, David Holmes <david.holmes at oracle.com
>>>> <mailto:david.holmes at oracle.com><mailto:david.holmes at oracle.com>> wrote:
>>>>
>>>> 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