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

[JC Beyler]

Hi all,

@David/Chris: should I then push this RFR to the hotspot mailing or the
runtime one? For what it's worth, a lot of the tests under the vmTestbase
are jvmti so the review also affects serviceability; it just turns out I
started with the GC originally and then hit some other tests I had touched
via the assignment extraction.

@Serguei: Done for the method renaming, for the indent, are you talking
about going from the 8-indent to 4-indent? If so, would it not just be
better to do a new JBS bug and do the whole files in one go? I ask because
otherwise, it will look a bit weird to have parts of the file as 8-indent
and others 4-indent?

Thanks for looking at it!
Jc

On Mon, Nov 19, 2018 at 1:25 AM serguei.spitsyn at oracle.com <
serguei.spitsyn at oracle.com> wrote:

> 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 <jcbeyler at 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
>
>
>

-- 

Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181119/1ba2209d/attachment.html>