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.
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. ??
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.
---
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".
---
For the tests we no longer use bug numbers as part of the test names.
Looks like some recent tests slipped by unfortunately. :(
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.
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.
I had assumed this was just some debugging code you had left in by mistake.
Thanks,
David H.
-------
In-line...
On 23/08/16 14:16, David Holmes wrote:
Hi David
On 23/08/2016 8:24 PM, David Simms wrote:
Reply in-line...
On 19/08/16 14:29, David Holmes wrote:
Hi David,
The changes in the native wrapper seem okay though I'm not an expert
on the machine specific encodings.
I'm a little surprised there are not more things that need changing
though. Does the JIT use those wrappers too?
Yeah they do, I double checked Nils from compiler group. I also tested
with -Xcomp, test failed without sharedRuntime fix. The test execution
time was over 10 seconds, so I removed it from the jtreg test itself
(hard-coded ProcessTools.executeTestJVM()) since it is part of
"hotspot_fast_runtime".
Can we transition from Java to VM to native and then back - and if so
might we need to clear the pending exception check? (I'm not sure if
from in the VM a native call could actually be a JNI call, or will
only be a direct native call?).
At first I thought JavaCallWrapper needs it, following all the places we
manipulate the thread's active handle block (besides manual push/pop).
But then call helper just ends up calling the native wrapper, which
takes care of it. Not a direct native call. So I left it, as-is.
That's not the case I was thinking of. We have ThreadToNativeFromVM
and then we do native stuff - if any of that were JNI-based (perhaps
it is not) then we would enable the check but not disable it again
when returning from VM to Java.
Got you now: Java->VM->Native i.e. VM code using JNI may miss an
exception check. So I check the call hierarchy from
"ThreadToNativeFromVM" and found whitebox.cpp had a few spots where
checks were missing, added them in now.
There's an extra comment stating ThreadToNativeFromVM is expected to be
"well behaved" (i.e. check for exceptions), which it is with the
whitebox.cpp fixes, so we don't require any extra code or overhead in
VM->Java transitions. As far as maintaining "well behaved" JNI code, we
do static code checking with "Parfait" as part of testing, and there are
a few other related bugs that already exist to address these issues.
Did you intend to leave in the changes to
jdk/src/java.base/share/native/libjli/java.c? It looks like debug/test
code to me.
The launcher produces warnings (Java method invokes) that break the
jtreg test, so yeah, thought it was best to check and print them. Some
of the existing code checks and silently returns, I followed the same
pattern where that pattern was in place.
This needs to be looked at closer then and reviewed by the launcher
folk (ie Kumar).
CC:ed core-libs & Kumar. Thanks for pointing that out.
The test I'm finding a bit hard to follow but don't you need to check
for pending exceptions here:
29 static jmethodID get_method_id(JNIEnv *env, jclass clz, jstring
jname, jstring jsig) {
30 jmethodID mid;
31 const char *name, *sig;
32 name = (*env)->GetStringUTFChars(env, jname, NULL);
33 sig = (*env)->GetStringUTFChars(env, jsig, NULL);
34 mid = (*env)->GetMethodID(env, clz, name, sig);
to avoid triggering the warning?
Those methods don't require an explicit check since there return values
denote an error condition.
Whilst Java invoke return values are user defined, so they do need
it
https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/design.html#asynchronous_exceptions).
Technically array stores need to check for AIOOBE, but given most
code handles index/bounds checks, it seemed way too pedantic
(commented in jniCheck.cpp:176).
Not following. GetStringUTFChars can post OOME so we would enable the
check-flag if that happens on the first call above, then the second
call would be made with the exception pending and trigger the warning.
So as we mentioned off-list, yes this test code should also follow spec,
updated.
Thanks for looking at this, David
/David Simms
