[lworld] RFR: 8367151: [lworld] CorrectlyRestoreRfp.java triggers "bad oop found" during deoptimization

[Tobias Hartmann]

On Thu, 20 Nov 2025 12:13:53 GMT, Marc Chevalier <mchevalier at openjdk.org> 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