On Wed, 18 Feb 2026 16:18:53 GMT, Marc Chevalier <[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 r... > > 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. On second thought, we are very much in the guts of this exact logic, so maybe if we wander here, we know, or we come from `remove_frame`'s neighborhood, so maybe it'd just be noise to add such a comment. ------------- PR Review Comment: https://git.openjdk.org/valhalla/pull/2085#discussion_r2823225235
