RFR (S): JDK-8164086: Checked JNI pending exception check should be cleared when returning to Java frame
David Simms
david.simms at oracle.com
Fri Aug 26 11:55:48 UTC 2016
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
More information about the core-libs-dev
mailing list