Discussion:
RFR (M) 8213003: Remove more assignments in ifs for vmTestbase/[a-s]
(too old to reply)
JC Beyler
2018-10-31 04:42:57 UTC
Permalink
Hi all,

I worked on updating my script and handling more assignments so I redid a
second pass on the same files to get all the cases. Now, on those files the
regex "if.* = " no longer shows any cases we would want to fix.

Could I get a review for this webrev:
Webrev: http://cr.openjdk.java.net/~jcbeyler/8213003/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8213003

I tested this on my dev machine by passing all the tests that were modified.

Thanks!
Jc
s***@oracle.com
2018-10-31 07:11:18 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>
It looks good - thanks!<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 10/30/18 21:42, JC Beyler wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAF9BGBzyphWSUNfTrvrPSwD2-***@mail.gmail.com">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">Hi all,
<div><br>
</div>
<div>I worked on updating my script and handling more
assignments so I redid a second pass on the same files to
get all the cases. Now, on those files the regex "if.* = "
no longer shows any cases we would want to fix. <br>
<div><br>
</div>
<div>Could I get a review for this webrev:</div>
<div>Webrev: <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8213003/webrev.00/"
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8213003/webrev.00/</a></div>
<div>Bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8213003"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8213003</a></div>
<div dir="ltr" class="gmail_signature">
<div dir="ltr">
<div><br>
</div>
<div>I tested this on my dev machine by passing all
the tests that were modified.</div>
<div><br>
</div>
Thanks!
<div>Jc</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
</body>
</html>
Claes Redestad
2018-10-31 10:16:06 UTC
Permalink
Hi,

here's a case that your script seems to miss:

http://cr.openjdk.java.net/%7Ejcbeyler/8213003/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassLoaderClasses/clsldrclss002/clsldrclss002.cpp.udiff.html

if (!NSK_JNI_VERIFY(jni, (testedFieldID =
jni->GetStaticFieldID(testedClass, FIELD_NAME, FIELD_SIGNATURE)) != NULL))

I'd note that with some of your changes here you're unconditionally executing logic outside of NSK_JNI_VERIFY macros. Does care need be taken to wrap the code blocks in equivalent macros/ifdefs? Or are the NSK_JNI_VERIFY macros always-on in these tests (and essentially pointless)?

/Claes
Post by JC Beyler
Hi all,
I worked on updating my script and handling more assignments so I
redid a second pass on the same files to get all the cases. Now, on
those files the regex "if.* = " no longer shows any cases we would
want to fix.
Webrev: http://cr.openjdk.java.net/~jcbeyler/8213003/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8213003
I tested this on my dev machine by passing all the tests that were modified.
Thanks!
Jc
Chris Plummer
2018-10-31 18:21:41 UTC
Permalink
Hi Claes,

It's good that you brought this up. NSK_JNI_VERIFY is always "on", but
moving the assignment outside of it does slightly change the behavior of
the macro (although the code still executes "correctly"):

/**
 * Execute action with JNI call, check result for true and
 * pending exception and complain error if required.
 * Also trace action execution if tracing mode enabled.
 */
#define NSK_JNI_VERIFY(jni, action)  \
(nsk_ltrace(NSK_TRACE_BEFORE,__FILE__,__LINE__,"%s\n",#action), \
nsk_jni_lverify(NSK_TRUE,jni,action,__FILE__,__LINE__,"%s\n",#action))

So you will no longer see the JNI call in the trace or error output. You
will just see the comparison to the JNI result, which gives no context
as to the source of the result. So I'm now starting to think that doing
the assignments within the NSK_JNI_VERIFY was intentional so we would
see the JNI call in the trace output.

Chris
Post by Claes Redestad
Hi,
http://cr.openjdk.java.net/%7Ejcbeyler/8213003/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassLoaderClasses/clsldrclss002/clsldrclss002.cpp.udiff.html
     if (!NSK_JNI_VERIFY(jni, (testedFieldID =
             jni->GetStaticFieldID(testedClass, FIELD_NAME,
FIELD_SIGNATURE)) != NULL))
I'd note that with some of your changes here you're unconditionally
executing logic outside of NSK_JNI_VERIFY macros. Does care need be
taken to wrap the code blocks in equivalent macros/ifdefs? Or are the
NSK_JNI_VERIFY macros always-on in these tests (and essentially
pointless)?
/Claes
Post by JC Beyler
Hi all,
I worked on updating my script and handling more assignments so I
redid a second pass on the same files to get all the cases. Now, on
those files the regex "if.* = " no longer shows any cases we would
want to fix.
Webrev: http://cr.openjdk.java.net/~jcbeyler/8213003/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8213003
I tested this on my dev machine by passing all the tests that were modified.
Thanks!
Jc
s***@oracle.com
2018-10-31 18:30:18 UTC
Permalink
It prints the FILE/LINE which is enough to identify the error point.
But I agree that doing the assignments within the NSK_JNI_VERIFY was
intentional as it gives more context.
From the other hand it make the code ugly and less readable.
Not sure, what direction is better to go.

Thanks,
Serguei
Post by Chris Plummer
Hi Claes,
It's good that you brought this up. NSK_JNI_VERIFY is always "on", but
moving the assignment outside of it does slightly change the behavior
/**
 * Execute action with JNI call, check result for true and
 * pending exception and complain error if required.
 * Also trace action execution if tracing mode enabled.
 */
#define NSK_JNI_VERIFY(jni, action)  \
(nsk_ltrace(NSK_TRACE_BEFORE,__FILE__,__LINE__,"%s\n",#action), \
nsk_jni_lverify(NSK_TRUE,jni,action,__FILE__,__LINE__,"%s\n",#action))
So you will no longer see the JNI call in the trace or error output.
You will just see the comparison to the JNI result, which gives no
context as to the source of the result. So I'm now starting to think
that doing the assignments within the NSK_JNI_VERIFY was intentional
so we would see the JNI call in the trace output.
Chris
Post by Claes Redestad
Hi,
http://cr.openjdk.java.net/%7Ejcbeyler/8213003/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassLoaderClasses/clsldrclss002/clsldrclss002.cpp.udiff.html
     if (!NSK_JNI_VERIFY(jni, (testedFieldID =
             jni->GetStaticFieldID(testedClass, FIELD_NAME,
FIELD_SIGNATURE)) != NULL))
I'd note that with some of your changes here you're unconditionally
executing logic outside of NSK_JNI_VERIFY macros. Does care need be
taken to wrap the code blocks in equivalent macros/ifdefs? Or are the
NSK_JNI_VERIFY macros always-on in these tests (and essentially
pointless)?
/Claes
Post by JC Beyler
Hi all,
I worked on updating my script and handling more assignments so I
redid a second pass on the same files to get all the cases. Now, on
those files the regex "if.* = " no longer shows any cases we would
want to fix.
Webrev: http://cr.openjdk.java.net/~jcbeyler/8213003/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8213003
I tested this on my dev machine by passing all the tests that were modified.
Thanks!
Jc
Chris Plummer
2018-10-31 18:52:38 UTC
Permalink
Post by s***@oracle.com
It prints the FILE/LINE which is enough to identify the error point.
Yes, but it also prints the JNI calls.
Post by s***@oracle.com
But I agree that doing the assignments within the NSK_JNI_VERIFY was
intentional as it gives more context.
From the other hand it make the code ugly and less readable.
Not sure, what direction is better to go.
Agreed. Somewhat of a tossup now as to whether or not this change should
be done. I'm fine either way.

Chris
Post by s***@oracle.com
Thanks,
Serguei
Post by Chris Plummer
Hi Claes,
It's good that you brought this up. NSK_JNI_VERIFY is always "on",
but moving the assignment outside of it does slightly change the
/**
 * Execute action with JNI call, check result for true and
 * pending exception and complain error if required.
 * Also trace action execution if tracing mode enabled.
 */
#define NSK_JNI_VERIFY(jni, action)  \
(nsk_ltrace(NSK_TRACE_BEFORE,__FILE__,__LINE__,"%s\n",#action), \
nsk_jni_lverify(NSK_TRUE,jni,action,__FILE__,__LINE__,"%s\n",#action))
So you will no longer see the JNI call in the trace or error output.
You will just see the comparison to the JNI result, which gives no
context as to the source of the result. So I'm now starting to think
that doing the assignments within the NSK_JNI_VERIFY was intentional
so we would see the JNI call in the trace output.
Chris
Post by Claes Redestad
Hi,
http://cr.openjdk.java.net/%7Ejcbeyler/8213003/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassLoaderClasses/clsldrclss002/clsldrclss002.cpp.udiff.html
     if (!NSK_JNI_VERIFY(jni, (testedFieldID =
             jni->GetStaticFieldID(testedClass, FIELD_NAME,
FIELD_SIGNATURE)) != NULL))
I'd note that with some of your changes here you're unconditionally
executing logic outside of NSK_JNI_VERIFY macros. Does care need be
taken to wrap the code blocks in equivalent macros/ifdefs? Or are
the NSK_JNI_VERIFY macros always-on in these tests (and essentially
pointless)?
/Claes
Post by JC Beyler
Hi all,
I worked on updating my script and handling more assignments so I
redid a second pass on the same files to get all the cases. Now, on
those files the regex "if.* = " no longer shows any cases we would
want to fix.
Webrev: http://cr.openjdk.java.net/~jcbeyler/8213003/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8213003
I tested this on my dev machine by passing all the tests that were modified.
Thanks!
Jc
JC Beyler
2018-10-31 20:54:55 UTC
Permalink
@Claes: you are right, I did not do a grep such as "if.* =$"; by adding the
space instead of the $, I missed a few :)

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:
- Line/File + JNI call?

I ask because I'd like to deprecate the NSK_VERIFY macros but the LINE/FILE
is a bit annoying to deprecate.

I'd rather be able to remove them entirely but we could do an alternative
and migrate the NSK_VERIFY to:

testedFieldID = jni->GetStaticFieldID(testedClass, FIELD_NAME,
FIELD_SIGNATURE);
if (!testedFieldID) {

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.

If lines and filenames are really important, then we could do something
like:
testedFieldID = NSK_TRACE(jni->GetStaticFieldID(testedClass,
FIELD_NAME, FIELD_SIGNATURE));
if (!testedFieldID) {

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

Thanks,
Jc
Post by Chris Plummer
Post by s***@oracle.com
It prints the FILE/LINE which is enough to identify the error point.
Yes, but it also prints the JNI calls.
Post by s***@oracle.com
But I agree that doing the assignments within the NSK_JNI_VERIFY was
intentional as it gives more context.
From the other hand it make the code ugly and less readable.
Not sure, what direction is better to go.
Agreed. Somewhat of a tossup now as to whether or not this change should
be done. I'm fine either way.
Chris
Post by s***@oracle.com
Thanks,
Serguei
Post by Chris Plummer
Hi Claes,
It's good that you brought this up. NSK_JNI_VERIFY is always "on",
but moving the assignment outside of it does slightly change the
/**
* Execute action with JNI call, check result for true and
* pending exception and complain error if required.
* Also trace action execution if tracing mode enabled.
*/
#define NSK_JNI_VERIFY(jni, action) \
(nsk_ltrace(NSK_TRACE_BEFORE,__FILE__,__LINE__,"%s\n",#action), \
nsk_jni_lverify(NSK_TRUE,jni,action,__FILE__,__LINE__,"%s\n",#action))
So you will no longer see the JNI call in the trace or error output.
You will just see the comparison to the JNI result, which gives no
context as to the source of the result. So I'm now starting to think
that doing the assignments within the NSK_JNI_VERIFY was intentional
so we would see the JNI call in the trace output.
Chris
Post by Claes Redestad
Hi,
http://cr.openjdk.java.net/%7Ejcbeyler/8213003/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassLoaderClasses/clsldrclss002/clsldrclss002.cpp.udiff.html
Post by s***@oracle.com
Post by Chris Plummer
Post by Claes Redestad
if (!NSK_JNI_VERIFY(jni, (testedFieldID =
jni->GetStaticFieldID(testedClass, FIELD_NAME,
FIELD_SIGNATURE)) != NULL))
I'd note that with some of your changes here you're unconditionally
executing logic outside of NSK_JNI_VERIFY macros. Does care need be
taken to wrap the code blocks in equivalent macros/ifdefs? Or are
the NSK_JNI_VERIFY macros always-on in these tests (and essentially
pointless)?
/Claes
Post by JC Beyler
Hi all,
I worked on updating my script and handling more assignments so I
redid a second pass on the same files to get all the cases. Now, on
those files the regex "if.* = " no longer shows any cases we would
want to fix.
Webrev: http://cr.openjdk.java.net/~jcbeyler/8213003/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8213003
I tested this on my dev machine by passing all the tests that were modified.
Thanks!
Jc
--
Thanks,
Jc
JC Beyler
2018-11-01 16:53:14 UTC
Permalink
Hi all,

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.

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:

if (!NSK_JNI_VERIFY(jni, (debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL)) {
nsk_jvmti_setFailStatus();
return;
}

Without a change and with the verbose system set up for the test, the error
message is:

The following fake exception stacktrace is for failuire analysis.
nsk.share.Fake_Exception_for_RULE_Creation: (settag001.cpp:65)
(debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL
at nsk_lvcomplain(nsk_tools.cpp:172)
# ERROR: settag001.cpp, 65: (debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL
# verified JNI assertion is FALSE
# ERROR: agent_tools.cpp, 282: Debuggee status sync aborted because agent
thread has finished

(Note the typo for failuire, I created JDK-8213246 to fix it).

With my proposed change to remove the assignment, the code becomes:
debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME);
if (!NSK_JNI_VERIFY(jni, debuggeeClass != NULL)) {
nsk_jvmti_setFailStatus();
return;
}

The following fake exception stacktrace is for failuire analysis.
nsk.share.Fake_Exception_for_RULE_Creation: (settag001.cpp:66) debugeeClass
!= NULL
at nsk_lvcomplain(nsk_tools.cpp:172)
# ERROR: settag001.cpp, 66: debugeeClass != NULL
# verified JNI assertion is FALSE
# ERROR: agent_tools.cpp, 282: Debuggee status sync aborted because agent
thread has finished
STDERR:

In this case, we do lose that the failure seems to have happened in
FindClass, we need to look at the code.

However, the ExceptionJNIWrapper that I implemented a little while ago will
provide this information:
FATAL ERROR in native method: FindClass : Return is NULL

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
allow two optional arguments and do this:

...
// For tracing purposes.
#define TRACE_JNI_CALL __LINE__, __FILE__
...

ExceptionCheckingJniEnvPtr jni_wrapper(jni);
...
debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);

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:

FATAL ERROR in native method: JNI method FindClass : Return is NULL from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69

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
then look like something like:

JNI method FindClass was called from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
with these parameters:
"nsk/jvmti/SetTag/settag001"
FATAL ERROR in native method: JNI method FindClass : Return is NULL from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69

Let me know what you think.

Thanks,
Jc
Post by JC Beyler
@Claes: you are right, I did not do a grep such as "if.* =$"; by adding
the space instead of the $, I missed a few :)
I've been meaning to ask if we could deprecate these anyway. Are these
- Line/File + JNI call?
I ask because I'd like to deprecate the NSK_VERIFY macros but the
LINE/FILE is a bit annoying to deprecate.
I'd rather be able to remove them entirely but we could do an alternative
testedFieldID = jni->GetStaticFieldID(testedClass, FIELD_NAME,
FIELD_SIGNATURE);
if (!testedFieldID) {
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.
If lines and filenames are really important, then we could do something
testedFieldID = NSK_TRACE(jni->GetStaticFieldID(testedClass,
FIELD_NAME, FIELD_SIGNATURE));
if (!testedFieldID) {
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).
Thanks,
Jc
Post by Chris Plummer
Post by s***@oracle.com
It prints the FILE/LINE which is enough to identify the error point.
Yes, but it also prints the JNI calls.
Post by s***@oracle.com
But I agree that doing the assignments within the NSK_JNI_VERIFY was
intentional as it gives more context.
From the other hand it make the code ugly and less readable.
Not sure, what direction is better to go.
Agreed. Somewhat of a tossup now as to whether or not this change should
be done. I'm fine either way.
Chris
Post by s***@oracle.com
Thanks,
Serguei
Post by Chris Plummer
Hi Claes,
It's good that you brought this up. NSK_JNI_VERIFY is always "on",
but moving the assignment outside of it does slightly change the
/**
* Execute action with JNI call, check result for true and
* pending exception and complain error if required.
* Also trace action execution if tracing mode enabled.
*/
#define NSK_JNI_VERIFY(jni, action) \
(nsk_ltrace(NSK_TRACE_BEFORE,__FILE__,__LINE__,"%s\n",#action), \
nsk_jni_lverify(NSK_TRUE,jni,action,__FILE__,__LINE__,"%s\n",#action))
So you will no longer see the JNI call in the trace or error output.
You will just see the comparison to the JNI result, which gives no
context as to the source of the result. So I'm now starting to think
that doing the assignments within the NSK_JNI_VERIFY was intentional
so we would see the JNI call in the trace output.
Chris
Post by Claes Redestad
Hi,
http://cr.openjdk.java.net/%7Ejcbeyler/8213003/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassLoaderClasses/clsldrclss002/clsldrclss002.cpp.udiff.html
Post by s***@oracle.com
Post by Chris Plummer
Post by Claes Redestad
if (!NSK_JNI_VERIFY(jni, (testedFieldID =
jni->GetStaticFieldID(testedClass, FIELD_NAME,
FIELD_SIGNATURE)) != NULL))
I'd note that with some of your changes here you're unconditionally
executing logic outside of NSK_JNI_VERIFY macros. Does care need be
taken to wrap the code blocks in equivalent macros/ifdefs? Or are
the NSK_JNI_VERIFY macros always-on in these tests (and essentially
pointless)?
/Claes
Post by JC Beyler
Hi all,
I worked on updating my script and handling more assignments so I
redid a second pass on the same files to get all the cases. Now, on
those files the regex "if.* = " no longer shows any cases we would
want to fix.
Webrev: http://cr.openjdk.java.net/~jcbeyler/8213003/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8213003
I tested this on my dev machine by passing all the tests that were modified.
Thanks!
Jc
--
Thanks,
Jc
--
Thanks,
Jc
Chris Plummer
2018-11-01 19:01:31 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/1/18 9:53 AM, JC Beyler wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAF9BGBzymWfTXycWXRtk5AcWnvXyOCN3mnp0hSCa_LD=***@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=utf-8">
<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-&gt;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-&gt;FindClass(DEBUGEE_CLASS_NAME)) !=
NULL</div>
<div><span style="white-space:pre"> </span>at
nsk_lvcomplain(nsk_tools.cpp:172)</div>
<div># ERROR: settag001.cpp, 65:
(debugeeClass =
jni-&gt;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"
cite="mid:CAF9BGBzymWfTXycWXRtk5AcWnvXyOCN3mnp0hSCa_LD=***@mail.gmail.com">
<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-&gt;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"> </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-&gt;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"
cite="mid:CAF9BGBzymWfTXycWXRtk5AcWnvXyOCN3mnp0hSCa_LD=***@mail.gmail.com">
<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"
cite="mid:CAF9BGBzymWfTXycWXRtk5AcWnvXyOCN3mnp0hSCa_LD=***@mail.gmail.com">
<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-&gt;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"> </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"
cite="mid:CAF9BGBzymWfTXycWXRtk5AcWnvXyOCN3mnp0hSCa_LD=***@mail.gmail.com">
<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 &lt;<a
href="mailto:***@google.com" target="_blank"
moz-do-not-send="true">***@google.com</a>&gt; 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-&gt;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-&gt;GetStaticFieldID(testedClass,
FIELD_NAME, FIELD_SIGNATURE));</div>
<div>    if (!testedFieldID) {<br>
</div>
<br
class="m_-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 &lt;<a href="mailto:***@oracle.com"
target="_blank" moz-do-not-send="true">***@oracle.com</a>&gt;
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:***@oracle.com" target="_blank"
moz-do-not-send="true">***@oracle.com</a>
wrote:<br>
&gt; It prints the FILE/LINE which is enough to identify
the error point.<br>
Yes, but it also prints the JNI calls.<br>
&gt; But I agree that doing the assignments within the
NSK_JNI_VERIFY was <br>
&gt; intentional as it gives more context.<br>
&gt; From the other hand it make the code ugly and less
readable.<br>
&gt; 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>
&gt;<br>
&gt; Thanks,<br>
&gt; Serguei<br>
&gt;<br>
&gt;<br>
&gt; On 10/31/18 11:21, Chris Plummer wrote:<br>
&gt;&gt; Hi Claes,<br>
&gt;&gt;<br>
&gt;&gt; It's good that you brought this up.
NSK_JNI_VERIFY is always "on", <br>
&gt;&gt; but moving the assignment outside of it does
slightly change the <br>
&gt;&gt; behavior of the macro (although the code still
executes "correctly"):<br>
&gt;&gt;<br>
&gt;&gt; /**<br>
&gt;&gt;  * Execute action with JNI call, check result for
true and<br>
&gt;&gt;  * pending exception and complain error if
required.<br>
&gt;&gt;  * Also trace action execution if tracing mode
enabled.<br>
&gt;&gt;  */<br>
&gt;&gt; #define NSK_JNI_VERIFY(jni, action)  \<br>
&gt;&gt;
(nsk_ltrace(NSK_TRACE_BEFORE,__FILE__,__LINE__,"%s\n",#action),
\<br>
&gt;&gt;
nsk_jni_lverify(NSK_TRUE,jni,action,__FILE__,__LINE__,"%s\n",#action))<br>
&gt;&gt;<br>
&gt;&gt; So you will no longer see the JNI call in the
trace or error output. <br>
&gt;&gt; You will just see the comparison to the JNI
result, which gives no <br>
&gt;&gt; context as to the source of the result. So I'm
now starting to think <br>
&gt;&gt; that doing the assignments within the
NSK_JNI_VERIFY was intentional <br>
&gt;&gt; so we would see the JNI call in the trace output.<br>
&gt;&gt;<br>
&gt;&gt; Chris<br>
&gt;&gt;<br>
&gt;&gt; On 10/31/18 3:16 AM, Claes Redestad wrote:<br>
&gt;&gt;&gt; Hi,<br>
&gt;&gt;&gt;<br>
&gt;&gt;&gt; here's a case that your script seems to miss:<br>
&gt;&gt;&gt;<br>
&gt;&gt;&gt; <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>
&gt;&gt;&gt;<br>
&gt;&gt;&gt;<br>
&gt;&gt;&gt;      if (!NSK_JNI_VERIFY(jni, (testedFieldID
=<br>
&gt;&gt;&gt;             
jni-&gt;GetStaticFieldID(testedClass, FIELD_NAME, <br>
&gt;&gt;&gt; FIELD_SIGNATURE)) != NULL))<br>
&gt;&gt;&gt;<br>
&gt;&gt;&gt; I'd note that with some of your changes here
you're unconditionally <br>
&gt;&gt;&gt; executing logic outside of NSK_JNI_VERIFY
macros. Does care need be <br>
&gt;&gt;&gt; taken to wrap the code blocks in equivalent
macros/ifdefs? Or are <br>
&gt;&gt;&gt; the NSK_JNI_VERIFY macros always-on in these
tests (and essentially <br>
&gt;&gt;&gt; pointless)?<br>
&gt;&gt;&gt;<br>
&gt;&gt;&gt; /Claes<br>
&gt;&gt;&gt;<br>
&gt;&gt;&gt; On 2018-10-31 05:42, JC Beyler wrote:<br>
&gt;&gt;&gt;&gt; Hi all,<br>
&gt;&gt;&gt;&gt;<br>
&gt;&gt;&gt;&gt; I worked on updating my script and
handling more assignments so I <br>
&gt;&gt;&gt;&gt; redid a second pass on the same files to
get all the cases. Now, on <br>
&gt;&gt;&gt;&gt; those files the regex "if.* = " no longer
shows any cases we would <br>
&gt;&gt;&gt;&gt; want to fix.<br>
&gt;&gt;&gt;&gt;<br>
&gt;&gt;&gt;&gt; Could I get a review for this webrev:<br>
&gt;&gt;&gt;&gt; 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>
&gt;&gt;&gt;&gt; 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>
&gt;&gt;&gt;&gt;<br>
&gt;&gt;&gt;&gt; I tested this on my dev machine by
passing all the tests that were <br>
&gt;&gt;&gt;&gt; modified.<br>
&gt;&gt;&gt;&gt;<br>
&gt;&gt;&gt;&gt; Thanks!<br>
&gt;&gt;&gt;&gt; Jc<br>
&gt;&gt;&gt;<br>
&gt;&gt;<br>
&gt;&gt;<br>
&gt;<br>
<br>
<br>
</blockquote>
</div>
<br clear="all">
<div><br>
</div>
-- <br>
<div dir="ltr"
class="m_-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_-9088134372540873412gmail_signature"
data-smartmail="gmail_signature">
<div dir="ltr">
<div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</blockquote>
<p><br>
</p>
</body>
</html>
JC Beyler
2018-11-01 20:03:10 UTC
Permalink
Hi Chris,

Thanks for going over my email :)

So I skipped over the tracing part for clarity and striving to be brief.
But if we can accept:
debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);

and use a mechanism such as
debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);
Post by JC Beyler
JNI FindClass with the following parameters from
"nsk/jvmti/SetTag/settag001"
...
<< JNI FindClass from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69:

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:

if (!NSK_JNI_VERIFY(jni, (debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL)) {
nsk_jvmti_setFailStatus();
return;
}


to:

#define TRACE_JNI_CALL __LINE__, __FILE__
...

ExceptionCheckingJniEnvPtr jni_wrapper(jni);
...
debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);

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.

Thanks,
Jc
Post by JC Beyler
Hi all,
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.
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
if (!NSK_JNI_VERIFY(jni, (debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL)) {
nsk_jvmti_setFailStatus();
return;
}
Without a change and with the verbose system set up for the test, the
The following fake exception stacktrace is for failuire analysis.
nsk.share.Fake_Exception_for_RULE_Creation: (settag001.cpp:65)
(debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL
at nsk_lvcomplain(nsk_tools.cpp:172)
# ERROR: settag001.cpp, 65: (debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL
# verified JNI assertion is FALSE
# ERROR: agent_tools.cpp, 282: Debuggee status sync aborted because agent
thread has finished
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.
#define NSK_JNI_VERIFY(jni, action) \
(nsk_ltrace(NSK_TRACE_BEFORE,__FILE__,__LINE__,"%s\n",#action), \
nsk_jni_lverify(NSK_TRUE,jni,action,__FILE__,__LINE__,"%s\n",#action))
(Note the typo for failuire, I created JDK-8213246 to fix it).
debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME);
if (!NSK_JNI_VERIFY(jni, debuggeeClass != NULL)) {
nsk_jvmti_setFailStatus();
return;
}
The following fake exception stacktrace is for failuire analysis.
nsk.share.Fake_Exception_for_RULE_Creation: (settag001.cpp:66)
debugeeClass != NULL
at nsk_lvcomplain(nsk_tools.cpp:172)
# ERROR: settag001.cpp, 66: debugeeClass != NULL
# verified JNI assertion is FALSE
# ERROR: agent_tools.cpp, 282: Debuggee status sync aborted because agent
thread has finished
In this case, we do lose that the failure seems to have happened in
FindClass, we need to look at the code.
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.
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.
In any case, a solution would be to have a wrapper script like
debugeeClass =
NSK_JNI_WRAPPER(jni->FindClass(DEBUGEE_CLASS_NAME));
if (!NSK_JNI_VERIFY(jni, debuggeeClass != NULL)) {
nsk_jvmti_setFailStatus();
return;
}
And NSK_JNI_WRAPPER would just to the tracing and execute the passed in
action.
However, the ExceptionJNIWrapper that I implemented a little while ago
FATAL ERROR in native method: FindClass : Return is NULL
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.
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
...
// For tracing purposes.
#define TRACE_JNI_CALL __LINE__, __FILE__
...
ExceptionCheckingJniEnvPtr jni_wrapper(jni);
...
debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);
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
FATAL ERROR in native method: JNI method FindClass : Return is NULL from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
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
JNI method FindClass was called from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
"nsk/jvmti/SetTag/settag001"
FATAL ERROR in native method: JNI method FindClass : Return is NULL from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
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.
thanks,
Chris
Let me know what you think.
Thanks,
Jc
@Claes: you are right, I did not do a grep such as "if.* =$"; by adding
the space instead of the $, I missed a few :)
I've been meaning to ask if we could deprecate these anyway. Are these
- Line/File + JNI call?
I ask because I'd like to deprecate the NSK_VERIFY macros but the
LINE/FILE is a bit annoying to deprecate.
I'd rather be able to remove them entirely but we could do an alternative
testedFieldID = jni->GetStaticFieldID(testedClass, FIELD_NAME,
FIELD_SIGNATURE);
if (!testedFieldID) {
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.
If lines and filenames are really important, then we could do something
testedFieldID = NSK_TRACE(jni->GetStaticFieldID(testedClass,
FIELD_NAME, FIELD_SIGNATURE));
if (!testedFieldID) {
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).
Thanks,
Jc
Post by Chris Plummer
Post by s***@oracle.com
It prints the FILE/LINE which is enough to identify the error point.
Yes, but it also prints the JNI calls.
Post by s***@oracle.com
But I agree that doing the assignments within the NSK_JNI_VERIFY was
intentional as it gives more context.
From the other hand it make the code ugly and less readable.
Not sure, what direction is better to go.
Agreed. Somewhat of a tossup now as to whether or not this change should
be done. I'm fine either way.
Chris
Post by s***@oracle.com
Thanks,
Serguei
Post by Chris Plummer
Hi Claes,
It's good that you brought this up. NSK_JNI_VERIFY is always "on",
but moving the assignment outside of it does slightly change the
/**
* Execute action with JNI call, check result for true and
* pending exception and complain error if required.
* Also trace action execution if tracing mode enabled.
*/
#define NSK_JNI_VERIFY(jni, action) \
(nsk_ltrace(NSK_TRACE_BEFORE,__FILE__,__LINE__,"%s\n",#action), \
nsk_jni_lverify(NSK_TRUE,jni,action,__FILE__,__LINE__,"%s\n",#action))
So you will no longer see the JNI call in the trace or error output.
You will just see the comparison to the JNI result, which gives no
context as to the source of the result. So I'm now starting to think
that doing the assignments within the NSK_JNI_VERIFY was intentional
so we would see the JNI call in the trace output.
Chris
Post by Claes Redestad
Hi,
http://cr.openjdk.java.net/%7Ejcbeyler/8213003/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassLoaderClasses/clsldrclss002/clsldrclss002.cpp.udiff.html
Post by s***@oracle.com
Post by Chris Plummer
Post by Claes Redestad
if (!NSK_JNI_VERIFY(jni, (testedFieldID =
jni->GetStaticFieldID(testedClass, FIELD_NAME,
FIELD_SIGNATURE)) != NULL))
I'd note that with some of your changes here you're unconditionally
executing logic outside of NSK_JNI_VERIFY macros. Does care need be
taken to wrap the code blocks in equivalent macros/ifdefs? Or are
the NSK_JNI_VERIFY macros always-on in these tests (and essentially
pointless)?
/Claes
Post by JC Beyler
Hi all,
I worked on updating my script and handling more assignments so I
redid a second pass on the same files to get all the cases. Now, on
those files the regex "if.* = " no longer shows any cases we would
want to fix.
Webrev: http://cr.openjdk.java.net/~jcbeyler/8213003/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8213003
I tested this on my dev machine by passing all the tests that were modified.
Thanks!
Jc
--
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
Chris Plummer
2018-11-01 20:24:16 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 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"
cite="mid:CAF9BGBy6S10o=kUfZv+RcbgC+***@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=utf-8">
<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-&gt;FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);</div>
</div>
<div><br>
</div>
<div>and use a mechanism such as </div>
<div>           debugeeClass =
jni-&gt;FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);</div> <div><br> </div> <div>it is actually easy to print out things like:</div> <div>&gt;&gt; JNI FindClass with the following
parameters from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69:</div> <div>&gt;&gt; <span style="white-space:pre-wrap"> </span>"nsk/jvmti/SetTag/settag001"<br>
</div>
<div>...</div>
<div>
<div>&lt;&lt; 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="gmail-im"
style="color:rgb(80,0,80)">
<div>  if
(!NSK_JNI_VERIFY(jni,
(debugeeClass =<br>
</div>
<div>
<div>                   
jni-&gt;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-&gt;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
&lt;<a href="mailto:***@oracle.com"
moz-do-not-send="true">***@oracle.com</a>&gt;
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_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-&gt;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-&gt;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-&gt;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-&gt;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-&gt;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-&gt;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
&lt;<a href="mailto:***@google.com"
target="_blank" moz-do-not-send="true">***@google.com</a>&gt;
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-&gt;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-&gt;GetStaticFieldID(testedClass,
FIELD_NAME, FIELD_SIGNATURE));</div>
<div>    if (!testedFieldID) {<br>
</div>
<br
class="m_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 &lt;<a
href="mailto:***@oracle.com"
target="_blank" moz-do-not-send="true">***@oracle.com</a>&gt;
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:***@oracle.com"
target="_blank" moz-do-not-send="true">***@oracle.com</a>
wrote:<br>
&gt; It prints the FILE/LINE which is enough to
identify the error point.<br>
Yes, but it also prints the JNI calls.<br>
&gt; But I agree that doing the assignments within
the NSK_JNI_VERIFY was <br>
&gt; intentional as it gives more context.<br>
&gt; From the other hand it make the code ugly and
less readable.<br>
&gt; 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>
&gt;<br>
&gt; Thanks,<br>
&gt; Serguei<br>
&gt;<br>
&gt;<br>
&gt; On 10/31/18 11:21, Chris Plummer wrote:<br>
&gt;&gt; Hi Claes,<br>
&gt;&gt;<br>
&gt;&gt; It's good that you brought this up.
NSK_JNI_VERIFY is always "on", <br>
&gt;&gt; but moving the assignment outside of it
does slightly change the <br>
&gt;&gt; behavior of the macro (although the code
still executes "correctly"):<br>
&gt;&gt;<br>
&gt;&gt; /**<br>
&gt;&gt;  * Execute action with JNI call, check
result for true and<br>
&gt;&gt;  * pending exception and complain error
if required.<br>
&gt;&gt;  * Also trace action execution if tracing
mode enabled.<br>
&gt;&gt;  */<br>
&gt;&gt; #define NSK_JNI_VERIFY(jni, action)  \<br>
&gt;&gt;
(nsk_ltrace(NSK_TRACE_BEFORE,__FILE__,__LINE__,"%s\n",#action),
\<br>
&gt;&gt;
nsk_jni_lverify(NSK_TRUE,jni,action,__FILE__,__LINE__,"%s\n",#action))<br>
&gt;&gt;<br>
&gt;&gt; So you will no longer see the JNI call in
the trace or error output. <br>
&gt;&gt; You will just see the comparison to the
JNI result, which gives no <br>
&gt;&gt; context as to the source of the result.
So I'm now starting to think <br>
&gt;&gt; that doing the assignments within the
NSK_JNI_VERIFY was intentional <br>
&gt;&gt; so we would see the JNI call in the trace
output.<br>
&gt;&gt;<br>
&gt;&gt; Chris<br>
&gt;&gt;<br>
&gt;&gt; On 10/31/18 3:16 AM, Claes Redestad
wrote:<br>
&gt;&gt;&gt; Hi,<br>
&gt;&gt;&gt;<br>
&gt;&gt;&gt; here's a case that your script seems
to miss:<br>
&gt;&gt;&gt;<br>
&gt;&gt;&gt; <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>
&gt;&gt;&gt;<br>
&gt;&gt;&gt;<br>
&gt;&gt;&gt;      if (!NSK_JNI_VERIFY(jni,
(testedFieldID =<br>
&gt;&gt;&gt;             
jni-&gt;GetStaticFieldID(testedClass, FIELD_NAME,
<br>
&gt;&gt;&gt; FIELD_SIGNATURE)) != NULL))<br>
&gt;&gt;&gt;<br>
&gt;&gt;&gt; I'd note that with some of your
changes here you're unconditionally <br>
&gt;&gt;&gt; executing logic outside of
NSK_JNI_VERIFY macros. Does care need be <br>
&gt;&gt;&gt; taken to wrap the code blocks in
equivalent macros/ifdefs? Or are <br>
&gt;&gt;&gt; the NSK_JNI_VERIFY macros always-on
in these tests (and essentially <br>
&gt;&gt;&gt; pointless)?<br>
&gt;&gt;&gt;<br>
&gt;&gt;&gt; /Claes<br>
&gt;&gt;&gt;<br>
&gt;&gt;&gt; On 2018-10-31 05:42, JC Beyler wrote:<br>
&gt;&gt;&gt;&gt; Hi all,<br>
&gt;&gt;&gt;&gt;<br>
&gt;&gt;&gt;&gt; I worked on updating my script
and handling more assignments so I <br>
&gt;&gt;&gt;&gt; redid a second pass on the same
files to get all the cases. Now, on <br>
&gt;&gt;&gt;&gt; those files the regex "if.* = "
no longer shows any cases we would <br>
&gt;&gt;&gt;&gt; want to fix.<br>
&gt;&gt;&gt;&gt;<br>
&gt;&gt;&gt;&gt; Could I get a review for this
webrev:<br>
&gt;&gt;&gt;&gt; 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>
&gt;&gt;&gt;&gt; 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>
&gt;&gt;&gt;&gt;<br>
&gt;&gt;&gt;&gt; I tested this on my dev machine
by passing all the tests that were <br>
&gt;&gt;&gt;&gt; modified.<br>
&gt;&gt;&gt;&gt;<br>
&gt;&gt;&gt;&gt; Thanks!<br>
&gt;&gt;&gt;&gt; Jc<br>
&gt;&gt;&gt;<br>
&gt;&gt;<br>
&gt;&gt;<br>
&gt;<br>
<br>
<br>
</blockquote>
</div>
<br clear="all">
<div><br>
</div>
-- <br>
<div dir="ltr"
class="m_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_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="gmail_signature"
data-smartmail="gmail_signature">
<div dir="ltr">
<div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</blockquote>
<p><br>
</p>
</body>
</html>
JC Beyler
2018-11-02 17:44:20 UTC
Permalink
Hi Chris,

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:
http://cr.openjdk.java.net/~jcbeyler/exception/

Basically:
- 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

http://cr.openjdk.java.net/~jcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp.udiff.html
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

- 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:

http://cr.openjdk.java.net/~jcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.hpp.udiff.html

- Finally, the real change comes from here:

http://cr.openjdk.java.net/~jcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.cpp.udiff.html

Where we do this:
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
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
c) at JNIVerifier construction, we now can call the print parameter
methods to pass the values of the call to be printed out
- 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
d) the JNI wrapper methods get augmented by the line/file
parameters which default to -1 and 0
e) the constructor is now also passing in the JNI method parameters

It's mostly boiler plate code but allows us to go from:
- if (!NSK_JNI_VERIFY(jni, (debugeeClass =
- jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL)) {
+ debugeeClass = jni_wrapper->FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);
+ if (debugeeClass != NULL) {
Post by Chris Plummer
Calling JNI method FindClass from
/usr/local/google/home/jcbeyler/mercurial/jdk12-verify/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
nsk/jvmti/SetTag/settag001
<< Called JNI method FindClass from
/usr/local/google/home/jcbeyler/mercurial/jdk12-verify/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
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

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.

Let me know what you think,
Jc

d) at JNIVerifier destruction, we can print out the "called the
method"
Post by Chris Plummer
Hi JC,
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.
thanks,
Chris
Hi Chris,
Thanks for going over my email :)
So I skipped over the tracing part for clarity and striving to be brief.
debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);
and use a mechanism such as
debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);
JNI FindClass with the following parameters from
"nsk/jvmti/SetTag/settag001"
...
<< JNI FindClass from
Before the actual call to the method, allowing to have the full
if (!NSK_JNI_VERIFY(jni, (debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL)) {
nsk_jvmti_setFailStatus();
return;
}
#define TRACE_JNI_CALL __LINE__, __FILE__
...
ExceptionCheckingJniEnvPtr jni_wrapper(jni);
...
debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);
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.
Thanks,
Jc
Hi all,
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.
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
if (!NSK_JNI_VERIFY(jni, (debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL)) {
nsk_jvmti_setFailStatus();
return;
}
Without a change and with the verbose system set up for the test, the
The following fake exception stacktrace is for failuire analysis.
nsk.share.Fake_Exception_for_RULE_Creation: (settag001.cpp:65)
(debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL
at nsk_lvcomplain(nsk_tools.cpp:172)
# ERROR: settag001.cpp, 65: (debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL
# verified JNI assertion is FALSE
# ERROR: agent_tools.cpp, 282: Debuggee status sync aborted because agent
thread has finished
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.
#define NSK_JNI_VERIFY(jni, action) \
(nsk_ltrace(NSK_TRACE_BEFORE,__FILE__,__LINE__,"%s\n",#action), \
nsk_jni_lverify(NSK_TRUE,jni,action,__FILE__,__LINE__,"%s\n",#action))
(Note the typo for failuire, I created JDK-8213246 to fix it).
debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME);
if (!NSK_JNI_VERIFY(jni, debuggeeClass != NULL)) {
nsk_jvmti_setFailStatus();
return;
}
The following fake exception stacktrace is for failuire analysis.
nsk.share.Fake_Exception_for_RULE_Creation: (settag001.cpp:66)
debugeeClass != NULL
at nsk_lvcomplain(nsk_tools.cpp:172)
# ERROR: settag001.cpp, 66: debugeeClass != NULL
# verified JNI assertion is FALSE
# ERROR: agent_tools.cpp, 282: Debuggee status sync aborted because agent
thread has finished
In this case, we do lose that the failure seems to have happened in
FindClass, we need to look at the code.
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.
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.
In any case, a solution would be to have a wrapper script like
debugeeClass =
NSK_JNI_WRAPPER(jni->FindClass(DEBUGEE_CLASS_NAME));
if (!NSK_JNI_VERIFY(jni, debuggeeClass != NULL)) {
nsk_jvmti_setFailStatus();
return;
}
And NSK_JNI_WRAPPER would just to the tracing and execute the passed in
action.
However, the ExceptionJNIWrapper that I implemented a little while ago
FATAL ERROR in native method: FindClass : Return is NULL
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.
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
...
// For tracing purposes.
#define TRACE_JNI_CALL __LINE__, __FILE__
...
ExceptionCheckingJniEnvPtr jni_wrapper(jni);
...
debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);
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
FATAL ERROR in native method: JNI method FindClass : Return is NULL from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
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
JNI method FindClass was called from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
"nsk/jvmti/SetTag/settag001"
FATAL ERROR in native method: JNI method FindClass : Return is NULL from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
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.
thanks,
Chris
Let me know what you think.
Thanks,
Jc
@Claes: you are right, I did not do a grep such as "if.* =$"; by adding
the space instead of the $, I missed a few :)
I've been meaning to ask if we could deprecate these anyway. Are these
- Line/File + JNI call?
I ask because I'd like to deprecate the NSK_VERIFY macros but the
LINE/FILE is a bit annoying to deprecate.
I'd rather be able to remove them entirely but we could do an
testedFieldID = jni->GetStaticFieldID(testedClass, FIELD_NAME,
FIELD_SIGNATURE);
if (!testedFieldID) {
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.
If lines and filenames are really important, then we could do something
testedFieldID = NSK_TRACE(jni->GetStaticFieldID(testedClass,
FIELD_NAME, FIELD_SIGNATURE));
if (!testedFieldID) {
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).
Thanks,
Jc
Post by Chris Plummer
Post by s***@oracle.com
It prints the FILE/LINE which is enough to identify the error point.
Yes, but it also prints the JNI calls.
Post by s***@oracle.com
But I agree that doing the assignments within the NSK_JNI_VERIFY was
intentional as it gives more context.
From the other hand it make the code ugly and less readable.
Not sure, what direction is better to go.
Agreed. Somewhat of a tossup now as to whether or not this change should
be done. I'm fine either way.
Chris
Post by s***@oracle.com
Thanks,
Serguei
Post by Chris Plummer
Hi Claes,
It's good that you brought this up. NSK_JNI_VERIFY is always "on",
but moving the assignment outside of it does slightly change the
/**
* Execute action with JNI call, check result for true and
* pending exception and complain error if required.
* Also trace action execution if tracing mode enabled.
*/
#define NSK_JNI_VERIFY(jni, action) \
(nsk_ltrace(NSK_TRACE_BEFORE,__FILE__,__LINE__,"%s\n",#action), \
nsk_jni_lverify(NSK_TRUE,jni,action,__FILE__,__LINE__,"%s\n",#action))
Post by s***@oracle.com
Post by Chris Plummer
So you will no longer see the JNI call in the trace or error output.
You will just see the comparison to the JNI result, which gives no
context as to the source of the result. So I'm now starting to think
that doing the assignments within the NSK_JNI_VERIFY was intentional
so we would see the JNI call in the trace output.
Chris
Post by Claes Redestad
Hi,
http://cr.openjdk.java.net/%7Ejcbeyler/8213003/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassLoaderClasses/clsldrclss002/clsldrclss002.cpp.udiff.html
Post by s***@oracle.com
Post by Chris Plummer
Post by Claes Redestad
if (!NSK_JNI_VERIFY(jni, (testedFieldID =
jni->GetStaticFieldID(testedClass, FIELD_NAME,
FIELD_SIGNATURE)) != NULL))
I'd note that with some of your changes here you're unconditionally
executing logic outside of NSK_JNI_VERIFY macros. Does care need be
taken to wrap the code blocks in equivalent macros/ifdefs? Or are
the NSK_JNI_VERIFY macros always-on in these tests (and essentially
pointless)?
/Claes
Post by JC Beyler
Hi all,
I worked on updating my script and handling more assignments so I
redid a second pass on the same files to get all the cases. Now,
on
Post by s***@oracle.com
Post by Chris Plummer
Post by Claes Redestad
Post by JC Beyler
those files the regex "if.* = " no longer shows any cases we would
want to fix.
Webrev: http://cr.openjdk.java.net/~jcbeyler/8213003/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8213003
I tested this on my dev machine by passing all the tests that were
modified.
Thanks!
Jc
--
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
Chris Plummer
2018-11-03 03:44:46 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>
So assuming the change to move the assignment outside of the
conditional is already in place, you are changing:<br>
<br>
               debugeeClass =
jni-&gt;FindClass(DEBUGEE_CLASS_NAME);<br>
<br>
to<br>
<br>
         ExceptionCheckingJniEnvPtr jni_wrapper(jni);<br>
         ...<br>
             debugeeClass =
jni_wrapper-&gt;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"
cite="mid:CAF9BGBzEizNAo558SX+2TMVWoiBCF0M_5zhv=***@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=utf-8">
<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/"
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"
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"
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"
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-&gt;FindClass(DEBUGEE_CLASS_NAME))
!= NULL)) {</div>
<div>+            debugeeClass =
jni_wrapper-&gt;FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);</div>
<div>+            if (debugeeClass !=
NULL) {</div> </div> <div><br> </div> <div>And print out:</div> <div> <div>&gt;&gt; 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>&gt;&gt; Calling with these
parameter(s):</div>
<div>        nsk/jvmti/SetTag/settag001</div>
<div>&lt;&lt; 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 &gt;&gt; and &lt;&lt; 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 &lt;<a
href="mailto:***@oracle.com"
moz-do-not-send="true">***@oracle.com</a>&gt;
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_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-&gt;FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);</div>
</div>
<div><br>
</div>
<div>and use a mechanism such as </div>
<div>           debugeeClass =
jni-&gt;FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);</div>
<div><br>
</div>
<div>it is actually easy to print out things
like:</div> <div>&gt;&gt; JNI FindClass with the following
parameters from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69:</div> <div>&gt;&gt; <span style="white-space:pre-wrap"> </span>"nsk/jvmti/SetTag/settag001"<br>
</div>
<div>...</div>
<div>
<div>&lt;&lt; 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_1232422551998170429gmail-im"
style="color:rgb(80,0,80)">
<div>  if
(!NSK_JNI_VERIFY(jni,
(debugeeClass =<br>
</div>
<div>
<div>             
     
jni-&gt;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-&gt;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 &lt;<a href="mailto:***@oracle.com"
target="_blank" moz-do-not-send="true">***@oracle.com</a>&gt;
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_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-&gt;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-&gt;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-&gt;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-&gt;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-&gt;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-&gt;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 &lt;<a
href="mailto:***@google.com"
target="_blank" moz-do-not-send="true">***@google.com</a>&gt;
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-&gt;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-&gt;GetStaticFieldID(testedClass,
FIELD_NAME, FIELD_SIGNATURE));</div>
<div>    if (!testedFieldID) {<br>
</div>
<br
class="m_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 &lt;<a
href="mailto:***@oracle.com"
target="_blank" moz-do-not-send="true">***@oracle.com</a>&gt;
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:***@oracle.com"
target="_blank" moz-do-not-send="true">***@oracle.com</a>
wrote:<br>
&gt; It prints the FILE/LINE which is
enough to identify the error point.<br>
Yes, but it also prints the JNI calls.<br>
&gt; But I agree that doing the
assignments within the NSK_JNI_VERIFY was
<br>
&gt; intentional as it gives more context.<br>
&gt; From the other hand it make the code
ugly and less readable.<br>
&gt; 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>
&gt;<br>
&gt; Thanks,<br>
&gt; Serguei<br>
&gt;<br>
&gt;<br>
&gt; On 10/31/18 11:21, Chris Plummer
wrote:<br>
&gt;&gt; Hi Claes,<br>
&gt;&gt;<br>
&gt;&gt; It's good that you brought this
up. NSK_JNI_VERIFY is always "on", <br>
&gt;&gt; but moving the assignment outside
of it does slightly change the <br>
&gt;&gt; behavior of the macro (although
the code still executes "correctly"):<br>
&gt;&gt;<br>
&gt;&gt; /**<br>
&gt;&gt;  * Execute action with JNI call,
check result for true and<br>
&gt;&gt;  * pending exception and complain
error if required.<br>
&gt;&gt;  * Also trace action execution if
tracing mode enabled.<br>
&gt;&gt;  */<br>
&gt;&gt; #define NSK_JNI_VERIFY(jni,
action)  \<br>
&gt;&gt;
(nsk_ltrace(NSK_TRACE_BEFORE,__FILE__,__LINE__,"%s\n",#action),
\<br>
&gt;&gt;
nsk_jni_lverify(NSK_TRUE,jni,action,__FILE__,__LINE__,"%s\n",#action))<br>
&gt;&gt;<br>
&gt;&gt; So you will no longer see the JNI
call in the trace or error output. <br>
&gt;&gt; You will just see the comparison
to the JNI result, which gives no <br>
&gt;&gt; context as to the source of the
result. So I'm now starting to think <br>
&gt;&gt; that doing the assignments within
the NSK_JNI_VERIFY was intentional <br>
&gt;&gt; so we would see the JNI call in
the trace output.<br>
&gt;&gt;<br>
&gt;&gt; Chris<br>
&gt;&gt;<br>
&gt;&gt; On 10/31/18 3:16 AM, Claes
Redestad wrote:<br>
&gt;&gt;&gt; Hi,<br>
&gt;&gt;&gt;<br>
&gt;&gt;&gt; here's a case that your
script seems to miss:<br>
&gt;&gt;&gt;<br>
&gt;&gt;&gt; <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>
&gt;&gt;&gt;<br>
&gt;&gt;&gt;<br>
&gt;&gt;&gt;      if (!NSK_JNI_VERIFY(jni,
(testedFieldID =<br>
&gt;&gt;&gt;             
jni-&gt;GetStaticFieldID(testedClass,
FIELD_NAME, <br>
&gt;&gt;&gt; FIELD_SIGNATURE)) != NULL))<br>
&gt;&gt;&gt;<br>
&gt;&gt;&gt; I'd note that with some of
your changes here you're unconditionally <br>
&gt;&gt;&gt; executing logic outside of
NSK_JNI_VERIFY macros. Does care need be <br>
&gt;&gt;&gt; taken to wrap the code blocks
in equivalent macros/ifdefs? Or are <br>
&gt;&gt;&gt; the NSK_JNI_VERIFY macros
always-on in these tests (and essentially
<br>
&gt;&gt;&gt; pointless)?<br>
&gt;&gt;&gt;<br>
&gt;&gt;&gt; /Claes<br>
&gt;&gt;&gt;<br>
&gt;&gt;&gt; On 2018-10-31 05:42, JC
Beyler wrote:<br>
&gt;&gt;&gt;&gt; Hi all,<br>
&gt;&gt;&gt;&gt;<br>
&gt;&gt;&gt;&gt; I worked on updating my
script and handling more assignments so I
<br>
&gt;&gt;&gt;&gt; redid a second pass on
the same files to get all the cases. Now,
on <br>
&gt;&gt;&gt;&gt; those files the regex
"if.* = " no longer shows any cases we
would <br>
&gt;&gt;&gt;&gt; want to fix.<br>
&gt;&gt;&gt;&gt;<br>
&gt;&gt;&gt;&gt; Could I get a review for
this webrev:<br>
&gt;&gt;&gt;&gt; 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>
&gt;&gt;&gt;&gt; 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>
&gt;&gt;&gt;&gt;<br>
&gt;&gt;&gt;&gt; I tested this on my dev
machine by passing all the tests that were
<br>
&gt;&gt;&gt;&gt; modified.<br>
&gt;&gt;&gt;&gt;<br>
&gt;&gt;&gt;&gt; Thanks!<br>
&gt;&gt;&gt;&gt; Jc<br>
&gt;&gt;&gt;<br>
&gt;&gt;<br>
&gt;&gt;<br>
&gt;<br>
<br>
<br>
</blockquote>
</div>
<br clear="all">
<div><br>
</div>
-- <br>
<div dir="ltr"
class="m_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_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_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="gmail_signature"
data-smartmail="gmail_signature">
<div dir="ltr">
<div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</blockquote>
<p><br>
</p>
</body>
</html>
JC Beyler
2018-11-03 04:09:23 UTC
Permalink
Hi Chris,

Yes that is correct, the webrev in this email thread would be postponed and
done differently.

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 http://cr.openjdk.java.net/~jcbeyler/exception/.

I guess the question becomes really: are the changes in
http://cr.openjdk.java.net/~jcbeyler/exception/ seem in theory at least
reasonable? Then I could start working on it but it will most likely be a
slower going process.

Let me know,
Jc
Post by Chris Plummer
Hi JC,
So assuming the change to move the assignment outside of the conditional
debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME);
to
ExceptionCheckingJniEnvPtr jni_wrapper(jni);
...
debugeeClass = jni_wrapper->FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);
But none of your ExceptionCheckingJni changes are pushed yet, right?
thanks,
Chris
Hi Chris,
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
http://cr.openjdk.java.net/~jcbeyler/exception/
- 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
http://cr.openjdk.java.net/~jcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp.udiff.html
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
- 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
http://cr.openjdk.java.net/~jcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.hpp.udiff.html
http://cr.openjdk.java.net/~jcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.cpp.udiff.html
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
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
c) at JNIVerifier construction, we now can call the print
parameter methods to pass the values of the call to be printed out
- 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
d) the JNI wrapper methods get augmented by the line/file
parameters which default to -1 and 0
e) the constructor is now also passing in the JNI method parameters
- if (!NSK_JNI_VERIFY(jni, (debugeeClass =
- jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL)) {
+ debugeeClass = jni_wrapper->FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);
+ if (debugeeClass != NULL) {
Post by Chris Plummer
Calling JNI method FindClass from
/usr/local/google/home/jcbeyler/mercurial/jdk12-verify/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
nsk/jvmti/SetTag/settag001
<< Called JNI method FindClass from
/usr/local/google/home/jcbeyler/mercurial/jdk12-verify/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
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
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.
Let me know what you think,
Jc
d) at JNIVerifier destruction, we can print out the "called the
method"
Post by Chris Plummer
Hi JC,
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.
thanks,
Chris
Hi Chris,
Thanks for going over my email :)
So I skipped over the tracing part for clarity and striving to be brief.
debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);
and use a mechanism such as
debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);
JNI FindClass with the following parameters from
"nsk/jvmti/SetTag/settag001"
...
<< JNI FindClass from
Before the actual call to the method, allowing to have the full
if (!NSK_JNI_VERIFY(jni, (debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL)) {
nsk_jvmti_setFailStatus();
return;
}
#define TRACE_JNI_CALL __LINE__, __FILE__
...
ExceptionCheckingJniEnvPtr jni_wrapper(jni);
...
debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);
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.
Thanks,
Jc
Hi all,
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.
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
if (!NSK_JNI_VERIFY(jni, (debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL)) {
nsk_jvmti_setFailStatus();
return;
}
Without a change and with the verbose system set up for the test, the
The following fake exception stacktrace is for failuire analysis.
nsk.share.Fake_Exception_for_RULE_Creation: (settag001.cpp:65)
(debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL
at nsk_lvcomplain(nsk_tools.cpp:172)
# ERROR: settag001.cpp, 65: (debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL
# verified JNI assertion is FALSE
# ERROR: agent_tools.cpp, 282: Debuggee status sync aborted because
agent thread has finished
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.
#define NSK_JNI_VERIFY(jni, action) \
(nsk_ltrace(NSK_TRACE_BEFORE,__FILE__,__LINE__,"%s\n",#action), \
nsk_jni_lverify(NSK_TRUE,jni,action,__FILE__,__LINE__,"%s\n",#action))
(Note the typo for failuire, I created JDK-8213246 to fix it).
debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME);
if (!NSK_JNI_VERIFY(jni, debuggeeClass != NULL)) {
nsk_jvmti_setFailStatus();
return;
}
The following fake exception stacktrace is for failuire analysis.
nsk.share.Fake_Exception_for_RULE_Creation: (settag001.cpp:66)
debugeeClass != NULL
at nsk_lvcomplain(nsk_tools.cpp:172)
# ERROR: settag001.cpp, 66: debugeeClass != NULL
# verified JNI assertion is FALSE
# ERROR: agent_tools.cpp, 282: Debuggee status sync aborted because
agent thread has finished
In this case, we do lose that the failure seems to have happened in
FindClass, we need to look at the code.
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.
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.
In any case, a solution would be to have a wrapper script like
debugeeClass =
NSK_JNI_WRAPPER(jni->FindClass(DEBUGEE_CLASS_NAME));
if (!NSK_JNI_VERIFY(jni, debuggeeClass != NULL)) {
nsk_jvmti_setFailStatus();
return;
}
And NSK_JNI_WRAPPER would just to the tracing and execute the passed in
action.
However, the ExceptionJNIWrapper that I implemented a little while ago
FATAL ERROR in native method: FindClass : Return is NULL
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.
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
...
// For tracing purposes.
#define TRACE_JNI_CALL __LINE__, __FILE__
...
ExceptionCheckingJniEnvPtr jni_wrapper(jni);
...
debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);
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
FATAL ERROR in native method: JNI method FindClass : Return is NULL from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
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
JNI method FindClass was called from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
"nsk/jvmti/SetTag/settag001"
FATAL ERROR in native method: JNI method FindClass : Return is NULL from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
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.
thanks,
Chris
Let me know what you think.
Thanks,
Jc
@Claes: you are right, I did not do a grep such as "if.* =$"; by adding
the space instead of the $, I missed a few :)
I've been meaning to ask if we could deprecate these anyway. Are these
- Line/File + JNI call?
I ask because I'd like to deprecate the NSK_VERIFY macros but the
LINE/FILE is a bit annoying to deprecate.
I'd rather be able to remove them entirely but we could do an
testedFieldID = jni->GetStaticFieldID(testedClass, FIELD_NAME,
FIELD_SIGNATURE);
if (!testedFieldID) {
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.
If lines and filenames are really important, then we could do something
testedFieldID = NSK_TRACE(jni->GetStaticFieldID(testedClass,
FIELD_NAME, FIELD_SIGNATURE));
if (!testedFieldID) {
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).
Thanks,
Jc
On Wed, Oct 31, 2018 at 11:52 AM Chris Plummer <
Post by Chris Plummer
Post by s***@oracle.com
It prints the FILE/LINE which is enough to identify the error point.
Yes, but it also prints the JNI calls.
Post by s***@oracle.com
But I agree that doing the assignments within the NSK_JNI_VERIFY was
intentional as it gives more context.
From the other hand it make the code ugly and less readable.
Not sure, what direction is better to go.
Agreed. Somewhat of a tossup now as to whether or not this change should
be done. I'm fine either way.
Chris
Post by s***@oracle.com
Thanks,
Serguei
Post by Chris Plummer
Hi Claes,
It's good that you brought this up. NSK_JNI_VERIFY is always "on",
but moving the assignment outside of it does slightly change the
behavior of the macro (although the code still executes
/**
* Execute action with JNI call, check result for true and
* pending exception and complain error if required.
* Also trace action execution if tracing mode enabled.
*/
#define NSK_JNI_VERIFY(jni, action) \
(nsk_ltrace(NSK_TRACE_BEFORE,__FILE__,__LINE__,"%s\n",#action), \
nsk_jni_lverify(NSK_TRUE,jni,action,__FILE__,__LINE__,"%s\n",#action))
Post by s***@oracle.com
Post by Chris Plummer
So you will no longer see the JNI call in the trace or error
output.
Post by s***@oracle.com
Post by Chris Plummer
You will just see the comparison to the JNI result, which gives no
context as to the source of the result. So I'm now starting to
think
Post by s***@oracle.com
Post by Chris Plummer
that doing the assignments within the NSK_JNI_VERIFY was
intentional
Post by s***@oracle.com
Post by Chris Plummer
so we would see the JNI call in the trace output.
Chris
Post by Claes Redestad
Hi,
http://cr.openjdk.java.net/%7Ejcbeyler/8213003/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassLoaderClasses/clsldrclss002/clsldrclss002.cpp.udiff.html
Post by s***@oracle.com
Post by Chris Plummer
Post by Claes Redestad
if (!NSK_JNI_VERIFY(jni, (testedFieldID =
jni->GetStaticFieldID(testedClass, FIELD_NAME,
FIELD_SIGNATURE)) != NULL))
I'd note that with some of your changes here you're
unconditionally
Post by s***@oracle.com
Post by Chris Plummer
Post by Claes Redestad
executing logic outside of NSK_JNI_VERIFY macros. Does care need
be
Post by s***@oracle.com
Post by Chris Plummer
Post by Claes Redestad
taken to wrap the code blocks in equivalent macros/ifdefs? Or are
the NSK_JNI_VERIFY macros always-on in these tests (and
essentially
Post by s***@oracle.com
Post by Chris Plummer
Post by Claes Redestad
pointless)?
/Claes
Post by JC Beyler
Hi all,
I worked on updating my script and handling more assignments so I
redid a second pass on the same files to get all the cases. Now,
on
Post by s***@oracle.com
Post by Chris Plummer
Post by Claes Redestad
Post by JC Beyler
those files the regex "if.* = " no longer shows any cases we
would
Post by s***@oracle.com
Post by Chris Plummer
Post by Claes Redestad
Post by JC Beyler
want to fix.
Webrev: http://cr.openjdk.java.net/~jcbeyler/8213003/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8213003
I tested this on my dev machine by passing all the tests that
were
Post by s***@oracle.com
Post by Chris Plummer
Post by Claes Redestad
Post by JC Beyler
modified.
Thanks!
Jc
--
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
Chris Plummer
2018-11-06 19:14:35 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>
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>
<br>
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+***@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 &lt;<a
href="mailto:***@oracle.com"
moz-do-not-send="true">***@oracle.com</a>&gt;
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-&gt;FindClass(DEBUGEE_CLASS_NAME);<br>
<br>
to<br>
<br>
         ExceptionCheckingJniEnvPtr jni_wrapper(jni);<br>
         ...<br>
             debugeeClass =
jni_wrapper-&gt;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-&gt;FindClass(DEBUGEE_CLASS_NAME))
!= NULL)) {</div>
<div>+            debugeeClass =
jni_wrapper-&gt;FindClass(DEBUGEE_CLASS_NAME, TRACE_JNI_CALL);</div>
<div>+            if
(debugeeClass != NULL) {</div> </div> <div><br> </div> <div>And print out:</div> <div> <div>&gt;&gt; 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>&gt;&gt; Calling with these
parameter(s):</div>
<div>       
nsk/jvmti/SetTag/settag001</div>
<div>&lt;&lt; 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 &gt;&gt; and
&lt;&lt; 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 &lt;<a href="mailto:***@oracle.com"
target="_blank" moz-do-not-send="true">***@oracle.com</a>&gt;
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-&gt;FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);</div>
</div>
<div><br>
</div>
<div>and use a mechanism such as </div>
<div>           debugeeClass =
jni-&gt;FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);</div>
<div><br>
</div>
<div>it is actually easy to print out
things like:</div> <div>&gt;&gt; JNI FindClass with the
following parameters from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69:</div> <div>&gt;&gt; <span style="white-space:pre-wrap"> </span>"nsk/jvmti/SetTag/settag001"<br>
</div>
<div>...</div>
<div>
<div>&lt;&lt; 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-&gt;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-&gt;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 &lt;<a
href="mailto:***@oracle.com"
target="_blank" moz-do-not-send="true">***@oracle.com</a>&gt;
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-&gt;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-&gt;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-&gt;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-&gt;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-&gt;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-&gt;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 &lt;<a
href="mailto:***@google.com"
target="_blank"
moz-do-not-send="true">***@google.com</a>&gt;
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-&gt;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-&gt;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 &lt;<a
href="mailto:***@oracle.com"
target="_blank"
moz-do-not-send="true">***@oracle.com</a>&gt;
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:***@oracle.com"
target="_blank"
moz-do-not-send="true">***@oracle.com</a>
wrote:<br>
&gt; It prints the FILE/LINE which
is enough to identify the error
point.<br>
Yes, but it also prints the JNI
calls.<br>
&gt; But I agree that doing the
assignments within the
NSK_JNI_VERIFY was <br>
&gt; intentional as it gives more
context.<br>
&gt; From the other hand it make
the code ugly and less readable.<br>
&gt; 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>
&gt;<br>
&gt; Thanks,<br>
&gt; Serguei<br>
&gt;<br>
&gt;<br>
&gt; On 10/31/18 11:21, Chris
Plummer wrote:<br>
&gt;&gt; Hi Claes,<br>
&gt;&gt;<br>
&gt;&gt; It's good that you
brought this up. NSK_JNI_VERIFY is
always "on", <br>
&gt;&gt; but moving the assignment
outside of it does slightly change
the <br>
&gt;&gt; behavior of the macro
(although the code still executes
"correctly"):<br>
&gt;&gt;<br>
&gt;&gt; /**<br>
&gt;&gt;  * Execute action with
JNI call, check result for true
and<br>
&gt;&gt;  * pending exception and
complain error if required.<br>
&gt;&gt;  * Also trace action
execution if tracing mode enabled.<br>
&gt;&gt;  */<br>
&gt;&gt; #define
NSK_JNI_VERIFY(jni, action)  \<br>
&gt;&gt;
(nsk_ltrace(NSK_TRACE_BEFORE,__FILE__,__LINE__,"%s\n",#action),
\<br>
&gt;&gt;
nsk_jni_lverify(NSK_TRUE,jni,action,__FILE__,__LINE__,"%s\n",#action))<br>
&gt;&gt;<br>
&gt;&gt; So you will no longer see
the JNI call in the trace or error
output. <br>
&gt;&gt; You will just see the
comparison to the JNI result,
which gives no <br>
&gt;&gt; context as to the source
of the result. So I'm now starting
to think <br>
&gt;&gt; that doing the
assignments within the
NSK_JNI_VERIFY was intentional <br>
&gt;&gt; so we would see the JNI
call in the trace output.<br>
&gt;&gt;<br>
&gt;&gt; Chris<br>
&gt;&gt;<br>
&gt;&gt; On 10/31/18 3:16 AM,
Claes Redestad wrote:<br>
&gt;&gt;&gt; Hi,<br>
&gt;&gt;&gt;<br>
&gt;&gt;&gt; here's a case that
your script seems to miss:<br>
&gt;&gt;&gt;<br>
&gt;&gt;&gt; <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>
&gt;&gt;&gt;<br>
&gt;&gt;&gt;<br>
&gt;&gt;&gt;      if
(!NSK_JNI_VERIFY(jni,
(testedFieldID =<br>
&gt;&gt;&gt;             
jni-&gt;GetStaticFieldID(testedClass,
FIELD_NAME, <br>
&gt;&gt;&gt; FIELD_SIGNATURE)) !=
NULL))<br>
&gt;&gt;&gt;<br>
&gt;&gt;&gt; I'd note that with
some of your changes here you're
unconditionally <br>
&gt;&gt;&gt; executing logic
outside of NSK_JNI_VERIFY macros.
Does care need be <br>
&gt;&gt;&gt; taken to wrap the
code blocks in equivalent
macros/ifdefs? Or are <br>
&gt;&gt;&gt; the NSK_JNI_VERIFY
macros always-on in these tests
(and essentially <br>
&gt;&gt;&gt; pointless)?<br>
&gt;&gt;&gt;<br>
&gt;&gt;&gt; /Claes<br>
&gt;&gt;&gt;<br>
&gt;&gt;&gt; On 2018-10-31 05:42,
JC Beyler wrote:<br>
&gt;&gt;&gt;&gt; Hi all,<br>
&gt;&gt;&gt;&gt;<br>
&gt;&gt;&gt;&gt; I worked on
updating my script and handling
more assignments so I <br>
&gt;&gt;&gt;&gt; redid a second
pass on the same files to get all
the cases. Now, on <br>
&gt;&gt;&gt;&gt; those files the
regex "if.* = " no longer shows
any cases we would <br>
&gt;&gt;&gt;&gt; want to fix.<br>
&gt;&gt;&gt;&gt;<br>
&gt;&gt;&gt;&gt; Could I get a
review for this webrev:<br>
&gt;&gt;&gt;&gt; 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>
&gt;&gt;&gt;&gt; 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>
&gt;&gt;&gt;&gt;<br>
&gt;&gt;&gt;&gt; I tested this on
my dev machine by passing all the
tests that were <br>
&gt;&gt;&gt;&gt; modified.<br>
&gt;&gt;&gt;&gt;<br>
&gt;&gt;&gt;&gt; Thanks!<br>
&gt;&gt;&gt;&gt; Jc<br>
&gt;&gt;&gt;<br>
&gt;&gt;<br>
&gt;&gt;<br>
&gt;<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>
</body>
</html>
s***@oracle.com
2018-11-06 19:57:11 UTC
Permalink
Post by Chris Plummer
Hi JC,
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.
The exception changes need to be discussed after a separate RFR is posted.

Thanks,
Serguei
Post by Chris Plummer
thanks,
Chris
Post by JC Beyler
Hi Chris,
Yes that is correct, the webrev in this email thread would be
postponed and done differently.
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 http://cr.openjdk.java.net/~jcbeyler/exception/
<http://cr.openjdk.java.net/%7Ejcbeyler/exception/>.
I guess the question becomes really: are the changes in
http://cr.openjdk.java.net/~jcbeyler/exception/
<http://cr.openjdk.java.net/%7Ejcbeyler/exception/> seem in theory at
least reasonable? Then I could start working on it but it will most
likely be a slower going process.
Let me know,
Jc
On Fri, Nov 2, 2018 at 8:44 PM Chris Plummer
Hi JC,
So assuming the change to move the assignment outside of the
               debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME);
to
         ExceptionCheckingJniEnvPtr jni_wrapper(jni);
         ...
             debugeeClass =
jni_wrapper->FindClass(DEBUGEE_CLASS_NAME, TRACE_JNI_CALL);
But none of your ExceptionCheckingJni changes are pushed yet, right?
thanks,
Chris
Post by JC Beyler
Hi Chris,
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,
http://cr.openjdk.java.net/~jcbeyler/exception/
<http://cr.openjdk.java.net/%7Ejcbeyler/exception/>
   - 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
http://cr.openjdk.java.net/~jcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp.udiff.html
<http://cr.openjdk.java.net/%7Ejcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp.udiff.html>
      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
   - 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
http://cr.openjdk.java.net/~jcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.hpp.udiff.html
<http://cr.openjdk.java.net/%7Ejcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.hpp.udiff.html>
http://cr.openjdk.java.net/~jcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.cpp.udiff.html
<http://cr.openjdk.java.net/%7Ejcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.cpp.udiff.html>
        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
        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
        c) at JNIVerifier construction, we now can call the
print parameter methods to pass the values of the call to be
printed out
          - 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
        d) the JNI wrapper methods get augmented by the
line/file parameters which default to -1 and 0
        e) the constructor is now also passing in the JNI method
parameters
-            if (!NSK_JNI_VERIFY(jni, (debugeeClass =
- jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL)) {
+            debugeeClass =
jni_wrapper->FindClass(DEBUGEE_CLASS_NAME, TRACE_JNI_CALL);
+            if (debugeeClass != NULL) {
Post by JC Beyler
Calling JNI method FindClass from
/usr/local/google/home/jcbeyler/mercurial/jdk12-verify/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
nsk/jvmti/SetTag/settag001
<< Called JNI method FindClass from
/usr/local/google/home/jcbeyler/mercurial/jdk12-verify/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
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
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.
Let me know what you think,
Jc
        d) at JNIVerifier destruction, we can print out the
"called the method"
On Thu, Nov 1, 2018 at 1:24 PM Chris Plummer
Hi JC,
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.
thanks,
Chris
Post by JC Beyler
Hi Chris,
Thanks for going over my email :)
So I skipped over the tracing part for clarity and striving
           debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME, TRACE_JNI_CALL);
and use a mechanism such as
           debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME, TRACE_JNI_CALL);
JNI FindClass with the following parameters from
"nsk/jvmti/SetTag/settag001"
...
<< JNI FindClass from
Before the actual call to the method, allowing to have the
full NSK_*_VERIFY system. The hardest question is do we
  if (!NSK_JNI_VERIFY(jni, (debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL)) {
nsk_jvmti_setFailStatus();
      return;
  }
#define TRACE_JNI_CALL __LINE__, __FILE__
...
 ExceptionCheckingJniEnvPtr jni_wrapper(jni);
...
           debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME, TRACE_JNI_CALL);
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.
Thanks,
Jc
On Thu, Nov 1, 2018 at 12:01 PM Chris Plummer
Hi all,
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.
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
            if (!NSK_JNI_VERIFY(jni, (debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL)) {
nsk_jvmti_setFailStatus();
  return;
            }
Without a change and with the verbose system set up
The following fake exception stacktrace is for
failuire analysis.
(settag001.cpp:65) (debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL
at nsk_lvcomplain(nsk_tools.cpp:172)
# ERROR: settag001.cpp, 65: (debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL
#   verified JNI assertion is FALSE
# ERROR: agent_tools.cpp, 282: Debuggee status sync
aborted because agent thread has finished
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.
#define NSK_JNI_VERIFY(jni, action)  \
 (nsk_ltrace(NSK_TRACE_BEFORE,__FILE__,__LINE__,"%s\n",#action),
\
nsk_jni_lverify(NSK_TRUE,jni,action,__FILE__,__LINE__,"%s\n",#action))
(Note the typo for failuire, I created JDK-8213246 to
fix it).
With my proposed change to remove the assignment, the
debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME);
if (!NSK_JNI_VERIFY(jni, debuggeeClass != NULL)) {
nsk_jvmti_setFailStatus();
    return;
}
The following fake exception stacktrace is for
failuire analysis.
(settag001.cpp:66) debugeeClass != NULL
at nsk_lvcomplain(nsk_tools.cpp:172)
# ERROR: settag001.cpp, 66: debugeeClass != NULL
#   verified JNI assertion is FALSE
# ERROR: agent_tools.cpp, 282: Debuggee status sync
aborted because agent thread has finished
In this case, we do lose that the failure seems to
have happened in FindClass, we need to look at the code.
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.
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.
In any case, a solution would be to have a wrapper
script like NSK_JNI_VERIFY, but just for the purpose of
            debugeeClass =
NSK_JNI_WRAPPER(jni->FindClass(DEBUGEE_CLASS_NAME));
            if (!NSK_JNI_VERIFY(jni, debuggeeClass !=
NULL)) {
nsk_jvmti_setFailStatus();
                return;
            }
And NSK_JNI_WRAPPER would just to the tracing and
execute the passed in action.
However, the ExceptionJNIWrapper that I implemented a
FATAL ERROR in native method: FindClass : Return is NULL
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.
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
...
// For tracing purposes.
#define TRACE_JNI_CALL __LINE__, __FILE__
...
 ExceptionCheckingJniEnvPtr jni_wrapper(jni);
...
 debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);
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
Return is NULL from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
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
JNI method FindClass was called from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
"nsk/jvmti/SetTag/settag001"
Return is NULL from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
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.
thanks,
Chris
Let me know what you think.
Thanks,
Jc
On Wed, Oct 31, 2018 at 1:54 PM JC Beyler
@Claes: you are right, I did not do a grep such as
"if.* =$"; by adding the space instead of the $, I
missed a few :)
I've been meaning to ask if we could deprecate
these anyway. Are these really being used? And if
   - Line/File + JNI call?
I ask because I'd like to deprecate the NSK_VERIFY
macros but the LINE/FILE is a bit annoying to
deprecate.
I'd rather be able to remove them entirely but we
   testedFieldID
= jni->GetStaticFieldID(testedClass, FIELD_NAME,
FIELD_SIGNATURE);
    if (!testedFieldID) {
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.
If lines and filenames are really important, then
    testedFieldID =
NSK_TRACE(jni->GetStaticFieldID(testedClass,
FIELD_NAME, FIELD_SIGNATURE));
    if (!testedFieldID) {
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).
Thanks,
Jc
On Wed, Oct 31, 2018 at 11:52 AM Chris Plummer
On 10/31/18 11:30 AM,
It prints the FILE/LINE which is enough to
identify the error point.
Yes, but it also prints the JNI calls.
But I agree that doing the assignments
within the NSK_JNI_VERIFY was
intentional as it gives more context.
From the other hand it make the code ugly
and less readable.
Not sure, what direction is better to go.
Agreed. Somewhat of a tossup now as to whether
or not this change should
be done. I'm fine either way.
Chris
Thanks,
Serguei
Post by Chris Plummer
Hi Claes,
It's good that you brought this up.
NSK_JNI_VERIFY is always "on",
Post by Chris Plummer
but moving the assignment outside of it
does slightly change the
Post by Chris Plummer
behavior of the macro (although the code
/**
 * Execute action with JNI call, check
result for true and
Post by Chris Plummer
 * pending exception and complain error if
required.
Post by Chris Plummer
 * Also trace action execution if tracing
mode enabled.
Post by Chris Plummer
 */
#define NSK_JNI_VERIFY(jni, action)  \
(nsk_ltrace(NSK_TRACE_BEFORE,__FILE__,__LINE__,"%s\n",#action),
\
nsk_jni_lverify(NSK_TRUE,jni,action,__FILE__,__LINE__,"%s\n",#action))
Post by Chris Plummer
So you will no longer see the JNI call in
the trace or error output.
Post by Chris Plummer
You will just see the comparison to the JNI
result, which gives no
Post by Chris Plummer
context as to the source of the result. So
I'm now starting to think
Post by Chris Plummer
that doing the assignments within the
NSK_JNI_VERIFY was intentional
Post by Chris Plummer
so we would see the JNI call in the trace
output.
Post by Chris Plummer
Chris
Hi,
http://cr.openjdk.java.net/%7Ejcbeyler/8213003/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassLoaderClasses/clsldrclss002/clsldrclss002.cpp.udiff.html
Post by Chris Plummer
     if (!NSK_JNI_VERIFY(jni, (testedFieldID =
jni->GetStaticFieldID(testedClass,
FIELD_NAME,
Post by Chris Plummer
FIELD_SIGNATURE)) != NULL))
I'd note that with some of your changes
here you're unconditionally
Post by Chris Plummer
executing logic outside of NSK_JNI_VERIFY
macros. Does care need be
Post by Chris Plummer
taken to wrap the code blocks in
equivalent macros/ifdefs? Or are
Post by Chris Plummer
the NSK_JNI_VERIFY macros always-on in
these tests (and essentially
Post by Chris Plummer
pointless)?
/Claes
Post by JC Beyler
Hi all,
I worked on updating my script and
handling more assignments so I
Post by Chris Plummer
Post by JC Beyler
redid a second pass on the same files to
get all the cases. Now, on
Post by Chris Plummer
Post by JC Beyler
those files the regex "if.* = " no longer
shows any cases we would
Post by Chris Plummer
Post by JC Beyler
want to fix.
http://cr.openjdk.java.net/~jcbeyler/8213003/webrev.00/
<http://cr.openjdk.java.net/%7Ejcbeyler/8213003/webrev.00/>
https://bugs.openjdk.java.net/browse/JDK-8213003
Post by Chris Plummer
Post by JC Beyler
I tested this on my dev machine by
passing all the tests that were
Post by Chris Plummer
Post by JC Beyler
modified.
Thanks!
Jc
--
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
s***@oracle.com
2018-11-06 20:23:04 UTC
Permalink
Okay.
I'm not sure I fully understandd what is current plan.

My view is that we can do the following steps:
 1) Jc can push what was already reviewed.
    With this change we will miss extra tracing for JNI calls and results.
 2) Work on using the ExceptionCheckingJni that will restore
    this tracing but in a different fashion.

Is it correct?

Thanks,
Serguei
Post by s***@oracle.com
Post by Chris Plummer
Hi JC,
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.
The exception changes need to be discussed after a separate RFR is posted.
Thanks,
Serguei
Post by Chris Plummer
thanks,
Chris
Post by JC Beyler
Hi Chris,
Yes that is correct, the webrev in this email thread would be
postponed and done differently.
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 http://cr.openjdk.java.net/~jcbeyler/exception/
<http://cr.openjdk.java.net/%7Ejcbeyler/exception/>.
I guess the question becomes really: are the changes in
http://cr.openjdk.java.net/~jcbeyler/exception/
<http://cr.openjdk.java.net/%7Ejcbeyler/exception/> seem in theory
at least reasonable? Then I could start working on it but it will
most likely be a slower going process.
Let me know,
Jc
On Fri, Nov 2, 2018 at 8:44 PM Chris Plummer
Hi JC,
So assuming the change to move the assignment outside of the
               debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME);
to
         ExceptionCheckingJniEnvPtr jni_wrapper(jni);
         ...
             debugeeClass =
jni_wrapper->FindClass(DEBUGEE_CLASS_NAME, TRACE_JNI_CALL);
But none of your ExceptionCheckingJni changes are pushed yet, right?
thanks,
Chris
Post by JC Beyler
Hi Chris,
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,
http://cr.openjdk.java.net/~jcbeyler/exception/
<http://cr.openjdk.java.net/%7Ejcbeyler/exception/>
   - 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
http://cr.openjdk.java.net/~jcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp.udiff.html
<http://cr.openjdk.java.net/%7Ejcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp.udiff.html>
      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
   - 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
http://cr.openjdk.java.net/~jcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.hpp.udiff.html
<http://cr.openjdk.java.net/%7Ejcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.hpp.udiff.html>
http://cr.openjdk.java.net/~jcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.cpp.udiff.html
<http://cr.openjdk.java.net/%7Ejcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.cpp.udiff.html>
        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
        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
        c) at JNIVerifier construction, we now can call the
print parameter methods to pass the values of the call to be
printed out
          - 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
        d) the JNI wrapper methods get augmented by the
line/file parameters which default to -1 and 0
        e) the constructor is now also passing in the JNI
method parameters
-            if (!NSK_JNI_VERIFY(jni, (debugeeClass =
- jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL)) {
+ debugeeClass = jni_wrapper->FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);
+            if (debugeeClass != NULL) {
Post by JC Beyler
Calling JNI method FindClass from
/usr/local/google/home/jcbeyler/mercurial/jdk12-verify/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
nsk/jvmti/SetTag/settag001
<< Called JNI method FindClass from
/usr/local/google/home/jcbeyler/mercurial/jdk12-verify/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
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
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.
Let me know what you think,
Jc
        d) at JNIVerifier destruction, we can print out the
"called the method"
On Thu, Nov 1, 2018 at 1:24 PM Chris Plummer
Hi JC,
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.
thanks,
Chris
Post by JC Beyler
Hi Chris,
Thanks for going over my email :)
So I skipped over the tracing part for clarity and
           debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME, TRACE_JNI_CALL);
and use a mechanism such as
           debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME, TRACE_JNI_CALL);
JNI FindClass with the following parameters from
"nsk/jvmti/SetTag/settag001"
...
<< JNI FindClass from
Before the actual call to the method, allowing to have the
full NSK_*_VERIFY system. The hardest question is do we
  if (!NSK_JNI_VERIFY(jni, (debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL)) {
nsk_jvmti_setFailStatus();
      return;
  }
#define TRACE_JNI_CALL __LINE__, __FILE__
...
 ExceptionCheckingJniEnvPtr jni_wrapper(jni);
...
           debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME, TRACE_JNI_CALL);
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.
Thanks,
Jc
On Thu, Nov 1, 2018 at 12:01 PM Chris Plummer
Hi all,
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.
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
if (!NSK_JNI_VERIFY(jni, (debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL)) {
nsk_jvmti_setFailStatus();
    return;
}
Without a change and with the verbose system set up
The following fake exception stacktrace is for
failuire analysis.
(settag001.cpp:65) (debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL
at nsk_lvcomplain(nsk_tools.cpp:172)
# ERROR: settag001.cpp, 65: (debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL
#  verified JNI assertion is FALSE
# ERROR: agent_tools.cpp, 282: Debuggee status sync
aborted because agent thread has finished
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.
#define NSK_JNI_VERIFY(jni, action)  \
 (nsk_ltrace(NSK_TRACE_BEFORE,__FILE__,__LINE__,"%s\n",#action),
\
nsk_jni_lverify(NSK_TRUE,jni,action,__FILE__,__LINE__,"%s\n",#action))
(Note the typo for failuire, I created JDK-8213246 to
fix it).
With my proposed change to remove the assignment, the
debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME);
if (!NSK_JNI_VERIFY(jni, debuggeeClass != NULL)) {
nsk_jvmti_setFailStatus();
      return;
  }
The following fake exception stacktrace is for
failuire analysis.
(settag001.cpp:66) debugeeClass != NULL
at nsk_lvcomplain(nsk_tools.cpp:172)
# ERROR: settag001.cpp, 66: debugeeClass != NULL
#  verified JNI assertion is FALSE
# ERROR: agent_tools.cpp, 282: Debuggee status sync
aborted because agent thread has finished
In this case, we do lose that the failure seems to
have happened in FindClass, we need to look at the code.
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.
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.
In any case, a solution would be to have a wrapper
script like NSK_JNI_VERIFY, but just for the purpose
            debugeeClass =
NSK_JNI_WRAPPER(jni->FindClass(DEBUGEE_CLASS_NAME));
            if (!NSK_JNI_VERIFY(jni, debuggeeClass !=
NULL)) {
nsk_jvmti_setFailStatus();
                return;
            }
And NSK_JNI_WRAPPER would just to the tracing and
execute the passed in action.
However, the ExceptionJNIWrapper that I implemented a
FATAL ERROR in native method: FindClass : Return is NULL
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.
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
...
// For tracing purposes.
#define TRACE_JNI_CALL __LINE__, __FILE__
...
 ExceptionCheckingJniEnvPtr jni_wrapper(jni);
...
 debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);
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
Return is NULL from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
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
JNI method FindClass was called from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
"nsk/jvmti/SetTag/settag001"
Return is NULL from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
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.
thanks,
Chris
Let me know what you think.
Thanks,
Jc
On Wed, Oct 31, 2018 at 1:54 PM JC Beyler
@Claes: you are right, I did not do a grep such
as "if.* =$"; by adding the space instead of the
$, I missed a few :)
I've been meaning to ask if we could deprecate
these anyway. Are these really being used? And if
   - Line/File + JNI call?
I ask because I'd like to deprecate the
NSK_VERIFY macros but the LINE/FILE is a bit
annoying to deprecate.
I'd rather be able to remove them entirely but we
could do an alternative and migrate the
   testedFieldID
= jni->GetStaticFieldID(testedClass, FIELD_NAME,
FIELD_SIGNATURE);
    if (!testedFieldID) {
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.
If lines and filenames are really important, then
    testedFieldID =
NSK_TRACE(jni->GetStaticFieldID(testedClass,
FIELD_NAME, FIELD_SIGNATURE));
    if (!testedFieldID) {
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).
Thanks,
Jc
On Wed, Oct 31, 2018 at 11:52 AM Chris Plummer
On 10/31/18 11:30 AM,
It prints the FILE/LINE which is enough to
identify the error point.
Yes, but it also prints the JNI calls.
But I agree that doing the assignments
within the NSK_JNI_VERIFY was
intentional as it gives more context.
From the other hand it make the code ugly
and less readable.
Not sure, what direction is better to go.
Agreed. Somewhat of a tossup now as to
whether or not this change should
be done. I'm fine either way.
Chris
Thanks,
Serguei
Post by Chris Plummer
Hi Claes,
It's good that you brought this up.
NSK_JNI_VERIFY is always "on",
Post by Chris Plummer
but moving the assignment outside of it
does slightly change the
Post by Chris Plummer
behavior of the macro (although the code
/**
 * Execute action with JNI call, check
result for true and
Post by Chris Plummer
 * pending exception and complain error if
required.
Post by Chris Plummer
 * Also trace action execution if tracing
mode enabled.
Post by Chris Plummer
 */
#define NSK_JNI_VERIFY(jni, action)  \
(nsk_ltrace(NSK_TRACE_BEFORE,__FILE__,__LINE__,"%s\n",#action),
\
nsk_jni_lverify(NSK_TRUE,jni,action,__FILE__,__LINE__,"%s\n",#action))
Post by Chris Plummer
So you will no longer see the JNI call in
the trace or error output.
Post by Chris Plummer
You will just see the comparison to the
JNI result, which gives no
Post by Chris Plummer
context as to the source of the result. So
I'm now starting to think
Post by Chris Plummer
that doing the assignments within the
NSK_JNI_VERIFY was intentional
Post by Chris Plummer
so we would see the JNI call in the trace
output.
Post by Chris Plummer
Chris
Hi,
http://cr.openjdk.java.net/%7Ejcbeyler/8213003/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassLoaderClasses/clsldrclss002/clsldrclss002.cpp.udiff.html
Post by Chris Plummer
     if (!NSK_JNI_VERIFY(jni,
(testedFieldID =
Post by Chris Plummer
jni->GetStaticFieldID(testedClass,
FIELD_NAME,
Post by Chris Plummer
FIELD_SIGNATURE)) != NULL))
I'd note that with some of your changes
here you're unconditionally
Post by Chris Plummer
executing logic outside of NSK_JNI_VERIFY
macros. Does care need be
Post by Chris Plummer
taken to wrap the code blocks in
equivalent macros/ifdefs? Or are
Post by Chris Plummer
the NSK_JNI_VERIFY macros always-on in
these tests (and essentially
Post by Chris Plummer
pointless)?
/Claes
Post by JC Beyler
Hi all,
I worked on updating my script and
handling more assignments so I
Post by Chris Plummer
Post by JC Beyler
redid a second pass on the same files to
get all the cases. Now, on
Post by Chris Plummer
Post by JC Beyler
those files the regex "if.* = " no
longer shows any cases we would
Post by Chris Plummer
Post by JC Beyler
want to fix.
http://cr.openjdk.java.net/~jcbeyler/8213003/webrev.00/
<http://cr.openjdk.java.net/%7Ejcbeyler/8213003/webrev.00/>
https://bugs.openjdk.java.net/browse/JDK-8213003
Post by Chris Plummer
Post by JC Beyler
I tested this on my dev machine by
passing all the tests that were
Post by Chris Plummer
Post by JC Beyler
modified.
Thanks!
Jc
--
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
JC Beyler
2018-11-06 21:10:39 UTC
Permalink
Hi Serguei,

Thanks for looking at it. You are right that there are various ways of
doing this:

A) Continue removing the assignments via
https://bugs.openjdk.java.net/browse/JDK-8210687
- This requires a few more webrevs but as you've said, we will miss the
extra tracing

B) Start extending the ExceptionCheckingJni and start propagating that in
the vmTestbase already handled in A to bring back the tracing

And we can do A + B in parallel so that the time without tracing is reduced.

Otherwise we can do:

C) Start extending the ExceptionCheckingJni and start propagating that in
the vmTestbase already handled in A to bring back the tracing
- When propagating in code that is using the NSK_VERIFY macros, we
remove the macro at the same time

-----------------------

I have no real preference, A is easy to do with my scripts and B will be
relatively straightforward now that I've worked out most of the details. C
postpones the bug due
to just taking more time to roll-out but there was no real rush for it,
it's just going to be slower going. Also, the (C) route makes each file
changed a bit bigger and less automatic to understand
what is going on but not by much.

I would imagine the community and reviewers would prefer the (C) route as
it is the least disruptive from a test tracing/debugging perspective, no?

Let me know,
Jc
Post by s***@oracle.com
Okay.
I'm not sure I fully understandd what is current plan.
1) Jc can push what was already reviewed.
With this change we will miss extra tracing for JNI calls and results.
2) Work on using the ExceptionCheckingJni that will restore
this tracing but in a different fashion.
Is it correct?
Thanks,
Serguei
Hi JC,
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.
The exception changes need to be discussed after a separate RFR is posted.
Thanks,
Serguei
thanks,
Chris
Hi Chris,
Yes that is correct, the webrev in this email thread would be postponed
and done differently.
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 http://cr.openjdk.java.net/~jcbeyler/exception/.
I guess the question becomes really: are the changes in
http://cr.openjdk.java.net/~jcbeyler/exception/ seem in theory at least
reasonable? Then I could start working on it but it will most likely be a
slower going process.
Let me know,
Jc
Post by Chris Plummer
Hi JC,
So assuming the change to move the assignment outside of the conditional
debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME);
to
ExceptionCheckingJniEnvPtr jni_wrapper(jni);
...
debugeeClass = jni_wrapper->FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);
But none of your ExceptionCheckingJni changes are pushed yet, right?
thanks,
Chris
Hi Chris,
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
http://cr.openjdk.java.net/~jcbeyler/exception/
- 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
http://cr.openjdk.java.net/~jcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp.udiff.html
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
- 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
http://cr.openjdk.java.net/~jcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.hpp.udiff.html
http://cr.openjdk.java.net/~jcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.cpp.udiff.html
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
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
c) at JNIVerifier construction, we now can call the print
parameter methods to pass the values of the call to be printed out
- 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
d) the JNI wrapper methods get augmented by the line/file
parameters which default to -1 and 0
e) the constructor is now also passing in the JNI method parameters
- if (!NSK_JNI_VERIFY(jni, (debugeeClass =
- jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL)) {
+ debugeeClass = jni_wrapper->FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);
+ if (debugeeClass != NULL) {
Post by Chris Plummer
Calling JNI method FindClass from
/usr/local/google/home/jcbeyler/mercurial/jdk12-verify/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
nsk/jvmti/SetTag/settag001
<< Called JNI method FindClass from
/usr/local/google/home/jcbeyler/mercurial/jdk12-verify/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
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
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.
Let me know what you think,
Jc
d) at JNIVerifier destruction, we can print out the "called the
method"
Post by Chris Plummer
Hi JC,
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.
thanks,
Chris
Hi Chris,
Thanks for going over my email :)
So I skipped over the tracing part for clarity and striving to be brief.
debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);
and use a mechanism such as
debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);
JNI FindClass with the following parameters from
"nsk/jvmti/SetTag/settag001"
...
<< JNI FindClass from
Before the actual call to the method, allowing to have the full
if (!NSK_JNI_VERIFY(jni, (debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL)) {
nsk_jvmti_setFailStatus();
return;
}
#define TRACE_JNI_CALL __LINE__, __FILE__
...
ExceptionCheckingJniEnvPtr jni_wrapper(jni);
...
debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);
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.
Thanks,
Jc
Hi all,
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.
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
if (!NSK_JNI_VERIFY(jni, (debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL)) {
nsk_jvmti_setFailStatus();
return;
}
Without a change and with the verbose system set up for the test, the
The following fake exception stacktrace is for failuire analysis.
nsk.share.Fake_Exception_for_RULE_Creation: (settag001.cpp:65)
(debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL
at nsk_lvcomplain(nsk_tools.cpp:172)
# ERROR: settag001.cpp, 65: (debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL
# verified JNI assertion is FALSE
# ERROR: agent_tools.cpp, 282: Debuggee status sync aborted because
agent thread has finished
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.
#define NSK_JNI_VERIFY(jni, action) \
(nsk_ltrace(NSK_TRACE_BEFORE,__FILE__,__LINE__,"%s\n",#action), \
nsk_jni_lverify(NSK_TRUE,jni,action,__FILE__,__LINE__,"%s\n",#action))
(Note the typo for failuire, I created JDK-8213246 to fix it).
debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME);
if (!NSK_JNI_VERIFY(jni, debuggeeClass != NULL)) {
nsk_jvmti_setFailStatus();
return;
}
The following fake exception stacktrace is for failuire analysis.
nsk.share.Fake_Exception_for_RULE_Creation: (settag001.cpp:66)
debugeeClass != NULL
at nsk_lvcomplain(nsk_tools.cpp:172)
# ERROR: settag001.cpp, 66: debugeeClass != NULL
# verified JNI assertion is FALSE
# ERROR: agent_tools.cpp, 282: Debuggee status sync aborted because
agent thread has finished
In this case, we do lose that the failure seems to have happened in
FindClass, we need to look at the code.
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.
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.
In any case, a solution would be to have a wrapper script like
debugeeClass =
NSK_JNI_WRAPPER(jni->FindClass(DEBUGEE_CLASS_NAME));
if (!NSK_JNI_VERIFY(jni, debuggeeClass != NULL)) {
nsk_jvmti_setFailStatus();
return;
}
And NSK_JNI_WRAPPER would just to the tracing and execute the passed in
action.
However, the ExceptionJNIWrapper that I implemented a little while ago
FATAL ERROR in native method: FindClass : Return is NULL
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.
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
...
// For tracing purposes.
#define TRACE_JNI_CALL __LINE__, __FILE__
...
ExceptionCheckingJniEnvPtr jni_wrapper(jni);
...
debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);
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
FATAL ERROR in native method: JNI method FindClass : Return is NULL from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
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
JNI method FindClass was called from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
"nsk/jvmti/SetTag/settag001"
FATAL ERROR in native method: JNI method FindClass : Return is NULL from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
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.
thanks,
Chris
Let me know what you think.
Thanks,
Jc
@Claes: you are right, I did not do a grep such as "if.* =$"; by
adding the space instead of the $, I missed a few :)
I've been meaning to ask if we could deprecate these anyway. Are these
- Line/File + JNI call?
I ask because I'd like to deprecate the NSK_VERIFY macros but the
LINE/FILE is a bit annoying to deprecate.
I'd rather be able to remove them entirely but we could do an
testedFieldID = jni->GetStaticFieldID(testedClass, FIELD_NAME,
FIELD_SIGNATURE);
if (!testedFieldID) {
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.
If lines and filenames are really important, then we could do
testedFieldID = NSK_TRACE(jni->GetStaticFieldID(testedClass,
FIELD_NAME, FIELD_SIGNATURE));
if (!testedFieldID) {
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).
Thanks,
Jc
On Wed, Oct 31, 2018 at 11:52 AM Chris Plummer <
Post by Chris Plummer
Post by s***@oracle.com
It prints the FILE/LINE which is enough to identify the error point.
Yes, but it also prints the JNI calls.
Post by s***@oracle.com
But I agree that doing the assignments within the NSK_JNI_VERIFY
was
Post by s***@oracle.com
intentional as it gives more context.
From the other hand it make the code ugly and less readable.
Not sure, what direction is better to go.
Agreed. Somewhat of a tossup now as to whether or not this change should
be done. I'm fine either way.
Chris
Post by s***@oracle.com
Thanks,
Serguei
Post by Chris Plummer
Hi Claes,
It's good that you brought this up. NSK_JNI_VERIFY is always "on",
but moving the assignment outside of it does slightly change the
behavior of the macro (although the code still executes
/**
* Execute action with JNI call, check result for true and
* pending exception and complain error if required.
* Also trace action execution if tracing mode enabled.
*/
#define NSK_JNI_VERIFY(jni, action) \
(nsk_ltrace(NSK_TRACE_BEFORE,__FILE__,__LINE__,"%s\n",#action), \
nsk_jni_lverify(NSK_TRUE,jni,action,__FILE__,__LINE__,"%s\n",#action))
Post by s***@oracle.com
Post by Chris Plummer
So you will no longer see the JNI call in the trace or error
output.
Post by s***@oracle.com
Post by Chris Plummer
You will just see the comparison to the JNI result, which gives no
context as to the source of the result. So I'm now starting to
think
Post by s***@oracle.com
Post by Chris Plummer
that doing the assignments within the NSK_JNI_VERIFY was
intentional
Post by s***@oracle.com
Post by Chris Plummer
so we would see the JNI call in the trace output.
Chris
Post by Claes Redestad
Hi,
http://cr.openjdk.java.net/%7Ejcbeyler/8213003/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassLoaderClasses/clsldrclss002/clsldrclss002.cpp.udiff.html
Post by s***@oracle.com
Post by Chris Plummer
Post by Claes Redestad
if (!NSK_JNI_VERIFY(jni, (testedFieldID =
jni->GetStaticFieldID(testedClass, FIELD_NAME,
FIELD_SIGNATURE)) != NULL))
I'd note that with some of your changes here you're
unconditionally
Post by s***@oracle.com
Post by Chris Plummer
Post by Claes Redestad
executing logic outside of NSK_JNI_VERIFY macros. Does care need
be
Post by s***@oracle.com
Post by Chris Plummer
Post by Claes Redestad
taken to wrap the code blocks in equivalent macros/ifdefs? Or are
the NSK_JNI_VERIFY macros always-on in these tests (and
essentially
Post by s***@oracle.com
Post by Chris Plummer
Post by Claes Redestad
pointless)?
/Claes
Post by JC Beyler
Hi all,
I worked on updating my script and handling more assignments so
I
Post by s***@oracle.com
Post by Chris Plummer
Post by Claes Redestad
Post by JC Beyler
redid a second pass on the same files to get all the cases. Now,
on
Post by s***@oracle.com
Post by Chris Plummer
Post by Claes Redestad
Post by JC Beyler
those files the regex "if.* = " no longer shows any cases we
would
Post by s***@oracle.com
Post by Chris Plummer
Post by Claes Redestad
Post by JC Beyler
want to fix.
Webrev: http://cr.openjdk.java.net/~jcbeyler/8213003/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8213003
I tested this on my dev machine by passing all the tests that
were
Post by s***@oracle.com
Post by Chris Plummer
Post by Claes Redestad
Post by JC Beyler
modified.
Thanks!
Jc
--
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
s***@oracle.com
2018-11-07 00:19:15 UTC
Permalink
Hi Jc,
Post by JC Beyler
Hi Serguei,
Thanks for looking at it. You are right that there are various ways of
A) Continue removing the assignments via
https://bugs.openjdk.java.net/browse/JDK-8210687
    - This requires a few more webrevs but as you've said, we will
miss the extra tracing
B) Start extending the ExceptionCheckingJni and start propagating that
in the vmTestbase already handled in A to bring back the tracing
And we can do A + B in parallel so that the time without tracing is
reduced.
I'd prefer to do this in parallel.
Having jni calls traced is not a strong requirement, it is kind of nice
to have.
So, it has to be Okay to miss this tracing for several weeks.
Post by JC Beyler
C) Start extending the ExceptionCheckingJni and start propagating that
in the vmTestbase already handled in A to bring back the tracing
    - When propagating in code that is using the NSK_VERIFY macros, we
remove the macro at the same time
-----------------------
I have no real preference, A is easy to do with my scripts and B will
be relatively straightforward now that I've worked out most of the
details. C postpones the bug due
to just taking more time to roll-out but there was no real rush for
it, it's just going to be slower going. Also, the (C) route makes each
file changed a bit bigger and less automatic to understand
what is going on but not by much.
I would imagine the community and reviewers would prefer the (C) route
as it is the least disruptive from a test tracing/debugging
perspective, no?
No. It is not very important.

I prefer to continue with A+B because the A is already in progress,
So you will need to finish it.
We could potentially live without B but better to have it.
So, it could be done in a parallel cycle.

Thanks,
Serguei
Post by JC Beyler
Let me know,
Jc
Okay.
I'm not sure I fully understandd what is current plan.
 1) Jc can push what was already reviewed.
    With this change we will miss extra tracing for JNI calls and results.
 2) Work on using the ExceptionCheckingJni that will restore
    this tracing but in a different fashion.
Is it correct?
Thanks,
Serguei
Post by s***@oracle.com
Post by Chris Plummer
Hi JC,
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.
The exception changes need to be discussed after a separate RFR is posted.
Thanks,
Serguei
Post by Chris Plummer
thanks,
Chris
Post by JC Beyler
Hi Chris,
Yes that is correct, the webrev in this email thread would be
postponed and done differently.
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
http://cr.openjdk.java.net/~jcbeyler/exception/
<http://cr.openjdk.java.net/%7Ejcbeyler/exception/>.
I guess the question becomes really: are the changes in
http://cr.openjdk.java.net/~jcbeyler/exception/
<http://cr.openjdk.java.net/%7Ejcbeyler/exception/> seem in
theory at least reasonable? Then I could start working on it
but it will most likely be a slower going process.
Let me know,
Jc
On Fri, Nov 2, 2018 at 8:44 PM Chris Plummer
Hi JC,
So assuming the change to move the assignment outside of
               debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME);
to
         ExceptionCheckingJniEnvPtr jni_wrapper(jni);
         ...
             debugeeClass =
jni_wrapper->FindClass(DEBUGEE_CLASS_NAME, TRACE_JNI_CALL);
But none of your ExceptionCheckingJni changes are pushed yet, right?
thanks,
Chris
Post by JC Beyler
Hi Chris,
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
http://cr.openjdk.java.net/~jcbeyler/exception/
<http://cr.openjdk.java.net/%7Ejcbeyler/exception/>
   - 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
http://cr.openjdk.java.net/~jcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp.udiff.html
<http://cr.openjdk.java.net/%7Ejcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp.udiff.html>
      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
   - 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
http://cr.openjdk.java.net/~jcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.hpp.udiff.html
<http://cr.openjdk.java.net/%7Ejcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.hpp.udiff.html>
http://cr.openjdk.java.net/~jcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.cpp.udiff.html
<http://cr.openjdk.java.net/%7Ejcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.cpp.udiff.html>
        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
        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
        c) at JNIVerifier construction, we now can call
the print parameter methods to pass the values of the call
to be printed out
          - 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
        d) the JNI wrapper methods get augmented by the
line/file parameters which default to -1 and 0
        e) the constructor is now also passing in the JNI
method parameters
-            if (!NSK_JNI_VERIFY(jni, (debugeeClass =
- jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL)) {
+ debugeeClass =
jni_wrapper->FindClass(DEBUGEE_CLASS_NAME, TRACE_JNI_CALL);
+            if (debugeeClass != NULL) {
Post by JC Beyler
Calling JNI method FindClass from
/usr/local/google/home/jcbeyler/mercurial/jdk12-verify/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
nsk/jvmti/SetTag/settag001
<< Called JNI method FindClass from
/usr/local/google/home/jcbeyler/mercurial/jdk12-verify/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
Return is NULL from
/usr/local/google/home/jcbeyler/mercurial/jdk12-verify/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
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.
Let me know what you think,
Jc
        d) at JNIVerifier destruction, we can print out
the "called the method"
On Thu, Nov 1, 2018 at 1:24 PM Chris Plummer
Hi JC,
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.
thanks,
Chris
Post by JC Beyler
Hi Chris,
Thanks for going over my email :)
So I skipped over the tracing part for clarity and
 debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);
and use a mechanism such as
 debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);
JNI FindClass with the following parameters from
"nsk/jvmti/SetTag/settag001"
...
<< JNI FindClass from
Before the actual call to the method, allowing to
have the full NSK_*_VERIFY system. The hardest
  if (!NSK_JNI_VERIFY(jni, (debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL)) {
nsk_jvmti_setFailStatus();
      return;
  }
#define TRACE_JNI_CALL __LINE__, __FILE__
...
 ExceptionCheckingJniEnvPtr jni_wrapper(jni);
...
 debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME,
TRACE_JNI_CALL);
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.
Thanks,
Jc
On Thu, Nov 1, 2018 at 12:01 PM Chris Plummer
Hi all,
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.
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
  if (!NSK_JNI_VERIFY(jni, (debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL)) {
nsk_jvmti_setFailStatus();
      return;
  }
Without a change and with the verbose system set
The following fake exception stacktrace is for
failuire analysis.
(settag001.cpp:65) (debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL
at nsk_lvcomplain(nsk_tools.cpp:172)
# ERROR: settag001.cpp, 65: (debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL
#  verified JNI assertion is FALSE
# ERROR: agent_tools.cpp, 282: Debuggee status
sync aborted because agent thread has finished
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.
#define NSK_JNI_VERIFY(jni, action)  \
 (nsk_ltrace(NSK_TRACE_BEFORE,__FILE__,__LINE__,"%s\n",#action),
\
nsk_jni_lverify(NSK_TRUE,jni,action,__FILE__,__LINE__,"%s\n",#action))
(Note the typo for failuire, I
created JDK-8213246 to fix it).
With my proposed change to remove the
  debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME);
  if (!NSK_JNI_VERIFY(jni, debuggeeClass != NULL)) {
nsk_jvmti_setFailStatus();
      return;
  }
The following fake exception stacktrace is for
failuire analysis.
(settag001.cpp:66) debugeeClass != NULL
at nsk_lvcomplain(nsk_tools.cpp:172)
# ERROR: settag001.cpp, 66: debugeeClass != NULL
#  verified JNI assertion is FALSE
# ERROR: agent_tools.cpp, 282: Debuggee status
sync aborted because agent thread has finished
In this case, we do lose that the failure seems
to have happened in FindClass, we need to look
at the code.
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.
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.
In any case, a solution would be to have a
wrapper script like NSK_JNI_VERIFY, but just for
            debugeeClass =
NSK_JNI_WRAPPER(jni->FindClass(DEBUGEE_CLASS_NAME));
            if (!NSK_JNI_VERIFY(jni,
debuggeeClass != NULL)) {
nsk_jvmti_setFailStatus();
                return;
            }
And NSK_JNI_WRAPPER would just to the tracing and
execute the passed in action.
However, the ExceptionJNIWrapper that I
implemented a little while ago will provide this
FATAL ERROR in native method: FindClass : Return
is NULL
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.
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
...
// For tracing purposes.
#define TRACE_JNI_CALL __LINE__, __FILE__
...
 ExceptionCheckingJniEnvPtr jni_wrapper(jni);
...
 debugeeClass =
jni->FindClass(DEBUGEE_CLASS_NAME, TRACE_JNI_CALL);
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
FATAL ERROR in native method: JNI method
FindClass : Return is NULL from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
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
JNI method FindClass was called from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
"nsk/jvmti/SetTag/settag001"
FATAL ERROR in native method: JNI method
FindClass : Return is NULL from
test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
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.
thanks,
Chris
Let me know what you think.
Thanks,
Jc
On Wed, Oct 31, 2018 at 1:54 PM JC Beyler
@Claes: you are right, I did not do a grep
such as "if.* =$"; by adding the space
instead of the $, I missed a few :)
I've been meaning to ask if we could
deprecate these anyway. Are these really
being used? And if so, are we saying
   - Line/File + JNI call?
I ask because I'd like to deprecate the
NSK_VERIFY macros but the LINE/FILE is a bit
annoying to deprecate.
I'd rather be able to remove them entirely
but we could do an alternative and migrate
 testedFieldID
= jni->GetStaticFieldID(testedClass,
FIELD_NAME, FIELD_SIGNATURE);
    if (!testedFieldID) {
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.
If lines and filenames are really important,
testedFieldID =
NSK_TRACE(jni->GetStaticFieldID(testedClass,
FIELD_NAME, FIELD_SIGNATURE));
    if (!testedFieldID) {
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).
Thanks,
Jc
On Wed, Oct 31, 2018 at 11:52 AM Chris
On 10/31/18 11:30 AM,
It prints the FILE/LINE which is
enough to identify the error point.
Yes, but it also prints the JNI calls.
But I agree that doing the assignments
within the NSK_JNI_VERIFY was
intentional as it gives more context.
From the other hand it make the code
ugly and less readable.
Not sure, what direction is better to go.
Agreed. Somewhat of a tossup now as to
whether or not this change should
be done. I'm fine either way.
Chris
Thanks,
Serguei
Post by Chris Plummer
Hi Claes,
It's good that you brought this up.
NSK_JNI_VERIFY is always "on",
Post by Chris Plummer
but moving the assignment outside of
it does slightly change the
Post by Chris Plummer
behavior of the macro (although the
/**
 * Execute action with JNI call,
check result for true and
Post by Chris Plummer
 * pending exception and complain
error if required.
Post by Chris Plummer
 * Also trace action execution if
tracing mode enabled.
Post by Chris Plummer
 */
#define NSK_JNI_VERIFY(jni, action)  \
(nsk_ltrace(NSK_TRACE_BEFORE,__FILE__,__LINE__,"%s\n",#action),
\
nsk_jni_lverify(NSK_TRUE,jni,action,__FILE__,__LINE__,"%s\n",#action))
Post by Chris Plummer
So you will no longer see the JNI
call in the trace or error output.
Post by Chris Plummer
You will just see the comparison to
the JNI result, which gives no
Post by Chris Plummer
context as to the source of the
result. So I'm now starting to think
Post by Chris Plummer
that doing the assignments within the
NSK_JNI_VERIFY was intentional
Post by Chris Plummer
so we would see the JNI call in the
trace output.
Post by Chris Plummer
Chris
On 10/31/18 3:16 AM, Claes Redestad
Post by Claes Redestad
Hi,
here's a case that your script seems
http://cr.openjdk.java.net/%7Ejcbeyler/8213003/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassLoaderClasses/clsldrclss002/clsldrclss002.cpp.udiff.html
Post by Chris Plummer
Post by Claes Redestad
     if (!NSK_JNI_VERIFY(jni,
(testedFieldID =
Post by Chris Plummer
Post by Claes Redestad
jni->GetStaticFieldID(testedClass,
FIELD_NAME,
Post by Chris Plummer
Post by Claes Redestad
FIELD_SIGNATURE)) != NULL))
I'd note that with some of your
changes here you're unconditionally
Post by Chris Plummer
Post by Claes Redestad
executing logic outside of
NSK_JNI_VERIFY macros. Does care need be
Post by Chris Plummer
Post by Claes Redestad
taken to wrap the code blocks in
equivalent macros/ifdefs? Or are
Post by Chris Plummer
Post by Claes Redestad
the NSK_JNI_VERIFY macros always-on
in these tests (and essentially
Post by Chris Plummer
Post by Claes Redestad
pointless)?
/Claes
Post by JC Beyler
Hi all,
I worked on updating my script and
handling more assignments so I
Post by Chris Plummer
Post by Claes Redestad
Post by JC Beyler
redid a second pass on the same
files to get all the cases. Now, on
Post by Chris Plummer
Post by Claes Redestad
Post by JC Beyler
those files the regex "if.* = " no
longer shows any cases we would
Post by Chris Plummer
Post by Claes Redestad
Post by JC Beyler
want to fix.
http://cr.openjdk.java.net/~jcbeyler/8213003/webrev.00/
<http://cr.openjdk.java.net/%7Ejcbeyler/8213003/webrev.00/>
https://bugs.openjdk.java.net/browse/JDK-8213003
Post by Chris Plummer
Post by Claes Redestad
Post by JC Beyler
I tested this on my dev machine by
passing all the tests that were
Post by Chris Plummer
Post by Claes Redestad
Post by JC Beyler
modified.
Thanks!
Jc
--
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
Loading...