RFR (S) 8210842: Handle JNIGlobalRefLocker.cpp

Mikael Vidstedt mikael.vidstedt at oracle.com
Thu Sep 27 04:47:20 UTC 2018


Sorry for being late to the game. Can I request a helpful comment in SafeJNIEnv.hpp describing what its purpose is?

Also, I don’t necessarily have a better suggestion (and don’t consider this blocking), but Is there another word than “Safe” to describe this wrapper? “Checked”? ExceptionChecking? Just something to consider.

Cheers,
Mikael

> On Sep 26, 2018, at 8:52 PM, JC Beyler <jcbeyler at google.com> wrote:
> 
> Ping on this, anybody have time to do a review and give a LGTM? Or David if you have time and will to provide an explicit LGTM :)
> 
> Thanks,
> Jc
> 
> On Mon, Sep 24, 2018 at 9:18 AM JC Beyler <jcbeyler at google.com <mailto:jcbeyler at google.com>> wrote:
> Hi David,
> 
> Sounds good to me, Alex gave me one LGTM, so it seems I'm waiting for an explicit LGTM from you or from someone else on this list to do a review.
> 
> Thanks again for your help,
> Jc
> 
> On Sun, Sep 23, 2018 at 9:22 AM David Holmes <david.holmes at oracle.com <mailto:david.holmes at oracle.com>> wrote:
> Hi Jc,
> 
> I don't think it is an issue for this to go ahead. If the static 
> analysis tool has an issue then we can either find out how to teach it 
> about this code structure, or else flag the issues as false positives. 
> I'd be relying on one of the serviceability team here to handle that if 
> the problem arises.
> 
> Thanks,
> David
> 
> On 23/09/2018 12:17 PM, JC Beyler wrote:
> > Hi David,
> > 
> > No worries with the time to answer; I appreciate the review!
> > 
> > That's a fair point. Is it possible to "describe" to the static analysis 
> > tool to "trust" calls made in the SafeJNIEnv class?
> > 
> > Otherwise, do you have any idea on how to go forward? For what it's 
> > worth, this current webrev does not try to replace exception checking 
> > with the SafeJNIEnv (it actually adds it), so for now the 
> > question/solution could be delayed. I could continue working on this in 
> > the scope of both the nsk/gc/lock/jniref code base and the NSK_VERIFIER 
> > macro removal and we can look at how to tackle the cases where the tests 
> > are actually calling exception checking (I know my heapmonitor does for 
> > example).
> > 
> > Let me know what you think and thanks for the review,
> > Jc
> > 
> > 
> > On Sun, Sep 23, 2018 at 6:48 AM David Holmes <david.holmes at oracle.com <mailto:david.holmes at oracle.com> 
> > <mailto:david.holmes at oracle.com <mailto:david.holmes at oracle.com>>> wrote:
> > 
> >     Hi Jc,
> > 
> >     Sorry for the delay on getting back to this but I'm travelling at the
> >     moment.
> > 
> >     This looks quite neat. Thanks also to Alex for all the suggestions.
> > 
> >     My only remaining concern is that static analysis tools may not like
> >     this because they may not be able to determine that we won't make
> >     subsequent JNI calls when an exception happens in the first. That's not
> >     a reason not to do this of course, just flagging that we may have to do
> >     something to deal with that problem.
> > 
> >     Thanks,
> >     David
> > 
> >     On 20/09/2018 11:56 AM, JC Beyler wrote:
> >      > Hi Alex,
> >      >
> >      > Done here, thanks for the review:
> >      >
> >      > Webrev: http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.03/ <http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.03/>
> >     <http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.03/ <http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.03/>>
> >      > <http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.03/ <http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.03/>>
> >      > Bug: https://bugs.openjdk.java.net/browse/JDK-8210842 <https://bugs.openjdk.java.net/browse/JDK-8210842>
> >      >
> >      > Thanks again!
> >      > Jc
> >      >
> >      >
> >      > On Wed, Sep 19, 2018 at 5:13 PM Alex Menkov
> >     <alexey.menkov at oracle.com <mailto:alexey.menkov at oracle.com> <mailto:alexey.menkov at oracle.com <mailto:alexey.menkov at oracle.com>>
> >      > <mailto:alexey.menkov at oracle.com <mailto:alexey.menkov at oracle.com>
> >     <mailto:alexey.menkov at oracle.com <mailto:alexey.menkov at oracle.com>>>> wrote:
> >      >
> >      >     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/~jcbeyler/8210842/webrev.02/>
> >     <http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.02/ <http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.02/>>
> >      >     <http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.02/ <http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.02/>>
> >      >      > <http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.02/ <http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.02/>>
> >      >      > Bug: https://bugs.openjdk.java.net/browse/JDK-8210842 <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 <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> <mailto:alexey.menkov at oracle.com <mailto:alexey.menkov at oracle.com>>
> >     <mailto:alexey.menkov at oracle.com <mailto:alexey.menkov at oracle.com> <mailto:alexey.menkov at oracle.com <mailto:alexey.menkov at oracle.com>>>
> >      >      > <mailto:alexey.menkov at oracle.com <mailto:alexey.menkov at oracle.com>
> >     <mailto:alexey.menkov at oracle.com <mailto:alexey.menkov at oracle.com>>
> >      >     <mailto:alexey.menkov at oracle.com <mailto:alexey.menkov at oracle.com>
> >     <mailto: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/~jcbeyler/8210842/webrev.01/>
> >     <http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.01/ <http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.01/>>
> >      >     <http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.01/ <http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.01/>>
> >      >      >   
> >       <http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.01/ <http://cr.openjdk.java.net/%7Ejcbeyler/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 <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 <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 <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/~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/ <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/>>
> >      >      >      >
> >      >       <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/ <http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.00/>>
> >      >      >      >      > Bug:
> >     https://bugs.openjdk.java.net/browse/JDK-8210842 <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/~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>>
> >      >   
> >       <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>>
> >      >      >
> >      >     
> >       <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>>
> >      >      >
> >      >      >      >
> >      >      >
> >      >     
> >       <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/~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>>
> >      >   
> >       <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>>
> >      >      >
> >      >     
> >       <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>>
> >      >      >
> >      >      >      >
> >      >      >
> >      >     
> >       <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
> >      >
> >      >
> >      >
> >      > --
> >      >
> >      > Thanks,
> >      > Jc
> > 
> > 
> > 
> > -- 
> > 
> > Thanks,
> > Jc
> 
> 
> -- 
> 
> Thanks,
> Jc
> 
> 
> -- 
> 
> Thanks,
> Jc

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180926/e633919d/attachment-0001.html>


More information about the serviceability-dev mailing list