On Sep 27, 2018, at 10:45 AM, JC Beyler <[email protected]
<mailto:[email protected]>> 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
<[email protected] <mailto:[email protected]>>
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
<[email protected] <mailto:[email protected]>
> <mailto:[email protected]
<mailto:[email protected]>>> 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
> <[email protected]
<mailto:[email protected]>
<mailto:[email protected]
<mailto:[email protected]>>> 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
> <[email protected]
<mailto:[email protected]>
<mailto:[email protected]
<mailto:[email protected]>>
> > <mailto:[email protected]
<mailto:[email protected]>
> <mailto:[email protected]
<mailto:[email protected]>>>> 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
> > <[email protected]
<mailto:[email protected]>
> <mailto:[email protected]
<mailto:[email protected]>>
> <mailto:[email protected]
<mailto:[email protected]>
<mailto:[email protected]
<mailto:[email protected]>>>
> > > <mailto:[email protected]
<mailto:[email protected]>
> <mailto:[email protected]
<mailto:[email protected]>>
> > <mailto:[email protected]
<mailto:[email protected]>
> <mailto:[email protected]
<mailto:[email protected]>>>>> 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
> > > <[email protected]
<mailto:[email protected]>
> <mailto:[email protected]
<mailto:[email protected]>>
> <mailto:[email protected]
<mailto:[email protected]>
<mailto:[email protected]
<mailto:[email protected]>>>
> > <mailto:[email protected]
<mailto:[email protected]>
> <mailto:[email protected]
<mailto:[email protected]>>
> <mailto:[email protected]
<mailto:[email protected]>
<mailto:[email protected]
<mailto:[email protected]>>>>
> > > >
<mailto:[email protected]
<mailto:[email protected]>
> <mailto:[email protected]
<mailto:[email protected]>>
> > <mailto:[email protected]
<mailto:[email protected]>
> <mailto:[email protected]
<mailto:[email protected]>>>
> > > <mailto:[email protected]
<mailto:[email protected]>
> <mailto:[email protected]
<mailto:[email protected]>>
> > <mailto:[email protected]
<mailto:[email protected]>
> <mailto:[email protected]
<mailto:[email protected]>>>>>> 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
<mailto: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
> > > >
> > > >
> > > >
> > > > --
> > > >
> >
