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

[David Holmes]

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/

hotspot/src/share/vm/prims/whitebox.cpp

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 
CHECK_JNI_EXCEPTION_ macro simply calls HAS_PENDING_EXCEPTION and so 
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);
1236   CHECK_JNI_EXCEPTION_(env, 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);
1262   CHECK_JNI_EXCEPTION_(env, NULL);
1263
1264   return result;

to be elided.

---

hotspot/src/share/vm/runtime/thread.hpp

!   // 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.
> CHECK_EXCEPTION_RETURN) ?

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.

Thanks,
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
>