[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