[Nestmates] RFR (S): 8197915: [Nestmates] Implement receiver typecheck for private invokeinterface use
David Holmes
david.holmes at oracle.com
Tue May 8 22:26:50 UTC 2018
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.
> 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?
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.
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>> 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