[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR (M) 8223044: Add back exception checking in tests
From: Chris Plummer <chris.plummer () oracle ! com>
Date: 2019-04-30 22:46:00
Message-ID: 268e791d-f69e-9a93-55c5-cb45d1bf8a33 () oracle ! com
[Download RAW message or body]
<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 4/30/19 3:36 PM, Jean Christophe
Beyler wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAF9BGBxH4Hev=12QTCDRgT3ke6rw6tdc22Oqhp+OkSghvZxBXA@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=utf-8">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">You are right I was overthinking the naming, so
I did this now:
<div><br>
</div>
<div>
<div>- loadedClass = (jclass)
jni_env->CallObjectMethod(loader, methodID,
TRACE_JNI_CALL, className);</div>
<div>+ loadedClass = (jclass)
jni_env->CallObjectMethod(loader, methodID,
TRACE_VARARGS(className));</div>
</div>
</div>
</div>
</div>
</blockquote>
Ok. I like this.<br>
<blockquote type="cite"
cite="mid:CAF9BGBxH4Hev=12QTCDRgT3ke6rw6tdc22Oqhp+OkSghvZxBXA@mail.gmail.com">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div><br>
</div>
<div>Question now is: this variadic won't work if there are
no arguments in a portable way, so if there are no
arguments for the call, should we use a TRACE_JNI_CALL
such as:</div>
<div>
<div> jni_env->CallVoidMethod(thread, methodID,
TRACE_JNI_CALL);</div>
</div>
<div><br>
</div>
<div>or should I create a TRACE_VARARGS for no arguments:</div>
<div> jni_env->CallVoidMethod(thread, methodID,
TRACE_EMPTY_VARARGS());<br>
</div>
<div><br>
</div>
<div>Advantage of the latter is that you would know it is a
VARARG at least and you can see it; draw-back is yet
another macro.</div>
</div>
</div>
</div>
</blockquote>
<blockquote type="cite"
cite="mid:CAF9BGBxH4Hev=12QTCDRgT3ke6rw6tdc22Oqhp+OkSghvZxBXA@mail.gmail.com">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div><br>
</div>
<div>What do you think?</div>
</div>
</div>
</div>
</blockquote>
In general we don't know it is vararg when looking at a callsite, so
I don't feel the need to advertise it as such with
TRACE_EMPTY_VARARGS(). I'd recommend just directly passing
TRACE_JNI_CALL.<br>
<br>
Chris<br>
<blockquote type="cite"
cite="mid:CAF9BGBxH4Hev=12QTCDRgT3ke6rw6tdc22Oqhp+OkSghvZxBXA@mail.gmail.com">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div>Jc</div>
<div><br>
</div>
<div><br>
</div>
</div>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Tue, Apr 30, 2019 at 11:37
AM Chris Plummer <<a href="mailto:chris.plummer@oracle.com"
moz-do-not-send="true">chris.plummer@oracle.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div bgcolor="#FFFFFF">
<div class="gmail-m_7897375457534681579moz-cite-prefix">Hi
JC,<br>
<br>
I'm not sure why you are suggesting TRACED_JNI_CALL
instead of TRACED_VARARGS. How are they different? Are you
suggesting non-varargs calls would just end up using an
empty TRACED_JNI_CALL()?<br>
<br>
Chris<br>
<br>
On 4/30/19 10:05 AM, Jean Christophe Beyler wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">I do have more of these coming and there
are 86 CallXMethod cases where this will occur and 49
NewObject cases. So it would be good to figure out
something that does not hurt our eyes too many times.
<div><br>
</div>
<div>I think I would go with the TRACED_VARARGS, it at
least has the advantage of not being too bad. I
would move toward doing:</div>
<div> 73 loadedClass = (jclass)
jni_env->CallObjectMethod(loader, methodID,
TRACED_JNI_CALL(className));<br>
</div>
<div><br>
</div>
<div>To be consisted with the non-vararg cases, what
do you think?</div>
</div>
</div>
</blockquote>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div>Jc</div>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Tue, Apr 30, 2019
at 9:51 AM Chris Plummer <<a
href="mailto:chris.plummer@oracle.com"
target="_blank" \
moz-do-not-send="true">chris.plummer@oracle.com</a>> wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px
0px 0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div bgcolor="#FFFFFF">
<div
class="gmail-m_7897375457534681579gmail-m_-7679072770957371823moz-cite-prefix">The
advantage of the TRACED_VARARGS approach (or
leaving it as-is) is that there are limited APIs
and callsites affected (only 1 of each in this
review, but it's unclear to me if you have more of
these changes coming).<br>
<br>
Chris<br>
<br>
On 4/29/19 9:49 PM, Jean Christophe Beyler wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">Yes I would fix them all in the
next webrev and then continue the porting. I
100% agree to which is less offensive. I find
that
<div><br>
</div>
<div>
<blockquote type="cite">
<div dir="ltr">
<div style="color:rgb(80,0,80)">
<div>
<div> 73 loadedClass = (jclass)
jni_env->CALLOBJECTMETHODTRACED(loader,
methodID, className);</div>
<div><span style="color:rgb(34,34,34)"><br>
</span></div>
<div><span style="color:rgb(34,34,34)">to
be offensive as well.</span></div>
</div>
</div>
<div style="color:rgb(80,0,80)"><span
style="color:rgb(34,34,34)"><br>
</span></div>
<div style="color:rgb(80,0,80)"><span
style="color:rgb(34,34,34)">So perhaps
the TRACED_VARARGS is best:</span></div>
<div style="color:rgb(80,0,80)"><span
style="color:rgb(34,34,34)"><br>
</span></div>
<div><font color="#500050"><br>
</font></div>
</div>
</blockquote>
<blockquote type="cite"
style="color:rgb(80,0,80)">
<div dir="ltr">
<div>
<div>
<div> 73 loadedClass = (jclass)
jni_env->CallObjectMethod(loader,
methodID,
TRACED_VARARGS(className));</div>
</div>
</div>
<div><br>
</div>
<div>What do you think?</div>
</div>
</blockquote>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Mon, Apr
29, 2019 at 9:44 PM Chris Plummer <<a
href="mailto:chris.plummer@oracle.com"
target="_blank" \
moz-do-not-send="true">chris.plummer@oracle.com</a>> wrote:<br>
</div>
<blockquote class="gmail_quote"
style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div bgcolor="#FFFFFF">
<div
class="gmail-m_7897375457534681579gmail-m_-7679072770957371823gmail-m_-4440136783817257860moz-cite-prefix">Hi
JC,<br>
</div>
<div
class="gmail-m_7897375457534681579gmail-m_-7679072770957371823gmail-m_-4440136783817257860moz-cite-prefix"><br>
</div>
<div
class="gmail-m_7897375457534681579gmail-m_-7679072770957371823gmail-m_-4440136783817257860moz-cite-prefix">On
4/29/19 4:25 PM, Jean Christophe Beyler
wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">Hi Chris,
<div><br>
</div>
<div>I agree it's not ideal, how about
putting it first?<br>
<div><br
class="gmail-m_7897375457534681579gmail-m_-7679072770957371823gmail-m_-4440136783817257860gmail-Apple-interchange-newline">
73 loadedClass = (jclass)
jni_env->CallObjectMethod(TRACE_JNI_CALL, \
loader, methodID, className);<br>
</div>
</div>
</div>
</blockquote>
It's not consistent with other uses, or are
you suggesting changing them all?<br>
<blockquote type="cite">
<div dir="ltr">
<div><br>
</div>
<div>I find that less awkward than the
VAR_ARGS, no?</div>
<div><br>
</div>
<div>Or something like:</div>
<div>
<div>
<div> 73 loadedClass = (jclass)
jni_env->CallObjectMethodTraced(loader, methodID, className);<br>
</div>
</div>
<div><br>
</div>
<div>Where CallObjectMethodTraced is
actually a define in the exception
handling system that adds the line
and filename...</div>
</div>
</div>
</blockquote>
I don't like macroizing a method call in
this manner (macros should be all upper
case, right?). Also it's inconsistent with
the other tracing calls, unless you plan on
doing it to all of them.<br>
<blockquote type="cite">
<div dir="ltr">
<div>
<div><br>
</div>
<div>Which looks better?</div>
</div>
</div>
</blockquote>
<p>Not sure. I wouldn't ask which is better.
I'd ask which is less offensive. :)</p>
<p>thanks,</p>
<p>Chris<br>
</p>
<blockquote type="cite">
<div dir="ltr">
<div>
<div><br>
</div>
<div>Thanks again,</div>
<div>Jc</div>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On
Mon, Apr 29, 2019 at 3:40 PM Chris
Plummer <<a
href="mailto:chris.plummer@oracle.com"
target="_blank"
\
moz-do-not-send="true">chris.plummer@oracle.com</a>> wrote:<br>
</div>
<blockquote class="gmail_quote"
style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div bgcolor="#FFFFFF">
<div
class="gmail-m_7897375457534681579gmail-m_-7679072770957371823gmail-m_-4440136783817257860gmail-m_3730223652915053487moz-cite-prefix">Ok.
I only half heatedly suggest the
following<br>
</div>
<div
class="gmail-m_7897375457534681579gmail-m_-7679072770957371823gmail-m_-4440136783817257860gmail-m_3730223652915053487moz-cite-prefix"><br>
</div>
<div
class="gmail-m_7897375457534681579gmail-m_-7679072770957371823gmail-m_-4440136783817257860gmail-m_3730223652915053487moz-cite-prefix">
73 loadedClass = (jclass)
jni_env->CallObjectMethod(loader,
methodID, VAR_ARGS(className));</div>
<div
class="gmail-m_7897375457534681579gmail-m_-7679072770957371823gmail-m_-4440136783817257860gmail-m_3730223652915053487moz-cite-prefix"><br>
</div>
<div
class="gmail-m_7897375457534681579gmail-m_-7679072770957371823gmail-m_-4440136783817257860gmail-m_3730223652915053487moz-cite-prefix">And
then VAR_ARGS can contain the
reference to TRACE_JNI_CALL.</div>
<div
class="gmail-m_7897375457534681579gmail-m_-7679072770957371823gmail-m_-4440136783817257860gmail-m_3730223652915053487moz-cite-prefix"><br>
</div>
<div
class="gmail-m_7897375457534681579gmail-m_-7679072770957371823gmail-m_-4440136783817257860gmail-m_3730223652915053487moz-cite-prefix">Chris</div>
<div
class="gmail-m_7897375457534681579gmail-m_-7679072770957371823gmail-m_-4440136783817257860gmail-m_3730223652915053487moz-cite-prefix"><br>
</div>
<div
class="gmail-m_7897375457534681579gmail-m_-7679072770957371823gmail-m_-4440136783817257860gmail-m_3730223652915053487moz-cite-prefix">On
4/29/19 2:58 PM, Jean Christophe
Beyler wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">Hi Chris,
<div><br>
</div>
<div>Thanks for the review! It
is the only issue I have with
the system now and I had not
found a good solution for:</div>
<div><br>
</div>
<div>as CallObjectMethod is a
variadic method, I can't put
TRACE_JNI_CALL at the end; so
I put right before the end;
that allows me to do this:</div>
<div>
<pre style="color:rgb(0,0,0)"><span \
class="gmail-m_7897375457534681579gmail-m_-7679072770957371823gmail-m_-4440136783817257860gmail-m_3730223652915053487gmail-new" \
style="color:blue"> +jobject ExceptionCheckingJniEnv::CallObjectMethod(jobject obj, \
jmethodID methodID, int line,</span> <span \
class="gmail-m_7897375457534681579gmail-m_-7679072770957371823gmail-m_-4440136783817257860gmail-m_3730223652915053487gmail-new" \
style="color:blue">+ const char* file_name, ...) {</span> \
<span class="gmail-m_7897375457534681579gmail-m_-7679072770957371823gmail-m_-4440136783817257860gmail-m_3730223652915053487gmail-new" \
style="color:blue">+ JNIVerifier<> marker(this, "CallObjectMethod", obj, \
methodID, line, file_name);</span> <span \
class="gmail-m_7897375457534681579gmail-m_-7679072770957371823gmail-m_-4440136783817257860gmail-m_3730223652915053487gmail-new" \
style="color:blue">+</span> <span \
class="gmail-m_7897375457534681579gmail-m_-7679072770957371823gmail-m_-4440136783817257860gmail-m_3730223652915053487gmail-new" \
style="color:blue">+ va_list args;</span> <span \
class="gmail-m_7897375457534681579gmail-m_-7679072770957371823gmail-m_-4440136783817257860gmail-m_3730223652915053487gmail-new" \
style="color:blue">+ va_start(args, file_name);</span> <span \
class="gmail-m_7897375457534681579gmail-m_-7679072770957371823gmail-m_-4440136783817257860gmail-m_3730223652915053487gmail-new" \
style="color:blue">+ jobject result = _jni_env->CallObjectMethodV(obj, methodID, \
args);</span> <span class="gmail-m_7897375457534681579gmail-m_-7679072770957371823gmail-m_-4440136783817257860gmail-m_3730223652915053487gmail-new" \
style="color:blue">+ va_end(args);</span> <span \
class="gmail-m_7897375457534681579gmail-m_-7679072770957371823gmail-m_-4440136783817257860gmail-m_3730223652915053487gmail-new" \
style="color:blue">+ return result;</span> <span \
class="gmail-m_7897375457534681579gmail-m_-7679072770957371823gmail-m_-4440136783817257860gmail-m_3730223652915053487gmail-new" \
style="color:blue">+}</span></pre> <pre style="color:rgb(0,0,0)">Putting it at the \
end would mean I would have to play with the va_list to remove the last one, and from \
what I have understood with va_list, it's kind of a no-no to do so. So I left at \
this.</pre>
<pre style="color:rgb(0,0,0)">Do you have \
any better suggestion? </pre>
<pre style="color:rgb(0,0,0)">Thanks!</pre>
<pre style="color:rgb(0,0,0)">Jc</pre>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr"
class="gmail_attr">On Mon, Apr
29, 2019 at 10:38 AM Chris
Plummer <<a
href="mailto:chris.plummer@oracle.com"
target="_blank"
\
moz-do-not-send="true">chris.plummer@oracle.com</a>> wrote:<br>
</div>
<blockquote class="gmail_quote"
style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div bgcolor="#FFFFFF">
<div
class="gmail-m_7897375457534681579gmail-m_-7679072770957371823gmail-m_-4440136783817257860gmail-m_3730223652915053487gmail-m_-4848090240670792596moz-cite-prefix">Hi
JC,<br>
<br>
In em01t002.cpp, is this
correct?<br>
<br>
73 loadedClass =
(jclass)
jni_env->CallObjectMethod(loader,
methodID, TRACE_JNI_CALL,
className);<br>
<br>
Shouldn't the
TRACE_JNI_CALL arg be
last? If so, can you look
into why this test didn't
fail as a result.<br>
<br>
Other than that the
changes look good.<br>
<br>
thanks,<br>
<br>
Chris<br>
<br>
On 4/26/19 5:52 PM, Jean
Christophe Beyler wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">Hi all,
<div><br>
</div>
<div>(Re-sending with
subject line complete
:-))</div>
<div><br>
</div>
<div>Since JDK-8213501
finally merged (sorry
it took so long), I am
able to continue this
work. Here is the work
that puts back the
messages for any calls
that were moved around
due to JDK-8212884.<br
clear="all">
<div><br>
</div>
<div>Webrev: <a
\
href="http://cr.openjdk.java.net/%7Ejcbeyler/8223044/webrev.00/" target="_blank"
\
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8223044/webrev.00/</a></div>
<div>Bug: <a
\
href="https://bugs.openjdk.java.net/browse/JDK-8223044" target="_blank"
\
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8223044</a></div> \
<div><br> </div>
<div>All modified
tests pass on my
local dev machine.</div>
<div><br>
</div>
<div dir="ltr"
class="gmail-m_7897375457534681579gmail-m_-7679072770957371823gmail-m_-444013678381725 \
7860gmail-m_3730223652915053487gmail-m_-4848090240670792596gmail-m_2236599136067120510gmail_signature">
<div dir="ltr">Thanks,
<div>Jc</div>
</div>
</div>
</div>
</div>
</blockquote>
<p><br>
</p>
</div>
</blockquote>
</div>
<br clear="all">
<div><br>
</div>
-- <br>
<div dir="ltr"
class="gmail-m_7897375457534681579gmail-m_-7679072770957371823gmail-m_-4440136783817257860gmail-m_3730223652915053487gmail_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-m_7897375457534681579gmail-m_-7679072770957371823gmail-m_-4440136783817257860gmail_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-m_7897375457534681579gmail-m_-7679072770957371823gmail_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-m_7897375457534681579gmail_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">
<div dir="ltr">
<div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</blockquote>
<p><br>
</p>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic