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

[David Simms]

Updated the webrev here: http://cr.openjdk.java.net/~dsimms/8164086/webrev1/

core-libs & Kumar: java launcher: are you okay with the 
CHECK_EXCEPTION_PRINT macro, or would you rather it was silent (i.e. 
CHECK_EXCEPTION_RETURN) ?

In-line...


On 23/08/16 14:16, David Holmes wrote:
> 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.
>


Got you now: Java->VM->Native i.e. VM code using JNI may miss an 
exception check. So I check the call hierarchy from 
"ThreadToNativeFromVM" and found whitebox.cpp had a few spots where 
checks were missing, added them in now.

There's an extra comment stating ThreadToNativeFromVM is expected to be 
"well behaved" (i.e. check for exceptions), which it is with the 
whitebox.cpp fixes, so we don't require any extra code or overhead in 
VM->Java transitions. As far as maintaining "well behaved" JNI code, we 
do static code checking with "Parfait" as part of testing, and there are 
a few other related bugs that already exist to address these issues.

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

CC:ed core-libs & Kumar. Thanks for pointing that out.

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

So as we mentioned off-list, yes this test code should also follow spec, 
updated.

Thanks for looking at this, David
/David Simms