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

[David Holmes]

Hi David,

I still do not understand why you think you need to make any changes in 
libjli ?? Certainly I do not think you should be printing anything about 
exceptions.

Thanks,
David H.

On 26/08/2016 9:55 PM, David Simms wrote:
> 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