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.hol...@oracle.com
<mailto:david.hol...@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 <jcbey...@google.com
<mailto:jcbey...@google.com>
> <mailto:jcbey...@google.com <mailto:jcbey...@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.hol...@oracle.com <mailto:david.hol...@oracle.com>
<mailto:david.hol...@oracle.com <mailto:david.hol...@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.hol...@oracle.com <mailto:david.hol...@oracle.com>
<mailto:david.hol...@oracle.com <mailto:david.hol...@oracle.com>>
> > <mailto:david.hol...@oracle.com
<mailto:david.hol...@oracle.com>
> <mailto:david.hol...@oracle.com
<mailto:david.hol...@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.men...@oracle.com
<mailto:alexey.men...@oracle.com>
> <mailto:alexey.men...@oracle.com
<mailto:alexey.men...@oracle.com>>
> <mailto:alexey.men...@oracle.com
<mailto:alexey.men...@oracle.com> <mailto:alexey.men...@oracle.com
<mailto:alexey.men...@oracle.com>>>
> > > <mailto:alexey.men...@oracle.com
<mailto:alexey.men...@oracle.com>
> <mailto:alexey.men...@oracle.com
<mailto:alexey.men...@oracle.com>>
> > <mailto:alexey.men...@oracle.com
<mailto:alexey.men...@oracle.com>
> <mailto:alexey.men...@oracle.com
<mailto:alexey.men...@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.men...@oracle.com
<mailto:alexey.men...@oracle.com>
> <mailto:alexey.men...@oracle.com
<mailto:alexey.men...@oracle.com>>
> <mailto:alexey.men...@oracle.com
<mailto:alexey.men...@oracle.com> <mailto:alexey.men...@oracle.com
<mailto:alexey.men...@oracle.com>>>
> > <mailto:alexey.men...@oracle.com
<mailto:alexey.men...@oracle.com>
> <mailto:alexey.men...@oracle.com
<mailto:alexey.men...@oracle.com>>
> <mailto:alexey.men...@oracle.com
<mailto:alexey.men...@oracle.com> <mailto:alexey.men...@oracle.com
<mailto:alexey.men...@oracle.com>>>>
> > > > <mailto:alexey.men...@oracle.com
<mailto:alexey.men...@oracle.com>
> <mailto:alexey.men...@oracle.com
<mailto:alexey.men...@oracle.com>>
> > <mailto:alexey.men...@oracle.com
<mailto:alexey.men...@oracle.com>
> <mailto:alexey.men...@oracle.com
<mailto:alexey.men...@oracle.com>>>
> > > <mailto:alexey.men...@oracle.com
<mailto:alexey.men...@oracle.com>
> <mailto:alexey.men...@oracle.com
<mailto:alexey.men...@oracle.com>>
> > <mailto:alexey.men...@oracle.com
<mailto:alexey.men...@oracle.com>
> <mailto:alexey.men...@oracle.com
<mailto:alexey.men...@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@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
