RFR: 8337958: Out-of-bounds array access in secondary_super_cache [v2]

[Aleksey Shipilev]

On Wed, 7 Aug 2024 23:56:14 GMT, Andrew Haley <aph at openjdk.org> wrote:

>> src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 1734:
>> 
>>> 1732:   assert(Klass::SECONDARY_SUPERS_BITMAP_FULL == ~uintx(0), "");
>>> 1733:   cmpw(r_array_length, (u1)(Klass::SECONDARY_SUPERS_TABLE_SIZE - 2));
>>> 1734:   br(GT, L_huge);
>> 
>> Silly questions:
>>  1. Why is it `(u1)`, when we are comparing with `cmpw` (4 bytes)? Also, should it really be unsigned? x86 code uses signed `int32_t`.
>>  2. I was trying to see if there is anything special about `-2` here. Would it be a bit cleaner to say `GE` `Klass::SECONDARY_SUPERS_TABLE_SIZE - 1`?
>
>> Silly questions:
>> 
>>     1. Why is it `(u1)`, when we are comparing with `cmpw` (4 bytes)? Also, should it really be unsigned? x86 code uses signed `int32_t`.
> 
> Yeah, but AArch64 has a restricted rage of operand sizes. There's a very long thread where we discussed all of this, but we ended up defining `cmpw` for `(u1)`. This means we never see an overflow at runtime.
> 
>>     2. I was trying to see if there is anything special about `-2` here. Would it be a bit cleaner to say `GE` `Klass::SECONDARY_SUPERS_TABLE_SIZE - 1`?
> 
> Mmm, maybe, but it means the same to me. It's just a performance optimization that does a linear search when the table is almost full, because in measurements it's faster to do so.

All right, fine.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20483#discussion_r1709029997