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

Wed Sep 6 10:32:15 UTC 2017

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?

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