RFR: 8328665: serviceability/jvmti/vthread/PopFrameTest failed with a timeout [v4]

[Chris Plummer]

On Wed, 27 Mar 2024 06:44:37 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> This PR fixes a synchronization issue in the test:
>>   `test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest`
>>   
>> The method `notifyAtBreakpoint()` can notify the `TestTask` thread when it has not reached an expected breakpoint yet.
>> The fix is to add a call to the method `ensureAtBreakpoint()` one more time in the `B2` sub-test. It is needed after the top-most frame was popped with the JVMTI `PopFrame`, and the target thread needs to reach the breakpoint again after its execution was resumed.
>> 
>> The time is very intermittent. At least, I was not able to reproduce the timeout failure in thousands of mach5 runs with the `-Xcomp` option.
>> 
>> Testing:
>>  - Run the test `test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest` thousands times in mach5
>
> Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   review: improve diagnostics and reliability

test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest/PopFrameTest.java line 148:

> 146:             log("Main #B.2: got expected JVMTI_ERROR_NONE");
> 147:             resumeThread(testTaskThread);
> 148: 

You probably don't need this minor edit or the copyright update any more.

However, it's still unclear to me how the ensureAtBreakpoint() below is suppose to work if we already called notifyAtBreakpoint() between the 1st and 2nd ensureAtBreakpoint(). From what I can tell, notifyAtBreakpoint() clears the flag that ensureAtBreakpoint() checks for, and there is no 2nd breakpoint to set it again. I would expect the ensureAtBreakpoint() below to always wait indefinitely, but that doesn't appear to be the case. So how is this working? I must be missing something.

test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest/libPopFrameTest.cpp line 58:

> 56:   LOG("Breakpoint: In method TestTask.B(): after sync section\n");
> 57: 
> 58:   if (do_pop_frame != JNI_FALSE) {

Suggestion:

  if (do_pop_frame) {

test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest/libPopFrameTest.cpp line 161:

> 159:   int attempts = 0;
> 160:   while (!bp_sync_reached) {
> 161:     LOG("Main: ensureAtBreakpoint: waitig 5 millis\n");

Suggestion:

    LOG("Main: ensureAtBreakpoint: waiting 10 millis\n");


I think this should be moved directly above the wait(10) call.

test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest/libPopFrameTest.cpp line 163:

> 161:     LOG("Main: ensureAtBreakpoint: waitig 5 millis\n");
> 162:     if (++attempts > 100) {
> 163:       fatal(jni, "Main: ensureAtBreakpoint: waited 1 sec");

1 second isn't very long when you are talking about something that is relying on debugger/debuggee communication. Maybe wait 100ms at a time for a total of 10 seconds.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/18419#discussion_r1541957419
PR Review Comment: https://git.openjdk.org/jdk/pull/18419#discussion_r1541958086
PR Review Comment: https://git.openjdk.org/jdk/pull/18419#discussion_r1541947950
PR Review Comment: https://git.openjdk.org/jdk/pull/18419#discussion_r1541960905