Hi Jc,
Updated tests looks good.
Some notes about implementation.
- FatalOnException implements both verification and error handling
It would be better to separate them (and this makes easy to implement
error handling different from JNIEnv::FatalError).
The simplest way is to define error handler as
class SafeJNIEnv {
public:
typedef void (*ErrorHandler)(JNIEnv *env, const char* errorMsg);
// error handler which terminates jvm by using FatalError()
static void FatalError(JNIEnv *env, const char *errrorMsg);
SafeJNIEnv(JNIEnv* jni_env, ErrorHandler errorHandler = FatalError);
(SafeJNIEnv methods should create FatalOnException objects passing
errorHandler)
- FatalOnException is used in SafeJNIEnv methods as
FatalOnException marker(this, "msg");
ret = <JNI call>
(optional)marker.check_for_null(ret);
return ret;
But actually I'd call it something like JNICallResultVerifier and create
the object after JNI call. like
ret = <JNI call>
JNICallResultVerifier(this, "msg")
(optional).notNull(ret);
return ret;
or even (if you like such syntax sugar) you can define
template<typename T>
T resultNotNull(T result) {
notNull(result);
return result;
}
and do
ret = <JNI call>
return JNICallResultVerifier(this, "msg").resultNotNull(ret);
- you added #include <memory> in the test (and you have to add it to
every test).
It would be simpler to add the include to SafeJNIEnv.hpp and define
typedef std::unique_ptr<SafeJNIEnv> SafeJNIEnvPtr;
Then each in the test methods:
SafeJNIEnvPtr env(new SafeJNIEnv(jni_env));
or you can add
static SafeJNIEnv::SafeJNIEnvPtr wrap(JNIEnv* jni_env, ErrorHandler
errorHandler = fatalError)
and get
SafeJNIEnvPtr env = SafeJNIEnv::wrap(jni_env);
- it would be better to wrap internal classes (FatalOnException) into
unnamed namespace - this helps to avoid conflicts with other classes)
- FatalOnException::check_for_null(void* ptr)
should be
FatalOnException::check_for_null(const void* ptr)
And calling the method you don't need reinterpret_cast
--alex
On 09/18/2018 11:07, JC Beyler wrote:
Hi David,
Thanks for the quick review and thoughts. I have now a new version here
that addresses your comments:
Webrev: http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.01/
<http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.01/>
Bug:https://bugs.openjdk.java.net/browse/JDK-8210842
I've also inlined my answers/comments.
> I've slowly started working on JDK-8191519
> <https://bugs.openjdk.java.net/browse/JDK-8191519>. However before
> starting to really refactor all the tests, I thought I'd get a few
> opinions. I am working on internalizing the error handling of JNI
calls
> via a SafeJNIEnv class that redefines all the JNI calls to add an
error
> checker.
>
> The advantage is that the test code will look and feel like
normal JNI
> code and calls but will have the checks we want to have for our
tests.
Not sure I get that. Normal JNI code has to check for errors/exceptions
after every call. The tests need those checks too. Today they are
explicit, with this change they become implicit. Not sure what we are
gaining here ??
In my humble opinion, having the error checking out of the way allows
the code reader to concentrate on the JNI code while maintaining error
checking. We use something similar internally, so perhaps I'm biased to
it :-).
If this is not a desired/helpful "feature" to simplify tests in general,
I will backtrack it and just add the explicit tests to the native code
of the locks for the fix
https://bugs.openjdk.java.net/browse/JDK-8191519 instead.
Let me however try to make my case and let me know what you all think!
> If agreed with this, we can augment the SafeJNIEnv class as needed.
> Also, if the tests require something else than fatal errors, we
can add
> a different marker and make it a parameter to the base class.
>
> Webrev: http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.00/
<http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.00/>
> <http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.00/>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8210842
>
> Let me know what you think,
Two initial suggestions:
1. FatalOnException should be constructed with a description string so
that it can report the failing operation when calling FatalError rather
than the general "Problem with a JNI call".
Agreed with you, the new webrev produces:
FATAL ERROR in native method: GetObjectClass
at
nsk.share.gc.lock.CriticalSectionTimedLocker.criticalSection(CriticalSectionTimedLocker.java:47)
at nsk.share.gc.lock.jniref.JNIGlobalRefLocker.criticalNative(Native
Method)
at
nsk.share.gc.lock.jniref.JNIGlobalRefLocker.criticalSection(JNIGlobalRefLocker.java:44)
at
nsk.share.gc.lock.CriticalSectionLocker$1.run(CriticalSectionLocker.java:56)
at java.lang.Thread.run(java.base@12-internal/Thread.java:834)
and for a return NULL in NewGlobalRef, we would get automatically:
FATAL ERROR in native method: NewGlobalRef:Return is NULL
at nsk.share.gc.lock.jniref.JNIGlobalRefLocker.criticalNative(Native
Method)
at
nsk.share.gc.lock.jniref.JNIGlobalRefLocker.criticalSection(JNIGlobalRefLocker.java:44)
Again as we port and simplify more tests (I'll only do the locks for now
and we can figure out the next steps as start working on moving tests
out of vmTestbase),
we can add, if needed, other exception handlers that don't throw or do
other things depending on the JNI method outputs.
2. Make the local SafeJNIEnv a pointer called env so that the change is
less disruptive. All the env->op() calls will remain and only the local
error checking will be removed.
Done, I used a unique_ptr to make the object created/destroyed/available
as a pointer do-able in one line:
std::unique_ptr<SafeJNIEnv> env(new SafeJNIEnv(jni_env));
and then you can see that, as you mentioned, the disruption to the code
is much less:
http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/share/gc/lock/jniref/JNIGlobalRefLocker.cpp.udiff.html
<http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/share/gc/lock/jniref/JNIGlobalRefLocker.cpp.udiff.html>
Basically the tests now become internal to the SafeJNIEnv and the code
now only contains the JNI calls happening but we are protected and will
fail any test that has an exception or a NULL return for the call. Of
course, this is not 100% proof in terms of not having any error handling
in the test but in some cases like this, the new code seems to just work
better:
http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/share/gc/lock/jniref/JNIGlobalRefLocker.cpp.html
<http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/share/gc/lock/jniref/JNIGlobalRefLocker.cpp.html>
The switch from, e.g., checking NULL returns to checking for pending
exceptions, need to be sure that the JNI operations clearly specify
that
NULL implies there will be an exception pending. This change may be an
issue for static code analysis if not smart enough to understand the
RAII wrappers.
Agreed, I fixed it to be more strict and say: in normal test handling,
the JNI calls should never return NULL or throw an exception. This
should hold for tests I imagine but if not we can add a different call
verifier as we go.
Thanks,
David
> Jc
Let me know what you all think. When talking about it a bit, I had
gotten some interest to see what it would look like. Here it is :-). Now
if it is not wanted like I said, I can backtrack and just put the error
checks after each JNI call for all the tests as we are porting them.
Jc
