RFR: 8352141: UBSAN: fix the left shift of negative value in relocInfo.cpp, internal_word_Relocation::pack_data_to()

[Kim Barrett]

On Mon, 24 Mar 2025 13:18:25 GMT, Afshin Zafari <azafari at openjdk.org> wrote:

> The `offset` variable used in left-shift op can be a large number with its sign-bit set. This makes a negative value which is UB for left-shift and is reported as 
> `runtime error: left shift of negative value -25 at relocInfo.cpp:...`
>  
> Using `java_left_shif()` function is the workaround to avoid UB. This function uses reinterpret_cast to cast from signed to unsigned and back.
> 
> Tests:
> linux-x64-debug tier1 on a UBSAN enabled build.

I think the description in the PR is wrong.  I think the `offset` variable is
not a positive number so large that the sign bit gets set (effectively
treating it as unsigned).  Rather, it's explicitly negated in `scaled_offset`.
So this code is (almost) always doing a shift of a negative value.  (Looks
like @dean-long found this too.)

The comments I made about shift of negative values in
https://github.com/openjdk/jdk/pull/24184
also apply here. I'm inclined to call the ubsan warning here an effectively
false positive, because we don't (and never have) cared about the technical UB
that no implementation is making use of and will be going away with C++20.

I think a new `left_shift_no_overflow()` operation isn't really needed here.
As @vnkozlov notes, the range of values is just not going to be so large that
overflow is an issue here. Though such an operation might be useful for other
reasons. It would be a shared place to hang the ubsan-ignore of the shift of a
negative value, rather than ATTRIBUTE_NO_UBSAN littering.

Maybe instead the negation of the offset could be removed, and wherever the
value is used could be updated to account for that change.  Or maybe we should
just ignore the ubsan warning here.

This is not an appropriate use of a JAVA_INTEGER{_SHIFT}_OP.
https://github.com/openjdk/jdk/blame/a81250c55312dfdeb4d65970cff683e6f0783ca7/src/hotspot/share/utilities/globalDefinitions.hpp#L1201-L1204

// Sum and product which can never overflow: they wrap, just like the
// Java operations.  Note that we don't intend these to be used for
// general-purpose arithmetic: their purpose is to emulate Java
// operations.


That is, they should only be used where we're emulating Java arithmetic, such
as by the compiler constant folding a Java arithmetic expression. That isn't
the case here.

(The comment only mentions "sum and product" because it wasn't updated to
account for the later addition of the shift operations.)

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

PR Comment: https://git.openjdk.org/jdk/pull/24196#issuecomment-2754822720