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

[David Simms]

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.


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


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