#4113 Incorrect equality comparisons involving long long constants

[Christopher Bazley]

1: Sample code that reproduces the problem.

_Static_assert(0x10000LL != 0, "high bits");
_Static_assert(0x20000000000000ULL != 0x20000000000001ULL, "distinct values");
_Static_assert(65436U != -100LL, "unsigned value differs from negative value");

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

sdcc -mz80 --std-c11 -S longlong-equality.c

3: SDCC version tested (type "sdcc -v" to find it).

SDCC 4.6.4 #0 (Linux), built from SVN r16960 sources.

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

longlong-equality.c:1: warning 215: static assertion failed: "high bits"
longlong-equality.c:2: warning 215: static assertion failed: "distinct values"
longlong-equality.c:3: warning 215: static assertion failed: "unsigned value differs from negative value"

All three assertions are true. The first comparison must account for bit 16. The second compares two distinct integers above the range in which double can represent every integer exactly. In the third, 65436U converts to long long and remains positive, so it differs from -100LL.

valCompare can truncate equality comparisons involving long long constants to unsigned int. Separately, isAstEqual compares literal values after conversion to double, which can round distinct large integers to the same value.

The same failures occur on mcs51 (default, medium, large and stack-auto), ds390, z180, r2k, r4k, sm83, tlcs90, hc08, s08, stm8 and pdk13/14/15. They also occur with Z80 in C23 mode.

GCC 16.2.0 and Clang 23.1.0 accept the example without diagnostics: https://godbolt.org/z/nna4fKed9

[Christopher Bazley]

While working on [bugs:#3952], I noticed a workaround in stdckdint.h which avoids using _Generic in the way intended by its designer (to select a function before calling it), for performance reasons relating to feature request 880. In turn, that workaround required me to add casts that I would rather not have added because they undermine type safety.

My candidate implementation of feature request 880 exposes this bug in checked integer arithmetic, so I believe this bug needs to be fixed first. In any case, it is clearly behaviour that is incompatible with Clang and GCC.

[Christopher Bazley]

Please find a candidate patch to fix this bug attached.

[Philipp Klaus Krause]

• This adds to the code duplication in valCompare. How about the following (which should have the same semantics as your patch) instead?

    case EQ_OP:
    case NE_OP:
      bool equal;
      if (SPEC_NOUN (lval->type) == V_FLOAT || SPEC_NOUN (rval->type) == V_FLOAT ||
        SPEC_NOUN (lval->type) == V_FIXED16X16 || SPEC_NOUN (rval->type) == V_FIXED16X16)
        {
          equal = floatFromVal (lval) == floatFromVal (rval);
        }
      else
        {
          /* integrals: ignore signedness */
          TYPE_TARGET_ULONGLONG l, r;

          l = (TYPE_TARGET_ULONGLONG) ullFromVal (lval);
          r = (TYPE_TARGET_ULONGLONG) ullFromVal (rval);
          /* In order to correctly compare 'signed int' and 'unsigned int' it's
             necessary to strip them to 16 bit.
             Literals are reduced to their cheapest type, therefore left and
             right might have different types. It's necessary to find a
             common type: int (used for char too) or long */
          if (!IS_LONGLONG (lval->etype) && !IS_BITINT (lval->etype) && !IS_LONGLONG (rval->etype) && !IS_BITINT (rval->etype))
            {
              r = (TYPE_TARGET_ULONG) r;
              l = (TYPE_TARGET_ULONG) l;
            }
          if (!IS_LONG (lval->etype) && !IS_LONG (rval->etype) &&
              !IS_LONGLONG (lval->etype) && !IS_LONGLONG (rval->etype) &&
              !IS_BITINT (lval->etype) && !IS_BITINT (rval->etype))
            {
              r = (TYPE_TARGET_UINT) r;
              l = (TYPE_TARGET_UINT) l;
            }
          equal = l == r;
        }
      SPEC_CVAL (val->type).v_int = (ctype == EQ_OP) ? equal : !equal;
      break;

• As the comment in valCompare about missing long long support states, there probably is still a problem for relational operators (I guess when precision in the lower bits is lost as a value is converted to floating-point format).

• The fix (both yours and my deduplicated variant above) make a test fail:

results/ucz80/longlong/longlong_test_bit.out:16:--- FAIL: "Assertion failed" on (y & x) == (0x69aaaaaaaaaa55aaull & 0x69555555555555aall) at cases/longlong/longlong_test_bit.c:268

[Christopher Bazley]

Hi Philipp, please find a revised candidate patch attached.

It shares valCompare between AST and iCode constant folding for all six comparison operators, removing the duplicated equality handling and fixing relational comparisons. This significantly reduces the amount of code in SDCCicode.c.

Integer operands undergo integer promotion and common-type conversion before comparison, preserving wide values and signed ordering. The unsigned > 0 simplification now checks the full constant value.

The failing longlong_test_bit assertion exposed incorrect iCode bitwise constant folding. Using valBitwise for &, | and ^ avoids rounding operands through double or truncating them to 32 bits.

The added tests cover wide comparisons, mixed signedness, checked subtraction into a narrower result type, and agreement between constant-folded and runtime bitwise operations.

This fix is a prerequisite for the patch that implements [feature-requests:#880].

[Philipp Klaus Krause]

Most of this looks good to me. I do have a question on the changes in isAstEqual though: the changes clearly fix false positives for long long comparisons, but I think they introduce new false negatives for mixed sign. That might not cause real bugs, since looking at the uses of isAstEqual, I think false negatives just result in some missed optimizations. Still it feels like the false negatives should either be fixed, or clearly stated in a comment.

[Christopher Bazley]

Hi Philipp, thanks for the review.

Unfortunately, I think that some attractive-looking optimisations that SDCC currently performs are unsafe. For example, consider the following function:

int f(long x)
{
    return x / 2L == x / 2UL;
}

It might seem as though the two subexpressions are equal, but in fact the righthand expression is equivalent to (unsigned long)x / 2UL, which produces a huge quotient if x is negative. For example, if x == -4L then (unsigned long)x is ULONG_MAX - 4L + 1 and the righthand side is (ULONG_MAX - 4L + 1) / 2UL. In contrast, the first subexpression would be -4L / 2L, which equals -2L. When converted to the common type, -2L is ULONG_MAX - 2L + 1, which is much greater than (ULONG_MAX - 4L + 1) / 2UL.

SDCC trunk outputs (for 6502):

_f_PARM_1:
        .ds 4
_f:
        lda     #0x01
        ldx     #0x00
        rts

Whereas GCC 16.1.0 outputs (for ARM):

f:
        add     r3, r0, r0, lsr #31
        lsrs    r0, r0, #1
        cmp     r0, r3, asr #1
        ite     eq
        moveq   r0, #1
        movne   r0, #0
        bx      lr

See https://godbolt.org/z/d1zKMheKE

[Philipp Klaus Krause]

Somehow the AST feels like the wrong place to do optimizations anyway. But these are old optimizations that have been there for a long time, and removing them might result in code size regressions, unless we have other optimizations (on the iCode) to replace them.
I guess we can go with your patch for now, and just consider singed vs. unsigned different AST (though that also feels odd when signed char is still the same as signed long long, and unsigned char the same as unsigned long long).

[Christopher Bazley]

The attached patch only adds comments and tests for the optimisations that were previously done despite not being strictly correct.

[Philipp Klaus Krause]

Fixed in [r16974] via your patch.

[Maarten Brock]

If you consider this bug closed, please reopen a new one for the unsafe optimisation. It silently generates wrong code.

[Philipp Klaus Krause]

It no longer does with the patch in (despite the patch originally intended to fix the bug reported here)?