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

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

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

serguei.spitsyn@oracle.com
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
Reply | Threaded
Open this post in threaded view
|

Re: 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?

 - 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.
 

  - 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 [hidden email] <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

serguei.spitsyn@oracle.com
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

On Mon, Nov 5, 2018 at 9:36 PM [hidden email] <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

serguei.spitsyn@oracle.com
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, [hidden email] 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

On Mon, Nov 5, 2018 at 9:36 PM [hidden email] <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

JC Beyler
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.

http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8080406-jvmti-get-local.2/test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/GetLocalVariable/getlocal004/getlocal004.cpp.html
  - mid is only used in one method (Java_nsk_jvmti_unit_GetLocalVariable_getlocal004_getMeth) and technically it doesn't get anything, it checks if the method is there.
  - not sure you wanted the capabilities to be static in the method

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...

Thanks,
Jc

On Wed, Nov 7, 2018 at 12:04 AM [hidden email] <[hidden email]> 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, [hidden email] 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

On Mon, Nov 5, 2018 at 9:36 PM [hidden email] <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

serguei.spitsyn@oracle.com
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.


http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8080406-jvmti-get-local.2/test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/GetLocalVariable/getlocal004/getlocal004.cpp.html
  - mid is only used in one method (Java_nsk_jvmti_unit_GetLocalVariable_getlocal004_getMeth) and technically it doesn't get anything, it checks if the method is there.

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

Thanks,
Jc

On Wed, Nov 7, 2018 at 12:04 AM [hidden email] <[hidden email]> 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, [hidden email] 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

On Mon, Nov 5, 2018 at 9:36 PM [hidden email] <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

Chris Plummer
In reply to this post by serguei.spitsyn@oracle.com
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, [hidden email] 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, [hidden email] 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

On Mon, Nov 5, 2018 at 9:36 PM [hidden email] <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

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

serguei.spitsyn@oracle.com
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.

thanks,

Chris

On 11/7/18 12:04 AM, [hidden email] 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, [hidden email] 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

On Mon, Nov 5, 2018 at 9:36 PM [hidden email] <[hidden email]> 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




Reply | Threaded
Open this post in threaded view
|

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

serguei.spitsyn@oracle.com
On 11/7/18 12:37, [hidden email] 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, [hidden email] 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, [hidden email] 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

On Mon, Nov 5, 2018 at 9:36 PM [hidden email] <[hidden email]> 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





Reply | Threaded
Open this post in threaded view
|

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

JC Beyler
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 [hidden email] <[hidden email]> wrote:
On 11/7/18 12:37, [hidden email] 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, [hidden email] 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, [hidden email] 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

On Mon, Nov 5, 2018 at 9:36 PM [hidden email] <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

serguei.spitsyn@oracle.com
Thanks a lot, Jc!
Serguei


On 11/7/18 21:55, JC Beyler wrote:
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 [hidden email] <[hidden email]> wrote:
On 11/7/18 12:37, [hidden email] 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, [hidden email] 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, [hidden email] 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

On Mon, Nov 5, 2018 at 9:36 PM [hidden email] <[hidden email]> 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