RFR (S) 8210842: Handle JNIGlobalRefLocker.cpp
Alex Menkov
alexey.menkov at oracle.com
Wed Sep 19 21:30:40 UTC 2018
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 at 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
More information about the serviceability-dev
mailing list