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

Wed Sep 6 12:12:12 UTC 2017

On 6/09/2017 8:32 PM, Maurizio Cimadamore wrote:
> On 06/09/17 10:48, David Holmes wrote:
>> 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. :(
> But isn't your code calling it? If so, if there was an assertion you 
> should have seen it when running tests - but maybe you mean that the path:
> 
> if (code == Bytecodes::_invokeinterface && resolved_method->is_private()) {
> 
> 
> Is not exercised that much in tests - so you're not even sure you even 
> run the cast?

Sorry, that path is executed by one of my tests - where obviously 
current_klass is not null. But there should be pre-existing tests that 
would take that path to throw the exception - those tests would fail by 
not throwing the exception if they existed because the version check 
would skip the exception.

But the more I think about this the more I'm convinced we can never have 
a null current_klass if we're in this code.

David
-----

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