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

[Roman Kennke]

On Wed, 8 May 2024 07:40:52 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

>> 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");
>
> Also right now the `length_is_8aligned` very much now just look like a `!oopDesc::has_klass_gap()` check.
> 
> The validity of choosing to start from the klass when `!length_is_8aligned` is that we must then be using `UseCompressedClassPointers /* && !UseCompactObjectHeaders  for Lilliput */`. Until Lilliput starting from the Klass is valid for both `+UseCompressedClassPointers` and `-UseCompressedClassPointers` as the klass, length and base will always be tightly packed (no padding) for byte and char type arrays. 
> 
> In all modes what we effectively do is `int start_offset = align_down(length_offset, BytesPerWord )`. Not sure if the intent gets clearer then. Something along the lines of:
> Suggestion:
> 
>   // When the length offset is not aligned to 8 bytes,
>   // then we align it down, this is valid as the new
>   // offset will always be the klass which is the same
>   // for type arrays.
>   int start_offset = align_down(length_offset, BytesPerWord);
>   int extra_length =  base_offset - start_offset;
>   assert(start_offset == length_offset || start_offset == klass_offset, 
>          "start offset must be 8-byte-aligned or be the klass offset");
>   assert(base_offset != start_offset, "must include the length field");

Right, not assuming layout of Klass* seems saner.
BTW, including the Klass* is not only safe because it's only char[] or byte[], it's also already guaranteed to be the same by the calling APIs. And even if it were not (arraycopy-style) - if the Klass* could be different, then array-equality would have to check this, also, anyway.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18948#discussion_r1593613289