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>> 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/>
> 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>>> 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/>
> > 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>>>> 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/>
> > > 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/>
> > > > 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>
> > >
> > > 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>
> > >
> > >
> > >
> > > 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
