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

Tue Nov 20 22:14:45 UTC 2018

Hi all,

Chris thought it made sense to have more eyes on this change than just
serviceability as it will modify to tests that are not only serviceability
tests so I've moved this to conversation here :)

For convenience, I've copy-pasted the initial RFR:

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.01
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.

Thanks!
Jc

On Mon, Nov 19, 2018 at 11:34 AM Chris Plummer <chris.plummer at oracle.com>
wrote:

> On 11/19/18 10:07 AM, JC Beyler wrote:
>
> 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.
>
> I think hotspot would be best.
>
> Chris
>
>
> @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
>
>
>

-- 

Thanks,
Jc