RFR (S) 8210842: Handle JNIGlobalRefLocker.cpp
Alex Menkov
alexey.menkov at oracle.com
Fri Sep 28 17:30:03 UTC 2018
+1 for webrev.05
--alex
On 09/28/2018 04:25, serguei.spitsyn at oracle.com wrote:
> 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
>
More information about the serviceability-dev
mailing list