RFR (M) 8080406: VM_GetOrSetLocal doesn't check local slot type against requested type

[JC Beyler]

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

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181106/d49fb53f/attachment.html>