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

[Kim Barrett]

On Sun, 17 Aug 2025 08:42:53 GMT, Andrew Haley <aph at openjdk.org> wrote:

>> There was a left-shift of negative value UB in `set_high` function where the high value sign bit is on and is left-shifted 32 bits to put it in high word of the destination address.
>> To address it, first the left 32 bits of the provided `high` arg is cleared and then left-shifted 32 bits. 
>> 
>> Tests:
>> mach5 tiers 1-5 {macosx-aarch64, linux-x64, windows-x64} x {debug, product}
>
> 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.)

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

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