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

David Holmes david.holmes at oracle.com
Fri Aug 26 00:27:31 UTC 2016

Hi David,

I'm missing some pieces of this puzzle I'm afraid.

On 25/08/2016 8:05 PM, David Simms wrote:
> Updated the webrev here:
> http://cr.openjdk.java.net/~dsimms/8164086/webrev1/


First I'm not sure that Whitebox isn't a special case here that could be 
handled in the WB_START/END macros - see below.

More generally you state below that the transition from native back to 
the VM doesn't have to do anything with the pending_exception_check flag 
because well behaved native code in that context will explicitly check 
for exceptions, and so the pending-exception-check will already be 
disabled before returning to Java. First, if that is the case then we 
should assert that it is so in the native->VM return transition.

Second though, it doesn't seem to be the case in Whitebox because the 
won't touch the pending-exception-check flag. ??

It was a good pick up that some whitebox code was using values that 
might be NULL because an exception had occurred. There are a couple of 
changes that are unnecessary though:

1235   result = env->NewObjectArray(5, clazz, NULL);
1237   if (result == NULL) {
1238     return result;
1239   }

(and similarly at 1322)

result will be NULL iff there is a pending exception; and vice-versa. So 
the existing check for NULL suffices for correctness. If you want to 
check exceptions for the side-effect of clearing the 
pending-exception-check flag then lines 1237-1239 can be deleted. 
However I would suggest that if you truly do want to clear the 
pending-exception-check flag then the place to do it is the WB_END 
macro. That allows allows exception checks at the end of methods, eg:

1261   env->SetObjectArrayElement(result, 4, entry_point);
1264   return result;

to be elided.



!   // which function name. Returning to a Java frame should implicitly 
clear the
!   // need for, this is done for Native->Java transitions.

Seems to be some text missing after "need for".


For the tests we no longer use bug numbers as part of the test names. 
Looks like some recent tests slipped by unfortunately. :(

You should be able to get rid of the:

* @modules java.base/jdk.internal.misc

with Christian's just pushed changes to ProcessTools to isolate the 
Unsafe dependency.

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

I'm not seeing the point of this logic. Any exceptions that remain 
pending when the main thread detaches from the VM will be reported by 
the uncaught-exception handling logic. The checks you put in are in most 
cases immediately before a return so there is no need to check for a 
pending exception and do an earlier return. And in one case you would 
bypass tracing logic by doing an early return.

I had assumed this was just some debugging code you had left in by mistake.

David H.

> 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

More information about the hotspot-runtime-dev mailing list