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

Discussion:

JC Beyler

2018-11-08 05:53:59 UTC

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

Calling JNI method FindClass from ap04t003.cpp:343

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

JC Beyler

2018-11-17 03:43:27 UTC

Permalink

Hi all,

Anybody motivated to review this? :)
Jc

Post by 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.
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

Calling JNI method FindClass from ap04t003.cpp:343

java/lang/Threadd
Wait for thread to finish
<< Called JNI method FindClass from ap04t003.cpp:343
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)
- 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
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 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

Chris Plummer

2018-11-18 07:16:38 UTC

Permalink

<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Hi JC,<br>
<br>
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.<br>
<br>
cheers,<br>
<br>
Chris<br>
<br>
On 11/16/18 7:43 PM, JC Beyler wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAF9BGBzjf5b30E=0sWU7HszRsEG_zNTRmx-***@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=utf-8">
<div dir="ltr">Hi all,<br>
<div><br>
</div>
<div>Anybody motivated to review this? :)</div>
<div>Jc</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Wed, Nov 7, 2018 at 9:53 PM JC Beyler <<a
href="mailto:***@google.com" moz-do-not-send="true">***@google.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">Hi all,
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>
<div>Webrev: <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.00/"
target="_blank" moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.00/</a></div>
<div>Bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8213501"
target="_blank" moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8213501</a></div>
<div><br>
</div>
<div>For illustration, if I force an error
to the AP04/ap04t03 test and set the
verbosity on, I get something like:</div> <div><br> </div> <div> <div>>> Calling JNI method FindClass
from ap04t003.cpp:343</div> <div>>> Calling with these
parameter(s):</div>
<div> java/lang/Threadd</div>
<div>Wait for thread to finish</div>
<div><< Called JNI method FindClass
from ap04t003.cpp:343</div>
<div>Exception in thread "Thread-0"
java.lang.NoClassDefFoundError:
java/lang/Threadd</div>
<div> at
nsk.jvmti.scenarios.allocation.AP04.ap04t003.runIterateOverHeap(Native
Method)</div>
<div> at
nsk.jvmti.scenarios.allocation.AP04.ap04t003HeapIterator.runIteration(ap04t003.java:140)</div>
<div> at
nsk.jvmti.scenarios.allocation.AP04.ap04t003Thread.run(ap04t003.java:201)</div>
<div>Caused by:
java.lang.ClassNotFoundException:
java.lang.Threadd</div>
<div> at
java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:583)</div>
<div> at
java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)</div>
<div> at
java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521)</div>
<div> ... 3 more</div>
<div>FATAL ERROR in native method: JNI
method FindClass : internal error from
ap04t003.cpp:343</div>
<div> at
nsk.jvmti.scenarios.allocation.AP04.ap04t003.runIterateOverHeap(Native
Method)</div>
<div> at
nsk.jvmti.scenarios.allocation.AP04.ap04t003HeapIterator.runIteration(ap04t003.java:140)</div>
<div> at
nsk.jvmti.scenarios.allocation.AP04.ap04t003Thread.run(ap04t003.java:201)</div>
</div>
</div>
<div>
<div dir="ltr"
class="m_-5336086771686025688gmail-m_2982675986737217387gmail_signature">
<div dir="ltr">
<div><br>
</div>
<div>Questions/comments I have about
this are:</div>
<div> - 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:</div>
<div>
<div> thrClass =
jni->FindClass("java/lang/Threadd",
TRACE_JNI_CALL);</div>
<div> if (thrClass == NULL) {</div>
</div>
<div><br>
</div>
<div>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?</div>
<div> - 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</div>
<div><br>
</div>
<div> - 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?</div>
<div><br>
</div>
<div>On a different subject:</div>
<div> - 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</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>Thank you for the reviews/comments
:)</div>
<div>Jc</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
</div>
<br clear="all">
<div><br>
</div>
-- <br>
<div dir="ltr" class="gmail_signature"
data-smartmail="gmail_signature">
<div dir="ltr">
<div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</blockquote>
<p><br>
</p>
</body>
</html>

David Holmes

2018-11-18 11:48:40 UTC

Permalink

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

David

Post by 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

Post by JC Beyler
Hi all,
Anybody motivated to review this? :)
Jc
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

Calling JNI method FindClass from ap04t003.cpp:343

java/lang/Threadd
Wait for thread to finish
<< Called JNI method FindClass from ap04t003.cpp:343
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)
- 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
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 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

s***@oracle.com

2018-11-19 09:25:11 UTC

Permalink

<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Hi Jc,<br>
<br>
We have to start this review anyway. :)<br>
It looks good to me in general.<br>
Thank you for your consistency in this refactoring!<br>
<br>
Some minor comments.<br>
<br>
<pre><span class="new"><a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.cpp.udiff.html">http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.cpp.udiff.html</a>

+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
</span></pre>
<br>
On 11/16/18 19:43, JC Beyler wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAF9BGBzjf5b30E=0sWU7HszRsEG_zNTRmx-***@mail.gmail.com">
<div dir="ltr">Hi all,<br>
<div><br>
</div>
<div>Anybody motivated to review this? :)</div>
<div>Jc</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Wed, Nov 7, 2018 at 9:53 PM JC Beyler <<a
href="mailto:***@google.com" moz-do-not-send="true">***@google.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">Hi all,
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>
<div>Webrev: <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.00/"
target="_blank" moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.00/</a></div>
<div>Bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8213501"
target="_blank" moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8213501</a></div>
<div><br>
</div>
<div>For illustration, if I force an error
to the AP04/ap04t03 test and set the
verbosity on, I get something like:</div> <div><br> </div> <div> <div>>> Calling JNI method FindClass
from ap04t003.cpp:343</div> <div>>> Calling with these
parameter(s):</div>
<div> java/lang/Threadd</div>
<div>Wait for thread to finish</div>
<div><< Called JNI method FindClass
from ap04t003.cpp:343</div>
<div>Exception in thread "Thread-0"
java.lang.NoClassDefFoundError:
java/lang/Threadd</div>
<div> at
nsk.jvmti.scenarios.allocation.AP04.ap04t003.runIterateOverHeap(Native
Method)</div>
<div> at
nsk.jvmti.scenarios.allocation.AP04.ap04t003HeapIterator.runIteration(ap04t003.java:140)</div>
<div> at
nsk.jvmti.scenarios.allocation.AP04.ap04t003Thread.run(ap04t003.java:201)</div>
<div>Caused by:
java.lang.ClassNotFoundException:
java.lang.Threadd</div>
<div> at
java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:583)</div>
<div> at
java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)</div>
<div> at
java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521)</div>
<div> ... 3 more</div>
<div>FATAL ERROR in native method: JNI
method FindClass : internal error from
ap04t003.cpp:343</div>
<div> at
nsk.jvmti.scenarios.allocation.AP04.ap04t003.runIterateOverHeap(Native
Method)</div>
<div> at
nsk.jvmti.scenarios.allocation.AP04.ap04t003HeapIterator.runIteration(ap04t003.java:140)</div>
<div> at
nsk.jvmti.scenarios.allocation.AP04.ap04t003Thread.run(ap04t003.java:201)</div>
</div>
</div>
<div>
<div dir="ltr"
class="m_-5336086771686025688gmail-m_2982675986737217387gmail_signature">
<div dir="ltr">
<div><br>
</div>
<div>Questions/comments I have about
this are:</div>
<div> - 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:</div>
<div>
<div> thrClass =
jni->FindClass("java/lang/Threadd",
TRACE_JNI_CALL);</div>
<div> if (thrClass == NULL) {</div>
</div>
<div><br>
</div>
<div>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?</div>
<div> - 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</div>
<div><br>
</div>
<div> - 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?</div>
<div><br>
</div>
<div>On a different subject:</div>
<div> - 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</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>Thank you for the reviews/comments
:)</div>
<div>Jc</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
</div>
<br>
<div><br>
</div>
-- <br>
<div dir="ltr" class="gmail_signature"
data-smartmail="gmail_signature">
<div dir="ltr">
<div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</blockquote>
<br>
</body>
</html>

JC Beyler

2018-11-19 18:07:19 UTC

Permalink

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

Post by s***@oracle.com
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
Hi all,
Anybody motivated to review this? :)
Jc

Calling JNI method FindClass from ap04t003.cpp:343

--
Thanks,
Jc

Chris Plummer

2018-11-19 19:34:09 UTC

Permalink

<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 11/19/18 10:07 AM, JC Beyler wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAF9BGBx6oDP_fHpym0pXBYMVXrM8=+***@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=utf-8">
<div dir="ltr">Hi all,
<div><br>
</div>
<div>@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.</div>
</div>
</blockquote>
I think hotspot would be best.<br>
<br>
Chris<br>
<blockquote type="cite"
cite="mid:CAF9BGBx6oDP_fHpym0pXBYMVXrM8=+***@mail.gmail.com">
<div dir="ltr">
<div><br>
</div>
<div>@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?<br>
</div>
<div><br>
</div>
<div>Thanks for looking at it!<br>
</div>
<div>Jc</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Mon, Nov 19, 2018 at 1:25 AM <a
href="mailto:***@oracle.com"
moz-do-not-send="true">***@oracle.com</a> <<a
href="mailto:***@oracle.com"
moz-do-not-send="true">***@oracle.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF">
<div class="m_3356609488169393251moz-cite-prefix">Hi Jc,<br>
<br>
We have to start this review anyway. :)<br>
It looks good to me in general.<br>
Thank you for your consistency in this refactoring!<br>
<br>
Some minor comments.<br>
<br>
<pre><span class="m_3356609488169393251new"><a class="m_3356609488169393251moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.cpp.udiff.html" target="_blank" moz-do-not-send="true">http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.cpp.udiff.html</a>

+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
</span></pre>
<br>
On 11/16/18 19:43, JC Beyler wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">Hi all,<br>
<div><br>
</div>
<div>Anybody motivated to review this? :)</div>
<div>Jc</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Wed, Nov 7, 2018 at 9:53 PM JC Beyler
<<a href="mailto:***@google.com"
target="_blank" moz-do-not-send="true">***@google.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">Hi all,
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>
<div>Webrev: <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.00/"
target="_blank"
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.00/</a></div>
<div>Bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8213501"
target="_blank"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8213501</a></div>
<div><br>
</div>
<div>For illustration, if I force an
error to the AP04/ap04t03 test and
set the verbosity on, I get
something like:</div> <div><br> </div> <div> <div>>> Calling JNI method
FindClass from ap04t003.cpp:343</div> <div>>> Calling with these
parameter(s):</div>
<div> java/lang/Threadd</div>
<div>Wait for thread to finish</div>
<div><< Called JNI method
FindClass from ap04t003.cpp:343</div>
<div>Exception in thread
"Thread-0"
java.lang.NoClassDefFoundError:
java/lang/Threadd</div>
<div> at
nsk.jvmti.scenarios.allocation.AP04.ap04t003.runIterateOverHeap(Native
Method)</div>
<div> at
nsk.jvmti.scenarios.allocation.AP04.ap04t003HeapIterator.runIteration(ap04t003.java:140)</div>
<div> at
nsk.jvmti.scenarios.allocation.AP04.ap04t003Thread.run(ap04t003.java:201)</div>
<div>Caused by:
java.lang.ClassNotFoundException:
java.lang.Threadd</div>
<div> at
java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:583)</div>
<div> at
java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)</div>
<div> at
java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521)</div>
<div> ... 3 more</div>
<div>FATAL ERROR in native method:
JNI method FindClass : internal
error from ap04t003.cpp:343</div>
<div> at
nsk.jvmti.scenarios.allocation.AP04.ap04t003.runIterateOverHeap(Native
Method)</div>
<div> at
nsk.jvmti.scenarios.allocation.AP04.ap04t003HeapIterator.runIteration(ap04t003.java:140)</div>
<div> at
nsk.jvmti.scenarios.allocation.AP04.ap04t003Thread.run(ap04t003.java:201)</div>
</div>
</div>
<div>
<div dir="ltr"
class="m_3356609488169393251m_-5336086771686025688gmail-m_2982675986737217387gmail_signature">
<div dir="ltr">
<div><br>
</div>
<div>Questions/comments I have
about this are:</div>
<div> - 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:</div>
<div>
<div> thrClass =
jni->FindClass("java/lang/Threadd",
TRACE_JNI_CALL);</div>
<div> if (thrClass == NULL)
{</div>
</div>
<div><br>
</div>
<div>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?</div>
<div> - 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</div>
<div><br>
</div>
<div> - 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?</div>
<div><br>
</div>
<div>On a different subject:</div>
<div> - 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</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>Thank you for the
reviews/comments :)</div>
<div>Jc</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
</div>
<br>
<div><br>
</div>
-- <br>
<div dir="ltr"
class="m_3356609488169393251gmail_signature"
data-smartmail="gmail_signature">
<div dir="ltr">
<div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</blockquote>
<br>
</div>
</blockquote>
</div>
<br clear="all">
<div><br>
</div>
-- <br>
<div dir="ltr" class="gmail_signature"
data-smartmail="gmail_signature">
<div dir="ltr">
<div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</blockquote>
<p><br>
</p>
</body>
</html>