RFR: 8331098: [Aarch64] Fix crash in Arrays.equals() intrinsic with -CCP [v2]
Axel Boldt-Christmas
aboldtch at openjdk.org
Fri Apr 26 10:31:35 UTC 2024
On Thu, 25 Apr 2024 11:50:52 GMT, Roman Kennke <rkennke at openjdk.org> wrote:
>> The implementations of Arrays.equals() in macroAssembler_aarch64.cpp, MacroAssembler::arrays_equals() assumes that the start of arrays is 8-byte-aligned. Since [JDK-8139457](https://bugs.openjdk.org/browse/JDK-8139457) this is no longer the case, at least when running with -CompressedClassPointers (or Lilliput). The effect is that the loops may run over the array end, and if the array is at heap boundary, and that memory is unmapped, then it may crash.
>>
>> The proposed fix aims to always enter the main loop(s) with an aligned address:
>> - When the array base is 8-byte-aligned (default, with +CCP), then compare the array lengths separately, then enter the main loop with the array base.
>> - When the array base is not 8-byte-aligned (-CCP and Lilliput), then enter the loop with the address of the array-length (which is then 8-byte-aligned), and compare array lengths in the main loop, and elide the explicit array lengths comparison.
>>
>> Testing:
>> - [ ] tier1 (+CCP)
>> - [ ] tier1 (-CCP)
>> - [ ] tier2 (+CCP)
>> - [ ] tier2 (-CCP)
>
> Roman Kennke has updated the pull request incrementally with two additional commits since the last revision:
>
> - Remove excess whitespace
> - Avoid loading cnt2 on paths that don't need it
src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 5553:
> 5551: bool is_8aligned = is_aligned(base_offset, BytesPerWord);
> 5552: assert(is_aligned(base_offset, BytesPerWord) || is_aligned(length_offset, BytesPerWord),
> 5553: "base_offset or length_offset must be 8-byte aligned");
Not that I see this changing. But the correctness of using `length_offset` relies on the length and the payload being consecutive in memory. Should probably assert this.
Suggestion:
assert(is_aligned(base_offset, BytesPerWord) || is_aligned(length_offset, BytesPerWord),
"base_offset or length_offset must be 8-byte aligned");
assert(is_aligned(base_offset, BytesPerWord) || base_offset == length_offset + BytesPerInt,
"base_offset must be 8-byte aligned or no padding between base and length");
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18948#discussion_r1580836583
More information about the hotspot-dev
mailing list