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

[Lois Foltan]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20160405/eb03ce99/attachment.html>