<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<br>
<br>
<div class="moz-cite-prefix">On 11/6/18 11:14 AM, Chris Plummer
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:b279ee97-92e6-9d1f-1e8a-a066a9c1a1cf@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<div class="moz-cite-prefix">Hi JC,<br>
<br>
The exception changes looked ok to me, but it would be good to
get a second approval before moving forward with them since they
are pretty significant.<br>
</div>
</blockquote>
<br>
The exception changes need to be discussed after a separate RFR is
posted.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<blockquote type="cite"
cite="mid:b279ee97-92e6-9d1f-1e8a-a066a9c1a1cf@oracle.com">
<div class="moz-cite-prefix"> thanks,<br>
<br>
Chris<br>
<br>
On 11/2/18 9:09 PM, JC Beyler wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAF9BGBzdQ_4NMb7tp3+BJqW6nXmhenb+owPQDggZORWOB_eqFA@mail.gmail.com">
<meta http-equiv="content-type" content="text/html;
charset=utf-8">
<div dir="ltr">Hi Chris,
<div><br>
</div>
<div>Yes that is correct, the webrev in this email thread
would be postponed and done differently. </div>
<div><br>
</div>
<div>Most likely we'd have to do smaller changes to extend
ExceptionCheckingJni and work on replacing the NSK*VERIFY
macros by using the ExceptionCheckingJni wrapper using
something similar to the change I show in <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/exception/"
target="_blank" moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/exception/</a>.</div>
<div><br>
</div>
<div>I guess the question becomes really: are the changes in <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/exception/"
target="_blank" moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/exception/</a>Â seem
in theory at least reasonable? Then I could start working on
it but it will most likely be a slower going process.</div>
<div><br>
</div>
<div>Let me know,</div>
<div>Jc </div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Fri, Nov 2, 2018 at 8:44 PM Chris Plummer
<<a href="mailto:chris.plummer@oracle.com"
moz-do-not-send="true">chris.plummer@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_2577563816354213271moz-cite-prefix">Hi JC,<br>
<br>
So assuming the change to move the assignment outside of
the conditional is already in place, you are changing:<br>
<br>
              debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME);<br>
<br>
to<br>
<br>
       ExceptionCheckingJniEnvPtr jni_wrapper(jni);<br>
        ...<br>
            debugeeClass =
jni_wrapper->FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);<br>
<br>
But none of your ExceptionCheckingJni changes are pushed
yet, right?<br>
<br>
thanks,<br>
<br>
Chris<br>
<br>
On 11/2/18 10:44 AM, JC Beyler wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<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 Chris,
<div><br>
</div>
<div>Here is what it would "look
like", there is a bit more
clean up to make it true for
all methods, handling the
"verbose" flag, etc but it
should help see what I'm
talking about:</div>
<div><a
href="http://cr.openjdk.java.net/%7Ejcbeyler/exception/"
target="_blank"
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/exception/</a><br>
</div>
<div><br>
</div>
<div>Basically:</div>
<div>Â Â - The test now no longer
needs the NSK_JNI_VERIFY but
requires a TRACE_JNI_CALL as
last argument to the JNI calls
if you want </div>
<div>Â Â Â Â Â <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp.udiff.html"
target="_blank"
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp.udiff.html</a></div>
<div>Â Â Â Note: I left the
nsk_setVerboseMode call though
this version does not care
about that flag but I would do
it to be conforming of course</div>
<div><br>
</div>
<div>Â Â - The wrapper class
would have a new macro
TRACE_JNI_CALL and the methods
would have new parameters for
the line and file that are
defaulted to -1 and 0 so that
we can know if they are used
or not:</div>
<div>Â Â Â Â Â <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.hpp.udiff.html"
target="_blank"
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.hpp.udiff.html</a></div>
<div><br>
</div>
<div>Â Â - Finally, the real
change comes from here:</div>
<div>Â Â Â Â <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.cpp.udiff.html"
target="_blank"
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.cpp.udiff.html</a></div>
<div><br>
</div>
<div>Â Â Â Where we do this:</div>
<div>Â Â Â Â a) add a new
constructor for the
JNIVerifier class for having
the parameter of the JNI call
be passed in for printing the
value of, this will sadly have
to be duplicated by the number
of different argument numbers
since variadic templates is
C++11 onwards but that is not
a huge problem since it is
contained to this file</div>
<div>Â Â Â Â b) at
JNIVerifier construction, we
print it out the "before" call
and this should be protected
by the verbose flag like the
nsk_ltrace/nsk_verify methods
do it for compatibility</div>
<div>Â Â Â Â c) at JNIVerifier
construction, we now can call
the print parameter methods to
pass the values of the call to
be printed out</div>
<div>Â Â Â Â Â - again sadly
without c++11 we will have to
have a few of these here but
that is limited to this part
of the code so should be fine</div>
<div>Â Â Â Â d) the JNI wrapper
methods get augmented by the
line/file parameters which
default to -1 and 0</div>
<div>Â Â Â Â e) the constructor
is now also passing in the JNI
method parameters</div>
<div><br>
</div>
<div>It's mostly boiler plate
code but allows us to go from:</div>
<div>
<div>-Â Â Â Â Â Â if
(!NSK_JNI_VERIFY(jni,
(debugeeClass =</div>
<div>-Â Â Â Â Â Â Â Â Â Â
jni->FindClass(DEBUGEE_CLASS_NAME))
!= NULL)) {</div>
<div>+Â Â Â Â Â Â debugeeClass
=
jni_wrapper->FindClass(DEBUGEE_CLASS_NAME, TRACE_JNI_CALL);</div>
<div>+Â Â Â Â Â Â if
(debugeeClass != NULL) {</div>
</div>
<div><br>
</div>
<div>And print out:</div>
<div>
<div>>> Calling JNI
method FindClass from
/usr/local/google/home/jcbeyler/mercurial/jdk12-verify/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69</div>
<div>>> Calling with
these parameter(s):</div>
<div>Â Â Â Â
nsk/jvmti/SetTag/settag001</div>
<div><< Called JNI
method FindClass from
/usr/local/google/home/jcbeyler/mercurial/jdk12-verify/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69</div>
<div>FATAL ERROR in native
method: JNI method FindClass
: Return is NULL from
/usr/local/google/home/jcbeyler/mercurial/jdk12-verify/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69</div>
</div>
<div><br>
</div>
<div>With the >> and
<< lines being the
"equivalent" to the trace
methods and the "FATAL ERROR
being the equivalent to the
NSK_COMPLAIN done in the
verify method.</div>
<div><br>
</div>
<div>Let me know what you think,</div>
<div>Jc</div>
<div><br>
</div>
<div>Â Â Â Â d) at JNIVerifier
destruction, we can print out
the "called the method"</div>
<div><br>
</div>
<div><br>
</div>
<div>Â </div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Thu, Nov 1, 2018 at 1:24 PM Chris
Plummer <<a
href="mailto:chris.plummer@oracle.com"
target="_blank" moz-do-not-send="true">chris.plummer@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_2577563816354213271m_1232422551998170429moz-cite-prefix">Hi
JC,<br>
<br>
I would need to see a webrev with the
ExceptionCheckingJni support, new macros, and a
few example uses. You don't need to fix
everything. I just want to get a better feel for
the changes and what the implementation would
look like.<br>
<br>
thanks,<br>
<br>
Chris<br>
<br>
On 11/1/18 1:03 PM, JC Beyler wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">Hi Chris,
<div><br>
</div>
<div>Thanks for going over my email
:)</div>
<div><br>
</div>
<div>So I skipped over the tracing
part for clarity and striving to
be brief. But if we can accept:</div>
<div>
<div>Â Â Â Â Â Â debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);</div>
</div>
<div><br>
</div>
<div>and use a mechanism such as </div>
<div>Â Â Â Â Â Â debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);</div>
<div><br>
</div>
<div>it is actually easy to print
out things like:</div>
<div>>> JNI FindClass with the
following parameters from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69:</div>
<div>>>Â <span style="white-space:pre-wrap"> </span>"nsk/jvmti/SetTag/settag001"<br>
</div>
<div>...</div>
<div>
<div><< JNI FindClass from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69:</div>
<div><br>
</div>
</div>
<div>Before the actual call to the
method, allowing to have the full
NSK_*_VERIFY system. The hardest
question is do we accept to go
from:</div>
<div>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr"><span
class="m_2577563816354213271m_1232422551998170429gmail-im"
style="color:rgb(80,0,80)">
<div>Â if
(!NSK_JNI_VERIFY(jni,
(debugeeClass
=<br>
</div>
<div>
<div>Â Â Â Â Â
    Â
jni->FindClass(DEBUGEE_CLASS_NAME))
!= NULL)) {</div>
<div>Â Â Â Â Â
  Â
nsk_jvmti_setFailStatus();</div>
<div>Â Â Â Â Â
   return;</div>
<div>Â Â Â Â Â
 }</div>
</div>
</span></div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
</div>
<div><br>
</div>
<div>to:</div>
<div>
<div><br>
</div>
<div>#define TRACE_JNI_CALL
__LINE__, __FILE__</div>
<div>...</div>
<div><br>
</div>
<div>Â Â Â Â Â
 ExceptionCheckingJniEnvPtr
jni_wrapper(jni);</div>
<div>...</div>
<div>Â Â Â Â Â Â debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);</div>
</div>
<div><br>
</div>
<div>What I would do if this is
accepted is postpone the
assignment extraction and move the
code to migrating the NSK*VERIFY
macros to using the exception
wrapper and then moving the
assignments will be easier and a
nop from a debug printout point of
view.</div>
<div><br>
</div>
<div>Thanks,</div>
<div>Jc</div>
<div><br>
</div>
<div><br>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Thu, Nov 1, 2018 at 12:01 PM
Chris Plummer <<a
href="mailto:chris.plummer@oracle.com"
target="_blank" moz-do-not-send="true">chris.plummer@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_2577563816354213271m_1232422551998170429m_5548951279341903837moz-cite-prefix">On
11/1/18 9:53 AM, JC Beyler wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<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>Sorry this is a
bit of a long
reply to myself
but I've tried to
see what we have,
what we could
have, is it
better. Here is my
analysis of how to
keep the
information we
currently provide
when a test fails
at a NSK*VERIFY
point and how we
could maintain the
level of detail
(and even expand)
while still
cleaning up the
macros/assignments.</div>
<div><br>
</div>
<div>I've been
looking at this
and am going to go
through examples
to provide
alternatives :).
For this, I'll use
the SetTag test
and will be
forcing a failure
on the line:</div>
<div><br>
</div>
<div>Â Â Â Â Â Â if
(!NSK_JNI_VERIFY(jni, (debugeeClass =<br>
</div>
<div>
<div>Â Â Â Â Â Â Â
  Â
jni->FindClass(DEBUGEE_CLASS_NAME))
!= NULL)) {</div>
<div>Â Â Â Â Â Â Â
Â
nsk_jvmti_setFailStatus();</div>
<div>Â Â Â Â Â Â Â
 return;</div>
<div>Â Â Â Â Â Â }</div>
</div>
<div><br>
</div>
<div>Without a
change and with
the verbose system
set up for the
test, the error
message is:</div>
<div><br>
</div>
<div>
<div>The following
fake exception
stacktrace is
for failuire
analysis. </div>
<div>nsk.share.Fake_Exception_for_RULE_Creation:
(settag001.cpp:65) (debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL</div>
<div><span style="white-space:pre-wrap"> </span>at
nsk_lvcomplain(nsk_tools.cpp:172)</div>
<div># ERROR:
settag001.cpp,
65:
(debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL</div>
<div>#Â Â verified
JNI assertion is
FALSE</div>
<div># ERROR:
agent_tools.cpp,
282: Debuggee
status sync
aborted because
agent thread has
finished</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
I think you are missing something here.
This appears to be the output from
nsk_jni_lverify() when the condition
indicates failure. However, I don't see
the verbose tracing from nsk_ltrace(),
which I think is more relevant because
that's were you are likely to want to see
the actual JNI or JVMTI call being made.<br>
<br>
#define NSK_JNI_VERIFY(jni, action)Â \<br>
 (nsk_ltrace(NSK_TRACE_BEFORE,__FILE__,__LINE__,"%s\n",#action), \<br>
Â
nsk_jni_lverify(NSK_TRUE,jni,action,__FILE__,__LINE__,"%s\n",#action))<br>
<br>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div>
<div><br>
</div>
</div>
<div>(Note the typo
for failuire, I
created JDK-8213246
to fix it).</div>
<div><br>
</div>
<div>With my
proposed change to
remove the
assignment, the
code becomes:</div>
<div>Â Â Â Â Â Â
debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME);</div>
<div>
<div>Â Â Â Â Â Â
if
(!NSK_JNI_VERIFY(jni, debuggeeClass != NULL)) {</div>
<div>
<div>Â Â Â Â Â Â
 Â
nsk_jvmti_setFailStatus();</div>
<div>Â Â Â Â Â Â
  return;</div>
<div>Â Â Â Â Â Â
}</div>
</div>
</div>
<div><br>
</div>
<div>
<div>The following
fake exception
stacktrace is
for failuire
analysis. </div>
<div>nsk.share.Fake_Exception_for_RULE_Creation:
(settag001.cpp:66) debugeeClass != NULL</div>
<div><span style="white-space:pre-wrap"> </span>at
nsk_lvcomplain(nsk_tools.cpp:172)</div>
<div># ERROR:
settag001.cpp,
66: debugeeClass
!= NULL</div>
<div>#Â Â verified
JNI assertion is
FALSE</div>
<div># ERROR:
agent_tools.cpp,
282: Debuggee
status sync
aborted because
agent thread has
finished</div>
<div>STDERR:</div>
</div>
<div><br>
</div>
<div>In this case,
we do lose that
the failure seems
to have happened
in FindClass, we
need to look at
the code.</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
I think losing the actual call info in the
trace for failures is fine. All we really
need is the line number info. I don't
think anyone is going to be trying to
debug the root cause based on trace
message, which is no different than the
source that can be viewed given the line
number info.<br>
<br>
But back to my point above, if you enable
tracing, seeing which JVMTI and JNI calls
are being made in the trace output could
be useful. You could scan all logs and see
what JVMTI and JNI calls were made, and
how often. Possibly useful for coverage
analysis of the tests. However, I don't
consider this critical, and am not opposed
to losing it in order to make the code
more readable.<br>
<br>
In any case, a solution would be to have a
wrapper script like NSK_JNI_VERIFY, but
just for the purpose of tracing, not error
checking:<br>
<br>
<div>Â Â Â Â Â Â debugeeClass =
NSK_JNI_WRAPPER(jni->FindClass(DEBUGEE_CLASS_NAME));</div>
<div>
<div>Â Â Â Â Â Â if
(!NSK_JNI_VERIFY(jni, debuggeeClass !=
NULL)) {</div>
<div>
<div>Â Â Â Â Â Â Â Â
nsk_jvmti_setFailStatus();</div>
<div>Â Â Â Â Â Â Â Â return;</div>
<div>Â Â Â Â Â Â }<br>
<br>
And NSK_JNI_WRAPPER would just to
the tracing and execute the passed
in action.<br>
</div>
</div>
</div>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div><br>
</div>
<div>However, the
ExceptionJNIWrapper
that I implemented
a little while ago
will provide this
information:</div>
<div>FATAL ERROR in
native method:
FindClass : Return
is NULL<br>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
As I mentioned above, I don't consider the
API that failed to be all that useful in
error output as long as we have the line
number info. At best maybe it helps
identify two separate failures as being
the same if the line numbers were to
change.<br>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div><br>
</div>
<div>which is not
sufficient to
really figure out
where this
happened and what
were the
arguments. But we
could extend the
ExceptionJNIWrapper
to have each JNI
call</div>
<div>allow two
optional arguments
and do this:</div>
<div><br>
</div>
<div>...</div>
<div>// For tracing
purposes.<br>
</div>
<div>#define
TRACE_JNI_CALL
__LINE__, __FILE__</div>
<div>...</div>
<div><br>
</div>
<div>Â Â Â Â Â
 ExceptionCheckingJniEnvPtr
jni_wrapper(jni);</div>
<div>...</div>
<div>
<div>Â Â Â Â Â
 debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME, TRACE_JNI_CALL);</div>
<div><br>
</div>
</div>
<div>Where now the
JNI call looks
"almost" like
non-test code and
a real JNI call
but with just that
optional macro
added. Then the
ExceptionCheckingJniEnvPtr
can be modified to
print out:</div>
<div><br>
</div>
<div>
<div>FATAL ERROR
in native
method: JNI
method FindClass
: Return is NULL
from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69</div>
</div>
<div><br>
</div>
<div>Which now
contains all the
same information
except what the
line looks like.
I'm not sure that
is useful,
instead, if we
wanted to go one
step further, we
could add prints
to all parameters
that are passed in
the JNI methods
via the wrapper,
which would</div>
<div>then look like
something like:</div>
<div><br>
</div>
<div>
<div>JNI method
FindClass was
called from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
with these
parameters:</div>
</div>
<div>
<div><span style="white-space:pre-wrap"> </span>"nsk/jvmti/SetTag/settag001"</div>
<div>FATAL ERROR
in native
method: JNI
method FindClass
: Return is NULL
from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
I'm not familiar with you
ExceptionJNIWrapper changes. The error
output above looks fine, but as I
mentioned earlier, I'm ok with just the
source line number info.<br>
<br>
thanks,<br>
<br>
Chris<br>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div><br>
</div>
<div>Let me know
what you think.</div>
<div><br>
</div>
<div>Thanks,</div>
<div>Jc</div>
<div><br>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Wed, Oct 31, 2018 at
1:54 PM JC Beyler <<a
href="mailto:jcbeyler@google.com"
target="_blank"
moz-do-not-send="true">jcbeyler@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">@Claes: you are
right, I did not do a grep such
as "if.* =$"; by adding the
space instead of the $, I missed
a few :)</div>
<div dir="ltr"><br>
</div>
<div dir="ltr">I've been meaning
to ask if we could deprecate
these anyway. Are these really
being used? And if so, are we
saying everything here is
useful:
<div>  - Line/File + JNI call?<br>
<div><br>
</div>
<div>
<div>I ask because I'd like
to deprecate the
NSK_VERIFY macros but the
LINE/FILE is a bit
annoying to deprecate.</div>
<div><br>
</div>
<div>I'd rather be able to
remove them entirely but
we could do an alternative
and migrate the NSK_VERIFY
to:</div>
<div><br>
</div>
<div>Â Â testedFieldID
=Â jni->GetStaticFieldID(testedClass,
FIELD_NAME,
FIELD_SIGNATURE);</div>
<div>Â Â if (!testedFieldID)
{<br>
</div>
<div><br>
</div>
<div>where the print out of
the jni method and
argument values can still
be done in the JNI wrapper
I added
(ExceptionCheckingJniEnv.cpp)
so we'd have the printout
of the calls but not the
line and filename from
where the call was done.</div>
<div><br>
</div>
<div>If lines and filenames
are really important, then
we could do something
like:</div>
<div>
<div>Â Â testedFieldID =
NSK_TRACE(jni->GetStaticFieldID(testedClass,
FIELD_NAME,
FIELD_SIGNATURE));</div>
<div>Â Â if
(!testedFieldID) {<br>
</div>
<br
class="m_2577563816354213271m_1232422551998170429m_5548951279341903837m_-9088134372540873412m_-823222977695390093gmail-Apple-interchange-newline">
</div>
<div>Which would add a line
for which line/file is
doing the JNI call. The
verify part can go into
the wrapper I was talking
about
(ExceptionCheckingJniEnv.cpp). </div>
<div><br>
</div>
<div>
<div>Thanks,<br>
</div>
</div>
<div>Jc</div>
<div><br>
</div>
</div>
</div>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Wed, Oct 31,
2018 at 11:52 AM Chris Plummer
<<a
href="mailto:chris.plummer@oracle.com"
target="_blank"
moz-do-not-send="true">chris.plummer@oracle.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote"
style="margin:0 0 0
.8ex;border-left:1px #ccc
solid;padding-left:1ex">On
10/31/18 11:30 AM, <a
href="mailto:serguei.spitsyn@oracle.com"
target="_blank"
moz-do-not-send="true">serguei.spitsyn@oracle.com</a>
wrote:<br>
> It prints the FILE/LINE
which is enough to identify the
error point.<br>
Yes, but it also prints the JNI
calls.<br>
> But I agree that doing the
assignments within the
NSK_JNI_VERIFY was <br>
> intentional as it gives
more context.<br>
> From the other hand it make
the code ugly and less readable.<br>
> Not sure, what direction is
better to go.<br>
Agreed. Somewhat of a tossup now
as to whether or not this change
should <br>
be done. I'm fine either way.<br>
<br>
Chris<br>
><br>
> Thanks,<br>
> Serguei<br>
><br>
><br>
> On 10/31/18 11:21, Chris
Plummer wrote:<br>
>> Hi Claes,<br>
>><br>
>> It's good that you
brought this up. NSK_JNI_VERIFY
is always "on", <br>
>> but moving the
assignment outside of it does
slightly change the <br>
>> behavior of the macro
(although the code still
executes "correctly"):<br>
>><br>
>> /**<br>
>> Â * Execute action with
JNI call, check result for true
and<br>
>> Â * pending exception
and complain error if required.<br>
>> Â * Also trace action
execution if tracing mode
enabled.<br>
>> Â */<br>
>> #define
NSK_JNI_VERIFY(jni, action)Â \<br>
>>
(nsk_ltrace(NSK_TRACE_BEFORE,__FILE__,__LINE__,"%s\n",#action),
\<br>
>>
nsk_jni_lverify(NSK_TRUE,jni,action,__FILE__,__LINE__,"%s\n",#action))<br>
>><br>
>> So you will no longer
see the JNI call in the trace or
error output. <br>
>> You will just see the
comparison to the JNI result,
which gives no <br>
>> context as to the
source of the result. So I'm now
starting to think <br>
>> that doing the
assignments within the
NSK_JNI_VERIFY was intentional <br>
>> so we would see the JNI
call in the trace output.<br>
>><br>
>> Chris<br>
>><br>
>> On 10/31/18 3:16 AM,
Claes Redestad wrote:<br>
>>> Hi,<br>
>>><br>
>>> here's a case that
your script seems to miss:<br>
>>><br>
>>> <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8213003/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassLoaderClasses/clsldrclss002/clsldrclss002.cpp.udiff.html"
rel="noreferrer"
target="_blank"
moz-do-not-send="true">http://cr.openjdk.java.net/%7Ejcbeyler/8213003/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassLoaderClasses/clsldrclss002/clsldrclss002.cpp.udiff.html</a>
<br>
>>><br>
>>><br>
>>> Â Â Â Â if
(!NSK_JNI_VERIFY(jni,
(testedFieldID =<br>
>>> Â Â Â Â Â Â Â Â Â Â Â Â
jni->GetStaticFieldID(testedClass,
FIELD_NAME, <br>
>>> FIELD_SIGNATURE))
!= NULL))<br>
>>><br>
>>> I'd note that with
some of your changes here you're
unconditionally <br>
>>> executing logic
outside of NSK_JNI_VERIFY
macros. Does care need be <br>
>>> taken to wrap the
code blocks in equivalent
macros/ifdefs? Or are <br>
>>> the NSK_JNI_VERIFY
macros always-on in these tests
(and essentially <br>
>>> pointless)?<br>
>>><br>
>>> /Claes<br>
>>><br>
>>> On 2018-10-31
05:42, JC Beyler wrote:<br>
>>>> Hi all,<br>
>>>><br>
>>>> I worked on
updating my script and handling
more assignments so I <br>
>>>> redid a second
pass on the same files to get
all the cases. Now, on <br>
>>>> those files the
regex "if.* = " no longer shows
any cases we would <br>
>>>> want to fix.<br>
>>>><br>
>>>> Could I get a
review for this webrev:<br>
>>>> Webrev: <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8213003/webrev.00/"
rel="noreferrer"
target="_blank"
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8213003/webrev.00/</a><br>
>>>> Bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8213003"
rel="noreferrer"
target="_blank"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8213003</a><br>
>>>><br>
>>>> I tested this
on my dev machine by passing all
the tests that were <br>
>>>> modified.<br>
>>>><br>
>>>> Thanks!<br>
>>>> Jc<br>
>>><br>
>><br>
>><br>
><br>
<br>
<br>
</blockquote>
</div>
<br clear="all">
<div><br>
</div>
-- <br>
<div dir="ltr"
class="m_2577563816354213271m_1232422551998170429m_5548951279341903837m_-9088134372540873412m_-823222977695390093gmail_signature"
data-smartmail="gmail_signature">
<div dir="ltr">
<div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</blockquote>
</div>
<br clear="all">
<div><br>
</div>
-- <br>
<div dir="ltr"
class="m_2577563816354213271m_1232422551998170429m_5548951279341903837m_-9088134372540873412gmail_signature"
data-smartmail="gmail_signature">
<div dir="ltr">
<div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</blockquote>
<p><br>
</p>
</div>
</blockquote>
</div>
<br clear="all">
<div><br>
</div>
-- <br>
<div dir="ltr"
class="m_2577563816354213271m_1232422551998170429gmail_signature"
data-smartmail="gmail_signature">
<div dir="ltr">
<div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</blockquote>
<p><br>
</p>
</div>
</blockquote>
</div>
<br clear="all">
<div><br>
</div>
-- <br>
<div dir="ltr"
class="m_2577563816354213271gmail_signature"
data-smartmail="gmail_signature">
<div dir="ltr">
<div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</blockquote>
<p><br>
</p>
</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>
</blockquote>
<br>
</body>
</html>