RFR: 8232613: Move Object.registerNatives into HotSpot

[Claes Redestad]

Hi Lois,

to make sure noone misses it, here's the new webrev:
http://cr.openjdk.java.net/~redestad/8232613/open.03

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