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