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

[Lois Foltan]

On 4/5/2016 6:08 PM, Igor Veresov wrote:
> Coleen and Lois,
>
> So, ok to push?
Yes, for me.  Have you addressed all of Karen's concerns as well?
Lois

>
> igor
>
>
>> On Apr 5, 2016, at 1:04 PM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
>>
>>
>>
>> On 4/5/16 3:59 PM, Coleen Phillimore wrote:
>>>
>>> On 4/5/16 3:54 PM, Lois Foltan wrote:
>>>> On 4/5/2016 1:56 PM, Igor Veresov wrote:
>>>>>> On Apr 5, 2016, at 10:44 AM, Igor Veresov <igor.veresov at oracle.com <mailto:igor.veresov at oracle.com>> wrote:
>>>>>>
>>>>>>> On Apr 5, 2016, at 10:34 AM, 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.
>>>>>>>
>>>>>> I looked at the call graphs of these guys and linktime_resolve_X_method() methods actually seem to only called within invokeX contexts. But may be I missed something. Can you give an example of a path that may cause, say, linktime_resolve_static_method() be invoked for non-invokestatic instruction?
>>>>> Actually, the easier way to think about it, would be: The answer returned by resolve_interface_method() is result of a method resolution in the interface class “as if” it were invoked by the given bytecode instruction. It of course doesn’t not mean that the invocation is really a result of the said instruction. The context is. As you may see the logic around “nostatic” did not change and the logic around resolve_interface_method() being called within the invokeinterface context is what we want it to be.
>>>>>
>>>>> The same effect could have been achieved by adding another bool argument to resolve_interface_method() to indicate a question within the invokeinterface context. But passing a bytecode makes it an easier to read code.
>>>> Okay, I saw Tom's reply as well to my comments and I reviewed all the call paths.  I think the change is okay.  I kind of wish that the original bytecode was stored in the LinkInfo structure so that we could just reference the actual bytecode used and make decisions based on that instead of the parameter approach, but that maybe a RFE to investigate later.
>>> There's a change coming that does store the original bytecode for bug https://bugs.openjdk.java.net/browse/JDK-8145148
>> No, sorry, this change passes the 'tag'.  nvm.
>>
>> Coleen
>>> Just waiting for some test changes.
>>>
>>> Coleen
>>>
>>>> Lois
>>>>
>>>>> igor
>>>>>
>>>>>>> Just curious did you also run the testbase default methods tests?
>>>>>> Yes, within the context of a closed project. That’s actually what made these changes necessary.
>>>>>>
>>>>>> igor
>>>>>>
>>>>>>> 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
>>>>>>>>>> Webrev: http://cr.openjdk.java.net/~iveresov/8153115/webrev.00/ <http://cr.openjdk.java.net/%7Eiveresov/8153115/webrev.00/>
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> igor