Some of us have lobbied to make openjdk source C++11, but it's not yet. If you're brave, you can try to change that flag to -std=gnu++11
On Thu, Sep 27, 2018 at 2:33 PM, JC Beyler <jcbey...@google.com> 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/ > > Sorry for the last minute rug pull, > Jc > > On Thu, Sep 27, 2018 at 11:32 AM Mikael Vidstedt > <mikael.vidst...@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 <jcbey...@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 >> >> For all, the new webrev is here: >> Webrev: http://cr.openjdk.java.net/~jcbeyler/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.hol...@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 <jcbey...@google.com >>> > <mailto:jcbey...@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.hol...@oracle.com <mailto:david.hol...@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.hol...@oracle.com <mailto:david.hol...@oracle.com> >>> > > <mailto:david.hol...@oracle.com >>> > <mailto:david.hol...@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/> >>> > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8210842 >>> > > > >>> > > > Thanks again! >>> > > > Jc >>> > > > >>> > > > >>> > > > On Wed, Sep 19, 2018 at 5:13 PM Alex Menkov >>> > > <alexey.men...@oracle.com >>> > <mailto:alexey.men...@oracle.com> >>> > <mailto:alexey.men...@oracle.com >>> > <mailto:alexey.men...@oracle.com>> >>> > > > <mailto:alexey.men...@oracle.com >>> > <mailto:alexey.men...@oracle.com> >>> > > <mailto:alexey.men...@oracle.com >>> > <mailto:alexey.men...@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/> >>> > > > > 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.men...@oracle.com >>> > <mailto:alexey.men...@oracle.com> >>> > <mailto:alexey.men...@oracle.com >>> > <mailto:alexey.men...@oracle.com>> >>> > > <mailto:alexey.men...@oracle.com >>> > <mailto:alexey.men...@oracle.com> >>> > <mailto:alexey.men...@oracle.com >>> > <mailto:alexey.men...@oracle.com>>> >>> > > > > <mailto:alexey.men...@oracle.com >>> > <mailto:alexey.men...@oracle.com> >>> > > <mailto:alexey.men...@oracle.com >>> > <mailto:alexey.men...@oracle.com>> >>> > > > <mailto:alexey.men...@oracle.com >>> > <mailto:alexey.men...@oracle.com> >>> > > <mailto:alexey.men...@oracle.com >>> > <mailto:alexey.men...@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/> >>> > > > > > >>> > 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/> >>> > > > > > > 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@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> >>> > > > > > >>> > > > > > 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> >>> > > > > > >>> > > > > > >>> > > > > > >>> > > > > > 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
