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

[David Holmes]

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
>