|
Hi Jc,
Thank you a lot for looking at the update!
On 11/7/18 09:30, JC Beyler wrote:
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:
- 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.
Nice catches - fixed.
Fixed.
In general, I have no intention and time to cleanup this test, just
fixed a couple of things that are common with getlocal003.
- not sure you wanted the capabilities to be static in the method
I see a lot of warnings about missing initialization of each bit if
it is initialized with caps = {0}.
Making it static is the simplest way to get it initialized instead
of using local struct and memset.
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...
Moved these constants into testLocals().
I've updated the same webrev v2 (as not that many people are
involved into this review yet):
http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8080406-jvmti-get-local.2/
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
--
--
|
