Sorry for being late to the game. Can I request a helpful comment in SafeJNIEnv.hpp describing what its purpose is?
Also, I don’t necessarily have a better suggestion (and don’t consider this blocking), but Is there another word than “Safe” to describe this wrapper? “Checked”? ExceptionChecking? Just something to consider. Cheers, Mikael > On Sep 26, 2018, at 8:52 PM, JC Beyler <jcbey...@google.com> 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/~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 > > <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/~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/ > > <http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.02/>> > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8210842 > > <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 > > <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/~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/>> > > > > > > > <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 > > <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 > > <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 > > <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/~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/ > > <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 > > <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/~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>> > > > > > > > > > > > > > > > > > > > > <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/~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>> > > > > > > > > > > > > > > > > > > > > <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 > > > > > > > > -- > > > > Thanks, > > Jc > > > -- > > Thanks, > Jc > > > -- > > Thanks, > Jc
