Hi all, Anybody motivated to review this? :) Jc
On Wed, Nov 7, 2018 at 9:53 PM JC Beyler <jcbey...@google.com> wrote: > Hi all, > > Could I have a review for the extension and usage of the > ExceptionJniWrapper. This adds lines and filenames to the end of the > wrapper JNI methods, adds tracing, and throws an error if need be. I've > ported the gc/lock files to use the new TRACE_JNI_CALL add-on and I've > ported a few of the tests that were already changed for the assignment > webrev for JDK-8212884. > > Webrev: http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.00/ > Bug: https://bugs.openjdk.java.net/browse/JDK-8213501 > > For illustration, if I force an error to the AP04/ap04t03 test and set the > verbosity on, I get something like: > > >> Calling JNI method FindClass from ap04t003.cpp:343 > >> Calling with these parameter(s): > java/lang/Threadd > Wait for thread to finish > << Called JNI method FindClass from ap04t003.cpp:343 > Exception in thread "Thread-0" java.lang.NoClassDefFoundError: > java/lang/Threadd > at > nsk.jvmti.scenarios.allocation.AP04.ap04t003.runIterateOverHeap(Native > Method) > at > nsk.jvmti.scenarios.allocation.AP04.ap04t003HeapIterator.runIteration(ap04t003.java:140) > at > nsk.jvmti.scenarios.allocation.AP04.ap04t003Thread.run(ap04t003.java:201) > Caused by: java.lang.ClassNotFoundException: java.lang.Threadd > at > java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:583) > at > java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178) > at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521) > ... 3 more > FATAL ERROR in native method: JNI method FindClass : internal error from > ap04t003.cpp:343 > at > nsk.jvmti.scenarios.allocation.AP04.ap04t003.runIterateOverHeap(Native > Method) > at > nsk.jvmti.scenarios.allocation.AP04.ap04t003HeapIterator.runIteration(ap04t003.java:140) > at > nsk.jvmti.scenarios.allocation.AP04.ap04t003Thread.run(ap04t003.java:201) > > Questions/comments I have about this are: > - Do we want to force fatal errors when a JNI call fails in general? > Most of these tests do the right thing and test the return of the JNI > calls, for example: > thrClass = jni->FindClass("java/lang/Threadd", TRACE_JNI_CALL); > if (thrClass == NULL) { > > but now the wrapper actually would do a fatal if the FindClass call would > return a nullptr, so we could remove that test altogether. What do you > think? > - I prefer to leave them as the tests then become closer to what real > users would have in their code and is the "recommended" way of doing it > > - The alternative is to use the NonFatalError I added which then just > prints out that something went wrong, letting the test continue. Question > will be what should be the default? The fatal or the non-fatal error > handling? > > On a different subject: > - On the new tests, I've removed the NSK_JNI_VERIFY since the JNI > wrapper handles the tracing and the verify in almost the same way; only > difference I can really tell is that the complain method from NSK has a max > complain before stopping to "complain"; I have not added that part of the > code in this webrev > > Once we decide on these, I can continue on the files from JDK-8212884 and > then do both the assignment in an if extraction followed-by this type of > webrev in an easier fashion. Depending on decisions here, NSK*VERIFY can be > deprecated as well as we go forward. > > Thank you for the reviews/comments :) > Jc > -- Thanks, Jc
