On Thu, 20 Nov 2025 12:13:53 GMT, Marc Chevalier <[email protected]> wrote:
> We are on aarch64. > > When a function needs stack extension, we build a stack that has this shape: > > https://github.com/openjdk/valhalla/blob/b1d14c658511cced2a860d9e2741ae89827e25ba/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp#L6040-L6073 > > Currently, when leaving the frame, we use LR §1 (I use § not to mess with > github rendering that interpret `#` as PR references) as return address > (because it can be patched for deoptimization), and FP §2 to restore x29 > (because when it contains an oop, the GC is only aware of this copy). > > In our failing case, we have a C2-compiled frame that is being deoptimized > when returning from a call to an interpreted method. During deoptimization, > the function `frame::sender_for_compiled_frame(RegisterMap*) const` is used > to locate the location on the stack where rfp (x29) is saved. > > https://github.com/openjdk/valhalla/blob/b1d14c658511cced2a860d9e2741ae89827e25ba/src/hotspot/cpu/aarch64/frame_aarch64.inline.hpp#L446 > > Actually this function is a bit more general: it computes the sender frame of > a compiled frame, and build the `RegisterMap`. The problem is that during > deoptimization, this function locates the wrong save of rfp (FP §1) because > the C2 frame is being modified by the deoptimization process and it's not > anymore recognized as a C2-compiled method that needs stack repairs. In this > modified frame the sender's sp is correctly known (or the deoptimization > mechanism would not work), and the saved FP is taken just 2 words above: that > is FP §1. On top of that, if rfp contained an oop and the GC moved the > pointed object during the call we are returning from, the value we get for > rfp is not valid anymore. > > The good and bad news is that the GC also locates the saved location of rfp > thanks to the same function. The bad news is that GC sees the C2 frame > correctly, and so `sender_for_compiled_frame` can locate FP §2. We can follow > a few ideas: > - make the deoptimized frame bottom under FP/LR §2. This is not possible, for > many reasons: we need LR §1, we need to remove the whole frame to find the > sender's frame... > - make `sender_for_compiled_frame` detects when the deoptimized frame is the > one of a C2 compiled method that needs stack repair. No idea how to do that! > Also, it seems brittle, and more complicated than the next solution. > - always pick FP §1: since the deoptimized frame will pick FP §1, in case > it's a regular C2 frame, we can also make sure to use FP §1. It is the > simplest solution and the one I explain after. > > In [JDK-836... Thanks for the detailed explanation, this is great for future reference! The changes look good to me. Just a few comments. > I don't have much opinion about the names of compiled_frame_details and > CompiledFramePointers, feel free to suggest better if you have a better idea. Naming is fine with me but do we even need to factor this logic out? Do you expect it to be used in more places in the future? There seems to be a merge conflict. src/hotspot/cpu/aarch64/frame_aarch64.cpp line 836: > 834: > 835: #ifdef ASSERT > 836: // when the stack was extnded (so LR #1 and LR #2 are distinct) and LR > #1 was patched Suggestion: // when the stack was extended (so LR #1 and LR #2 are distinct) and LR #1 was patched src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 6079: > 6077: // from C++ in pointer arithmetic; you might need to divide it by > wordSize. > 6078: // > 6079: // TODO 8371993 store fake values instyead of LR/FP#2 Suggestion: // TODO 8371993 store fake values instead of LR/FP#2 ------------- Marked as reviewed by thartmann (Committer). PR Review: https://git.openjdk.org/valhalla/pull/1751#pullrequestreview-3491681707 PR Comment: https://git.openjdk.org/valhalla/pull/1751#issuecomment-3561978100 PR Review Comment: https://git.openjdk.org/valhalla/pull/1751#discussion_r2548934377 PR Review Comment: https://git.openjdk.org/valhalla/pull/1751#discussion_r2548940387
