[lworld] RFR: 8377714: [lworld] Re-enable virtual thread tests

Wed Feb 18 16:35:26 UTC 2026

On Thu, 12 Feb 2026 00:49:09 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

> Please review the following patch which re-enables virtual thread tests `TestVirtualThreads.java`‎ and `Fuzz.java`.
> 
> First, this patch fixes the AArch64 virtual thread code to adapt to the changes in [JDK-8367151](https://bugs.openjdk.org/browse/JDK-8367151) where the valid FP is now stored in copy `#1` instead of copy `#2`:
> 
> https://github.com/openjdk/valhalla/blob/c0b679480a10bc9c71aa75ed26b3cfd0e69d294c/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp#L5960-L6007
> 
> With that change we can now simplify the `stackChunk` walking code and the thawing patching logic, since we don't need to keep track of two separate stack pointers, one to access the saved pc (`_unextended_sp[-1]`) and another to access the saved fp (`_sp[-2]`). Also, method `compiled_frame_details()` enables simplification of `FreezeBase::sender`.
> Follow-up changes from [JDK-8371993](https://bugs.openjdk.org/browse/JDK-8371993) uncovered that in `ContinuationHelper::Frame::return_pc_address` we were reading the saved pc from the wrong location. This is also fixed in this PR. Going forward though, it seems the issue is with `f.real_fp()` which should include the extra added size for extended frames.
> 
> Second, the x64 code has been updated so that the layout of the extended frames is the same as with AArch64, and that we also only use the `#1` copies. This not only aligns behavior across both platforms but also allows simplification of the virtual thread code as mentioned above with AArch64. Changes in [JDK-8372806](https://bugs.openjdk.org/browse/JDK-8372806) uncovered the same issue with `ContinuationHelper::Frame::return_pc_address` and that is fixed as well.
> Note: While working on this, I noticed that for extended frames, with `-XX:+PreserveFramePointer`, rbp is set to the location of copy `#2` rather than copy `#1`. Storing bad values in these locations will break the chain of pointers though, so we probably want to set rbp to location of copy `#1`. Same with AArch64.
> 
> Finally, for both AArch64 and x64 I updated `frame::describe_pd` to show correct locations of saved return pc and FP for extended frames. I still kept the locations where `#2` copies are stored. This has proven useful for debugging purposes.
> 
> Changes were tested by running both `TestVirtualThreads.java‎` and `Fuzz.java`, job `valhalla-comp-stress` in mach5, as well as tiers1-3. I also run `TestVirtualThreads.java‎` with extra flags, including `-Xcomp -XX:+SafepointALot -XX:+DeoptimizeALot` as reported in [JDK-8370177](https...

So far, it looks good, but I have a few questions in the way of my understanding. I probably simply miss context on virtual threads.

src/hotspot/cpu/aarch64/continuationHelper_aarch64.inline.hpp line 132:

> 130:     log_trace(continuations)("real_frame_size is addr is " INTPTR_FORMAT, p2i(real_frame_size_addr));
> 131:     return (address*)(f.unextended_sp() + (*real_frame_size_addr/wordSize) + 1);
> 132:   } else {

Why `compiled_frame_details()` can't be used here? I probably don't understand what this method does. At first glance, it looks like it's `sender_pc_addr`, but I imagine there is something subtle going on.

Of course, the same question applies to the x64 version of it.

src/hotspot/cpu/x86/frame_x86.cpp line 660:

> 658:     // and does not account for the return address.
> 659:     intptr_t* real_frame_size_addr = (intptr_t*) (saved_fp_addr - 1);
> 660:     int real_frame_size = (*real_frame_size_addr / wordSize) + 2;

I think it'd be good to add a little comment to explain where this `2` comes from. Maybe mentioning what it's used for, or just send the reader to `remove_frame`'s comment.

src/hotspot/cpu/x86/macroAssembler_x86.cpp line 6524:

> 6522:     // |---------------------------|  <-- caller's SP
> 6523:     // | Return address #1         |
> 6524:     // | Saved RBP #1              |

I like it! I can't swear there is no weird shift in `extend_stack_for_inline_args` but it looks good, and if there were such a mistakes., tests would be on fire.

src/hotspot/share/runtime/continuationFreezeThaw.cpp line 2893:

> 2891:   // As a result, f.is_deoptimized_frame() is always false and we must test hf to know if the frame is deoptimized.
> 2892:   frame f = new_stack_frame<ContinuationHelper::CompiledFrame>(hf, caller, is_bottom_frame, augmented ? fsize - hf.cb()->frame_size() : 0);
> 2893:   assert((int)(caller.sp() - f.sp()) == (augmented ? fsize : f.cb()->frame_size()), "");

The expression `f.cb()->frame_size()` is not crashing only if `f` is compiled otherwise `f.cb() == nullptr`. But `f` seems to be a compiled frame (since `new_stack_frame<ContinuationHelper::CompiledFrame>`), so why `f.cb()->frame_size()` isn't always what we want?

src/hotspot/share/runtime/stackChunkFrameStream.inline.hpp line 231:

> 229: #ifndef ZERO
> 230:       } else if (cb()->is_nmethod() && cb()->as_nmethod()->needs_stack_repair()) {
> 231:         _sp = frame::repair_sender_sp(cb()->as_nmethod(), _unextended_sp, (intptr_t**)(_sp - frame::sender_sp_offset));

It is slightly unfortunate we have to call `repair_sender_sp` outside the internal mysteries of `frame`, but I don't see a much better option. We are doing some low-level frame tweaking, so it's not entirely out-of-place.

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

PR Review: https://git.openjdk.org/valhalla/pull/2085#pullrequestreview-3820862499
PR Review Comment: https://git.openjdk.org/valhalla/pull/2085#discussion_r2823194167
PR Review Comment: https://git.openjdk.org/valhalla/pull/2085#discussion_r2823214844
PR Review Comment: https://git.openjdk.org/valhalla/pull/2085#discussion_r2823240103
PR Review Comment: https://git.openjdk.org/valhalla/pull/2085#discussion_r2823266106
PR Review Comment: https://git.openjdk.org/valhalla/pull/2085#discussion_r2823274173