#4102 Host overflow in unsigned multiplication

[Christopher Bazley]

1: Sample code that reproduces the problem.

_Static_assert((unsigned int)65535 * (unsigned int)65535 ==
               (unsigned int)4294836225UL, "unsigned product");

2: Exact command used to run SDCC on this sample code

./bin/sdcc -S bug.c

3: SDCC version tested

SDCC : mcs51/z80/z180/r2k/r2ka/r3ka/r4k/r5k/r6k/sm83/tlcs90/ez80/z80n/r800/ds390/TININative/ds400/hc08/s08/stm8/pdk13/pdk14/pdk15/mos6502/mos65c02/huc6280/f8/f8l TD- 4.6.2 #0 (Linux)

Built from upstream revision 16945 (Git commit 5c14f028791cc1756418966e62cfd0a53bafdb33).

4: Copy of the error message or incorrect output, or a clear description of the observed versus expected behavior.

This is valid unsigned arithmetic. In SDCC, where unsigned int has 16 bits, the result is 1. SDCC 4.6.0 also emits warning 165: integer overflow in expression.

This report concerns undefined behaviour inside the compiler, rather than an observed wrong target result. Instrumenting SDCCval.c with GCC's undefined-behaviour sanitizer and compiling this expression produces:

SDCCval.c:2637:66: runtime error: signed integer overflow: 65535 * 65535 cannot be represented in type 'int'

In the unsigned-int branch of valMult, both operands are cast to TYPE_TARGET_UINT. On this host that is uint16_t, so both operands undergo integer promotion to signed int before multiplication. The product exceeds INT_MAX. Assigning the result to TYPE_TARGET_ULONG afterwards does not prevent this overflow.

The candidate fix converts both operands to TYPE_TARGET_ULONG before multiplication. With the same instrumentation, compilation completes without a sanitizer error. The existing overflow-warning check is retained. The fixed compiler emits warning 165, "integer overflow in expression", for this sample; the optimised baseline does not (maybe because of undefined behaviour?). The patch includes a regression test for the target result; the sanitiser check detects the internal overflow that ordinary execution can conceal.

GCC 16.1.0 and Clang 23.1.0 accept the sample without emitting a diagnostic. Its unsigned int is 32 bits, so the expected value in the assertion is correspondingly different. (SDCC treats (unsigned int)4294836225UL as 1, so the assertion passes; it passes with GCC too, despite evaluating to a different constant.)
https://godbolt.org/z/1dxs18aTh

The problem was reproduced with mcs51 (default, large and stack-auto), ds390, z80, z180, r2k, r4k, sm83, tlcs90, hc08, s08, mos6502, huc6280, stm8, f8, pdk13, pdk14 and pdk15.

[Philipp Klaus Krause]

I see the problem. However, wouldn't the code in that patch still have the same problem if host int is wider than TYPE_TARGET_ULONG? Though that would be less likely. Could we just multiply as host unsigned int? Though that also would make assumptions about the width of host vs. target types.
Either way, IMO we should check the assumption made (via _Static_assert or such) here.

[Christopher Bazley]

Hi Philipp, thanks for the review.
The attached revision of the candidate patch uses type unsigned long instead of TYPE_TARGET_ULONG for the factors and product, and adds a _Static_assert to check that the product cannot overflow the host's unsigned long type. The new test file is also renamed (after this bug number).
HTH.

[Philipp Klaus Krause]

Thanks. Fixed in [r16976] via your patch.