RFR (S) 8210842: Handle JNIGlobalRefLocker.cpp

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Fri Sep 28 11:25:39 UTC 2018


Hi Jc,

I agree it can be refactored later so I'm Okay with the current webrev.

Thanks,
Serguei

On 9/27/18 8:57 PM, JC Beyler wrote:
> Hi Serguei,
>
> Exactly, I'm taking the lazy approach and just doing the ones I need. 
> Ideally I will find a better means to wrap around the methods without 
> having to redefine all of them but I've looked around and nothing 
> seems really perfect even with heavy utilization of C++ templates. 
> Perhaps I can use some macro definitions to make the code easier to be 
> generated but I did not want to go in either direction now, instead 
> preferring to keep it simple and direct.
>
> If you agree, as we add more methods we can always refactor this at 
> some point if someone (or us) finds a better solution to this but that 
> is an internal problem to the exception checking class and won't 
> affect the tests.
>
> Does that sound reasonable?
>
> Let me know,
> Jc
>
> On Thu, Sep 27, 2018 at 8:00 PM <serguei.spitsyn at oracle.com 
> <mailto:serguei.spitsyn at oracle.com>> wrote:
>
>     Hi Jc,
>
>     Sorry for being late to the party.
>     The webrev5 looks good to me.
>     I don't think you have to try to fix the build system.
>     Avoiding using unique_ptr is good enough.
>
>     Do I understand it right that the ExceptionCheckingJniEnv class
>     is going to enhanced with more JNI functions?
>     I'm wonder if it can be anyhow generalized to avoid this.
>
>     Thanks,
>     Serguei
>     **
>
>     On 9/27/18 2:33 PM, JC Beyler wrote:
>>     Hi all,
>>
>>     Sorry to come back to this so late in the game, but somehow when
>>     I synced my hg clone (or the issue was always there and it is a
>>     flaky build), it seems that something in the build might have
>>     changed? Basically now it seems that the build is adding flags
>>     that makes my usage of unique_ptr fail.
>>
>>     I "think" it is due to the build adding the gnu++98 standard (But
>>     this has been there for a while it seems so most likely a
>>     side-effect is it is being now used):
>>
>>         CXXSTD_CXXFLAG="-std=gnu++98"
>>     FLAGS_CXX_COMPILER_CHECK_ARGUMENTS(ARGUMENT: [$CXXSTD_CXXFLAG
>>     -Werror],
>>     IF_FALSE: [CXXSTD_CXXFLAG=""])
>>
>>     (from:
>>     https://hg.openjdk.java.net/jdk/jdk/file/dade6dd87bb4/make/autoconf/flags-cflags.m4)
>>     but I'm not sure.
>>
>>     When I remove that flag, my g++ goes to a more recent standard
>>     and unique_ptr works.
>>
>>     So I now have to ask you all:
>>       1) Should we try to fix the build system to at least have C++11
>>     for the C++ tests, then my webrev.04 can stay as is but has to
>>     wait for that to happen
>>       2) Should we push a new version that does not use unique_ptr?
>>     That solution would look like the following webrev:
>>     http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.05/
>>     <http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.05/>
>>
>>     Sorry for the last minute rug pull,
>>     Jc
>>
>>     On Thu, Sep 27, 2018 at 11:32 AM Mikael Vidstedt
>>     <mikael.vidstedt at oracle.com <mailto:mikael.vidstedt at oracle.com>>
>>     wrote:
>>
>>
>>         Very, very nice! Thanks for adding the comment and renaming
>>         the class! Ship it!
>>
>>         Cheers,
>>         Mikael
>>
>>
>>>         On Sep 27, 2018, at 10:45 AM, JC Beyler <jcbeyler at google.com
>>>         <mailto:jcbeyler at google.com>> 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
>>>         <david.holmes at oracle.com <mailto:david.holmes at 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
>>>             <jcbeyler at google.com <mailto:jcbeyler at google.com>
>>>             > <mailto:jcbeyler at google.com
>>>             <mailto:jcbeyler at 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.holmes at oracle.com
>>>             <mailto:david.holmes at oracle.com>
>>>             <mailto:david.holmes at oracle.com
>>>             <mailto:david.holmes at 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.holmes at oracle.com
>>>             <mailto:david.holmes at oracle.com>
>>>             <mailto:david.holmes at oracle.com
>>>             <mailto:david.holmes at oracle.com>>
>>>             >          > <mailto:david.holmes at oracle.com
>>>             <mailto:david.holmes at oracle.com>
>>>             >         <mailto:david.holmes at oracle.com
>>>             <mailto:david.holmes at 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.menkov at oracle.com
>>>             <mailto:alexey.menkov at oracle.com>
>>>             >         <mailto:alexey.menkov at oracle.com
>>>             <mailto:alexey.menkov at oracle.com>>
>>>             >         <mailto:alexey.menkov at oracle.com
>>>             <mailto:alexey.menkov at oracle.com>
>>>             <mailto:alexey.menkov at oracle.com
>>>             <mailto:alexey.menkov at oracle.com>>>
>>>             >          >      > <mailto:alexey.menkov at oracle.com
>>>             <mailto:alexey.menkov at oracle.com>
>>>             >         <mailto:alexey.menkov at oracle.com
>>>             <mailto:alexey.menkov at oracle.com>>
>>>             >          >     <mailto:alexey.menkov at oracle.com
>>>             <mailto:alexey.menkov at oracle.com>
>>>             >         <mailto:alexey.menkov at oracle.com
>>>             <mailto:alexey.menkov at 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.menkov at oracle.com
>>>             <mailto:alexey.menkov at oracle.com>
>>>             >         <mailto:alexey.menkov at oracle.com
>>>             <mailto:alexey.menkov at oracle.com>>
>>>             >         <mailto:alexey.menkov at oracle.com
>>>             <mailto:alexey.menkov at oracle.com>
>>>             <mailto:alexey.menkov at oracle.com
>>>             <mailto:alexey.menkov at oracle.com>>>
>>>             >          >     <mailto:alexey.menkov at oracle.com
>>>             <mailto:alexey.menkov at oracle.com>
>>>             >         <mailto:alexey.menkov at oracle.com
>>>             <mailto:alexey.menkov at oracle.com>>
>>>             >         <mailto:alexey.menkov at oracle.com
>>>             <mailto:alexey.menkov at oracle.com>
>>>             <mailto:alexey.menkov at oracle.com
>>>             <mailto:alexey.menkov at oracle.com>>>>
>>>             >          >      >      >
>>>             <mailto:alexey.menkov at oracle.com
>>>             <mailto:alexey.menkov at oracle.com>
>>>             >         <mailto:alexey.menkov at oracle.com
>>>             <mailto:alexey.menkov at oracle.com>>
>>>             >          >     <mailto:alexey.menkov at oracle.com
>>>             <mailto:alexey.menkov at oracle.com>
>>>             >         <mailto:alexey.menkov at oracle.com
>>>             <mailto:alexey.menkov at oracle.com>>>
>>>             >          >      >  <mailto:alexey.menkov at oracle.com
>>>             <mailto:alexey.menkov at oracle.com>
>>>             >         <mailto:alexey.menkov at oracle.com
>>>             <mailto:alexey.menkov at oracle.com>>
>>>             >          >     <mailto:alexey.menkov at oracle.com
>>>             <mailto:alexey.menkov at oracle.com>
>>>             >         <mailto: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/>
>>>             >          >   
>>>              <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 at 12-internal/Thread.java:834
>>>             <mailto: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>
>>>             >          >
>>>             >         
>>>              <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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180928/c4d38c22/attachment-0001.html>


More information about the serviceability-dev mailing list