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

[Kim Barrett]

On Wed, 26 Mar 2025 09:33:46 GMT, Afshin Zafari <azafari at openjdk.org> wrote:

>> signed short int x = -32768;
>>     signed short int y = x << 1;
>> 
>> 
>> That does seem like an interestingly weird case. Unless I'm missing something,
>> there's no UB-overflow in that. The shift expression promotes `short x` to
>> `int x`, sign extending it. The `int`-typed shift is fine (since C++20, and
>> effectively so prior to that in non-constexpr-required contexts - see below).
>> And the implicit conversion to `short y` is implementation-defined (before
>> C++20, though gcc may warn (-Woverflow)) or fine (since C++20).
>> 
>> gcc warns about x being negative in C++11 to C++17 modes
>> (-Wshift-negative-value enabled by default), but doesn't treat it as UB.
>> Before C++20 gcc errors (warns if -fpermissive) if it's in a
>> required-constexpr-context, even if -Wshift-negative-value is disabled.
>> That all seems consistent.
>
> I had to emphasize that the case shown in the example may happen at run-time where compiler has no chance to warn/avoid/address it. 
> My concern is that developers should not rely on the compiler to check the validation of left-shift op. They should be aware of the `signed` <-> `unsigned` and `int` <-> `long` <-> `long long` conversions during the left-shift.
> To find invalid cases of left-shift, UBSAN instruments them with assertions to catch them at run-time. If the assertion raised, good we found the problem. However, if no assertion raised for some left-shift ops, it doesn't mean that they are valid.

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.)

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

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