RFR: 8365163: [ubsan] left-shift issue in globalDefinitions.hpp

[Afshin Zafari]

On Sun, 17 Aug 2025 12:41:52 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> src/hotspot/share/utilities/globalDefinitions.hpp line 1079:
>> 
>>> 1077:   return result;
>>> 1078: }
>>> 1079: 
>> 
>> This still has an overflowing left shift, and it's hard for the reader to follow.
>> 
>> I'd do something like this:
>> 
>> 
>> inline void set_low (jlong* value, jint low ) {
>>   union {
>>     jlong value_s;
>>     julong value_u;
>>   };
>>   value_s = *value;
>>   value_u = (value_u & ~(julong)0xffffffff) | (julong)low;
>>   *value = value_s;
>> }
>> 
>> inline void set_high(jlong* value, jint high) {
>>   union {
>>     jlong value_s;
>>     julong value_u;
>>   };
>>   value_s = *value;
>>   value_u = (value_u & (julong)0xfffffffful) | ((julong)high << 32);
>>   *value = value_s;
>> }
>
> First, we do not care about left shift of negative value on its own
> (JDK-8240259). Indicating this issue has anything to do with that is just
> confusing matters.
> 
> What we care about is signed integer overflow, which is UB. And there is
> indeed such overflow in this code. Having the JBS issue and the PR description
> be more direct about that would have avoided confusion on my part.
> 
> (I'm somewhat surprised none of our compilers just breaks this obvious
> overflow. Or maybe they do and we've not noticed, though that seems unlikely.)
> 
> As @theRealAph points out, the proposed change still contains overflows. For example
> 
> 1061:  *value &= (jlong)0xffffffff << 32;
> 
> 
> But this can be vastly simplified to just
> 
> inline jlong jlong_from(jint h, jint l) {
>   // First cast jint values to juint, so cast to julong will zero-extend.
>   julong high = (julong)(juint)h << 32;
>   julong low = (julong)(juint)l;
>   return (jlong)(high | low);
> }
> 
> and delete the now unused set_high and set_low functions. (There are set_high
> and set_low functions in sharedRuntimeMath.hpp, but they deal with double
> rather than jlong.)

I am aware of not caring the issues of 'left-shift of negative values' since after C++20 they will not be UB anymore. I use UBSAN left-shift checks for finding other hidden/potential problems around left-shifts.

Sorry if missing enough explanation in title and/or description parts brought you confusion.

AFAIU, the code `(jlong)0xffffffff << 32` is not problematic since `jlong` is 64 bits and `0xffffffff` is `unsigned int` then casting to `jlong` makes it as `0x00000000ffffffff` and shifting it 32 bits to left results in `0xffffffff00000000` which means the the result is still representable with no loss. What am I missing here?

So, if my understanding above is correct, there will be no issue in these two functions since left-shift o f negative values are to be ignored for now. IOW, we can close this PR without integration.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26809#discussion_r2287721038