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

Maurizio Cimadamore maurizio.cimadamore at oracle.com
Wed Sep 6 09:15:39 UTC 2017



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

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



More information about the valhalla-dev mailing list