RFR: 8347006: LoadRangeNode floats above array guard in arraycopy intrinsic

[Aleksey Shipilev]

On Wed, 8 Jan 2025 12:07:16 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

> C2's arraycopy intrinsic adds guards that check that the source and destination objects are arrays:
> https://github.com/openjdk/jdk/blob/afe543414f58a04832d4f07dea88881d64954a0b/src/hotspot/share/opto/library_call.cpp#L5917-L5919
> 
> If these guards pass, the array length is loaded:
> https://github.com/openjdk/jdk/blob/afe543414f58a04832d4f07dea88881d64954a0b/src/hotspot/share/opto/library_call.cpp#L5930-L5933
> 
> But since the `LoadRangeNode` is not pinned, it might float above the array guard:
> https://github.com/openjdk/jdk/blob/afe543414f58a04832d4f07dea88881d64954a0b/src/hotspot/share/opto/graphKit.cpp#L1214
> 
> If the object is not an array, we will read garbage. That's usually fine because the result will not be used (the array guard will trigger) but with `-XX:+UseCompactObjectHeaders` it can happen that the memory right after the header is not mapped and we crash.
> 
> The fix is to add a `CheckCastPPNode` to propagate the information that the operand is an array and prevent the load from floating. 
> 
> Thanks to @shipilev for identifying the root cause!
> 
> I was able to reliably reproduce the issue with `compiler/arraycopy/TestArrayCopyNoInit.java` and `-XX:-UseTLAB -XX:+UnlockExperimentalVMOptions -XX:-UseCompressedClassPointers` on Linux AArch64 and verified that the fix solves the problem.
> 
> Best regards,
> Tobias

I have a question about the test :)

test/hotspot/jtreg/compiler/arraycopy/TestArrayCopyNoInit.java line 32:

> 30:  *                   compiler.arraycopy.TestArrayCopyNoInit
> 31:  * @run main/othervm -XX:-BackgroundCompilation -XX:-UseOnStackReplacement -XX:TypeProfileLevel=020
> 32:  *                   -XX:+UnlockExperimentalVMOptions -XX:+UseCompactObjectHeaders -XX:-UseTLAB

You have been able to reproduce this with `-UseCompressedClassPointers`, right? If so, I'd suggest we do a run config with `-UseCCP` instead of `+UseCOH`, because this gives us a cleaner way for backports, if we need one later.

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

PR Review: https://git.openjdk.org/jdk/pull/22967#pullrequestreview-2537002862
PR Review Comment: https://git.openjdk.org/jdk/pull/22967#discussion_r1907099990