RFR (S) 8210842: Handle JNIGlobalRefLocker.cpp
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Fri Sep 28 03:00:47 UTC 2018
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)
>> > > > > >
>> > > > > >
>> > > > > > 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180927/7f463fc6/attachment-0001.html>
More information about the serviceability-dev
mailing list