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

[Coleen Phillimore]

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

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