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