Updated the webrev here: http://cr.openjdk.java.net/~dsimms/8164086/webrev1/
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) ?
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
