RFR: 8232613: Move Object.registerNatives into HotSpot

[coleen.phillimore at oracle.com]

On 10/22/19 9:41 AM, 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

This is looking better to me.  One comment (that I don't need to see).

http://cr.openjdk.java.net/~redestad/8232613/open.03/src/hotspot/share/classfile/javaClasses.cpp.udiff.html

In the first 4 calls, the last argument should be CHECK (or CATCH), so 
it doesn't continue after an exception.

Well done Lois for finding the interaction with default methods.  It 
seems like it should be fine, but than you for filing the CSR.

The code change looks fine to me. I don't need to see any more.
Coleen

>
> 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,
>>
>> 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.
> :-)
>
> We're relaxing a very subtle interaction, so I think compatibility
> issues with existing code is non-existing. 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.
>
> 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
>>
>>
>>
>>