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

[Harold Seigel]

Looks good.

One nit is that the assignment to 'caller' could be moved inside the 
'if' statement starting at line 2828.

Thanks, Harold

On 3/16/2020 7:12 PM, David Holmes wrote:
> 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
>>>