Re: RFR (L) 8213501 : Deploy ExceptionJniWrapper for a few tests

Mon, 19 Nov 2018 01:26:17 -0800

Hi Jc,

We have to start this review anyway. :)
It looks good to me in general.
Thank you for your consistency in this refactoring!

Some minor comments.

http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.cpp.udiff.html

+static const char* remove_folders(const char* fullname) {

I'd suggest to rename the function name to something traditional like get_basename.
Otherwise, it sounds like this function has to really remove folders. :)


Also, all *Locker.cpp have wrong indent in the bodies of if and while statements.
Could this be fixed with the refactoring?

I did not look on how this impacts the tests other than serviceability.

Thanks,
Serguei

On 11/16/18 19:43, JC Beyler wrote:
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.


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

Reply via email to