RFR 8153629: Need to cover JVMTI's GetOwnedMonitorStackDepthInfo function

[serguei.spitsyn at oracle.com]

On 1/12/18 15:42, Chris Plummer wrote:
> On 1/12/18 3:14 PM, serguei.spitsyn at oracle.com wrote:
>> On 1/12/18 15:07, Chris Plummer wrote:
>>> On 1/12/18 2:52 PM, serguei.spitsyn at oracle.com wrote:
>>>> Hi Chris,
>>>>
>>>>
>>>> On 1/12/18 14:31, Chris Plummer wrote:
>>>>> Hi Serguei,
>>>>>
>>>>> On 1/12/18 2:25 PM, 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.
>>>>> 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
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>