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> 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> > 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>> 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/> >> > > 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>>> 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/> >> > > > 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>>>> 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/> >> > > > > 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/> >> > > > > > 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 >> > >> > > > > >> > > > > 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 >> > >> > > > > >> > > > > >> > > > > >> > > > > 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
