RFR (M) 8080406: VM_GetOrSetLocal doesn't check local slot type against requested type
JC Beyler
jcbeyler at google.com
Thu Nov 8 05:55:41 UTC 2018
Hi Serguei,
If ever there was a doubt: looks good to me too now :)
Thanks!
Jc
On Wed, Nov 7, 2018 at 12:46 PM serguei.spitsyn at oracle.com <
serguei.spitsyn at oracle.com> wrote:
> On 11/7/18 12:37, serguei.spitsyn at oracle.com wrote:
>
> 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
>
>
> thanks,
>
> Chris
>
> On 11/7/18 12:04 AM, serguei.spitsyn at oracle.com 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, serguei.spitsyn at oracle.com 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.
>
>
> -> 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.
>
> - 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.
>
> 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
> 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
>
> On Mon, Nov 5, 2018 at 9:36 PM serguei.spitsyn at oracle.com <
> serguei.spitsyn at oracle.com> wrote:
>
>> 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
>>
>
>
> --
>
> Thanks,
> Jc
>
>
>
>
>
>
>
--
Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181107/7363fee8/attachment-0001.html>
More information about the serviceability-dev
mailing list