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

[David Holmes]

Update looks fine - though I see you already pushed it.

David

On 22/05/2020 7:32 pm, 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
>>>>>
>>>>>
>>>>>
>>>>
>>
>