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

[Daniel D. Daugherty]

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