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

[Kim Barrett]

On Tue, 25 Mar 2025 06:46:14 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> src/hotspot/share/oops/klass.hpp line 527:
>> 
>>> 525:   }
>>> 526:   static jint array_layout_helper(jint tag, int hsize, BasicType etype, int log2_esize) {
>>> 527:     return ((juint)tag << _lh_array_tag_shift)
>> 
>> Doesn't this turn the type of the return expression to unsigned, causing an implicit conversion back to signed, which is implementation-defined?  I think that's the reason for the weird-looking reinterpret_cast<> in JAVA_INTEGER_OP.
>
> Note we seem to do this a lot e.g.
> 
>> ./share/oops/klass.cpp:  `int  tag   =  isobj ? _lh_array_tag_obj_value : _lh_array_tag_type_value;`
> 
> `_lh_array_tag_type_value` is unsigned. So this call sequence goes from unsigned -> signed -> unsigned -> signed

A problem we are facing here is that C++20 makes some integral operations
defined that were previously undefined. This followed what implementations were
actually doing. And yet, tools like ubsan (and constexpr-processing until
C++20) treat them as UB.

For example,
https://gcc.gnu.org/onlinedocs/gcc-14.2.0/gcc/Integers-implementation.html
"The results of some bitwise operations on signed integers (C90 6.3, C99 and C11 6.5).
...
As an extension to the C language, GCC does not use the latitude given in C99
and C11 only to treat certain aspects of signed ‘<<’ as undefined. However,
-fsanitize=shift (and -fsanitize=undefined) will diagnose such cases. They are
also diagnosed where constant expressions are required."

Hence, I'm at least somewhat inclined to call a ubsan complaint about left
shift of a negative value a false positive.

The implementation-defined behavior of unsigned => signed conversions is
another thing that C++20 changed to be defined.

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

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