On Wed, 28 Oct 2026 06:01:51 GMT, Shiv Shah <[email protected]> wrote:
>> When a virtual thread suspended at a breakpoint gets unmounted and mounted
>> again only the top frame is thawed. The caller stays frozen in the stack
>> chunk and the popped frame returns into the continuation return barrier. The
>> interpreter's popframe entry treats that return address like a deoptimized
>> caller, preserves the arguments, sets the reexecution bit and returns. The
>> barrier then thawed the caller as a plain return. Arguments and bit left
>> behind, no reexecution of the invoke, run() went on with a zero result.
>>
>> Thaw finishes it now. An interpreted caller gets the preserved arguments
>> back on its expression stack and its return slot is patched to the
>> interpreter deopt entry after push_return_frame, the x86 deopt entry reloads
>> rsp from last_sp for that, aarch64 already does. A compiled caller frozen in
>> the chunk is deoptimized from UpdateForPopTopFrameClosure::doit the same way
>> set_frame_pop does it. Thaw leaves the arguments alone in that case and
>> unpack_on_stack puts them back. A frozen compiled frame being popped stays
>> opaque.
>>
>> A linkTo caller needs one more thing. The linker consumed the trailing
>> MemberName before the target ran and the preserved arguments are sized by
>> the target, so the MemberName is not among them. Thaw takes it from the
>> caller's local 0 the way the interpreter's popframe entry does, the lookup
>> from member_name_arg_or_null moved into a plain helper both can call. The
>> arguments go one slot up and the MemberName goes back on top.
>>
>> popframes002 runs popframes001 on a virtual thread with the join Chris
>> Plummer wrote enabled by a property in the debuggee. It unmounts the tested
>> thread before the breakpoint so the caller is frozen when popped, and run()
>> is excluded from compilation so this stays the interpreted case.
>> popframes003 is the compiled caller case, -Xcomp with the breakpoint in a
>> separate stop() so testedMethod keeps its compiled frame and run() stays
>> frozen, the debugger pops frame 1 and the debuggee checks with WhiteBox that
>> run() and testedMethod are compiled and that testedMethod ran twice.
>> -XX:+PreserveFramePointer keeps the thaw on the slow path. Both fail on
>> master without the hotspot change. popframes001 itself runs the plain case
>> as before and is off the virtual problem list. If separate copies of the
>> debugger and debuggee are preferred over the properties that is easy to
>> change. PopFrameMethodNameInvariantTest got an unmount mode, the same
>> linkToStatic pop on a virtual thread that sleeps
before the breakp...
>
> Shiv Shah has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add the invokeExact cases, the LambdaForm frame keeps the
> DirectMethodHandle in local0
Overall changes look good to me. I think reusing the deopt entry may be
problematic on some platforms, and it would be better to add a dedicated entry
for pop frame. Some other comments and suggestions below, thanks.
src/hotspot/cpu/x86/templateInterpreterGenerator_x86.cpp line 235:
> 233: __ jcc(Assembler::zero, L);
> 234: __ lea(rsp, Address(rbp, rscratch1, Address::times_ptr));
> 235: __ bind(L);
I’m not a fan of adjusting the deopt entry just to be able to reuse it for the
pop frame case. Also, looking at ppc for example, it seems the deopt entry
assumes the interpreter state has already been restored by the deoptimization
code. I think it would be better to have a dedicated pop frame entry, even if
the implementation on some platform just jumps to the deopt one.
src/hotspot/share/interpreter/interpreterRuntime.cpp line 1566:
> 1564: #endif
> 1565:
> 1566: #if INCLUDE_JVMTI
The `#if INCLUDE_JVMTI` conditional should stay here since
`popframe_member_name_arg_or_null` is only used by JVMTI. Same in header file.
src/hotspot/share/prims/jvmtiEnvBase.cpp line 2556:
> 2554: assert(_state->is_virtual(), "invariant");
> 2555: if (frame_count == 0) {
> 2556: _result = JVMTI_ERROR_OPAQUE_FRAME;
Suggestion:
// If the top frame is in the heap, PopFrame is not supported.
_result = JVMTI_ERROR_OPAQUE_FRAME;
And I would remove the comment above.
src/hotspot/share/prims/jvmtiEnvBase.cpp line 2560:
> 2558: }
> 2559: javaVFrame* jvf = vfs.asJavaVFrame();
> 2560: if (!jvf->fr().can_be_deoptimized()) {
This check should also apply for the stack frame case (although I don’t see how
it can fail for either case at this point).
src/hotspot/share/runtime/continuationFreezeThaw.cpp line 3088:
> 3086: // unpack_on_stack puts the arguments back. An interpreted caller was
> copied
> 3087: // without them, so put them back on its expression stack and resume at
> the
> 3088: // invoke, like vframeArrayElement::unpack_on_stack does.
Suggestion:
// A frame popped by JVMTI PopFrame returned into the continuation return
// barrier. The interpreter's popframe entry preserved the arguments and
// set the deopt reexecution bit as for a deoptimized caller. If the caller
// is compiled, UpdateForPopTopFrameClosure already marked it for
deoptimization,
// so we just return and let the deoptimization code handle the pop frame
// as usual. If the caller is interpreted, we need to copy the arguments
// back in the expression stack and patch the return address so that the
// invoke is reexecuted.
src/hotspot/share/runtime/continuationFreezeThaw.cpp line 3101:
> 3099:
> 3100: // Called after fix_thawed_frame, the locals must be in their thawed
> form to
> 3101: // read the MemberName.
Suggestion:
// Called after fix_thawed_frame in case we need to recover MemberName from
local0.
src/hotspot/share/runtime/continuationFreezeThaw.cpp line 3104:
> 3102: static void popframe_restore_args(JavaThread* thread, const frame& f) {
> 3103: // the thawed frame's sp already covers the argument slots, last_sp
> was
> 3104: // saved after the arguments were pushed
Suggestion:
// The expression stack already includes the callee arguments, since last_sp
// was saved after they were pushed.
src/hotspot/share/runtime/continuationFreezeThaw.cpp line 3106:
> 3104: // saved after the arguments were pushed
> 3105: int top_element = f.interpreter_frame_expression_stack_size() - 1;
> 3106: // for a linkTo* call the trailing MemberName was consumed by the
> linker and
Suggestion:
// For a linkTo* call the trailing MemberName was consumed by the linker and
src/hotspot/share/runtime/continuationFreezeThaw.cpp line 3107:
> 3105: int top_element = f.interpreter_frame_expression_stack_size() - 1;
> 3106: // for a linkTo* call the trailing MemberName was consumed by the
> linker and
> 3107: // is not among the preserved arguments, take it from local 0 like the
Suggestion:
// is not among the preserved arguments. Take it from local 0 like the
src/hotspot/share/runtime/continuationFreezeThaw.cpp line 3108:
> 3106: // for a linkTo* call the trailing MemberName was consumed by the
> linker and
> 3107: // is not among the preserved arguments, take it from local 0 like the
> 3108: // interpreter's popframe entry does
Suggestion:
// interpreter's popframe entry does.
src/hotspot/share/runtime/continuationFreezeThaw.cpp line 3111:
> 3109: Method* method = f.interpreter_frame_method();
> 3110: address local0 = method->max_locals() > 0 ?
> (address)*f.interpreter_frame_local_at(0) : nullptr;
> 3111: oop member_name =
> InterpreterRuntime::popframe_member_name_arg_or_null(local0, method,
> f.interpreter_frame_bcp());
Suggestion:
oop member_name =
InterpreterRuntime::popframe_member_name_arg_or_null(local0, method,
f.interpreter_frame_bcp());
src/hotspot/share/runtime/continuationFreezeThaw.cpp line 3114:
> 3112: int words = in_words(thread->popframe_preserved_args_size_in_words());
> 3113: if (words > 0) {
> 3114: int first = top_element - (member_name != nullptr ? 1 : 0);
Suggestion:
assert(f.interpreter_frame_expression_stack_size() >= words + (member_name
!= nullptr ? 1 : 0), "no room for arguments");
int first = top_element - (member_name != nullptr ? 1 : 0);
An alternative stronger assert is:
#ifdef ASSERT
InterpreterOopMap mask;
f.interpreted_frame_oop_map(&mask);
assert(f.interpreter_frame_expression_stack_size() ==
mask.expression_stack_size() + words + (member_name != nullptr ?
1 : 0), "not enough room for arguments");
#endif
src/hotspot/share/runtime/continuationFreezeThaw.cpp line 3121:
> 3119: *f.interpreter_frame_expression_stack_at(top_element) =
> cast_from_oop<intptr_t>(member_name);
> 3120: }
> 3121: thread->clear_popframe_condition();
We should also restore the mdp before reexecuting the call, as we do in the
interpreter for the normal pop frame case.
src/hotspot/share/runtime/continuationFreezeThaw.cpp line 3123:
> 3121: thread->clear_popframe_condition();
> 3122: }
> 3123:
These methods could be moved into the existing `#if INCLUDE_JVMTI` block above,
alongside the other JVMTI methods.
src/hotspot/share/runtime/continuationFreezeThaw.cpp line 3158:
> 3156: if (popframe_reexecute) {
> 3157: popframe_restore_args(_thread, f);
> 3158: }
We can consolidate all these 3 steps into a single method call here, wrapped in
`JVMTI_ONLY()`.
test/hotspot/jtreg/runtime/interpreter/PopFrameMethodNameInvariantTest.java
line 62:
> 60:
> 61: /*
> 62: * @test id=unmount
Suggestion:
* @test id=virtual
This would make it consistent with `id=platform` above. Using `unmount` would
make sense if there are different runs with and without unmounting
(`id=virtual`, `id=virtual-unmount`). Same with the `unmount-mh` variant.
test/hotspot/jtreg/runtime/interpreter/PopFrameMethodNameInvariantTest.java
line 65:
> 63: * @bug 8391440
> 64: * @summary MemberName in local0 must be preserved when PopFrame
> re-executes direct MethodHandle.linkToStatic
> 65: * also when that caller is frozen in a virtual thread's stack
> chunk
Suggestion:
* in a caller restored from the heap
test/hotspot/jtreg/runtime/interpreter/PopFrameMethodNameInvariantTest.java
line 73:
> 71: * jdk.jdi
> 72: * @build java.base/java.lang.invoke.MethodHandleHelper
> 73: * @run driver runtime.interpreter.PopFrameMethodNameInvariantTest unmount
Suggestion:
* @run driver runtime.interpreter.PopFrameMethodNameInvariantTest virtual
test/hotspot/jtreg/runtime/interpreter/PopFrameMethodNameInvariantTest.java
line 77:
> 75:
> 76: /*
> 77: * @test id=platform-mh
Suggestion:
* @test id=platform-invokeExact
Both variants use method handle machinery so this makes it more clear. Same in
the vthread variant.
test/hotspot/jtreg/runtime/interpreter/PopFrameMethodNameInvariantTest.java
line 87:
> 85: * jdk.jdi
> 86: * @build java.base/java.lang.invoke.MethodHandleHelper
> 87: * @run driver runtime.interpreter.PopFrameMethodNameInvariantTest mh
Suggestion:
* @run driver runtime.interpreter.PopFrameMethodNameInvariantTest invokeExact
test/hotspot/jtreg/runtime/interpreter/PopFrameMethodNameInvariantTest.java
line 94:
> 92: * @bug 8391440
> 93: * @summary MemberName must be restored from the DirectMethodHandle in
> local0 when PopFrame re-executes
> 94: * linkToStatic in a LambdaForm frame frozen in a virtual
> thread's stack chunk
Suggestion:
* linkToStatic in a LambdaForm caller restored from the heap
test/hotspot/jtreg/runtime/interpreter/PopFrameMethodNameInvariantTest.java
line 107:
> 105: public class PopFrameMethodNameInvariantTest {
> 106: public static class Target {
> 107: static final boolean UNMOUNT =
> Boolean.getBoolean("popframe.unmount");
Suggestion:
static final boolean VIRTUAL = Boolean.getBoolean("popframe.virtual");
And related code below.
test/hotspot/jtreg/runtime/interpreter/PopFrameMethodNameInvariantTest.java
line 111:
> 109: static final long LONG_ARG = 0x1234567890ABL;
> 110: static final Object MARKER = new Object();
> 111: static final boolean USE_MH = Boolean.getBoolean("popframe.mh");
Suggestion:
static final boolean USE_INVOKE_EXACT =
Boolean.getBoolean("popframe.invokeExact");
And related code below.
test/hotspot/jtreg/runtime/interpreter/PopFrameMethodNameInvariantTest.java
line 120:
> 118: if (UNMOUNT) {
> 119: // sleeping unmounts this virtual thread, when it comes
> back the
> 120: // frames below body stay frozen until they are returned
> into
Suggestion:
// Sleep unmounts the virtual thread. After mounting again, the
// caller of body is not thawed, so PopFrame must restore it
// from the heap.
test/hotspot/jtreg/runtime/interpreter/PopFrameMethodNameInvariantTest.java
line 131:
> 129:
> 130: // invokeExact reaches body through a LambdaForm frame that
> keeps the DirectMethodHandle in local0,
> 131: // the helper keeps the MemberName there
Suggestion:
// The caller of body keeps a DirectMethodHandle in local 0 for
invokeExact,
// or a MemberName for the explicit linkToStatic call.
test/hotspot/jtreg/vmTestbase/nsk/jdwp/StackFrame/PopFrames/popframes001a.java
line 68:
> 66: log.display(" ... thread created");
> 67:
> 68: if (TestedThreadClass.UNMOUNT) {
We don’t need the extra `UNMOUNT` parameter as it is a common path for both
virtual thread tests. So this check could be `thread.isVirtual()` instead. Same
below in `testedMethod` (or check `parker`).
test/hotspot/jtreg/vmTestbase/nsk/jdwp/StackFrame/PopFrames/popframes001a.java
line 70:
> 68: if (TestedThreadClass.UNMOUNT) {
> 69: // the thread the tested method joins, it ends once the
> tested thread is parked in that join.
> 70: // It is made here so testedMethod has nothing before the
> join that could deoptimize it
Suggestion:
// The thread joined by the virtual thread in testedMethod. It
exits once the
// virtual thread has unmounted. Created here to avoid setup code
in testedMethod
// that could trigger deoptimization before join().
test/hotspot/jtreg/vmTestbase/nsk/jdwp/StackFrame/PopFrames/popframes001a.java
line 96:
> 94:
> 95: // popping only stop() would reach the breakpoint twice as well
> 96: if (TestedThreadClass.COMPILED_CALLER &&
> TestedThreadClass.invokations != 2) {
Shouldn't this be true for all cases and not just for the compiled caller case?
Also preexisting typo, it should be `invocations`.
test/hotspot/jtreg/vmTestbase/nsk/jdwp/StackFrame/PopFrames/popframes001a.java
line 149:
> 147: // the first join unmounts a virtual tested thread, so
> the frame below is frozen when popped,
> 148: // the parker is gone by the second one. Nothing before
> this join may deoptimize this frame
> 149: // or the thaw brings the caller out with it
Suggestion:
// The first join unmounts the tested thread, leaving its
caller in the
// heap when PopFrame is executed. Code before this join must
not deoptimize
// this frame, as thawing an interpreted frame also thaws its
compiled caller.
-------------
PR Review: https://git.openjdk.org/jdk/pull/32661#pullrequestreview-5717597602
PR Review Comment: https://git.openjdk.org/jdk/pull/32661#discussion_r4427822572
PR Review Comment: https://git.openjdk.org/jdk/pull/32661#discussion_r4427645070
PR Review Comment: https://git.openjdk.org/jdk/pull/32661#discussion_r4427692220
PR Review Comment: https://git.openjdk.org/jdk/pull/32661#discussion_r4427698735
PR Review Comment: https://git.openjdk.org/jdk/pull/32661#discussion_r4427726797
PR Review Comment: https://git.openjdk.org/jdk/pull/32661#discussion_r4427735218
PR Review Comment: https://git.openjdk.org/jdk/pull/32661#discussion_r4427744284
PR Review Comment: https://git.openjdk.org/jdk/pull/32661#discussion_r4427748654
PR Review Comment: https://git.openjdk.org/jdk/pull/32661#discussion_r4427752152
PR Review Comment: https://git.openjdk.org/jdk/pull/32661#discussion_r4427749782
PR Review Comment: https://git.openjdk.org/jdk/pull/32661#discussion_r4427756060
PR Review Comment: https://git.openjdk.org/jdk/pull/32661#discussion_r4427774760
PR Review Comment: https://git.openjdk.org/jdk/pull/32661#discussion_r4427792298
PR Review Comment: https://git.openjdk.org/jdk/pull/32661#discussion_r4427797106
PR Review Comment: https://git.openjdk.org/jdk/pull/32661#discussion_r4427801480
PR Review Comment: https://git.openjdk.org/jdk/pull/32661#discussion_r4427850875
PR Review Comment: https://git.openjdk.org/jdk/pull/32661#discussion_r4427866477
PR Review Comment: https://git.openjdk.org/jdk/pull/32661#discussion_r4427869254
PR Review Comment: https://git.openjdk.org/jdk/pull/32661#discussion_r4427890540
PR Review Comment: https://git.openjdk.org/jdk/pull/32661#discussion_r4427902209
PR Review Comment: https://git.openjdk.org/jdk/pull/32661#discussion_r4427908853
PR Review Comment: https://git.openjdk.org/jdk/pull/32661#discussion_r4427911422
PR Review Comment: https://git.openjdk.org/jdk/pull/32661#discussion_r4427919590
PR Review Comment: https://git.openjdk.org/jdk/pull/32661#discussion_r4427934048
PR Review Comment: https://git.openjdk.org/jdk/pull/32661#discussion_r4427943021
PR Review Comment: https://git.openjdk.org/jdk/pull/32661#discussion_r4427962571
PR Review Comment: https://git.openjdk.org/jdk/pull/32661#discussion_r4427971310
PR Review Comment: https://git.openjdk.org/jdk/pull/32661#discussion_r4427979030
PR Review Comment: https://git.openjdk.org/jdk/pull/32661#discussion_r4427986796
