RFR 8153629: Need to cover JVMTI's GetOwnedMonitorStackDepthInfo function

[serguei.spitsyn at oracle.com]

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?

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