RFR: 8331098: [Aarch64] Fix crash in Arrays.equals() intrinsic with -CCP [v6]

[Axel Boldt-Christmas]

On Tue, 7 May 2024 14:00:07 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:
>>  - [x] tier1 (+CCP)
>>  - [x] tier1 (-CCP)
>>  - [x] tier2 (+CCP)
>>  - [x] tier2 (-CCP)
>
> Roman Kennke has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Unify impls for +/-CCP

The change to include the class looked a little scary to me. Both that this code makes even more assumptions of the object layout (without asserts), and with its interactions/meaning in Lilliput.

Because this is only used for byte and char type arrays including the klass should be harmless. 

I have two different suggestions, I believe both will work in Lilliput as well. The second is a bit cleaner imo.

Regardless the comments and asserts should be cleaned up.

src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 5558:

> 5556:   int start_offset = length_is_8aligned ? length_offset : klass_offset;
> 5557:   assert(is_aligned(start_offset, BytesPerWord), "start offset must be 8-byte-aligned");
> 5558:   int extra_length = length_is_8aligned ? base_offset - length_offset : base_offset - klass_offset;

Some of these asserts and comments seem a little bit out of place, inaccurate and/or incomplete with the new change to include the klass in the comparison. 

Suggestion:

  // When the length offset is not aligned to 8 bytes, 
  // then we let the compare loop include the klass
  // and length, otherwise start from the length.
  bool length_is_8aligned = is_aligned(length_offset, BytesPerWord);
  int start_offset = length_is_8aligned ? length_offset : klass_offset;
  assert(is_aligned(start_offset, BytesPerWord), "start offset must be 8-byte-aligned");
  int extra_length =  base_offset - start_offset;
  assert(!length_is_8aligned || extra_length == BytesPerInt,
         "no padding between length and base");
  assert(length_is_8aligned || extra_length == BytesPerWord,
         "no padding between klass, length and base");

src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 5593:

> 5591:     bind(A_IS_NOT_NULL);
> 5592:     ldrw(cnt1, Address(a1, length_offset));
> 5593:     assert(extra_length != 0, "expect extra length");

Not needed with my other suggested asserts.
Suggestion:

src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 5660:

> 5658:     ldrw(cnt1, Address(a1, length_offset));
> 5659:     cbz(a2, DONE);
> 5660:     assert(extra_length != 0, "expect extra length");

Not needed with my other suggested asserts.
Suggestion:

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

Changes requested by aboldtch (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18948#pullrequestreview-2044827759
PR Review Comment: https://git.openjdk.org/jdk/pull/18948#discussion_r1593510196
PR Review Comment: https://git.openjdk.org/jdk/pull/18948#discussion_r1593510782
PR Review Comment: https://git.openjdk.org/jdk/pull/18948#discussion_r1593511187