Discussion:
RFR (M) 8080406: VM_GetOrSetLocal doesn't check local slot type against requested type
(too old to reply)
s***@oracle.com
2018-11-06 05:33:58 UTC
Permalink
<html>
<head>

<meta http-equiv="content-type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Please, review a fix for:<br>
  <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8080406">https://bugs.openjdk.java.net/browse/JDK-8080406</a><br>
<br>
<font face="Monaco">Webrev:<br>
 
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8080406-jvmti-get-local.1/">http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8080406-jvmti-get-local.1/</a></font><br>
<br>
<br>
Summary:<br>
  The JVMTI GetLocal&lt;Type&gt;/SetLocal&lt;Type&gt; 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>
</body>
</html>
JC Beyler
2018-11-06 17:22:19 UTC
Permalink
Hi Serguei,

I saw this code:
+ BasicType next_slot_type = locals->at(_index + 1)->type();

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:
643 if (_index < 0 || _index + extra_slot >= method_oop->max_locals())
{

Should we put an assert though in case?

-> For the test
http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8080406-jvmti-get-local.1/test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/libGetLocalVars.cpp.html
:
- why not use the TranslateError from
test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/JVMTITools.cpp

- You do this in the test:
371 if (!caps.can_access_local_variables) {
372 return;
373 }

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?

- We could get rid of the caps global variable
- 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
- Why is it PASSED but STATUS_FAILED?

- With templates, you could simplify a bit the repetitive tests it seems:

template<typename T>
jint testGetter(jvmtiEnv *jvmti, jthread thr, jint depth, jint slot, const
char* exp_type,
jvmtiError (jvmtiEnv::*getter)(jthread, jint, jint, T*),
const char* getter_name) {
T val = 0;
jvmtiError err = (jvmti->*getter)(thr, depth, slot, &val);

printf(" %s: %s (%d)\n", getter_name, TranslateError(err), err);
if (err != JVMTI_ERROR_NONE) {
printf(" FAIL: %s failed to get value from a local %s\n", getter_name,
exp_type);
result = STATUS_FAILED;
} else {
printf(" %s got value from a local %s as expected\n", getter_name,
exp_type);
}
}

and then your code:

259 test_int(jvmti, thr, depth, slot, "byte");
260 test_long_inv_slot(jvmti, thr, depth, slot, "byte");
261 test_float(jvmti, thr, depth, slot, "byte");

Could become:
testGetter(jvmti, thr, depth, slot, "byte", &jvmtiEnv::GetLocalInt,
"GetLocalInt");
testGetter(jvmti, thr, depth, slot, "byte", &jvmtiEnv::GetLocalLong,
"GetLocalLong");
testGetter(jvmti, thr, depth, slot, "byte", &jvmtiEnv::GetLocalFloat,
"GetLocalFloat");

and by analogy, you could use templates for the invalid and the mismatch
types.

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.


For
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
:
- 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
- The code you added seems to also be able to be templatized via
something like:

template<typename T>
jint testGetter(jvmtiEnv *jvmti, jthread thr, jint slot, jint depth, T*
value,
jvmtiError (jvmtiEnv::*getter)(jthread, jint, jint, T*),
const char* getter_name,
char sig,
char expected_sig) {
jvmtiError err = (jvmti->*getter)(thr, slot, depth, value);
printf(" %s: %s (%d)\n", getter_name, TranslateError(err), err);
if (err != JVMTI_ERROR_NONE && sig == expected_sig) {
printf("FAIL: %s failed to get value of long\n", getter_name);
result = STATUS_FAILED;
} else if (err != JVMTI_ERROR_TYPE_MISMATCH && sig != expected_sig) {
printf("FAIL: %s did not return JVMTI_ERROR_TYPE_MISMATCH for
non-long\n", getter_name);
result = STATUS_FAILED;
}
}

Apart from that, it looks good to me, these are mostly style choices I
suppose and trying to reduce code repetitiveness :)
Jc
Post by s***@oracle.com
https://bugs.openjdk.java.net/browse/JDK-8080406
http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8080406-jvmti-get-local.1/
The JVMTI GetLocal<Type>/SetLocal<Type> implementation type checking is
based
on LVT (Local Variable Table) content. But there is almost no type check
if LVT
is not present in class file. This fix is an attempt to fill in the gap.
When LVT is absent, one issue is that just 3 types are available in the
- T_OBJECT: if local is an object
- T_INT: if local is a primitive type
- T_CONFLICT: if local is not valid at current location
So there is no way to distinguish primitive types unless the requested
type
occupies two slots and actual second slot is not T_INT or is out of
locals area.
hotspot/jtreg/serviceability/jvmti/GetLocalVariable
hotspot/jtreg/vmTestbase/nsk/jvmti/unit/GetLocalVariable
hotspot/jtreg/vmTestbase/nsk/jvmti/GetLocalVariable
hotspot/jtreg/vmTestbase/nsk/jvmti/SetLocalVariable
The same as above but with mach5 in different configs.
Thanks,
Serguei
--
Thanks,
Jc
s***@oracle.com
2018-11-07 01:13:17 UTC
Permalink
Hi Jc,

Thank you a lot for the code review!
Post by JC Beyler
Hi Serguei,
+    BasicType next_slot_type = locals->at(_index + 1)->type();
I think we are not worried about going out of bounds due to the work
 643     if (_index < 0 || _index + extra_slot >=
method_oop->max_locals()) {
Should we put an assert though in case?
It is a good suggestion.
But I'm thinking about moving the check_slot_type_no_lvt call into the
check_slot_type().
Then most likely this assert is not going to be needed.
Post by JC Beyler
-> For the test
http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8080406-jvmti-get-local.1/test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/libGetLocalVars.cpp.html
 - why not use the TranslateError from
test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/JVMTITools.cpp
We have several other serviceability/jvmti tests that use the same.
It is not good to use the TranslateError from the the vmTestbase library.
The TranslateError would better to be copied to the global test library.
Then the TranslateError macro definition would be removed for all of
these cases as one action.
Post by JC Beyler
 371   if (!caps.can_access_local_variables) {
 372     return;
 373   }
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?
Agreed - removed it.
Post by JC Beyler
 - We could get rid of the caps global variable
   - 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
   - Why is it PASSED but STATUS_FAILED?
Nice catch, fixed.
Post by JC Beyler
  - With templates, you could simplify a bit the repetitive tests it
template<typename T>
jint testGetter(jvmtiEnv *jvmti, jthread thr, jint depth, jint slot,
const char* exp_type,
              jvmtiError (jvmtiEnv::*getter)(jthread, jint, jint, T*),
              const char* getter_name) {
  T val = 0;
  jvmtiError err = (jvmti->*getter)(thr, depth, slot, &val);
  printf(" %s:     %s (%d)\n", getter_name, TranslateError(err), err);
  if (err != JVMTI_ERROR_NONE) {
    printf(" FAIL: %s failed to get value from a local %s\n",
getter_name, exp_type);
    result = STATUS_FAILED;
  } else {
    printf(" %s got value from a local %s as expected\n", getter_name,
exp_type);
  }
}
 259   test_int(jvmti, thr, depth, slot, "byte");
 260   test_long_inv_slot(jvmti, thr, depth, slot, "byte");
 261   test_float(jvmti, thr, depth, slot, "byte");
  testGetter(jvmti, thr, depth, slot, "byte", &jvmtiEnv::GetLocalInt,
"GetLocalInt");
  testGetter(jvmti, thr, depth, slot, "byte", &jvmtiEnv::GetLocalLong,
"GetLocalLong");
  testGetter(jvmti, thr, depth, slot, "byte",
&jvmtiEnv::GetLocalFloat, "GetLocalFloat");
and by analogy, you could use templates for the invalid and the
mismatch types.
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.
Thank you for the suggestion.
However, I wouldn't want to go this path.
I'll check if a macro can be used here in a simple way.
Post by JC Beyler
For
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
  - 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
I've fixed both getlocal003.cpp and getlocal004.cpp.
Post by JC Beyler
  - The code you added seems to also be able to be templatized via
 template<typename T>
jint testGetter(jvmtiEnv *jvmti, jthread thr, jint slot, jint depth,
T* value,
                jvmtiError (jvmtiEnv::*getter)(jthread, jint, jint, T*),
                const char* getter_name,
                char sig,
                char expected_sig) {
   jvmtiError err = (jvmti->*getter)(thr, slot, depth, value);
  printf(" %s:    %s (%d)\n", getter_name, TranslateError(err), err);
  if (err != JVMTI_ERROR_NONE && sig == expected_sig) {
    printf("FAIL: %s failed to get value of long\n", getter_name);
    result = STATUS_FAILED;
  } else if (err != JVMTI_ERROR_TYPE_MISMATCH && sig != expected_sig) {
    printf("FAIL: %s did not return JVMTI_ERROR_TYPE_MISMATCH for
non-long\n", getter_name);
    result = STATUS_FAILED;
  }
}
Thanks.
Please, see my reply above.

I'll send an updated webrev in a separate email.

Thanks!
Serguei
Post by JC Beyler
Apart from that, it looks good to me, these are mostly style choices I
suppose and trying to reduce code repetitiveness :)
Jc
https://bugs.openjdk.java.net/browse/JDK-8080406
http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8080406-jvmti-get-local.1/
<http://cr.openjdk.java.net/%7Esspitsyn/webrevs/2018/8080406-jvmti-get-local.1/>
  The JVMTI GetLocal<Type>/SetLocal<Type> implementation type
checking is based
  on LVT (Local Variable Table) content. But there is almost no
type check if LVT
  is not present in class file. This fix is an attempt to fill in
the gap.
  When LVT is absent, one issue is that just 3 types are available
in the
    - T_OBJECT:   if local is an object
    - T_INT:      if local is a primitive type
    - T_CONFLICT: if local is not valid at current location
  So there is no way to distinguish primitive types unless the
requested type
  occupies two slots and actual second slot is not T_INT or is out
of locals area.
hotspot/jtreg/serviceability/jvmti/GetLocalVariable
hotspot/jtreg/vmTestbase/nsk/jvmti/unit/GetLocalVariable
hotspot/jtreg/vmTestbase/nsk/jvmti/GetLocalVariable
hotspot/jtreg/vmTestbase/nsk/jvmti/SetLocalVariable
    The same as above but with mach5 in different configs.
Thanks,
Serguei
--
Thanks,
Jc
s***@oracle.com
2018-11-07 08:04:01 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 updated version of webrev:<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>
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:***@oracle.com">***@oracle.com</a> wrote:<br>
</div>
<blockquote type="cite"
cite="mid:0f7593a9-f7b5-f820-1276-***@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+-***@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-&gt;at(_index + 1)-&gt;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 &lt; 0
|| _index + extra_slot &gt;=
method_oop-&gt;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+-***@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>-&gt; 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+-***@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+-***@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+-***@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+-***@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&lt;typename T&gt;</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-&gt;*getter)(thr, depth, slot,
&amp;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",
&amp;jvmtiEnv::GetLocalInt,
"GetLocalInt");</div>
<div>  testGetter(jvmti, thr, depth,
slot, "byte",
&amp;jvmtiEnv::GetLocalLong,
"GetLocalLong");</div>
<div>  testGetter(jvmti, thr, depth,
slot, "byte",
&amp;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+-***@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+-***@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&lt;typename T&gt;</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-&gt;*getter)(thr, slot, depth,
value);</div>
<div>  printf(" %s:    %s (%d)\n",
getter_name, TranslateError(err),
err);</div>
<div>  if (err != JVMTI_ERROR_NONE
&amp;&amp; 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 &amp;&amp;
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+-***@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:***@oracle.com"
target="_blank"
moz-do-not-send="true">***@oracle.com</a>
&lt;<a
href="mailto:***@oracle.com"
target="_blank"
moz-do-not-send="true">***@oracle.com</a>&gt;
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&lt;Type&gt;/SetLocal&lt;Type&gt;
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>
</body>
</html>
JC Beyler
2018-11-07 17:30:12 UTC
Permalink
Hi Serguei,

I like the change you made in the VM, it is more clean and easier to see
what is going on :)

A few nits:

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

- There is a type: "Succes:" line 342.
- There is this check:

272 if (jvmti == NULL) {
273 return;
274 }

but the same check line 109 fails the test and prints a message.


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

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

- not sure you wanted the capabilities to be static in the method


The macros look good in
http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8080406-jvmti-get-local.2/test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/libGetLocalVars.cpp.html.
For reference,

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

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

- Note that the slot variables ByteSlot could be in the method
Java_GetLocalVars_testLocals

- You gain nothing of really calling the variables in the testLocals
method static, the method is called once...


Thanks,

Jc
Post by s***@oracle.com
Hi Jc,
http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8080406-jvmti-get-local.2/
I've resolved most of your comments.
I used macro definitions instead of templates you suggested.
- not sure how templates depends on the compiler versions over various
platforms
- macro definitions allow to make definitions more complex but not the
calls
getlocal003/getlocal003.cpp and getlocal004/getlocal004.cpp
Also, this update includes some change in the VM_GetOrSetLocal
implementation.
It is to move the call to check_slot_type_no_lvt() from the doit() to
prologue().
So, now the logic is more consistent and clear.
Please, let me know what do you think.
I hope that Vladimir I. will have a chance to look at the VM changes.
Also, one more review is needed on the tests side.
Thanks,
Serguei
Hi Jc,
Thank you a lot for the code review!
Hi Serguei,
+ BasicType next_slot_type = locals->at(_index + 1)->type();
I think we are not worried about going out of bounds due to the work done
643 if (_index < 0 || _index + extra_slot >=
method_oop->max_locals()) {
Should we put an assert though in case?
It is a good suggestion.
But I'm thinking about moving the check_slot_type_no_lvt call into the
check_slot_type().
Then most likely this assert is not going to be needed.
-> For the test
http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8080406-jvmti-get-local.1/test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/libGetLocalVars.cpp.html
- why not use the TranslateError from
test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/JVMTITools.cpp
We have several other serviceability/jvmti tests that use the same.
It is not good to use the TranslateError from the the vmTestbase library.
The TranslateError would better to be copied to the global test library.
Then the TranslateError macro definition would be removed for all of these
cases as one action.
371 if (!caps.can_access_local_variables) {
372 return;
373 }
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?
Agreed - removed it.
- We could get rid of the caps global variable
- 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
- Why is it PASSED but STATUS_FAILED?
Nice catch, fixed.
template<typename T>
jint testGetter(jvmtiEnv *jvmti, jthread thr, jint depth, jint slot, const
char* exp_type,
jvmtiError (jvmtiEnv::*getter)(jthread, jint, jint, T*),
const char* getter_name) {
T val = 0;
jvmtiError err = (jvmti->*getter)(thr, depth, slot, &val);
printf(" %s: %s (%d)\n", getter_name, TranslateError(err), err);
if (err != JVMTI_ERROR_NONE) {
printf(" FAIL: %s failed to get value from a local %s\n", getter_name,
exp_type);
result = STATUS_FAILED;
} else {
printf(" %s got value from a local %s as expected\n", getter_name,
exp_type);
}
}
259 test_int(jvmti, thr, depth, slot, "byte");
260 test_long_inv_slot(jvmti, thr, depth, slot, "byte");
261 test_float(jvmti, thr, depth, slot, "byte");
testGetter(jvmti, thr, depth, slot, "byte", &jvmtiEnv::GetLocalInt,
"GetLocalInt");
testGetter(jvmti, thr, depth, slot, "byte", &jvmtiEnv::GetLocalLong,
"GetLocalLong");
testGetter(jvmti, thr, depth, slot, "byte", &jvmtiEnv::GetLocalFloat,
"GetLocalFloat");
and by analogy, you could use templates for the invalid and the mismatch
types.
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.
Thank you for the suggestion.
However, I wouldn't want to go this path.
I'll check if a macro can be used here in a simple way.
For
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
- 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
I've fixed both getlocal003.cpp and getlocal004.cpp.
- The code you added seems to also be able to be templatized via
template<typename T>
jint testGetter(jvmtiEnv *jvmti, jthread thr, jint slot, jint depth, T*
value,
jvmtiError (jvmtiEnv::*getter)(jthread, jint, jint, T*),
const char* getter_name,
char sig,
char expected_sig) {
jvmtiError err = (jvmti->*getter)(thr, slot, depth, value);
printf(" %s: %s (%d)\n", getter_name, TranslateError(err), err);
if (err != JVMTI_ERROR_NONE && sig == expected_sig) {
printf("FAIL: %s failed to get value of long\n", getter_name);
result = STATUS_FAILED;
} else if (err != JVMTI_ERROR_TYPE_MISMATCH && sig != expected_sig) {
printf("FAIL: %s did not return JVMTI_ERROR_TYPE_MISMATCH for
non-long\n", getter_name);
result = STATUS_FAILED;
}
}
Thanks.
Please, see my reply above.
I'll send an updated webrev in a separate email.
Thanks!
Serguei
Apart from that, it looks good to me, these are mostly style choices I
suppose and trying to reduce code repetitiveness :)
Jc
Post by s***@oracle.com
https://bugs.openjdk.java.net/browse/JDK-8080406
http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8080406-jvmti-get-local.1/
The JVMTI GetLocal<Type>/SetLocal<Type> implementation type checking is
based
on LVT (Local Variable Table) content. But there is almost no type
check if LVT
is not present in class file. This fix is an attempt to fill in the gap.
When LVT is absent, one issue is that just 3 types are available in the
- T_OBJECT: if local is an object
- T_INT: if local is a primitive type
- T_CONFLICT: if local is not valid at current location
So there is no way to distinguish primitive types unless the requested
type
occupies two slots and actual second slot is not T_INT or is out of
locals area.
hotspot/jtreg/serviceability/jvmti/GetLocalVariable
hotspot/jtreg/vmTestbase/nsk/jvmti/unit/GetLocalVariable
hotspot/jtreg/vmTestbase/nsk/jvmti/GetLocalVariable
hotspot/jtreg/vmTestbase/nsk/jvmti/SetLocalVariable
The same as above but with mach5 in different configs.
Thanks,
Serguei
--
Thanks,
Jc
--
Thanks,
Jc
s***@oracle.com
2018-11-07 19:28: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">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> &lt;<a
href="mailto:***@oracle.com"
moz-do-not-send="true">***@oracle.com</a>&gt;
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-&gt;at(_index +
1)-&gt;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
&lt; 0 || _index + extra_slot
&gt;=
method_oop-&gt;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>-&gt; 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&lt;typename T&gt;</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-&gt;*getter)(thr,
depth, slot, &amp;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",
&amp;jvmtiEnv::GetLocalInt,
"GetLocalInt");</div>
<div>  testGetter(jvmti, thr,
depth, slot, "byte",
&amp;jvmtiEnv::GetLocalLong,
"GetLocalLong");</div>
<div>  testGetter(jvmti, thr,
depth, slot, "byte",
&amp;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&lt;typename
T&gt;</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-&gt;*getter)(thr, slot,
depth, value);</div>
<div>  printf(" %s:    %s
(%d)\n", getter_name,
TranslateError(err), err);</div>
<div>  if (err !=
JVMTI_ERROR_NONE &amp;&amp;
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
&amp;&amp; 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>
&lt;<a
href="mailto:***@oracle.com"
target="_blank"
moz-do-not-send="true">***@oracle.com</a>&gt;
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&lt;Type&gt;/SetLocal&lt;Type&gt;
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>
Chris Plummer
2018-11-07 20:35:59 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 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>
<br>
thanks,<br>
<br>
Chris<br>
<br>
On 11/7/18 12:04 AM, <a class="moz-txt-link-abbreviated" href="mailto:***@oracle.com">***@oracle.com</a> wrote:<br>
</div>
<blockquote type="cite"
cite="mid:d97befc8-e405-af4e-4271-***@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<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:***@oracle.com"
moz-do-not-send="true">***@oracle.com</a> wrote:<br>
</div>
<blockquote type="cite"
cite="mid:0f7593a9-f7b5-f820-1276-***@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+-***@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-&gt;at(_index +
1)-&gt;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 &lt;
0 || _index + extra_slot &gt;=
method_oop-&gt;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+-***@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>-&gt; 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+-***@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+-***@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+-***@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+-***@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&lt;typename T&gt;</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-&gt;*getter)(thr, depth,
slot, &amp;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",
&amp;jvmtiEnv::GetLocalInt,
"GetLocalInt");</div>
<div>  testGetter(jvmti, thr, depth,
slot, "byte",
&amp;jvmtiEnv::GetLocalLong,
"GetLocalLong");</div>
<div>  testGetter(jvmti, thr, depth,
slot, "byte",
&amp;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+-***@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+-***@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&lt;typename
T&gt;</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-&gt;*getter)(thr, slot,
depth, value);</div>
<div>  printf(" %s:    %s (%d)\n",
getter_name, TranslateError(err),
err);</div>
<div>  if (err != JVMTI_ERROR_NONE
&amp;&amp; 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 &amp;&amp;
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+-***@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:***@oracle.com"
target="_blank"
moz-do-not-send="true">***@oracle.com</a>
&lt;<a
href="mailto:***@oracle.com"
target="_blank"
moz-do-not-send="true">***@oracle.com</a>&gt;
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&lt;Type&gt;/SetLocal&lt;Type&gt;
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>
</body>
</html>
s***@oracle.com
2018-11-07 20:37:41 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">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-***@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>
<br>
thanks,<br>
<br>
Chris<br>
<br>
On 11/7/18 12:04 AM, <a class="moz-txt-link-abbreviated"
href="mailto:***@oracle.com"
moz-do-not-send="true">***@oracle.com</a> wrote:<br>
</div>
<blockquote type="cite"
cite="mid:d97befc8-e405-af4e-4271-***@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:***@oracle.com"
moz-do-not-send="true">***@oracle.com</a> wrote:<br>
</div>
<blockquote type="cite"
cite="mid:0f7593a9-f7b5-f820-1276-***@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+-***@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-&gt;at(_index +
1)-&gt;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
&lt; 0 || _index + extra_slot &gt;=
method_oop-&gt;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+-***@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>-&gt; 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+-***@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+-***@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+-***@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+-***@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&lt;typename T&gt;</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-&gt;*getter)(thr, depth,
slot, &amp;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",
&amp;jvmtiEnv::GetLocalInt,
"GetLocalInt");</div>
<div>  testGetter(jvmti, thr, depth,
slot, "byte",
&amp;jvmtiEnv::GetLocalLong,
"GetLocalLong");</div>
<div>  testGetter(jvmti, thr, depth,
slot, "byte",
&amp;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+-***@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+-***@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&lt;typename
T&gt;</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-&gt;*getter)(thr, slot,
depth, value);</div>
<div>  printf(" %s:    %s (%d)\n",
getter_name, TranslateError(err),
err);</div>
<div>  if (err != JVMTI_ERROR_NONE
&amp;&amp; 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
&amp;&amp; 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+-***@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:***@oracle.com"
target="_blank"
moz-do-not-send="true">***@oracle.com</a>
&lt;<a
href="mailto:***@oracle.com"
target="_blank"
moz-do-not-send="true">***@oracle.com</a>&gt;
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&lt;Type&gt;/SetLocal&lt;Type&gt;
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>
</body>
</html>
s***@oracle.com
2018-11-07 20:46:19 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/7/18 12:37,
<a class="moz-txt-link-abbreviated" href="mailto:***@oracle.com">***@oracle.com</a> wrote:<br>
</div>
<blockquote type="cite"
cite="mid:7e314e8f-7c07-8f8f-0365-***@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-***@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-***@oracle.com">
<blockquote type="cite"
cite="mid:36f739b0-e44d-0092-5dc7-***@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:***@oracle.com"
moz-do-not-send="true">***@oracle.com</a> wrote:<br>
</div>
<blockquote type="cite"
cite="mid:d97befc8-e405-af4e-4271-***@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:***@oracle.com"
moz-do-not-send="true">***@oracle.com</a>
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:0f7593a9-f7b5-f820-1276-***@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+-***@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-&gt;at(_index +
1)-&gt;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
&lt; 0 || _index + extra_slot
&gt;= method_oop-&gt;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+-***@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>-&gt; 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+-***@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+-***@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+-***@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+-***@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&lt;typename T&gt;</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-&gt;*getter)(thr, depth,
slot, &amp;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",
&amp;jvmtiEnv::GetLocalInt,
"GetLocalInt");</div>
<div>  testGetter(jvmti, thr,
depth, slot, "byte",
&amp;jvmtiEnv::GetLocalLong,
"GetLocalLong");</div>
<div>  testGetter(jvmti, thr,
depth, slot, "byte",
&amp;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+-***@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+-***@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&lt;typename
T&gt;</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-&gt;*getter)(thr, slot,
depth, value);</div>
<div>  printf(" %s:    %s (%d)\n",
getter_name,
TranslateError(err), err);</div>
<div>  if (err != JVMTI_ERROR_NONE
&amp;&amp; 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
&amp;&amp; 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+-***@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:***@oracle.com"
target="_blank"
moz-do-not-send="true">***@oracle.com</a>
&lt;<a
href="mailto:***@oracle.com"
target="_blank"
moz-do-not-send="true">***@oracle.com</a>&gt;
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&lt;Type&gt;/SetLocal&lt;Type&gt;
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>
JC Beyler
2018-11-08 05:55:41 UTC
Permalink
Hi Serguei,

If ever there was a doubt: looks good to me too now :)

Thanks!
Jc
Post by s***@oracle.com
Thank you a lot for review, Chris!
Serguei
Hi Serguei,
My review wasn't that thorough, but I think JC has given this enough
344 printf("\n Success: locations of vars with slot #2 are NOT
overlaped\n");
Should be "overlapped". Also the same error is on line 336.
Forgot to tell - fixed this typos.
Thanks,
Serguei
thanks,
Chris
Hi Jc,
http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8080406-jvmti-get-local.2/
I've resolved most of your comments.
I used macro definitions instead of templates you suggested.
- not sure how templates depends on the compiler versions over various
platforms
- macro definitions allow to make definitions more complex but not the
calls
getlocal003/getlocal003.cpp and getlocal004/getlocal004.cpp
Also, this update includes some change in the VM_GetOrSetLocal
implementation.
It is to move the call to check_slot_type_no_lvt() from the doit() to
prologue().
So, now the logic is more consistent and clear.
Please, let me know what do you think.
I hope that Vladimir I. will have a chance to look at the VM changes.
Also, one more review is needed on the tests side.
Thanks,
Serguei
Hi Jc,
Thank you a lot for the code review!
Hi Serguei,
+ BasicType next_slot_type = locals->at(_index + 1)->type();
I think we are not worried about going out of bounds due to the work done
643 if (_index < 0 || _index + extra_slot >=
method_oop->max_locals()) {
Should we put an assert though in case?
It is a good suggestion.
But I'm thinking about moving the check_slot_type_no_lvt call into the
check_slot_type().
Then most likely this assert is not going to be needed.
-> For the test
http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8080406-jvmti-get-local.1/test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/libGetLocalVars.cpp.html
- why not use the TranslateError from
test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/JVMTITools.cpp
We have several other serviceability/jvmti tests that use the same.
It is not good to use the TranslateError from the the vmTestbase library.
The TranslateError would better to be copied to the global test library.
Then the TranslateError macro definition would be removed for all of these
cases as one action.
371 if (!caps.can_access_local_variables) {
372 return;
373 }
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?
Agreed - removed it.
- We could get rid of the caps global variable
- 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
- Why is it PASSED but STATUS_FAILED?
Nice catch, fixed.
template<typename T>
jint testGetter(jvmtiEnv *jvmti, jthread thr, jint depth, jint slot, const
char* exp_type,
jvmtiError (jvmtiEnv::*getter)(jthread, jint, jint, T*),
const char* getter_name) {
T val = 0;
jvmtiError err = (jvmti->*getter)(thr, depth, slot, &val);
printf(" %s: %s (%d)\n", getter_name, TranslateError(err), err);
if (err != JVMTI_ERROR_NONE) {
printf(" FAIL: %s failed to get value from a local %s\n", getter_name,
exp_type);
result = STATUS_FAILED;
} else {
printf(" %s got value from a local %s as expected\n", getter_name,
exp_type);
}
}
259 test_int(jvmti, thr, depth, slot, "byte");
260 test_long_inv_slot(jvmti, thr, depth, slot, "byte");
261 test_float(jvmti, thr, depth, slot, "byte");
testGetter(jvmti, thr, depth, slot, "byte", &jvmtiEnv::GetLocalInt,
"GetLocalInt");
testGetter(jvmti, thr, depth, slot, "byte", &jvmtiEnv::GetLocalLong,
"GetLocalLong");
testGetter(jvmti, thr, depth, slot, "byte", &jvmtiEnv::GetLocalFloat,
"GetLocalFloat");
and by analogy, you could use templates for the invalid and the mismatch
types.
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.
Thank you for the suggestion.
However, I wouldn't want to go this path.
I'll check if a macro can be used here in a simple way.
For
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
- 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
I've fixed both getlocal003.cpp and getlocal004.cpp.
- The code you added seems to also be able to be templatized via
template<typename T>
jint testGetter(jvmtiEnv *jvmti, jthread thr, jint slot, jint depth, T*
value,
jvmtiError (jvmtiEnv::*getter)(jthread, jint, jint, T*),
const char* getter_name,
char sig,
char expected_sig) {
jvmtiError err = (jvmti->*getter)(thr, slot, depth, value);
printf(" %s: %s (%d)\n", getter_name, TranslateError(err), err);
if (err != JVMTI_ERROR_NONE && sig == expected_sig) {
printf("FAIL: %s failed to get value of long\n", getter_name);
result = STATUS_FAILED;
} else if (err != JVMTI_ERROR_TYPE_MISMATCH && sig != expected_sig) {
printf("FAIL: %s did not return JVMTI_ERROR_TYPE_MISMATCH for
non-long\n", getter_name);
result = STATUS_FAILED;
}
}
Thanks.
Please, see my reply above.
I'll send an updated webrev in a separate email.
Thanks!
Serguei
Apart from that, it looks good to me, these are mostly style choices I
suppose and trying to reduce code repetitiveness :)
Jc
Post by s***@oracle.com
https://bugs.openjdk.java.net/browse/JDK-8080406
http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8080406-jvmti-get-local.1/
The JVMTI GetLocal<Type>/SetLocal<Type> implementation type checking is
based
on LVT (Local Variable Table) content. But there is almost no type
check if LVT
is not present in class file. This fix is an attempt to fill in the gap.
When LVT is absent, one issue is that just 3 types are available in the
- T_OBJECT: if local is an object
- T_INT: if local is a primitive type
- T_CONFLICT: if local is not valid at current location
So there is no way to distinguish primitive types unless the requested
type
occupies two slots and actual second slot is not T_INT or is out of
locals area.
hotspot/jtreg/serviceability/jvmti/GetLocalVariable
hotspot/jtreg/vmTestbase/nsk/jvmti/unit/GetLocalVariable
hotspot/jtreg/vmTestbase/nsk/jvmti/GetLocalVariable
hotspot/jtreg/vmTestbase/nsk/jvmti/SetLocalVariable
The same as above but with mach5 in different configs.
Thanks,
Serguei
--
Thanks,
Jc
--
Thanks,
Jc
s***@oracle.com
2018-11-08 06:04:00 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">Thanks a lot, Jc!<br>
Serguei<br>
<br>
<br>
On 11/7/18 21:55, JC Beyler wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAF9BGBwt0e0gB2_s11nVvmgDSY0ZYUUR_gfZcn=yyQ-***@mail.gmail.com">
<div dir="ltr">Hi Serguei,
<div><br>
</div>
<div>If ever there was a doubt: looks good to me too now :)</div>
<div><br>
</div>
<div>Thanks!</div>
<div>Jc</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Wed, Nov 7, 2018 at 12:46 PM <a
href="mailto:***@oracle.com"
moz-do-not-send="true">***@oracle.com</a> &lt;<a
href="mailto:***@oracle.com"
moz-do-not-send="true">***@oracle.com</a>&gt;
wrote:<br>
</div>
<blockquote class="gmail_quote">
<div>
<div class="m_-3878707961274643190moz-cite-prefix">On
11/7/18 12:37, <a
class="m_-3878707961274643190moz-txt-link-abbreviated"
href="mailto:***@oracle.com" target="_blank"
moz-do-not-send="true">***@oracle.com</a>
wrote:<br>
</div>
<blockquote type="cite">
<div class="m_-3878707961274643190moz-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">
<div class="m_-3878707961274643190moz-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">
<blockquote type="cite">
<div class="m_-3878707961274643190moz-cite-prefix"> <br>
thanks,<br>
<br>
Chris<br>
<br>
On 11/7/18 12:04 AM, <a
class="m_-3878707961274643190moz-txt-link-abbreviated"
href="mailto:***@oracle.com"
target="_blank" moz-do-not-send="true">***@oracle.com</a>
wrote:<br>
</div>
<blockquote type="cite">
<div class="m_-3878707961274643190moz-cite-prefix">Hi
Jc,<br>
<br>
The updated version of webrev:<br>
  <a
class="m_-3878707961274643190moz-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_-3878707961274643190new">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_-3878707961274643190moz-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_-3878707961274643190moz-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-&gt;at(_index +
1)-&gt;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 &lt; 0 || _index +
extra_slot &gt;=
method_oop-&gt;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>-&gt; 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&lt;typename
T&gt;</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-&gt;*getter)(thr,
depth, slot, &amp;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",
&amp;jvmtiEnv::GetLocalInt,
"GetLocalInt");</div>
<div>  testGetter(jvmti,
thr, depth, slot,
"byte",
&amp;jvmtiEnv::GetLocalLong,
"GetLocalLong");</div>
<div>  testGetter(jvmti,
thr, depth, slot,
"byte",
&amp;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&lt;typename
T&gt;</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-&gt;*getter)(thr,
slot, depth, value);</div>
<div>  printf(" %s:    %s
(%d)\n", getter_name,
TranslateError(err),
err);</div>
<div>  if (err !=
JVMTI_ERROR_NONE
&amp;&amp; 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
&amp;&amp; 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_-3878707961274643190gmail-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>
&lt;<a
href="mailto:***@oracle.com"
target="_blank"
moz-do-not-send="true">***@oracle.com</a>&gt;
wrote:<br>
</div>
<blockquote
class="gmail_quote">
<div> Please, review a
fix for:<br>
  <a
class="m_-3878707961274643190gmail-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_-3878707961274643190gmail-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&lt;Type&gt;/SetLocal&lt;Type&gt;
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_-3878707961274643190gmail-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>
</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>

Loading...