RFR: 8187221 [Nestmates] Virtual invocation for private interface methods

[David Holmes]

On 6/09/2017 7:15 PM, Maurizio Cimadamore wrote:
> On 06/09/17 07:58, David Holmes wrote:
>> On 6/09/2017 7:34 AM, David Holmes wrote:
>>> Hi Maurizio,
>>>
>>> On 5/09/2017 11:20 PM, Maurizio Cimadamore wrote:
>>>> Langtools change looks good,
>>>
>>> You made it so simple for me to change. :)
>>>
>>>> VM changes also make sense. One question - I wonder why the old code 
>>>> assumed that link_info.current_klass() could be null? I see that 
>>>> your new code expects that not to be the case (otherwise the code 
>>>> would crash), and that's probably a consequence of the fact that 
>>>> you're using InstanceKlass::cast which I see asserts that value 
>>>> passed in is not NULL. So does it mean that this value cannot ever 
>>>> be NULL and that the old code was 'bogus' ?
>>>
>>> My take was that the current_klass can not possibly be null - but 
>>> looking further I think that was wishful thinking. We allow for a 
>>> NULL caller class:
>>>
>>>    // Case where we just find the method and don't check access 
>>> against the current class
>>>    LinkInfo(Klass* resolved_klass, Symbol*name, Symbol* signature) :
>>>      _resolved_klass(resolved_klass),
>>>      _name(name), _signature(signature), _current_klass(NULL), 
>>> _current_method(methodHandle()),
>>>      _check_access(false), _tag(JVM_CONSTANT_Invalid) {}
>>>
>>>
>>> and I see some use of this from JavaCalls and MethodHandle code. It 
>>> may be that if this code is called from JNI that there is no current 
>>> klass. 
>>
>> Given we're dealing with an invokeinterface bytecode I can't see how 
>> JNI can possibly be involved, nor how we could fail to have a current 
>> class. I've added an assert and re-run current testing to see if it 
>> trips, but ...
> Was thinking about that too - seems like if you are in invokeinterface, 
> then you are in Java-land, and then you should have some current class. 
> But there might be special cases I don't know about :-)
> 
> Anyway thx for checking, it just occurred to me when looking at the 
> code. Probably is not too important (I think an assertion should have 
> happened already as one is forced in the InstanceKlass::cast if the 
> operand is NULL, if I read the code correctly).

The InstanceKlass::cast is new code and at present there is nothing that 
goes down that path - hence the test coverage issue. :(

David

> Maurizio
>>
>>> That particularly code path may not be tested yet ... which suggests 
>>> there is no test that tries to trigger the ICCE, because it would now 
>>> not throw (such a test would have to be compiled with an earlier 
>>> classfile version).
>>
>> ... we do seem to be missing basic testing in this area, so this code 
>> path is not being exercised at present. Additionally I need to force 
>> the taking of this path by using different classfile versions which 
>> I'm not setup to do.
>>
>> David
>> -----
>>
>>> I'll have to dig deeper on this.
>>>
>>> Thanks,
>>> David
>>>
>>>> Maurizio
>>>>
>>>>
>>>> On 05/09/17 13:34, David Holmes wrote:
>>>>> webrev: http://cr.openjdk.java.net/~dholmes/8187221/
>>>>>
>>>>> Following up from JDK-8186763 this issue handles item #2:
>>>>>
>>>>> 2. Use invokeinterface for private interface method invocations, 
>>>>> instead of invokespecial
>>>>>
>>>>> This involves three basic pieces:
>>>>>
>>>>> 1. javac issues invokeinterface instead of invokespecial
>>>>> 2. Verifier rules requiring invokespecial are relaxed for latest 
>>>>> class file version (exact version TBD based on release timing)
>>>>> 3. Method resolution locates the expected method.
>>>>>
>>>>> As part of this change I'm also isolating the changes that allowed 
>>>>> invokespecial to be used for nestmate method invocations. In the 
>>>>> near future that code will be removed so that we are near final-form.
>>>>>
>>>>> Actual file changes are very simple in the end:
>>>>>
>>>>> src/share/vm/classfile/verifier.cpp
>>>>>
>>>>> Restore original (pre-nestmate) logic for invokespecial and add in 
>>>>> the nestmate-enabling logic under the UseNewCode guard.
>>>>>
>>>>> ---
>>>>>
>>>>> src/share/vm/interpreter/linkResolver.cpp
>>>>>
>>>>> LinkResolver::resolve_interface_method: Only prohibit 
>>>>> invokeinterface for private interface methods if : 
>>>>> InstanceKlass::cast(current_klass)->major_version() < 
>>>>> VIRTUAL_PRIVATE_ACCESS_VERSION
>>>>>
>>>>> LinkResolver::runtime_resolve_interface_method: Don't resolve in 
>>>>> the receiver class if dealing with a private interface method
>>>>>
>>>>> ---
>>>>>
>>>>> src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Gen.java
>>>>>
>>>>> Removed the check for interface methods when generating virtual calls.
>>>>>
>>>>> ---
>>>>>
>>>>> With these changes in place the following test options will test 
>>>>> the use of invokevirtual and invokeinterface for private method 
>>>>> invocations:
>>>>>
>>>>> -javacoption:-XDdisablePrivateAccessors 
>>>>> -javacoption:-XDvirtualizePrivateAccess
>>>>>
>>>>> If you still want to experiment with the invokespecial changes use:
>>>>>
>>>>> -javacoption:-XDdisablePrivateAccessors -javaoption:-XX:+UseNewCode
>>>>>
>>>>> (note the second one is a java option not javac)
>>>>>
>>>>> Next iteration I plan on removing the invokespecial related changes.
>>>>>
>>>>> ---
>>>>>
>>>>> Testing so far:
>>>>>  - all runtime jtreg tests
>>>>>  - langtools jtreg tests
>>>>>
>>>>> Cheers,
>>>>> David
>>>>> -----
>>>>
>