RFR (S): 8239593: Bogus warning "Re-registering of platform native method" for a JVMTI agent

[David Holmes]

Can I get a second review please.

Thanks,
David

On 13/03/2020 5:23 pm, David Holmes wrote:
> Thanks for the Review Dan! I'll fix up the two nits.
> 
> David
> 
> On 13/03/2020 9:38 am, Daniel D. Daugherty wrote:
>> Hi David,
>>
>> I started and stopped a couple of times due to other distractions.
>> Sorry about that...
>>
>>
>> On 3/10/20 10:08 PM, David Holmes wrote:
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8239593
>>> webrev: http://cr.openjdk.java.net/~dholmes/8239593/webrev/
>>
>> src/hotspot/share/prims/jni.cpp
>>      L2818:   .... That will catch changes
>>      L2819:   // to platform classes while excluding classes added to 
>> the bootclasspath.
>>          I'm having trouble with the "excluding classes added to the
>>          bootclasspath" part. Perhaps this:
>>
>>              .... That will catch changes
>>              // to platform classes while permitting changes to 
>> classes added to the bootclasspath.
>>
>>          Maybe it's just me, but "excluding classes" sounds like those
>>          classes won't be allowed or something.
>>
>>      L2828:     if ((cl ==  NULL || 
>> SystemDictionary::is_platform_class_loader(cl)) &&
>>          nit - not yours, but there's an extra space after '=='.
>>
>>      No comments on the code itself, although I did have to read it a
>>      couple of times to convince myself the cases were right/caught.
>>
>> Thumbs up. Your call on changing the comments or fixing the nit.
>> I don't need to see another webrev.
>>
>> Thanks for mentioning the testing done.
>>
>> Dan
>>
>>
>>>
>>> The original intent was to warn if changes were being made to 
>>> platform classes by non-platform classes. That was weakened slightly 
>>> to warn if classes loaded by the boot/platform loaders were modified 
>>> by a class not loaded by the boot/platform loader. But that 
>>> formulation also catches non-platform classes that have been added to 
>>> the bootclasspath. So we augment the check to also check that the 
>>> target class is in a named module** (loaded by the boot/platform 
>>> loader) - this ensures we catch changes to platform files whilst 
>>> excluding classes incidentally on the bootclasspath.
>>>
>>> ** Note that you cannot add a named module to the boot/platform 
>>> loaders at runtime - they are defined at build time.
>>>
>>> Tested against Yourkit reproducer in the bug report, and with 
>>> existing registerNatives test.
>>>
>>> Test builds on tier1 platforms.
>>>
>>> Thanks,
>>> David
>>