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

[David Simms]

Hi David,

Updated webrev: http://cr.openjdk.java.net/~dsimms/8164086/webrev2/

On 26/08/16 02:27, David Holmes wrote:
> 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.

Agreed, inserted assert.

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

Doh, you are correct...I mistook this for the CHECK_JNI_EXCEPTION macro 
in "java.c" which does perform check...

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

Agreed, introduce StackObj with appropriate destructor, removed the 
checks above.


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

Thanks for seeing that, fixed.

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

Moved to "test/runtime/jni/checked"

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

Done

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

Removed all the extra checks, add JNI exception check to within the 
existing CHECK_NULL0 macro (make more sense there).

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

The method invocations needed to find main class needs to check for the 
test to pass.

Cheers
/David