RFR(S) 8153115: Move private interface check to linktime

Tue Apr 12 17:18:32 UTC 2016

Igor,

My apologies, I thought you had already decided to push. Yes, I am good with the changes.
Sorry to keep you waiting.

thanks,
Karen

> On Apr 6, 2016, at 1:49 PM, Igor Veresov <igor.veresov at oracle.com> wrote:
> 
> Karen, am I correct to assume I can consider the current change reviewed? I’d like to push it. We can discuss how to harden/refactor other dimensions of the use of LinkResolver by compilers separately.
> 
> Thanks,
> igor
> 
>> On Apr 5, 2016, at 4:22 PM, Igor Veresov <igor.veresov at oracle.com> wrote:
>> 
>> 
>>> On Apr 5, 2016, at 3:33 PM, Karen Kinnear <karen.kinnear at oracle.com> wrote:
>>> 
>>> Igor,
>>> 
>>> Do you run all the tests with -Xcomp or whatever flag ensures you test JVMCI vs. interpreter
>>> for instance?
>> 
>> Yes, I ran our RBT round of testing that does that -Xcomp and -Xmixed.
>> 
>>> 
>>> If so, I am ok with checking this in - further notes below.
>>> 
>>>> On Apr 5, 2016, at 3:43 PM, Igor Veresov <igor.veresov at oracle.com <mailto:igor.veresov at oracle.com>> wrote:
>>>> 
>>>> 
>>>>> On Apr 5, 2016, at 12:04 PM, Karen Kinnear <karen.kinnear at oracle.com <mailto:karen.kinnear at oracle.com>> wrote:
>>>>> 
>>>>> I am in agreement with Lois that the JVMS looks good with moving the exception.
>>>> 
>>>> Thanks!
>>>>> 
>>>>> With the checking I have done so far, I believe that linktime_resolve_static_method is only called with an invoke static - but after my next
>>>>> meeting I will check one more time. It might be worth adding a comment.
>>>> 
>>>> Ok, I added a comment to resolve_interface_method(): http://cr.openjdk.java.net/~iveresov/8153115/webrev.01/ <http://cr.openjdk.java.net/~iveresov/8153115/webrev.01/>
>>>> Again, the bytecode argument here indicates the context, not the actual bytecode. Of course, for example, the method may be invoked with a method handle.
>>>> 
>>>>> 
>>>>> My concern is specific to the code in jvmciCompilerToVM.cpp there is code called resolveMethod that checks
>>>>> if holder_klass->is_interface -> LR::linktime_resolve_interface_method_or_null.
>>>>> 
>>>> 
>>>> That code needs fixing as well. We have the following issue filed for that: https://bugs.openjdk.java.net/browse/JDK-8152903 <https://bugs.openjdk.java.net/browse/JDK-8152903>
>>>> In the nutshell, resolveMethod() sort of emulates what happens during a virtual call (it is only called for invokevirtual and invokeinterface). It should do both linktime and runtime resolutions. The JVMCI version should work the same way as the CI version (see ciMethod::resolve_invoke() in ciMethod.cpp).
>>> 
>>> Hmmm - I see the ciMethod::resolve_invoke has the same problem that I just called out, so making the JVMCI one match
>>> the CI version makes me wonder if we have sufficient test cases. But I hear that you would like to address that as a followup.
>>> That is ok with me - I will add a note to the bug.
>> 
>> Could you please explain what is the problem again? Are you concerned that the bytecode is not passed to resolve_invoke, so we may call linktime_resolve_interface_or_null, for an interface holder when in reality it was an invokevirtual instruction and vice versa?
>> 
>>> 
>>> Also: I see a ciMethod::check_call that has a comment - 
>>> IT appears to fail when applied to an invoke interface call site.
>>> FIXME: Remove this method and resolve_method_statically; refactor to use the other LinkResolver entry points. 
>>> 
>> 
>> This comment is odd. I don’t see why it would fail for invokeinterface. The code certainly seems to account for it. May be the comment is wrong? Any ideas?
>> 
>> igor
>> 
>>> Could you possibly file a bug on this one? What I am seeing is a conditional for invoke static vs. invoke virtual that does not take
>>> the subtleties of invoke interface and invoke special into account. 
>>>> 
>>>> igor
>>>> 
>>>>> I think we need to study this one more closely - I suspect that you need a set of detailed tests that cover the
>>>>> corner cases here. I don’t know the code paths, but I would suggest following this approach of passing in the byte code,
>>>>> so that you get the correct behavior depending on the requesting byte code.
>>>>> 
>>>>> I am not sure what resolveMethod is doing here - since you seem to already have the resolved method and the receiver - so
>>>>> I could use help studying this a bit more to understand if this really is resolution or is really selection.
>>>>> 
>>>>> thanks,
>>>>> Karen
>>>>> 
>>>>>> On Apr 5, 2016, at 1:34 PM, Lois Foltan <lois.foltan at oracle.com <mailto:lois.foltan at oracle.com>> wrote:
>>>>>> 
>>>>>> 
>>>>>> On 4/5/2016 12:50 PM, Igor Veresov wrote:
>>>>>>> Hi Lois,
>>>>>>> 
>>>>>>> Thanks for looking at it. Yes, it passes all hotspot jtreg tests.
>>>>>>> 
>>>>>>> igor
>>>>>> Hi Igor,
>>>>>> 
>>>>>> Thanks for waiting on this.  A couple of comments:
>>>>>> 
>>>>>> - Section 6.5 Instructions for invokeinterface does indicate that a "Linking Exceptions" the VM can throw an ICCE if the resolved method is static or private.  So I think moving this exception from runtime to linktime is okay.
>>>>>> 
>>>>>> - I'm concerned about the change on line #998, #1030, #1316.  I don't think you are necessarily guaranteed to have the bytecodes that you are now passing to resolve_interface_method.  For example, line #998 within linktime_resolve_static_method, you may not have an invokestatic here, you may have another invoke* bytecode trying to invoke a static interface method.  Passing in Bytecodes::_invokestatic seems wrong, because even if the resolved method is static, "nostatics" was set to false.
>>>>>> 
>>>>>> Just curious did you also run the testbase default methods tests?
>>>>>> Lois
>>>>>> 
>>>>>>> 
>>>>>>>> On Apr 5, 2016, at 9:30 AM, Lois Foltan <lois.foltan at oracle.com <mailto:lois.foltan at oracle.com>> wrote:
>>>>>>>> 
>>>>>>>> Hi Igor,
>>>>>>>> 
>>>>>>>> I know you have two reviews for this but could you hold off committing until I or Karen Kinnear have a chance to review.  We both worked in this area a lot to support default methods in JDK 8. Also, have you run the hotspot/test/runtime/SelectionResolution tests on this?
>>>>>>>> 
>>>>>>>> Thanks,
>>>>>>>> Lois
>>>>>>>> 
>>>>>>>> On 4/1/2016 2:28 PM, Igor Veresov wrote:
>>>>>>>>> When invoking private interface methods with invokeinterface we throw ICCE. The check for that happens in the runtime part of the resolution, however, doing it at linktime seems like a better place, since the check doesn't depend on the receiver type. It also allows compiler interfaces that rely on linktime resolution to detect inconsistencies during parsing (see ciEnv::lookup_method() (CI) and ConstantPool.lookupMethod() (JVMCI) that are affected).
>>>>>>>>> 
>>>>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8153115 <https://bugs.openjdk.java.net/browse/JDK-8153115>
>>>>>>>>> Webrev: http://cr.openjdk.java.net/~iveresov/8153115/webrev.00/ <http://cr.openjdk.java.net/~iveresov/8153115/webrev.00/>
>>>>>>>>> 
>>>>>>>>> Thanks,
>>>>>>>>> igor
>> 
> 