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

[David Holmes]

Hi Harold,

On 17/03/2020 10:02 pm, Harold Seigel wrote:
> Looks good.

Thanks for the review.

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

Yes - good point - no need to do that unnecessarily!

Will update and push.

Thanks,
David

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