RFR 8153629: Need to cover JVMTI's GetOwnedMonitorStackDepthInfo function

[Daniil Titov]

Thank you, Serguei and Chris!

The test runs successfully in mach5 when –Xcomp Java option is added. Please review a new version of patch that changes the way how the error messages are printed as suggested.


Webrev: http://cr.openjdk.java.net/~dtitov/8153629/webrev.03/
Bug: https://bugs.openjdk.java.net/browse/JDK-8153629

Best regards,
Daniil


On 1/17/18, 12:29 PM, "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com> wrote:

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