RFR (S): JDK-8164086: Checked JNI pending exception check should be cleared when returning to Java frame

[David Holmes]

Hi David

On 23/08/2016 8:24 PM, David Simms wrote:
>
> Reply in-line...
>
> On 19/08/16 14:29, David Holmes wrote:
>> Hi David,
>>
>>
>> The changes in the native wrapper seem okay though I'm not an expert
>> on the machine specific encodings.
>>
>> I'm a little surprised there are not more things that need changing
>> though. Does the JIT use those wrappers too?
>
> Yeah they do, I double checked Nils from compiler group. I also tested
> with -Xcomp, test failed without sharedRuntime fix. The test execution
> time was over 10 seconds, so I removed it from the jtreg test itself
> (hard-coded ProcessTools.executeTestJVM()) since it is part of
> "hotspot_fast_runtime".
>
>> Can we transition from Java to VM to native and then back - and if so
>> might we need to clear the pending exception check? (I'm not sure if
>> from in the VM a native call could actually be a JNI call, or will
>> only be a direct native call?).
>
> At first I thought JavaCallWrapper needs it, following all the places we
> manipulate the thread's active handle block (besides manual push/pop).
> But then call helper just ends up calling the native wrapper, which
> takes care of it. Not a direct native call. So I left it, as-is.

That's not the case I was thinking of. We have ThreadToNativeFromVM and 
then we do native stuff - if any of that were JNI-based (perhaps it is 
not) then we would enable the check but not disable it again when 
returning from VM to Java.

>>
>> Did you intend to leave in the changes to
>> jdk/src/java.base/share/native/libjli/java.c? It looks like debug/test
>> code to me.
>
> The launcher produces warnings (Java method invokes) that break the
> jtreg test, so yeah, thought it was best to check and print them. Some
> of the existing code checks and silently returns, I followed the same
> pattern where that pattern was in place.

This needs to be looked at closer then and reviewed by the launcher folk 
(ie Kumar).

>>
>> The test I'm finding a bit hard to follow but don't you need to check
>> for pending exceptions here:
>>
>>   29 static jmethodID get_method_id(JNIEnv *env, jclass clz, jstring
>> jname, jstring jsig) {
>>   30   jmethodID mid;
>>   31   const char *name, *sig;
>>   32   name = (*env)->GetStringUTFChars(env, jname, NULL);
>>   33   sig  = (*env)->GetStringUTFChars(env, jsig, NULL);
>>   34   mid  = (*env)->GetMethodID(env, clz, name, sig);
>>
>> to avoid triggering the warning?
>>
> Those methods don't require an explicit check since there return values
> denote an error condition.
>
>     Whilst Java invoke return values are user defined, so they do need
>     it
>     https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/design.html#asynchronous_exceptions).
>
>     Technically array stores need to check for AIOOBE, but given most
>     code handles index/bounds checks, it seemed way too pedantic
>     (commented in jniCheck.cpp:176).

Not following. GetStringUTFChars can post OOME so we would enable the 
check-flag if that happens on the first call above, then the second call 
would be made with the exception pending and trigger the warning.

Thanks,
David H.

> Cheers
> /David Simms