RFR: 8232613: Move Object.registerNatives into HotSpot

[David Holmes]

On 22/10/2019 11:41 pm, Claes Redestad wrote:
> Hi Lois,
> 
> to make sure noone misses it, here's the new webrev:
> http://cr.openjdk.java.net/~redestad/8232613/open.03

I like the code placement in this version - Coleen's suggestion was 
better than mine :)

> Comments inline..

Comments inline ...

> On 2019-10-22 14:33, Lois Foltan wrote:
>> On 10/22/2019 7:13 AM, Claes Redestad wrote:
>>>
>>>
>>> On 2019-10-22 13:05, Andrew Dinn wrote:
>>>> On 22/10/2019 12:05, Claes Redestad wrote:
>>>>> On 2019-10-22 12:44, Andrew Dinn wrote:
>>>>>> Why not leave it void?
>>>>>
>>>>> I guess:
>>>>>
>>>>> http://cr.openjdk.java.net/~redestad/8232613/open.02/
>>>> Ship it ;-)
>>>
>>> No wait, I missed the use in jni.cpp where the bool return is actually
>>> used. I ignored it in systemDict since when I got an exception there
>>> (by misspelling a name or using the wrong signature) the exception
>>> bubbles up and crashes the VM.
>>>
>>> Perhaps this could be reworked to remove the bool cleanly, but let's 
>>> go back to open.01 and move that cleanup to a follow-up.
>>>
>>> /Claes
>>>
>> Hi Claes,
>>
>> I do have a concern that this will be a behavioral change for method 
>> resolution if Object::registerNatives is removed.  For example,

Good catch Lois!

>>
>> public class RegisterNatives {
>>    interface I { default public void registerNatives() { 
>> System.out.println("I"); } }
>>    static class A implements I { }
>>    public static void main(String... args) {
>>      A varA = new A();
>>      varA.registerNatives();
>>    }
>>
>> Currently class A finds registerNatives in its superclass Object and 
>> the test receives the following:
>>
>>     Exception in thread "main" java.lang.IllegalAccessError: class
>>     RegisterNatives tried to access private method 'void
>>     java.lang.Object.registerNatives()' (RegisterNatives is in unnamed
>>     module of loader 'app'; java.lang.Object is in module java.base of
>>     loader 'bootstrap')
>>              at RegisterNatives.main(RegisterNatives.java:6)
>>
>> With your change interface I's registerNatives default method is 
>> invoked successfully.  I don't think this is a major backward 
>> compatibilty issue but we should have someone from core-libs okay the 
>> removal of the method from Object before committing.  In addition, can 
>> you add this test as part of your change?  I think it would be okay to 
>> put it in open/test/hotspot/jtreg/runtime/8024804 which contains an 
>> existing registerNatives test.
> 
> Yeah, *not* throwing an IAE on this feels like an unintentional bug fix.
> :-)

Yes it is a somewhat surprising aspect of default method resolution, but 
methods in the class hierarchy must be considered ahead of any default 
method - even inaccessible ones.

> We're relaxing a very subtle interaction, so I think compatibility
> issues with existing code is non-existing. 

I don't think there is a compatibility issue here because we don't in 
general have to maintain error compatibility. However as it is a change 
in behaviour it does warrant a CSR request just so the compatibility 
argument is captured/recorded.

> What could be an issue is
> that someone might implement this pattern and then see it no longer
> working on older versions of the OpenJDK.

Unfortunate, but not a compatibility scenario we have to support.

Cheers,
David
-----

> However, that's already a possible scenario since the sample already
> runs fine on some other non-OpenJDK JCK-compliant java implementations.
> Thus I think this compatibility observation just adds an argument _in
> favor_ of this RFE.
> 
> I've updated the test you pointed me to to include your test scenario.
> 
> Thanks!
> 
> /Claes
> 
>>
>> Otherwise I think .01 from the Hotspot side looks good.  Only one 
>> minor comment:
>> method.cpp
>> - line #426 and 427:  Since you are in this code can your change to 
>> resourceMark(THREAD)?
>>
>> Thanks,
>> Lois
>>
>>
>>
>>