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

[Serguei Spitsyn]

On Wed, 27 Mar 2024 20:06:54 GMT, Chris Plummer <cjplummer at openjdk.org> wrote:

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

Thanks, Chris. I noticed that some an additional cleanup is needed in the Java part.
The `TestTask.run()` has two calls to the method `B()`:

        public void run() {
            log("TestTask.run: started");

            A();
            sleep(1); // to cause yield

            prepareAgent(TestTask.class, false); // No doPopFrame
            B();
            sleep(1); // to cause yield

            prepareAgent(TestTask.class, true); // doPopFrame
            B();
            sleep(1); // to cause yield

            C();
            finished = true;
        }

The second `B()` call does the JVMTI `PopFrame` call on its own thread in the `Breakpoint()` event hadler.
Please, note, `true` in the `prepareAgent` parameters:
   `prepareAgent(TestTask.class, true); // doPopFrame`
So, the second `ensureAtBreakpoint()` is a sync point with it.

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

Okay, thanks. I can do this change.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18419#discussion_r1543825742
PR Review Comment: https://git.openjdk.org/jdk/pull/18419#discussion_r1543826541