RFR (S) 8210842: Handle JNIGlobalRefLocker.cpp

David Holmes david.holmes at oracle.com
Thu Sep 27 13:03:12 UTC 2018


Sorry Jc, I thought my LGTM was implicit. :)

Thanks,
David

On 26/09/2018 11:52 PM, JC Beyler 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/%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
>          >      >
>          >      > 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/%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
>          >      >      >
>          >      >      > 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>
>         <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/%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
>          >      >      >      >
>          >      >      >      > 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/>
>          >      >      >
>          >       <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>
>          >      >
>          >     
>           <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/%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


More information about the serviceability-dev mailing list