RFR 8153629: Need to cover JVMTI's GetOwnedMonitorStackDepthInfo function

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

RFR 8153629: Need to cover JVMTI's GetOwnedMonitorStackDepthInfo function

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


Reply | Threaded
Open this post in threaded view
|

Re: RFR 8153629: Need to cover JVMTI's GetOwnedMonitorStackDepthInfo function

Chris Plummer
Hi Daniil,

Looks good. Thanks for adding this.

Chris

On 1/11/18 5:45 PM, 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
>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8153629: Need to cover JVMTI's GetOwnedMonitorStackDepthInfo function

serguei.spitsyn@oracle.com
In reply to this post by Daniil Titov
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
>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8153629: Need to cover JVMTI's GetOwnedMonitorStackDepthInfo function

Chris Plummer
Hi Serguei,

On 1/12/18 2:25 PM, [hidden email] 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.
This was copied from my comment in GetOwnedMonitorInfoTest, which I
assume this test was based on.
> What about moving the FindClass(env, TEST_CLASS) to the
> verifyOwnedMonitors() function?
> It will make the testClass variable local.
>
Also from GetOwnedMonitorInfoTest. This is code I reworked in that test
recently to fix 8191229. These two tests should be kept consistent.

Chris

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


Reply | Threaded
Open this post in threaded view
|

Re: RFR 8153629: Need to cover JVMTI's GetOwnedMonitorStackDepthInfo function

serguei.spitsyn@oracle.com
Hi Chris,


On 1/12/18 14:31, Chris Plummer wrote:

> Hi Serguei,
>
> On 1/12/18 2:25 PM, [hidden email] 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.
> This was copied from my comment in GetOwnedMonitorInfoTest, which I
> assume this test was based on.

Yes, I also assumed it was copied from the GetOwnedMonitorInfoTest.
The comment looks incorrect and creates some confusion.


>> What about moving the FindClass(env, TEST_CLASS) to the
>> verifyOwnedMonitors() function?
>> It will make the testClass variable local.
>>
> Also from GetOwnedMonitorInfoTest. This is code I reworked in that
> test recently to fix 8191229.

Yes, I remember.

> These two tests should be kept consistent.

I still think, making it a part of the verifyOwnedMonitors() would
simplify the test.
Why do we need the testClass to be volatile and global if it is used
only in the context of verification?
It generates less questions if it is local.
We could attempt to fix the GetOwnedMonitorInfoTest as well if we want
this kind of consistency.

Thanks,
Serguei


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

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8153629: Need to cover JVMTI's GetOwnedMonitorStackDepthInfo function

Chris Plummer
On 1/12/18 2:52 PM, [hidden email] wrote:

> Hi Chris,
>
>
> On 1/12/18 14:31, Chris Plummer wrote:
>> Hi Serguei,
>>
>> On 1/12/18 2:25 PM, [hidden email] 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.
>> This was copied from my comment in GetOwnedMonitorInfoTest, which I
>> assume this test was based on.
>
> Yes, I also assumed it was copied from the GetOwnedMonitorInfoTest.
> The comment looks incorrect and creates some confusion.
How is it incorrect? If testClass is NULL, that does imply that
JNI_OnLoad has not been called yet, which itself implies that the object
cannot be an instance of TEST_CLASS.

>
>
>>> What about moving the FindClass(env, TEST_CLASS) to the
>>> verifyOwnedMonitors() function?
>>> It will make the testClass variable local.
>>>
>> Also from GetOwnedMonitorInfoTest. This is code I reworked in that
>> test recently to fix 8191229.
>
> Yes, I remember.
>
>> These two tests should be kept consistent.
>
> I still think, making it a part of the verifyOwnedMonitors() would
> simplify the test.
> Why do we need the testClass to be volatile and global if it is used
> only in the context of verification?
> It generates less questions if it is local.
> We could attempt to fix the GetOwnedMonitorInfoTest as well if we want
> this kind of consistency.
I think in this test you could make looking up testClass local to
verifyOwnedMonitors() since calling it is under our control.  In
GetOwnedMonitorInfoTest, there is no place to safely make it local,
because we need testClass during a JVMTI callback, and we can't always
correctly look up TEST_CLASS in the callback. That's why the lookup was
moved as part of 8191229 into JNI_OnLoad.

thanks,

Chris

>
> Thanks,
> Serguei
>
>
>> Chris
>>>  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
>>>>
>>>>
>>>
>>
>>
>


Reply | Threaded
Open this post in threaded view
|

Re: RFR 8153629: Need to cover JVMTI's GetOwnedMonitorStackDepthInfo function

Daniil Titov
In reply to this post by serguei.spitsyn@oracle.com
Thank you, Chris and Serguei!

I will extend the test as suggested and send an updated patch for review.

Best regards,
Daniil

On 1/12/18, 2:52 PM, "[hidden email]" <[hidden email]> wrote:

    Hi Chris,
   
   
    On 1/12/18 14:31, Chris Plummer wrote:
    > Hi Serguei,
    >
    > On 1/12/18 2:25 PM, [hidden email] 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.
    > This was copied from my comment in GetOwnedMonitorInfoTest, which I
    > assume this test was based on.
   
    Yes, I also assumed it was copied from the GetOwnedMonitorInfoTest.
    The comment looks incorrect and creates some confusion.
   
   
    >> What about moving the FindClass(env, TEST_CLASS) to the
    >> verifyOwnedMonitors() function?
    >> It will make the testClass variable local.
    >>
    > Also from GetOwnedMonitorInfoTest. This is code I reworked in that
    > test recently to fix 8191229.
   
    Yes, I remember.
   
    > These two tests should be kept consistent.
   
    I still think, making it a part of the verifyOwnedMonitors() would
    simplify the test.
    Why do we need the testClass to be volatile and global if it is used
    only in the context of verification?
    It generates less questions if it is local.
    We could attempt to fix the GetOwnedMonitorInfoTest as well if we want
    this kind of consistency.
   
    Thanks,
    Serguei
   
   
    > Chris
    >>  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
    >>>
    >>>
    >>
    >
    >
   
   


Reply | Threaded
Open this post in threaded view
|

Re: RFR 8153629: Need to cover JVMTI's GetOwnedMonitorStackDepthInfo function

serguei.spitsyn@oracle.com
In reply to this post by Chris Plummer
On 1/12/18 15:07, Chris Plummer wrote:

> On 1/12/18 2:52 PM, [hidden email] wrote:
>> Hi Chris,
>>
>>
>> On 1/12/18 14:31, Chris Plummer wrote:
>>> Hi Serguei,
>>>
>>> On 1/12/18 2:25 PM, [hidden email] 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.
>>> This was copied from my comment in GetOwnedMonitorInfoTest, which I
>>> assume this test was based on.
>>
>> Yes, I also assumed it was copied from the GetOwnedMonitorInfoTest.
>> The comment looks incorrect and creates some confusion.
> How is it incorrect? If testClass is NULL, that does imply that
> JNI_OnLoad has not been called yet, which itself implies that the
> object cannot be an instance of TEST_CLASS.

It contradicts with the JNI_OnLoad code:

  100     testClass = (*env)->FindClass(env, TEST_CLASS);
  101     if (testClass != NULL) {
  102       testClass = (*env)->NewGlobalRef(env, testClass);
  103     }
  104     if (testClass == NULL) {
  105         fprintf(stderr, "Error: Could not load class %s!\n", TEST_CLASS);
  106         return JNI_ERR;
  107     }


>>>> What about moving the FindClass(env, TEST_CLASS) to the
>>>> verifyOwnedMonitors() function?
>>>> It will make the testClass variable local.
>>>>
>>> Also from GetOwnedMonitorInfoTest. This is code I reworked in that
>>> test recently to fix 8191229.
>>
>> Yes, I remember.
>>
>>> These two tests should be kept consistent.
>>
>> I still think, making it a part of the verifyOwnedMonitors() would
>> simplify the test.
>> Why do we need the testClass to be volatile and global if it is used
>> only in the context of verification?
>> It generates less questions if it is local.
>> We could attempt to fix the GetOwnedMonitorInfoTest as well if we
>> want this kind of consistency.
> I think in this test you could make looking up testClass local to
> verifyOwnedMonitors() since calling it is under our control.  In
> GetOwnedMonitorInfoTest, there is no place to safely make it local,
> because we need testClass during a JVMTI callback, and we can't always
> correctly look up TEST_CLASS in the callback. That's why the lookup
> was moved as part of 8191229 into JNI_OnLoad.

Good explanation, thanks!
So, we do not need a consistency with the GetOwnedMonitorInfoTest in
such a case.

Thanks,
Serguei

> thanks,
>
> Chris
>>
>> Thanks,
>> Serguei
>>
>>
>>> Chris
>>>>  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
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8153629: Need to cover JVMTI's GetOwnedMonitorStackDepthInfo function

serguei.spitsyn@oracle.com
On 1/12/18 15:14, [hidden email] wrote:

> On 1/12/18 15:07, Chris Plummer wrote:
>> On 1/12/18 2:52 PM, [hidden email] wrote:
>>> Hi Chris,
>>>
>>>
>>> On 1/12/18 14:31, Chris Plummer wrote:
>>>> Hi Serguei,
>>>>
>>>> On 1/12/18 2:25 PM, [hidden email] 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.
>>>> This was copied from my comment in GetOwnedMonitorInfoTest, which I
>>>> assume this test was based on.
>>>
>>> Yes, I also assumed it was copied from the GetOwnedMonitorInfoTest.
>>> The comment looks incorrect and creates some confusion.
>> How is it incorrect? If testClass is NULL, that does imply that
>> JNI_OnLoad has not been called yet, which itself implies that the
>> object cannot be an instance of TEST_CLASS.
>
> It contradicts with the JNI_OnLoad code:
>
>  100     testClass = (*env)->FindClass(env, TEST_CLASS);
>  101     if (testClass != NULL) {
>  102       testClass = (*env)->NewGlobalRef(env, testClass);
>  103     }
>  104     if (testClass == NULL) {
>  105         fprintf(stderr, "Error: Could not load class %s!\n",
> TEST_CLASS);
>  106         return JNI_ERR;
>  107     }

I forgot to tell that this comment is not needed if the fragment above
is moved to the verifyOwnedMonitors().

Thanks,
Serguei


>>>>> What about moving the FindClass(env, TEST_CLASS) to the
>>>>> verifyOwnedMonitors() function?
>>>>> It will make the testClass variable local.
>>>>>
>>>> Also from GetOwnedMonitorInfoTest. This is code I reworked in that
>>>> test recently to fix 8191229.
>>>
>>> Yes, I remember.
>>>
>>>> These two tests should be kept consistent.
>>>
>>> I still think, making it a part of the verifyOwnedMonitors() would
>>> simplify the test.
>>> Why do we need the testClass to be volatile and global if it is used
>>> only in the context of verification?
>>> It generates less questions if it is local.
>>> We could attempt to fix the GetOwnedMonitorInfoTest as well if we
>>> want this kind of consistency.
>> I think in this test you could make looking up testClass local to
>> verifyOwnedMonitors() since calling it is under our control. In
>> GetOwnedMonitorInfoTest, there is no place to safely make it local,
>> because we need testClass during a JVMTI callback, and we can't
>> always correctly look up TEST_CLASS in the callback. That's why the
>> lookup was moved as part of 8191229 into JNI_OnLoad.
>
> Good explanation, thanks!
> So, we do not need a consistency with the GetOwnedMonitorInfoTest in
> such a case.
>
> Thanks,
> Serguei
>
>> thanks,
>>
>> Chris
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>>> Chris
>>>>>  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
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8153629: Need to cover JVMTI's GetOwnedMonitorStackDepthInfo function

Chris Plummer
In reply to this post by serguei.spitsyn@oracle.com
On 1/12/18 3:14 PM, [hidden email] wrote:

> On 1/12/18 15:07, Chris Plummer wrote:
>> On 1/12/18 2:52 PM, [hidden email] wrote:
>>> Hi Chris,
>>>
>>>
>>> On 1/12/18 14:31, Chris Plummer wrote:
>>>> Hi Serguei,
>>>>
>>>> On 1/12/18 2:25 PM, [hidden email] 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.
>>>> This was copied from my comment in GetOwnedMonitorInfoTest, which I
>>>> assume this test was based on.
>>>
>>> Yes, I also assumed it was copied from the GetOwnedMonitorInfoTest.
>>> The comment looks incorrect and creates some confusion.
>> How is it incorrect? If testClass is NULL, that does imply that
>> JNI_OnLoad has not been called yet, which itself implies that the
>> object cannot be an instance of TEST_CLASS.
>
> It contradicts with the JNI_OnLoad code:
>
>  100     testClass = (*env)->FindClass(env, TEST_CLASS);
>  101     if (testClass != NULL) {
>  102       testClass = (*env)->NewGlobalRef(env, testClass);
>  103     }
>  104     if (testClass == NULL) {
>  105         fprintf(stderr, "Error: Could not load class %s!\n",
> TEST_CLASS);
>  106         return JNI_ERR;
>  107     }
>
I think your point is that possibly testClass could be NULL if
JNI_OnLoad was called but failed. I think that is a minor detail, and
the test will exit when that happens.

thanks,

Chris

>
>>>>> What about moving the FindClass(env, TEST_CLASS) to the
>>>>> verifyOwnedMonitors() function?
>>>>> It will make the testClass variable local.
>>>>>
>>>> Also from GetOwnedMonitorInfoTest. This is code I reworked in that
>>>> test recently to fix 8191229.
>>>
>>> Yes, I remember.
>>>
>>>> These two tests should be kept consistent.
>>>
>>> I still think, making it a part of the verifyOwnedMonitors() would
>>> simplify the test.
>>> Why do we need the testClass to be volatile and global if it is used
>>> only in the context of verification?
>>> It generates less questions if it is local.
>>> We could attempt to fix the GetOwnedMonitorInfoTest as well if we
>>> want this kind of consistency.
>> I think in this test you could make looking up testClass local to
>> verifyOwnedMonitors() since calling it is under our control. In
>> GetOwnedMonitorInfoTest, there is no place to safely make it local,
>> because we need testClass during a JVMTI callback, and we can't
>> always correctly look up TEST_CLASS in the callback. That's why the
>> lookup was moved as part of 8191229 into JNI_OnLoad.
>
> Good explanation, thanks!
> So, we do not need a consistency with the GetOwnedMonitorInfoTest in
> such a case.
>
> Thanks,
> Serguei
>
>> thanks,
>>
>> Chris
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>>> Chris
>>>>>  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
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>


Reply | Threaded
Open this post in threaded view
|

Re: RFR 8153629: Need to cover JVMTI's GetOwnedMonitorStackDepthInfo function

serguei.spitsyn@oracle.com
On 1/12/18 15:42, Chris Plummer wrote:

> On 1/12/18 3:14 PM, [hidden email] wrote:
>> On 1/12/18 15:07, Chris Plummer wrote:
>>> On 1/12/18 2:52 PM, [hidden email] wrote:
>>>> Hi Chris,
>>>>
>>>>
>>>> On 1/12/18 14:31, Chris Plummer wrote:
>>>>> Hi Serguei,
>>>>>
>>>>> On 1/12/18 2:25 PM, [hidden email] 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.
>>>>> This was copied from my comment in GetOwnedMonitorInfoTest, which
>>>>> I assume this test was based on.
>>>>
>>>> Yes, I also assumed it was copied from the GetOwnedMonitorInfoTest.
>>>> The comment looks incorrect and creates some confusion.
>>> How is it incorrect? If testClass is NULL, that does imply that
>>> JNI_OnLoad has not been called yet, which itself implies that the
>>> object cannot be an instance of TEST_CLASS.
>>
>> It contradicts with the JNI_OnLoad code:
>>
>>  100     testClass = (*env)->FindClass(env, TEST_CLASS);
>>  101     if (testClass != NULL) {
>>  102       testClass = (*env)->NewGlobalRef(env, testClass);
>>  103     }
>>  104     if (testClass == NULL) {
>>  105         fprintf(stderr, "Error: Could not load class %s!\n",
>> TEST_CLASS);
>>  106         return JNI_ERR;
>>  107     }
>>
> I think your point is that possibly testClass could be NULL if
> JNI_OnLoad was called but failed.
Right.

> I think that is a minor detail, and the test will exit when that happens.

The reader needs to make this conclusion to understand the comment.
But I see your point, thanks.

Thanks,
Serguei

> thanks,
>
> Chris
>>
>>>>>> What about moving the FindClass(env, TEST_CLASS) to the
>>>>>> verifyOwnedMonitors() function?
>>>>>> It will make the testClass variable local.
>>>>>>
>>>>> Also from GetOwnedMonitorInfoTest. This is code I reworked in that
>>>>> test recently to fix 8191229.
>>>>
>>>> Yes, I remember.
>>>>
>>>>> These two tests should be kept consistent.
>>>>
>>>> I still think, making it a part of the verifyOwnedMonitors() would
>>>> simplify the test.
>>>> Why do we need the testClass to be volatile and global if it is
>>>> used only in the context of verification?
>>>> It generates less questions if it is local.
>>>> We could attempt to fix the GetOwnedMonitorInfoTest as well if we
>>>> want this kind of consistency.
>>> I think in this test you could make looking up testClass local to
>>> verifyOwnedMonitors() since calling it is under our control. In
>>> GetOwnedMonitorInfoTest, there is no place to safely make it local,
>>> because we need testClass during a JVMTI callback, and we can't
>>> always correctly look up TEST_CLASS in the callback. That's why the
>>> lookup was moved as part of 8191229 into JNI_OnLoad.
>>
>> Good explanation, thanks!
>> So, we do not need a consistency with the GetOwnedMonitorInfoTest in
>> such a case.
>>
>> Thanks,
>> Serguei
>>
>>> thanks,
>>>
>>> Chris
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>>> Chris
>>>>>>  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
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8153629: Need to cover JVMTI's GetOwnedMonitorStackDepthInfo function

Daniil Titov
In reply to this post by Daniil Titov
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, "[hidden email]" <[hidden email]> 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
    >
    >
   
   
   
   


Reply | Threaded
Open this post in threaded view
|

Re: RFR 8153629: Need to cover JVMTI's GetOwnedMonitorStackDepthInfo function

Chris Plummer
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, "[hidden email]" <[hidden email]> 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
>      >
>      >
>      
>      
>      
>      
>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8153629: Need to cover JVMTI's GetOwnedMonitorStackDepthInfo function

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

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8153629: Need to cover JVMTI's GetOwnedMonitorStackDepthInfo function

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

Reply | Threaded
Open this post in threaded view
|

Re: 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, "[hidden email]" <[hidden email]> wrote:

    On 1/17/18 12:21, [hidden email] 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, "[hidden email]"
    >>> <[hidden email]> 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
    >>>      >
    >>>      >
    >>>
    >>>
    >>
    >
   
   


Reply | Threaded
Open this post in threaded view
|

Re: RFR 8153629: Need to cover JVMTI's GetOwnedMonitorStackDepthInfo function

serguei.spitsyn@oracle.com
Daniil,

Looks great!
Could you, please, make a minor cleanup of status handling?

60     jint status = FAILED;
   =>
60     jint status = PASSED;

At the lines 167, 173, 179, 186 and  192:
    return status;
   =>
    return FAILED;

And remove the line:

  205     status = PASSED;


No need for another round of review.

Thanks,
Serguei


On 1/17/18 13:36, Daniil Titov wrote:

> 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, "[hidden email]" <[hidden email]> wrote:
>
>      On 1/17/18 12:21, [hidden email] 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, "[hidden email]"
>      >>> <[hidden email]> 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
>      >>>      >
>      >>>      >
>      >>>
>      >>>
>      >>
>      >
>      
>      
>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8153629: Need to cover JVMTI's GetOwnedMonitorStackDepthInfo function

Chris Plummer
In reply to this post by Daniil Titov
Hi Daniil,

Looks good. Thanks for the -Xcomp testing.

Chris

On 1/17/18 1:36 PM, Daniil Titov wrote:

> 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, "[hidden email]" <[hidden email]> wrote:
>
>      On 1/17/18 12:21, [hidden email] 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, "[hidden email]"
>      >>> <[hidden email]> 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
>      >>>      >
>      >>>      >
>      >>>
>      >>>
>      >>
>      >
>      
>      
>
>