RFR 8153629: Need to cover JVMTI's GetOwnedMonitorStackDepthInfo function

[serguei.spitsyn at oracle.com]

On 1/17/18 12:21, serguei.spitsyn at oracle.com wrote:
> Hi Daniil,
>
>
> Some nits.
>
>  207     if (monitorCount == 3) {
>  208         for (idx = 0; idx < 3; idx++) {
>
>   Need to get rid of hardcoded number of monitors (3).
>
>  184     err = (*jvmti) -> GetCurrentThread(jvmti, &thread);
>
>   Extra spaces around '->'.
>
>  214                             TEST_CLASS, 
> TEST_OBJECT_LOCK_DEPTH,stackDepthInfo[idx].stack_depth);
>  221                             LOCK1_CLASS, 
> LOCK1_DEPTH,stackDepthInfo[idx].stack_depth);
>
>   Missed a space before stackDepthInfo[...
>
>
>
> Also, I have more suggestions to add to the Chris's comment:
>
>   - Remove the big condition if (monitorCount == 3) around the loop 
> and replace it with:
>       for (idx = 0; idx < monitorCount; idx++) {
>
>     and print monitor miscount separately before of after the loop:
>      if (monitorCount != 3) {
>        fprintf(stderr, "VerifyOwnedMonitors: FAIL: invalid 
> monitorCount, expected: %d, found: %d.\n",
>                        3, monitorCount);
>      }
>
>
> Then it would look like this:
>
>     status = PASSED;
>     if (monitorCount != EXP_MON_COUNT) {
>         fprintf(stderr, "VerifyOwnedMonitors: FAIL: invalid 
> monitorCount, expected: %d, found: %d.\n",
>                 EXP_MON_COUNT, monitorCount);
>         status = FAILED;
>     }
>     for (idx = 0; idx < monitorCount; idx++) {
>         if (CheckLockObject(env, stackDepthInfo[idx].monitor, 
> testClass) == JNI_TRUE) {
>             if (stackDepthInfo[idx].stack_depth != 
> TEST_OBJECT_LOCK_DEPTH) {
>                  fprintf(stderr, "FAILED: invalid stack_depth for %s 
> monitor, expected: %d, found: %d.\n",
>                          TEST_CLASS, TEST_OBJECT_LOCK_DEPTH, 
> stackDepthInfo[idx].stack_depth);
>                  status = FAILED;
>              }
>          } else if (CheckLockObject(env, stackDepthInfo[idx].monitor, 
> lock1Class) == JNI_TRUE) {
>              if (stackDepthInfo[idx].stack_depth != LOCK1_DEPTH) {
>                  fprintf(stderr, "FAILED: invalid stack_depth for %s 
> monitor, expected: %d, found: %d.\n",
>                          LOCK1_CLASS, 
> LOCK1_DEPTH,stackDepthInfo[idx].stack_depth);
>                  status = FAILED;
>              }
>          } else if (CheckLockObject(env, stackDepthInfo[idx].monitor, 
> lock2Class) == JNI_TRUE) {
>              if (stackDepthInfo[idx].stack_depth != LOCK2_DEPTH) {
>                  fprintf(stderr, "FAILED: invalid stack_depth for %s 
> monitor, expected: %d, found: %d.\n",
>                          LOCK2_CLASS, LOCK2_DEPTH, 
> stackDepthInfo[idx].stack_depth);
>                  status = FAILED;
>              }
>          } else {
>              fprintf(stderr, "FAILED: monitor[%d] should be instance 
> of %s, %s, or %s\n",
>                      idx, TEST_CLASS, LOCK1_CLASS, LOCK2_CLASS);
>              status = FAILED;
>          }
>      }
>
> Also, it does not seem that important to have two different lock 
> classes Lock1 and Lock2.
> Would it be simpler to keep just one of them?

Please, skip this last question.
I see the reason two have two different classes.
It is in the suggestion above this question.

Thanks,
Serguei


> Thanks,
> Serguei
>
>
> On 1/17/18 11:29, Chris Plummer wrote:
>> Hi Daniil,
>>
>> Have you run with -Xcomp. I'm concerned that inlining will cause the 
>> test to fail because stack depths will not be as expected. Also, if 
>> you find the owned monitor, but the depth is not as expected, I think 
>> you are better off printing an error message right away rather than 
>> the somewhat misleading "FAIL: invalid number of %s monitors" message 
>> later on.
>>
>> thanks,
>>
>> Chris
>>
>> On 1/16/18 6:31 PM, Daniil Titov wrote:
>>> Hello,
>>>
>>> Please review  an updated fix that makes the test more extensive and 
>>> includes suggested changes.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8153629
>>> Webrev: http://cr.openjdk.java.net/~dtitov/8153629/webrev.02/
>>>
>>> Thank you!
>>>
>>> Best regards,
>>> Daniil
>>>
>>> On 1/12/18, 2:26 PM, "serguei.spitsyn at oracle.com" 
>>> <serguei.spitsyn at oracle.com> wrote:
>>>
>>>      Hi Daniil,
>>>           It is pretty good in general.
>>>      Thank you for taking care about it!
>>>           Some comments though.
>>>           The test case is too trivial.
>>>      I'd suggest to extend it to have at least a couple of locks in the
>>>      returned array.
>>>      One way to do it would be to add a instance synchronized method 
>>> and
>>>      invoke it from the synchronized statement of the tested Thread.
>>>      Then the verifyOwnedMonitors() can be invoked from this method.
>>>           A couple of comments on the native agent.
>>>           72         // JNI_OnLoad has not been called yet, so can't 
>>> possibly be an instance of TEST_CLASS.
>>>           Could you, please, rewrite this comment?
>>>      Maybe just tell that there probably was an error in loading the 
>>> TEST_CLASS.
>>>      What about moving the FindClass(env, TEST_CLASS) to the
>>>      verifyOwnedMonitors() function?
>>>      It will make the testClass variable local.
>>>             200                 fprintf(stderr, 
>>> "VerifyOwnedMonitors: FAIL: stackDepthInfo[0].stack_depth should be 
>>> 1.\n");
>>>             207         fprintf(stderr, "VerifyOwnedMonitors: FAIL: 
>>> monitorCount should be 1.\n");
>>>                It'd better to rephrase the messages above to tell 
>>> about actual values
>>>      vs expected.
>>>      It normally simplifies the analysis of failures as there is no 
>>> need to find
>>>      what values were printed before and that they are exactly what 
>>> needed
>>>      for comparison.
>>>           Thanks,
>>>      Serguei
>>>                     On 1/11/18 17:45, Daniil Titov wrote:
>>>      > Hello,
>>>      >
>>>      > Please review the following fix that adds a jtreg test for 
>>> GetOwnedMonitorStackDepthInfo JVMTI function.
>>>      >
>>>      > Bug: https://bugs.openjdk.java.net/browse/JDK-8153629
>>>      > Webrev: http://cr.openjdk.java.net/~dtitov/8153629/webrev.00
>>>      >
>>>      >
>>>      > The tests ran successfully with Mach5.
>>>      >
>>>      > Best regards,
>>>      > Daniil
>>>      >
>>>      >
>>>
>>>
>>
>