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.
thanks,
Chris
On 11/7/18 12:04 AM, [email protected] wrote:
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
--
|
