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

[David Simms]

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.

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

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

Cheers
/David Simms