Undefined behavior in JS_NewFloat64 causes miscompilation in optimized 64-bit RISC-V builds

[hovav]

The 64-bit, non-NaN-boxed variant of JS_NewFloat64 tries to check whether the input value d can be represented as a signed 32-bit integer by comparing the bit representation of a double before and after a round trip through an int32_t variable:

    u.d = d;
    val = (int32_t)d;
    t.d = val;

Unfortunately, casting a double value to an integer type that can't hold it is undefined behavior, if I'm reading C17 section 6.3.1.4 right:

When a finite value of real floating type is converted to an integer type other than _Bool, the fractional part is discarded (i.e., the value is truncated toward zero). If the value of the integral part cannot be represented by the integer type, the behavior is undefined.

On 64-bit RISC-V, FreeBSD 13.0's Clang 11.0.1 in -O0 mode compiles this snippet to

fcvt.l.d a0, ft0, rtz
sw a0, -52(s0)
lw a0, -52(s0)
fcvt.d.l ft0, a0

where the sign extension performed by lw acts as a cast to int32_t, as intended. By contrast, Clang -O2 avoids the memory accesses, producing

fcvt.l.d. a0, fs0, rtz
fcvt.d.l ft0, a0

This means that the bit-representation test will catch negative 0 but not values in int64_t but outside int32_t range.

A simple test case:

$ ./qjs -e "print(-2147483648)"
2147483648
$ ./qjs-debug -e "print(-2147483648)"
-2147483648

If I patch JS_NewFloat64 to add comparisons against INT32_MIN and INT32_MAX the effect goes away and the built-in test suite passes.

[bellard]

Compiling with -fwrapv might help (it is now the default in QuickJS). Otherwise, adding a CPU specific asm macro is a possible solution.

[bnoordhuis]

FWIW, we fixed this with some careful checks in quickjs-ng/quickjs@0068db8.