RFR: 8329258: [AArch64] TailCall should not use frame pointer register for jump target

[Tobias Hartmann]

On Wed, 10 Apr 2024 14:22:14 GMT, Martin Doerr <mdoerr at openjdk.org> wrote:

>> Applying @danielogh's patch (see description of [JDK-8329258](https://bugs.openjdk.org/browse/JDK-8329258)) to enable `StressGCM` / `StressLCM` for stub compilations triggers a crash on AArch64. The problem is that the register allocator uses `R29` (`rfp`) which is usually used for the frame pointer to hold the `TailCall` `exc_target` when generating the `_slow_arraycopy_Java` stub:
>> https://github.com/openjdk/jdk/blob/6dfb8120c270a76fcba5a5c3c9ad91da3282d5fa/src/hotspot/share/opto/generateOptoStub.cpp#L258-L264
>> 
>> With `StressGCM` / `StressLCM` the register initialization is scheduled early and `R29` is corrupted by the `MachEpilogNode` which is opaque to the register allocator and inserted right before the `TailCall`:
>> https://github.com/openjdk/jdk/blob/b49ba426a721db5926ac1b45d573d468389d479c/src/hotspot/share/opto/output.cpp#L320-L326 
>> 
>> 
>> 028 mov R29, 0x0000ffff78cc0080 # ptr
>> 
>> [...]
>> 
>> 098     # pop frame 16
>>         ldp  lr, rfp, [sp,#0]              <- Epilog kills rfp (and lr + sp)
>>         add  sp, sp, #16
>> 
>> [...]
>> 
>> 0a0 br R29 # R12 holds method
>> 
>> 
>> As a result, we jump to a "garbage" location. See [bad code](https://bugs.openjdk.org/secure/attachment/108835/BAD_slow_arraycopy_Java.txt) vs. [good code](https://bugs.openjdk.org/secure/attachment/108834/GOOD_slow_arraycopy_Java.txt).
>> 
>> On x86, we use `no_rbp_RegP` instead:
>> https://github.com/openjdk/jdk/blob/b49ba426a721db5926ac1b45d573d468389d479c/src/hotspot/cpu/x86/x86_64.ad#L2564-L2566 https://github.com/openjdk/jdk/blob/b49ba426a721db5926ac1b45d573d468389d479c/src/hotspot/cpu/x86/x86_64.ad#L12470
>> 
>> I implemented the same on AArch64.
>> 
>> I think other platforms are affected as well but I don't have the hardware to test there. @offamitkumar (S390), @TheRealMDoerr (PPC), @RealFYang (RISC-V), @bulasevich (ARM32), could you please have a look?
>> 
>> I also wondered if `R29` shouldn't be a callee-save (SOE) register in the C calling convention?
>> https://github.com/openjdk/jdk/blob/b49ba426a721db5926ac1b45d573d468389d479c/src/hotspot/cpu/aarch64/aarch64.ad#L139-L140 On x86_64, `RBP` is SOE:
>> https://github.com/openjdk/jdk/blob/b49ba426a721db5926ac1b45d573d468389d479c/src/hotspot/cpu/x86/x86_64.ad#L86-L87
>> 
>> `TestTailCallInArrayCopyStub.java` will only work once [JDK-8330016](https://bugs.openjdk.org/browse/JDK-8330016) is integrated.
>> 
>> Thanks,
>> Tobias
>
> I don't think PPC64 or s390 are affected. These platforms don't have a frame pointer register. Both currently use a fixed register for the jump target (the same one as for inline caches).
> The test passes on PPC64. Thanks for pinging me!

@TheRealMDoerr , @offamitkumar thanks for checking!

> Both currently use a fixed register for the jump target (the same one as for inline caches).

Looks to me as if no fixed register but `iRegP` is used for `jump_target`:
https://github.com/openjdk/jdk/blob/b49ba426a721db5926ac1b45d573d468389d479c/src/hotspot/cpu/s390/s390.ad#L9505

But the `MachEpilog` code does look safe, as you said, there is no frame pointer register.

RISC-V and ARM32 look affected to me though. Waiting for confirmation. Please note that `TestTailCallInArrayCopyStub.java` needs [JDK-8330016](https://bugs.openjdk.org/browse/JDK-8330016) to trigger the issue.

> Maybe for documentation purposes, but I don't think it would have any effect on generated code.

Thanks for checking. @dean-long, @theRealAph any preferences? Or should we leave it as is?

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

PR Comment: https://git.openjdk.org/jdk/pull/18716#issuecomment-2048947292