On Thu, 12 Feb 2026 00:49:09 GMT, Patricio Chilano Mateo <[email protected]> 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... I can't follow the details of the address calculations in this, but I like some things about this change: 1. the consistency between x86 and aarch64, 2. the conditional argument callee_augmented is removed, 3. Using compiled_frame_details is nice, and 4. poisoning the unused pc and rbp addesses in the stack so you can find misuses quickly. Maybe compiled_frame_details should be merged up to mainline? src/hotspot/cpu/x86/c1_MacroAssembler_x86.cpp line 243: > 241: movl(Address(rsp, VMRegImpl::stack_slot_size), badRegWordVal); > 242: } > 243: #endif I like this. ------------- Marked as reviewed by coleenp (Committer). PR Review: https://git.openjdk.org/valhalla/pull/2085#pullrequestreview-3822125091 PR Review Comment: https://git.openjdk.org/valhalla/pull/2085#discussion_r2824359142
