RFR (S) 8210842: Handle JNIGlobalRefLocker.cpp
Alex Menkov
alexey.menkov at oracle.com
Thu Sep 20 00:13:43 UTC 2018
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/>
> 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.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/%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/>
> > > 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 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/%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>
> >
> >
> >
> > 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
More information about the serviceability-dev
mailing list