RFR: 8347006: LoadRangeNode floats above array guard in arraycopy intrinsic [v2]

[Quan Anh Mai]

On Wed, 8 Jan 2025 13:08:52 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:+UseCompactObjectHeaders` on Linux AArch64 and verified that the fix solves the problem.
>> 
>> Best regards,
>> Tobias
>
> Tobias Hartmann has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Added missing stopped checks, refactoring and updated copyright dates

src/hotspot/share/opto/library_call.cpp line 5921:

> 5919:     generate_non_array_guard(load_object_klass(src), slow_region);
> 5920:     if (!stopped()) {
> 5921:       src = _gvn.transform(new CheckCastPPNode(control(), src, TypeAryPtr::BOTTOM));

Why is this a `CheckCastPP` and not a `CastPP`? My understanding is that a `CheckCastPP` is used when we force changing the type of a node (e.g a raw pointer of `Allocate` into a typed pointer), so we do not join the type of the input with that of the output.

src/hotspot/share/opto/type.hpp line 1476:

> 1474: 
> 1475:   // Convenience common pre-built types.
> 1476:   static const TypeAryPtr* BOTTOM;

While you are here it may be better to change the other constant to `TypeAryPtr*` instead of `TypeAryPtr *`

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22967#discussion_r1907435011
PR Review Comment: https://git.openjdk.org/jdk/pull/22967#discussion_r1907440010