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