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

[David Holmes]

Hi David,

On 5/09/2016 6:02 PM, David Simms wrote:
> Hi David,
>
> I can make the checks silent, but the launcher itself produces warnings
> from checked JNI (it's use of unchecked Java method invocations), and
> this causes the test
> (http://cr.openjdk.java.net/~dsimms/8164086/webrev2/hotspot/test/runtime/jni/checked/TestCheckedJniExceptionCheck.java.html)
> to fail, with unchecked exception warnings popping up in the output.

I've looked back at your original changes in this area but I'm still not 
seeing exactly where the unchecked back-to-back JNI calls are happening. 
For example NewPlatformString can return with an exception pending, but 
will return NULL, and the calling code has checks for NULL.

> But yeah, I could adjust the test the ignore any start-up warnings and
> drop the changes to the launcher...

Unless we can more clearly identify exactly where the launcher has a 
problem I would prefer not to involve it in these changes. But I would 
like to understand exactly why the launcher is triggering the check.

Thanks,
David H.

>
> Thanks
>
> /David Simms
>
>
> On 29/08/2016 9:24 a.m., David Holmes wrote:
>> 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
>