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