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.spit...@oracle.com" <serguei.spit...@oracle.com>
wrote:
On 1/17/18 12:21, serguei.spit...@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.spit...@oracle.com"
>>> <serguei.spit...@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
>>> >
>>> >
>>>
>>>
>>
>
