RFR (S) 8210842: Handle JNIGlobalRefLocker.cpp

Alex Menkov alexey.menkov at oracle.com
Thu Sep 20 00:13:43 UTC 2018


Hi Jc,

Looks good to me.
A minor note:
- I'd move ErrorHandler typedef to SafeJNIEnv class to avoid global 
namespece pollution (ErrorHandler is too generic name).

--alex

On 09/19/2018 15:56, JC Beyler wrote:
> 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.menkov at oracle.com 
> <mailto:alexey.menkov at 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 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>
> 
>      >
>     <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


More information about the serviceability-dev mailing list