RFR: 8352140: UBSAN: fix the left shift of negative value in klass.hpp, array_layout_helper()

[Kim Barrett]

On Fri, 28 Mar 2025 13:21:02 GMT, Afshin Zafari <azafari at openjdk.org> wrote:

>> Is there a way to tell ubsan that we care about detecting overflows, but we do not care about detecting
>> left shift of a negative value?  Not that I can find, but maybe I missed something.  `-fsanitize=shift-base`
>> looks like it would check for both overflow and (prior to C++20) negative base.  We could disable
>> shift-base checking and do our own overflow assertion.  (Which might want to be packaged up in a helper,
>> as discussed in https://github.com/openjdk/jdk/pull/24196.)
>
> For the case in this PR, the left-shift results in overflow, since the operand is either 0xFFFFFFFF or 0xFFFFFFFE. Should we have two versions of `left_shift()` and `left_shift_no_overflow()`?

Sorry, I don't see where these 0xFFFFFFFF and 0xFFFFFFFE values come into
play.  (Also note that a left shift of a 32bit 0xFFFFFFFF cannot overflow
without exceeding the maximum allowed shift quantity (31), which is a
different UB form overflow.)

I think the fundamental problem here is that these masks and masking
operations are all using signed int, but should be using unsigned int.
Part of the problem here is (now) inappropriate use of jint.  (It might have
been slightly more appropriate when the code was originally written, as
[u]int32_t might not have been a thing then.  I suspect the basic framework
here is *very* old.)

Looking through this area more globally, I think things would be better if the
basic layout_helper type was uint32_t, with all the layout_helper_xxx
functions and masks adjusted accordingly.  I think that eliminates the need
for a whole mess of casts, and avoids UB from integer overflow.

So rather than working around one specific failure case, I suggest doing some
deeper cleaning here.

Related, I also just noticed a bogus test for "overflow" in
layout_helper_boolean_diffbit. UB is required for the assert to fail, so the
assert can be optimized away.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24184#discussion_r2055739014