<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>
Thank you a lot for looking at the update!<br>
<br>
<br>
On 11/7/18 09:30, JC Beyler wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAF9BGBwKexcEauqhcyvnpKOZW=***@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>Hi Serguei,</div>
<div><br>
</div>
<div>I like the change you made in the VM, it is
more clean and easier to see what is going on :)</div>
<div><br>
</div>
<div>A few nits:</div>
<div><br>
</div>
<div dir="ltr"><a
href="http://cr.openjdk.java.net/%7Esspitsyn/webrevs/2018/8080406-jvmti-get-local.2/test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/GetLocalVariable/getlocal003/getlocal003.cpp.udiff.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8080406-jvmti-get-local.2/test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/GetLocalVariable/getlocal003/getlocal003.cpp.udiff.html</a><br>
</div>
<div dir="ltr"><br>
</div>
<div>- There is a type: "Succes:" line 342.</div>
<div>- There is this check:</div>
<div dir="ltr">
<pre> 272 if (jvmti == NULL) {
273 return;
274 }</pre>
<pre>but the same check line 109 fails the test and prints a message.</pre>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
Nice catches - fixed.<br>
<br>
<br>
<blockquote type="cite"
cite="mid:CAF9BGBwKexcEauqhcyvnpKOZW=***@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">
<pre><a href="http://cr.openjdk.java.net/%7Esspitsyn/webrevs/2018/8080406-jvmti-get-local.2/test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/GetLocalVariable/getlocal004/getlocal004.cpp.html" moz-do-not-send="true">http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8080406-jvmti-get-local.2/test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/GetLocalVariable/getlocal004/getlocal004.cpp.html</a>
</pre>
<pre> - mid is only used in one method (Java_nsk_jvmti_unit_GetLocalVariable_getlocal004_getMeth) and technically it doesn't get anything, it checks if the method is there.
</pre>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
Fixed.<br>
In general, I have no intention and time to cleanup this test, just
fixed a couple of things that are common with getlocal003.<br>
<br>
<blockquote type="cite"
cite="mid:CAF9BGBwKexcEauqhcyvnpKOZW=***@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">
<pre> - not sure you wanted the capabilities to be static in the method</pre>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
I see a lot of warnings about missing initialization of each bit if
it is initialized with caps = {0}.<br>
Making it static is the simplest way to get it initialized instead
of using local struct and memset.<br>
<br>
<br>
<blockquote type="cite"
cite="mid:CAF9BGBwKexcEauqhcyvnpKOZW=***@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">
<pre>The macros look good in <a href="http://cr.openjdk.java.net/%7Esspitsyn/webrevs/2018/8080406-jvmti-get-local.2/test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/libGetLocalVars.cpp.html" moz-do-not-send="true">http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8080406-jvmti-get-local.2/test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/libGetLocalVars.cpp.html</a>. For reference,
</pre>
<pre>I think templates are fine, I've used them in the ExceptionJniWrapper and that has worked there. Templates get initialized and generate a separate method for each type needed. So it is doing the</pre>
<pre>same as what you've done here but it does it by hand (except that you don't have to pass the method in so things are a bit easier at the call-sites so that is a win in this case as you said :))</pre>
<pre> - Note that the slot variables ByteSlot could be in the method Java_GetLocalVars_testLocals</pre>
<pre> - You gain nothing of really calling the variables in the testLocals method static, the method is called once...</pre>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
Moved these constants into testLocals().<br>
<br>
I've updated the same webrev v2 (as not that many people are
involved into this review yet):<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8080406-jvmti-get-local.2/">http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8080406-jvmti-get-local.2/</a><br>
<br>
<br>
Thanks!<br>
Serguei<br>
<br>
<blockquote type="cite"
cite="mid:CAF9BGBwKexcEauqhcyvnpKOZW=***@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">
<pre>Thanks,
</pre>
<pre>Jc</pre>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Wed, Nov 7, 2018 at 12:04 AM <a
href="mailto:***@oracle.com"
moz-do-not-send="true">***@oracle.com</a> <<a
href="mailto:***@oracle.com"
moz-do-not-send="true">***@oracle.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote">
<div>
<div class="m_5674402648451573561moz-cite-prefix">Hi Jc,<br>
<br>
The updated version of webrev:<br>
<a class="m_5674402648451573561moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esspitsyn/webrevs/2018/8080406-jvmti-get-local.2/"
target="_blank" moz-do-not-send="true">http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8080406-jvmti-get-local.2/</a><br>
<br>
I've resolved most of your comments.<br>
I used macro definitions instead of templates you
suggested.<br>
Two reasons for it:<br>
- not sure how templates depends on the compiler
versions over various platforms<br>
- macro definitions allow to make definitions more
complex but not the calls<br>
<br>
<br>
Applied the same cleanups to both old tests:<br>
getlocal003/getlocal003.cpp and
getlocal004/getlocal004.cpp<br>
<br>
Also, this update includes some change in the
VM_GetOrSetLocal implementation.<br>
It is to move the call to <span
class="m_5674402648451573561new">check_slot_type_no_lvt()
from the doit() to prologue().<br>
So, now the logic is more consistent and clear.<br>
<br>
Please, let me know what do you think.<br>
I hope that Vladimir I. will have a chance to look at
the VM changes.<br>
Also, one more review is needed on the tests side.<br>
<br>
Thanks,<br>
Serguei<br>
</span><br>
<br>
On 11/6/18 17:13, <a
class="m_5674402648451573561moz-txt-link-abbreviated"
href="mailto:***@oracle.com" target="_blank"
moz-do-not-send="true">***@oracle.com</a>
wrote:<br>
</div>
<blockquote type="cite"> Hi Jc,<br>
<br>
Thank you a lot for the code review!<br>
<br>
<div class="m_5674402648451573561moz-cite-prefix">On
11/6/18 9:22 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">Hi Serguei,</div>
<div dir="ltr"><br>
</div>
<div dir="ltr">I saw this code:<br>
<div>
<div>+ BasicType
next_slot_type =
locals->at(_index +
1)->type();<br>
</div>
</div>
<div dir="ltr"><br>
</div>
I think we are not worried about
going out of bounds due to the
work done in the check_slot_type,
which is done in doit_prologue:</div>
<div dir="ltr">
<div dir="ltr"> 643 if (_index
< 0 || _index + extra_slot
>=
method_oop->max_locals()) {</div>
<div dir="ltr"><br>
</div>
<div>Should we put an assert
though in case?</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
It is a good suggestion.<br>
But I'm thinking about moving the check_slot_type_no_lvt
call into the check_slot_type().<br>
Then most likely this assert is not going to be needed.<br> <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 dir="ltr"> <div>-> For the test <a
href="http://cr.openjdk.java.net/%7Esspitsyn/webrevs/2018/8080406-jvmti-get-local.1/test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/libGetLocalVars.cpp.html"
target="_blank"
moz-do-not-send="true">http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8080406-jvmti-get-local.1/test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/libGetLocalVars.cpp.html</a>:</div>
<div> - why not use the
TranslateError from
test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/JVMTITools.cpp<br>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
We have several other serviceability/jvmti tests that use
the same.<br>
It is not good to use the TranslateError from the the
vmTestbase library.<br>
The TranslateError would better to be copied to the global
test library.<br>
Then the TranslateError macro definition would be removed
for all of these cases as one 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 dir="ltr">
<div>
<div> - You do this in the test:</div>
<div> 371 if
(!caps.can_access_local_variables)
{</div>
<div> 372 return;</div>
<div> 373 }</div>
<div><br>
</div>
</div>
<div>But if you cannot access
local variables, on the load of
the agent you would return
JNI_ERR which I believe fails
the JVM loading, no? Hence is
this even needed?</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
Agreed - removed it.<br>
<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 dir="ltr">
<div> - We could get rid of the
caps global variable<br>
</div>
<div> - Talking about global
variables: I think you can get
rid of all of them: jvmti is
always passed as an argument,
mid is not used except to see if
the method can be found, the
slots are used only locally in
one method</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<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 dir="ltr">
<div> - Why is it PASSED but
STATUS_FAILED?</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
Nice catch, fixed.<br>
<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 dir="ltr">
<div> - With templates, you could
simplify a bit the repetitive
tests it seems:</div> <div><br> </div> <div> <div>template<typename T></div>
<div>jint testGetter(jvmtiEnv
*jvmti, jthread thr, jint
depth, jint slot, const char*
exp_type,</div>
<div> jvmtiError
(jvmtiEnv::*getter)(jthread,
jint, jint, T*),</div>
<div> const char*
getter_name) {</div>
<div> T val = 0;</div>
<div> jvmtiError err =
(jvmti->*getter)(thr,
depth, slot, &val);</div>
<div><br>
</div>
<div> printf(" %s: %s
(%d)\n", getter_name,
TranslateError(err), err);</div>
<div> if (err !=
JVMTI_ERROR_NONE) {</div>
<div> printf(" FAIL: %s
failed to get value from a
local %s\n", getter_name,
exp_type);</div>
<div> result = STATUS_FAILED;</div>
<div> } else {</div>
<div> printf(" %s got value
from a local %s as
expected\n", getter_name,
exp_type);</div>
<div> }</div>
<div>}</div>
</div>
<div><br>
</div>
<div>and then your code:</div>
<div>
<div><br>
</div>
<div> 259 test_int(jvmti, thr,
depth, slot, "byte");</div>
<div> 260
test_long_inv_slot(jvmti,
thr, depth, slot, "byte");</div>
<div> 261 test_float(jvmti,
thr, depth, slot, "byte");</div>
</div>
<div><br>
</div>
<div>Could become:</div>
<div>
<div> testGetter(jvmti, thr,
depth, slot, "byte",
&jvmtiEnv::GetLocalInt,
"GetLocalInt");</div>
<div> testGetter(jvmti, thr,
depth, slot, "byte",
&jvmtiEnv::GetLocalLong,
"GetLocalLong");</div>
<div> testGetter(jvmti, thr,
depth, slot, "byte",
&jvmtiEnv::GetLocalFloat,
"GetLocalFloat");</div>
</div>
<div><br>
</div>
<div>and by analogy, you could use
templates for the invalid and
the mismatch types.</div>
<div><br>
</div>
<div>That way, there really are
three methods written with
templates and we are just
calling them with different
types. I checked that this seems
to work with gnu++98 so it
should work for OpenJDK.</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
Thank you for the suggestion.<br>
However, I wouldn't want to go this path.<br>
I'll check if a macro can be used here in a simple way.<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 dir="ltr">
<div>For <a
href="http://cr.openjdk.java.net/%7Esspitsyn/webrevs/2018/8080406-jvmti-get-local.1/test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/GetLocalVariable/getlocal003/getlocal003.cpp.html"
target="_blank"
moz-do-not-send="true">http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8080406-jvmti-get-local.1/test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/GetLocalVariable/getlocal003/getlocal003.cpp.html</a>:</div>
<div>
<div> - I have the same remarks
for the global variables but
it is trickier because it's a
more massive rewrite of the
test there it seems<br>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
I've fixed both getlocal003.cpp and getlocal004.cpp.<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 dir="ltr">
<div> - The code you added seems
to also be able to be
templatized via something like:</div>
<div dir="ltr"><br>
</div>
<div dir="ltr"> template<typename
T></div>
<div>
<div>jint testGetter(jvmtiEnv
*jvmti, jthread thr, jint
slot, jint depth, T* value,</div>
<div> jvmtiError
(jvmtiEnv::*getter)(jthread,
jint, jint, T*),</div>
<div> const char*
getter_name,</div>
<div> char sig,</div>
<div> char
expected_sig) {</div>
<div> jvmtiError err =
(jvmti->*getter)(thr, slot,
depth, value);</div>
<div> printf(" %s: %s
(%d)\n", getter_name,
TranslateError(err), err);</div>
<div> if (err !=
JVMTI_ERROR_NONE &&
sig == expected_sig) {</div>
<div> printf("FAIL: %s failed
to get value of long\n",
getter_name);</div>
<div> result = STATUS_FAILED;</div>
<div> } else if (err !=
JVMTI_ERROR_TYPE_MISMATCH
&& sig !=
expected_sig) {</div>
<div> printf("FAIL: %s did
not return
JVMTI_ERROR_TYPE_MISMATCH for
non-long\n", getter_name);</div>
<div> result = STATUS_FAILED;</div>
<div> }</div>
<div>}</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
Thanks.<br>
Please, see my reply above.<br>
<br>
I'll send an updated webrev in a separate email.<br>
<br>
Thanks!<br>
Serguei<br>
<br
class="m_5674402648451573561gmail-Apple-interchange-newline">
<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 class="gmail_quote">
<div dir="ltr">Apart from that,
it looks good to me, these are
mostly style choices I suppose
and trying to reduce code
repetitiveness :)<br>
</div>
<div>Jc</div>
<div><br>
</div>
<div dir="ltr">On Mon, Nov 5,
2018 at 9:36 PM <a
href="mailto:***@oracle.com"
target="_blank"
moz-do-not-send="true">***@oracle.com</a>
<<a
href="mailto:***@oracle.com"
target="_blank"
moz-do-not-send="true">***@oracle.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote">
<div> Please, review a fix
for:<br>
<a
class="m_5674402648451573561gmail-m_1712734469379532758m_-1627105428213637291moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8080406" target="_blank"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8080406</a><br>
<br>
Webrev:<br>
<a
class="m_5674402648451573561gmail-m_1712734469379532758m_-1627105428213637291moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esspitsyn/webrevs/2018/8080406-jvmti-get-local.1/"
target="_blank"
moz-do-not-send="true">http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8080406-jvmti-get-local.1/</a><br>
<br>
<br>
Summary:<br>
The JVMTI
GetLocal<Type>/SetLocal<Type>
implementation type checking
is based<br>
on LVT (Local Variable
Table) content. But there is
almost no type check if LVT<br>
is not present in class
file. This fix is an attempt
to fill in the gap.<br>
When LVT is absent, one
issue is that just 3 types
are available in the<br>
StackValueCollectionfor
locals at runtime:<br>
- T_OBJECT: if local
is an object<br>
- T_INT: if local
is a primitive type<br>
- T_CONFLICT: if local
is not valid at current
location<br>
So there is no way to
distinguish primitive types
unless the requested type<br>
occupies two slots and
actual second slot is not
T_INT or is out of locals
area.<br>
<br>
Testing:<br>
Tested locally on
Linux-x64 with:<br>
- 1 new jtreg test:
hotspot/jtreg/serviceability/jvmti/GetLocalVariable<br>
- 2 nsk jtreg tests:
hotspot/jtreg/vmTestbase/nsk/jvmti/unit/GetLocalVariable<br>
- 2 nsk jtreg tests:
hotspot/jtreg/vmTestbase/nsk/jvmti/GetLocalVariable<br>
- 4 nsk jtreg tests:
hotspot/jtreg/vmTestbase/nsk/jvmti/SetLocalVariable<br>
<br>
In progress:<br>
The same as above but
with mach5 in different
configs.<br>
<br>
Thanks,<br>
Serguei<br>
</div>
</blockquote>
</div>
<br>
<div><br>
</div>
-- <br>
<div dir="ltr"
class="m_5674402648451573561gmail-m_1712734469379532758gmail_signature">
<div dir="ltr">
<div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
</blockquote>
<br>
</div>
</blockquote>
</div>
<br>
<div><br>
</div>
-- <br>
<div dir="ltr" class="gmail_signature"
data-smartmail="gmail_signature">
<div dir="ltr">
<div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</blockquote>
<br>
</body>
</html>