Wrapping multiplication on u16 may produce undefined behavior with C backend

[ianprime0509]

Zig Version

0.14.0-dev.2173+bd8ef0036

Steps to Reproduce and Observed Behavior

With the following code as z.zig:

const std = @import("std");

test {
    var x: u16 = 59429;
    x *%= 58959;
    try std.testing.expect(x == 57707);
}

Run zig test -ofmt=c -lc z.zig. It crashes with the following output:

Illegal instruction at address 0x1272172
/var/home/ian/src/zig/lib/zig.h:601:1: 0x1272172 in zig_mulw_u16 (/var/home/ian/.cache/zig/o/d6bd83d1cd89ffd19da08cea0d4e5886/test.c)
zig_int_helpers(16)
^
/var/home/ian/.cache/zig/o/d6bd83d1cd89ffd19da08cea0d4e5886/test.c:9641:7: 0x12720d8 in z_test_0__100 (/var/home/ian/.cache/zig/o/d6bd83d1cd89ffd19da08cea0d4e5886/test.c)
 t1 = zig_mulw_u16(t1, UINT16_C(58959), UINT8_C(16));
      ^
/var/home/ian/.cache/zig/o/d6bd83d1cd89ffd19da08cea0d4e5886/test.c:29463:9: 0x12224d7 in test_runner_mainTerminal__290 (/var/home/ian/.cache/zig/o/d6bd83d1cd89ffd19da08cea0d4e5886/test.c)
  t34 = t33();
        ^
/var/home/ian/.cache/zig/o/d6bd83d1cd89ffd19da08cea0d4e5886/test.c:12440:2: 0x104614a in test_runner_main__288 (/var/home/ian/.cache/zig/o/d6bd83d1cd89ffd19da08cea0d4e5886/test.c)
 test_runner_mainTerminal__290();
 ^
/var/home/ian/.cache/zig/o/d6bd83d1cd89ffd19da08cea0d4e5886/test.c:9843:2: 0x1043e01 in main (/var/home/ian/.cache/zig/o/d6bd83d1cd89ffd19da08cea0d4e5886/test.c)
 test_runner_main__288();
 ^
???:?:?: 0x7fe6d4da8247 in ??? (libc.so.6)
Unwind information for `libc.so.6:0x7fe6d4da8247` was not available, trace may be incomplete

???:?:?: 0x7fe6d4da830a in ??? (libc.so.6)
???:?:?: 0x1043494 in ??? (???)
error: the following test command crashed:
/var/home/ian/src/zig/build/stage3/bin/zig run -I /var/home/ian/src/zig/lib -lc /var/home/ian/.cache/zig/o/d6bd83d1cd89ffd19da08cea0d4e5886/test.c

By running zig test -ofmt=c -lc --test-no-exec -femit-bin=z.c z.zig and compiling with clang -fsanitize=undefined (see also #5163), the resulting executable produces a more helpful error explaining what's going wrong:

/var/home/ian/src/zig/lib/zig.h:601:1: runtime error: signed integer overflow: 59429 * 58959 cannot be represented in type 'int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /var/home/ian/src/zig/lib/zig.h:601:1 
All 1 tests passed.

Expected Behavior

The test should pass.

Note that the same sort of bug can also be reproduced with i16 (zig_mulw_i16):

const std = @import("std");

test {
    var x: i16 = -6107;
    x *%= -6577;
    try std.testing.expect(x == -7829);
}

What I believe to be happening here is in the expression lhs * rhs in the implementation of zig_mulw_u16, the type uint16_t of the arguments is implicitly promoted to int, which on a typical system with 32-bit signed integers, will produce an overflow when evaluating 59429 * 58959.

[Rexicon226]

I think you're right about the implicit promotion that's happening, since uint16_t is smaller than int.
That being said, just casting both the lhs and the rhs to uint16_t like (uint16_t)lhs * (uint16_t)rhs won't work last time I checked.

I think the correct solution is to do something like (uint16_t)((uint32_t)lhs * (uint32_t)rhs), but I am very far from a C semantics expert, so take that with a large amount of salt.

[accelbread]

Yeah, to prevent integer promotion, you have to perform the multiplication in a type that does not fit in the range of int. (uint16_t)((uint32_t)lhs * (uint32_t)rhs) works if int is 32 bits, but will promote if int is 64 bits. So it should probably be (uint16_t)((unsigned)lhs * rhs). With just two numbers being multiplied, uint32_t is still fine either way though as in the 64-bit int case, the values will be promoted, but the multiplication of two uint16_t values cannot overflow a 64-bit signed integer (I have gotten sign conversion warnings on some compilers though, when casting back after promotion).