Hi Alex,
I've updated the webrev to:
Webrev: http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.02/
<http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.02/>
Bug: https://bugs.openjdk.java.net/browse/JDK-8210842
That webrev has the code that is shown here in snippets.
Thanks for the reviews, I've relatively followed your reviews except for
one detail due to me wanting to handle the NSK_JNI_VERIFY macros via
this system as well later down the road. For an example:
We currently have in the code:
if ( ! NSK_JNI_VERIFY(pEnv, (mhClass = NSK_CPP_STUB2(GetObjectClass,
pEnv, mhToCall)) != NULL) )
1) The NSK_CPP_STUB2 is trivially removed with a refactor (JDK-8210728)
<https://bugs.openjdk.java.net/browse/JDK-8210728> to:
if ( ! NSK_JNI_VERIFY(pEnv, (mhClass = pEnv->GetObjectClass(pEnv,
mhToCall)) != NULL) )
2) Then the NSK_JNI_VERIFY, I'd like to remove it to and it becomes via
this wrapping of JNIEnv:
if ( ! (mhClass = pEnv->GetObjectClass(pEnv, mhToCall)) )
3) Then, via removing the assignment, we'd arrive to a:
mhClass = pEnv->GetObjectClass(pEnv, mhToCall));
if (!mhClass)
Without any loss of checking for failures, etc.
So that is my motivation for most of this work with a concrete example
(hopefully it helps drive this conversation).
I inlined my answers here, let me know what you think.
On Wed, Sep 19, 2018 at 2:30 PM Alex Menkov <alexey.men...@oracle.com
<mailto:alexey.men...@oracle.com>> wrote:
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)
Agreed, I tried to keep the code simple. The concepts you talk about
here are generally what I reserve for when I need it (ie extensions and
handling new cases). But a lot are going to be needed soon so I think it
is a good time to iron the code out now on this "simple" webrev.
So done for this.
- 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);
So I renamed FatalOnException to now being JNIVerifier.
Though I like it, I don't think we can do it, except if we do it
slightly differently:
I'm trying to solve two problems with one stone:
- How to check for returns of JNI calls in the way that is done here
- How to remove NSK_JNI_VERIFY* (from nsk/share/jni/jni_tools)
However, the NSK_JNI_VERIFY need to start a tracing system before the
call to JNI, so it won't work this way. (Side question would be do we
still care about the tracing in NSK_JNI_VERIFY, if we don't then your
solution works well in most situations).
My vision or intuition is that we would throw a template at some point
on SafeJNIEnv to handle both cases and have JNIVerifier become a
specialization in certain cases and something different for the
NSK_JNI_VERIFY case (or have a different constructor to enable tracing).
But for now, I offer the implementation that does:
jclass SafeJNIEnv::GetObjectClass(jobject obj) {
JNIVerifier<jclass> marker(this, "GetObjectClass");
return marker.ResultNotNull(_jni_env->GetObjectClass(obj));
}
and:
void SafeJNIEnv::SetObjectField(jobject obj, jfieldID field, jobject
value) {
JNIVerifier<> marker(this, "SetObjectField");
_jni_env->SetObjectField(obj, field, value);
}
- 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);
Done, I like that, even less code change to tests then.
- 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
Also done, it got renamed to ResultNotNull, is using a template now, but
agreed, that worked.
Thanks again for the review,
Jc
--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/>
> <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/>
> > <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>
>
<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>
>
<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
--
Thanks,
Jc
