RFR (S) 8210842: Handle JNIGlobalRefLocker.cpp
David Holmes
david.holmes at oracle.com
Thu Sep 27 18:19:48 UTC 2018
Hi Jc,
This seems fine to me. I'll leave it to you and Mikael to wrestle out
the naming.
Thanks,
David
On 27/09/2018 1:45 PM, JC Beyler wrote:
> Hi Mikael and David,
>
> @David: I thought it was implicit but did not want to presume on this
> one because my goal is to start propagating this new class in the test
> base and get the checks to be done implicitly so I was probably being
> over-cautious
> @Mikael: done and done, what do you think of the comment here :
> http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.04/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.hpp.html
> <http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.04/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.hpp.html>
>
> For all, the new webrev is here:
> Webrev: http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.04/
> <http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.04/>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8210842
>
> Thanks,
> Jc
>
> On Thu, Sep 27, 2018 at 6:03 AM David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>> wrote:
>
> 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>
> > <mailto: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>
> <mailto: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>>
> > > <mailto: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/>
> > > >
> > <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>>>
> > > > <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,
> > > >
> > > > 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/>
> > > > >
> > <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>>>>
> > > > > <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
> <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/>
> > > > > >
> > >
> <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/>
> > > > > > >
> > > >
> > <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>
> > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> <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>
> > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> <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
>
>
>
> --
>
> Thanks,
> Jc
More information about the serviceability-dev
mailing list