Thank you a lot for review, Chris!
Serguei
On 11/7/18 12:35, Chris Plummer wrote:
Hi Serguei,
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:
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
Hi Jc,
The updated version of webrev:
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.
Two reasons for it:
- not sure how templates depends on the compiler versions
over various platforms
- macro definitions allow to make definitions more complex
but not the calls
Applied the same cleanups to both old tests:
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
On 11/6/18 17:13, [email protected]
wrote:
Hi Jc,
Thank you a lot for the code review!
On 11/6/18 9:22 AM, JC Beyler
wrote:
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?
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.
- 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.
- 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?
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.
- 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.
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.
- 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
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;
}
}
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
Please, review a fix for:
https://bugs.openjdk.java.net/browse/JDK-8080406
Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8080406-jvmti-get-local.1/
Summary:
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
StackValueCollectionfor
locals at runtime:
- 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.
Testing:
Tested locally on Linux-x64
with:
- 1 new jtreg test:
hotspot/jtreg/serviceability/jvmti/GetLocalVariable
- 2 nsk jtreg tests:
hotspot/jtreg/vmTestbase/nsk/jvmti/unit/GetLocalVariable
- 2 nsk jtreg tests:
hotspot/jtreg/vmTestbase/nsk/jvmti/GetLocalVariable
- 4 nsk jtreg tests:
hotspot/jtreg/vmTestbase/nsk/jvmti/SetLocalVariable
In progress:
The same as above but with
mach5 in different configs.
Thanks,
Serguei
--
|
