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

[Coleen Phillimore]

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
>>>
>>
>