RFR: 8346914: UB issue in scalbnA

[David Holmes]

On Thu, 5 Jun 2025 10:44:22 GMT, Fredrik Bredberg <fbredberg at openjdk.org> wrote:

>> src/hotspot/share/runtime/sharedRuntimeMath.hpp line 122:
>> 
>>> 120:     set_high(&x, (hx&0x800fffff)|((k+n)<<20));
>>> 121:     return x;
>>> 122:   }
>> 
>> Curious if this could be? It looks like this is what the ifs are doing. And it is at least easier for me to see that this is the same as the code before.
>> Suggestion:
>> 
>>   if ((int)u_k > 0) {
>>     if (u_k > 0x7fe) {
>>       return hugeX*copysignA(hugeX,x); /* overflow  */
>>     }
>>     set_high(&x, (hx&0x800fffff)|((k+n)<<20));
>>     return x;
>>   }
>
> I like this change. It's easier to follow, at least for me.

I think that is equivalent, but I did not want to disrupt the general control flow of the algorithm so that it still closely resembles the original fdlibm code.

I also initially misread your suggestion and now I am wondering if I can actually simplify it to a simple two line change:

unsigned u_k = k + n; // avoid UB signed integer overflow
k = (int) u_k;  // safely assign to k

does that bypass any UB?

>> src/hotspot/share/runtime/sharedRuntimeMath.hpp line 124:
>> 
>>> 122:   }
>>> 123: 
>>> 124:   if (u_k <= (unsigned)-54) {
>> 
>> Could this just be? Or is this less clear / easier to miss?
>> Suggestion:
>> 
>>   if (u_k <= -54u) {
>
> For me it's less clear and far easier to miss.
> I'll vote for `if (u_k <= (unsigned)-54) {`.
> Reading `if (u_k <= -54u) {` just cooks my brain.

I'm suprised that is even valid TBH. It strikes me as a numerical oxymoron.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25656#discussion_r2128800502
PR Review Comment: https://git.openjdk.org/jdk/pull/25656#discussion_r2128802270