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

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

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

JC Beyler
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
Reply | Threaded
Open this post in threaded view
|

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

JC Beyler
Hi all,

Anybody motivated to review this? :)
Jc

On Wed, Nov 7, 2018 at 9:53 PM JC Beyler <[hidden email]> 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 | Threaded
Open this post in threaded view
|

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

Chris Plummer
Hi JC,

I'll try to take a look early this week. Won't this eventually affect non-svc tests? If so, I think the general mechanism should be reviewed by a wider audience.

cheers,

Chris

On 11/16/18 7:43 PM, JC Beyler wrote:
Hi all,

Anybody motivated to review this? :)
Jc

On Wed, Nov 7, 2018 at 9:53 PM JC Beyler <[hidden email]> 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 | Threaded
Open this post in threaded view
|

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

David Holmes
This is already primarily a runtime change with some GC test changes and
serviceability test changes AFAICS!

David

On 18/11/2018 5:16 pm, Chris Plummer wrote:

> Hi JC,
>
> I'll try to take a look early this week. Won't this eventually affect
> non-svc tests? If so, I think the general mechanism should be reviewed
> by a wider audience.
>
> cheers,
>
> Chris
>
> On 11/16/18 7:43 PM, JC Beyler wrote:
>> Hi all,
>>
>> Anybody motivated to review this? :)
>> Jc
>>
>> On Wed, Nov 7, 2018 at 9:53 PM JC Beyler <[hidden email]
>> <mailto:[hidden email]>> 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/
>>     <http://cr.openjdk.java.net/%7Ejcbeyler/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
>
>
Reply | Threaded
Open this post in threaded view
|

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

serguei.spitsyn@oracle.com
In reply to this post by JC Beyler
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 <[hidden email]> 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 | Threaded
Open this post in threaded view
|

Re: 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 [hidden email] <[hidden email]> 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 <[hidden email]> 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



--

Thanks,
Jc
Reply | Threaded
Open this post in threaded view
|

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

Chris Plummer
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 [hidden email] <[hidden email]> 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 <[hidden email]> 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



--

Thanks,
Jc