Re: 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