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