<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/7/18 12:37,
<a class="moz-txt-link-abbreviated" href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> wrote:<br>
</div>
<blockquote type="cite"
cite="mid:7e314e8f-7c07-8f8f-0365-39a627dbf7cc@oracle.com">
<div class="moz-cite-prefix">Thank you a lot for review, Chris!<br>
Serguei<br>
<br>
On 11/7/18 12:35, Chris Plummer wrote:<br>
</div>
<blockquote type="cite"
cite="mid:36f739b0-e44d-0092-5dc7-99da2132cc0d@oracle.com">
<div class="moz-cite-prefix">Hi Serguei,<br>
<br>
My review wasn't that thorough, but I think JC has given this
enough scrutiny so it looks ok ot me. Just one minor typo:<br>
<br>
 344        printf("\n Success: locations of vars with slot
#2 are NOT overlaped\n");<br>
<br>
Should be "overlapped". Also the same error is on line 336.<br>
</div>
</blockquote>
</blockquote>
<br>
Forgot to tell - fixed this typos.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<blockquote type="cite"
cite="mid:7e314e8f-7c07-8f8f-0365-39a627dbf7cc@oracle.com">
<blockquote type="cite"
cite="mid:36f739b0-e44d-0092-5dc7-99da2132cc0d@oracle.com">
<div class="moz-cite-prefix"> <br>
thanks,<br>
<br>
Chris<br>
<br>
On 11/7/18 12:04 AM, <a class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com"
moz-do-not-send="true">serguei.spitsyn@oracle.com</a> wrote:<br>
</div>
<blockquote type="cite"
cite="mid:d97befc8-e405-af4e-4271-6f586877f5df@oracle.com">
<div class="moz-cite-prefix">Hi Jc,<br>
<br>
The updated version of webrev:<br>
 <a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esspitsyn/webrevs/2018/8080406-jvmti-get-local.2/"
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="new">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="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com"
moz-do-not-send="true">serguei.spitsyn@oracle.com</a>
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:0f7593a9-f7b5-f820-1276-7bf68d3c9c00@oracle.com">
Hi Jc,<br>
<br>
Thank you a lot for the code review!<br>
<br>
<div class="moz-cite-prefix">On 11/6/18 9:22 AM, JC Beyler
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAF9BGBxeSrexT1TwhQzD1a-jD8d4PS+-HoztA7nBZMBns0Dnkg@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 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"
cite="mid:CAF9BGBxeSrexT1TwhQzD1a-jD8d4PS+-HoztA7nBZMBns0Dnkg@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 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"
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"
cite="mid:CAF9BGBxeSrexT1TwhQzD1a-jD8d4PS+-HoztA7nBZMBns0Dnkg@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 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"
cite="mid:CAF9BGBxeSrexT1TwhQzD1a-jD8d4PS+-HoztA7nBZMBns0Dnkg@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 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"
cite="mid:CAF9BGBxeSrexT1TwhQzD1a-jD8d4PS+-HoztA7nBZMBns0Dnkg@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 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"
cite="mid:CAF9BGBxeSrexT1TwhQzD1a-jD8d4PS+-HoztA7nBZMBns0Dnkg@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 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"
cite="mid:CAF9BGBxeSrexT1TwhQzD1a-jD8d4PS+-HoztA7nBZMBns0Dnkg@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 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"
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"
cite="mid:CAF9BGBxeSrexT1TwhQzD1a-jD8d4PS+-HoztA7nBZMBns0Dnkg@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 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="gmail-Apple-interchange-newline">
<blockquote type="cite"
cite="mid:CAF9BGBxeSrexT1TwhQzD1a-jD8d4PS+-HoztA7nBZMBns0Dnkg@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 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:serguei.spitsyn@oracle.com"
target="_blank"
moz-do-not-send="true">serguei.spitsyn@oracle.com</a>
<<a
href="mailto:serguei.spitsyn@oracle.com"
target="_blank"
moz-do-not-send="true">serguei.spitsyn@oracle.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote">
<div> Please, review a fix for:<br>
 <a
class="gmail-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="gmail-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="gmail-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>
</blockquote>
<p><br>
</p>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>