RFR: 8308000: add PopFrame support for virtual threads

[Leonid Mesnik]

On Tue, 16 May 2023 08:12:21 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

> This enhancement adds `PopFrame` support for virtual threads. The spec defines minimal support that the JVMTI `PopFrame` can be used for a virtual thread suspended an an event. 
> Actually, the `PopFrame` can supports suspended and mounted virtual threads. 
> 
> CSR (approved): https://bugs.openjdk.org/browse/JDK-8308001: add PopFrame support for virtual threads
> 
> Testing:
> New test was developed: `serviceability/vthread/PopFrameTest`.
> Submitted mach5 tiers 1-6 are good.
> TBD: rerun mach5 tiers 1-6 at the end of review again if necessary.

It is also unclear how popFrame works if the the underlying frame is
 jdk.internal.vm.Continuation.enterSpecial().
Would it just return some error?

src/hotspot/share/prims/jvmtiEnv.cpp line 1886:

> 1884:   jvmtiError err = get_threadOop_and_JavaThread(tlh.list(), thread, &java_thread, &thread_obj);
> 1885: 
> 1886:   bool is_virtual = thread_obj != nullptr && thread_obj->is_a(vmClasses::BaseVirtualThread_klass());

I think it would be better to check 'err' and try to handle the error before using java_thread and thread_obj.

src/hotspot/share/prims/jvmtiEnv.cpp line 1896:

> 1894:     }
> 1895:   } else {
> 1896:     if (java_thread != current_thread && !java_thread->is_suspended() &&

This branch checks the state when the thread is platform OR current, that logic seems a little bit messy. Would not be better to clearly separate virtual and platform threads verification? (Also, it is unclear,  we need to check platform threads here now).
Might be some comments are needed?

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

> 60:     static final int FAILED = 2;
> 61: 
> 62:     static void log(String str) { System.out.println(str); }

Better to flush system.out after each print.

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

> 125:             resumeThread(testTaskThread);
> 126:             testTask.clearDoLoop();
> 127:             testTask.sleep(5);

Why sleep is needed here?

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

> 150: 
> 151:             log("\nMain #B.3: unsuspended, call PopFrame on own thread");
> 152:             ensureAtBreakpoint();

Am I understand correctly, that test expect here to pop frame and immediately get to the same breakpoint?

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

> 152:             ensureAtBreakpoint();
> 153:             notifyAtBreakpoint();
> 154:             testTask.sleep(5);

Why sleep is needed here?

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

> 220:         }
> 221: 
> 222:         // Method is blocked on entering a synchronized statement.

Not sure I see where this method is blocked.

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

> 239:         }
> 240: 
> 241:         // This method uses PoopFrame on its own thread. It is expected to succeed.

Isn't OPAQUE_FRAME expected?

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

> 47:   LOG("Breakpoint: In method TestTask.B(): before sync section enter\n");
> 48: 
> 49:   err = jvmti->RawMonitorEnter(monitor);

You could use RawMonitorLocker instead:

{
RawMonitorLocker rml(jvmti, jni, monitor);
bp_sync_reached = true;
rml.wait();
}

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

> 60: 
> 61:   err = jvmti->PopFrame(thread);
> 62:   LOG("Main: popFrame: PopFrame returned code: %s (%d)\n", TranslateError(err), err);

check_jvmti_status prints return code and translated error if fails. So this line is not needed,

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

> 179:   LOG("Main: notifyAtBreakpoint\n");
> 180: 
> 181:   err = jvmti->RawMonitorEnter(monitor);

You could use

RawMonitorLocker rml(jvmti, jni, monitor);
rml.notify_all();

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

Changes requested by lmesnik (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14002#pullrequestreview-1429501761
PR Review Comment: https://git.openjdk.org/jdk/pull/14002#discussion_r1195729952
PR Review Comment: https://git.openjdk.org/jdk/pull/14002#discussion_r1195738621
PR Review Comment: https://git.openjdk.org/jdk/pull/14002#discussion_r1195778145
PR Review Comment: https://git.openjdk.org/jdk/pull/14002#discussion_r1195785775
PR Review Comment: https://git.openjdk.org/jdk/pull/14002#discussion_r1195785443
PR Review Comment: https://git.openjdk.org/jdk/pull/14002#discussion_r1195785817
PR Review Comment: https://git.openjdk.org/jdk/pull/14002#discussion_r1195781893
PR Review Comment: https://git.openjdk.org/jdk/pull/14002#discussion_r1195782744
PR Review Comment: https://git.openjdk.org/jdk/pull/14002#discussion_r1195759443
PR Review Comment: https://git.openjdk.org/jdk/pull/14002#discussion_r1195772530
PR Review Comment: https://git.openjdk.org/jdk/pull/14002#discussion_r1195757356