PING: Re: RFR (XS): 8244571: assert(!_thread->is_pending_jni_exception_check()) failed: Pending JNI Exception Check during class loading

[serguei.spitsyn at oracle.com]

Thank you for the review, Chris!
Serguei


On 5/22/20 11:57, Chris Plummer wrote:
> Hi Serguei,
>
> Looks good, and I agree with David's comments. I was thinking the same 
> thing when I first looked at your original changes.
>
> thanks,
>
> Chris
>
> On 5/22/20 2:32 AM, serguei.spitsyn at oracle.com wrote:
>> Hi David,
>>
>> The updated webrev is with your comments addressed:
>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/8244571-jvmti-test-jnicheck.2/ 
>>
>>
>> Thanks,
>> Serguei
>>
>>
>> On 5/22/20 00:43, serguei.spitsyn at oracle.com wrote:
>>> Hi David,
>>>
>>> Thank you for the comments!
>>>
>>>
>>> On 5/21/20 23:58, David Holmes wrote:
>>>> Hi Serguei,
>>>>
>>>> On 22/05/2020 4:17 pm, serguei.spitsyn at oracle.com wrote:
>>>>> PING: This is pretty small and easy to review fix.
>>>>>
>>>>> Thanks!
>>>>> Serguei
>>>>>
>>>>>
>>>>> On 5/19/20 09:28, serguei.spitsyn at oracle.com wrote:
>>>>>> Please, review fix for:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8244571
>>>>>>
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/8244571-jvmti-test-jnicheck.1/ 
>>>>>>
>>>>>>
>>>>>> Summary:
>>>>>>   There are two places in the native part of test that cause 
>>>>>> assert and WARNING with the -Xcheck:jni.
>>>>>>   The assert is because there is no check for pending exception 
>>>>>> after the call to:
>>>>>> jni->CallBooleanMethod(klass, is_hid_mid);
>>>>>>   Using a JNI ExceptionCheck()after the call fixes the issue.
>>>>
>>>>   bool res = jni->CallBooleanMethod(klass, is_hid_mid);
>>>>   if (jni->ExceptionCheck()) {
>>>>     LOG0("is_hidden: Exception in jni CallBooleanMethod\n");
>>>>   }
>>>>   return res;
>>>>
>>>> That will fix the pending_jni_exception_check error, but if an 
>>>> exception actually occurs what will be returned? And whatever is 
>>>> returned, the callers of this method don't themselves check for 
>>>> pending exceptions so they will treat it as if the exception didn't 
>>>> occur - at least until we finally return to Java code. Perhaps any 
>>>> exception should result in jni->FatalError as happens with any 
>>>> JVMTI error?
>>> You are right, it would be more clean to call jni->FatalError.
>>> I was also thinking about it but also worried to get the exception 
>>> details.
>>> The exception can be printed before call to FatalError.
>>>
>>>
>>>>>>   The following call to the JVM TI function:
>>>>>> err = jvmti->GetClassLoaderClasses(loader, &count, &loader_classes);
>>>>>>   produces the warning (with a java level stack trace): WARNING: 
>>>>>> JNI local refs: 94, exceeds capacity: 32
>>>>>>   It is because the GetClassLoaderClasses returns an array of 
>>>>>> local references to the loader classes.
>>>>>>   Using a JNI EnsureLocalCapacity() before the JVM TI call also 
>>>>>> fixes the issue.
>>>>
>>>> The warning suggests using 1024 is a bit of overkill. :)
>>>
>>> What capacity would be more reasonable, 256 or 512?
>>> Let's pick 256. This is just a warning, the test is still passing.
>>>
>>> Thanks!
>>> Serguei
>>>
>>>> Cheers,
>>>> David
>>>>
>>>>>>
>>>>>> Testing:
>>>>>>   Running the test 
>>>>>> test/hotspot/jtreg/serviceability/jvmti/HiddenClass locally.
>>>>>>   Will run a mach5 job as well.
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>
>>
>
>