[Bug c/98269] New: gcc 6.5.0 __builtin_add_overflow() with small uint32_t values incorrectly detects overflow

[Bug c/98269] New: gcc 6.5.0 __builtin_add_overflow() with small uint32_t values incorrectly detects overflow

[stli at linux dot ibm.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98269

            Bug ID: 98269
           Summary: gcc 6.5.0 __builtin_add_overflow() with small uint32_t
                    values incorrectly detects overflow
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: stli at linux dot ibm.com
  Target Milestone: ---

Created attachment 49756
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=49756&action=edit
Build this tst-gcc-addoverflow.c with gcc 6.5.0 to see the ERROR

If build on s390x (I had no chance to test it on other architectures) with gcc
6.5.0 the attached testcase with small uint32_t input values for
__builtin_add_overflow() detects an overflow and fails:
      else if (__builtin_add_overflow (previous->offset,
                                       previous->length + 1,
                                       &current->offset))
        {
          printf ("ERROR: __builtin_add_overflow() OVERFLOWED: "
                  "previous->offset=%" PRIu32 " + "
                  "(previous->length=%" PRIu32 " + 1)"
                  " => current->offset=%" PRIu32 "\n",
                  previous->offset, previous->length, current->offset);
          return EXIT_FAILURE;
        }

=>
ERROR: __builtin_add_overflow() OVERFLOWED: previous->offset=7 +
(previous->length=3 + 1) => current->offset=11

I have not recognized this issue with gcc 7.1 and later.

The original issue was found if glibc is build with gcc 6.5.0:
__builtin_add_overflow is used in
<glibc>/elf/stringtable.c:stringtable_finalize()
(https://sourceware.org/git/?p=glibc.git;a=blob;f=elf/stringtable.c;h=099347d73ee70b8ffa4b4a91c493e0bba147ffa2;hb=HEAD#l185)
which leads to ldconfig failing with "String table is too large". This is
also recognizable in following glibc-tests:
FAIL: elf/tst-glibc-hwcaps-cache
FAIL: elf/tst-glibc-hwcaps-prepend-cache
FAIL: elf/tst-ldconfig-X
FAIL: elf/tst-ldconfig-bad-aux-cache
FAIL: elf/tst-ldconfig-ld_so_conf-update
FAIL: elf/tst-stringtable

Please also have a look at attached tst-gcc-addoverflow.c for some more details
from my gdb session showing the add and jump instruction.

[Bug c/98269] gcc 6.5.0 __builtin_add_overflow() with small uint32_t values incorrectly detects overflow

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98269

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
gcc 6.5 is unsupported since October 2018 and 7.x since November 2019, so if
currently supported branches work, there is nothing to do for us.
If you want, you can bisect which gcc change fixed it.

[Bug c/98269] gcc 6.5.0 __builtin_add_overflow() with small uint32_t values incorrectly detects overflow

[stli at linux dot ibm.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98269

stli at linux dot ibm.com <stli at linux dot ibm.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Target|                            |s390x
      Known to work|                            |10.1.0, 5.4.0, 5.5.0,
                   |                            |7.1.0, 8.1.0, 9.1.0
                 CC|                            |stli at linux dot ibm.com
      Known to fail|                            |6.3.0, 6.4.0, 6.5.0

--- Comment #2 from stli at linux dot ibm.com <stli at linux dot ibm.com> ---
That's okay for me. But I wanted to document it. Currently glibc is requiring
gcc 6.2 as minimum. For s390x, I will post a patch which requires gcc 7.1 as
minimum.

[Bug c/98269] gcc 6.5.0 __builtin_add_overflow() with small uint32_t values incorrectly detects overflow

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98269

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
For documentation, the bisection would be best, if you can preprocess the
testcase and git bisect it between some 6.0-ish and 8.0-ish trunk revisions, we
could know if the bug has been fixed or is just mere latent.

[Bug c/98269] gcc 6.5.0 __builtin_add_overflow() with small uint32_t values incorrectly detects overflow

[krebbel at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98269

Andreas Krebbel <krebbel at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |DUPLICATE
                 CC|                            |krebbel at gcc dot gnu.org

--- Comment #4 from Andreas Krebbel <krebbel at gcc dot gnu.org> ---
The problem is a CC mode mismatch generated by combine. After splitting the add
insn 135 generates a CCL1mode cc while the conditional jump consumes it as
CCUmode. This leads to the wrong condition code mask being generated in the
end.

(insn 135 56 136 7 (parallel [
            (set (reg:CCL1 33 %cc)
                (compare:CCL1 (plus:SI (reg:SI 108)
                        (mem:SI (plus:DI (reg:DI 88 [ ivtmp.10 ])
                                (const_int 12 [0xc])) [3 MEM[base: previous_25,
offset: 12B]+0 S4 A32]))
                    (reg:SI 108)))
            (set (reg:SI 109)
                (plus:SI (reg:SI 108)
                    (mem:SI (plus:DI (reg:DI 88 [ ivtmp.10 ])
                            (const_int 12 [0xc])) [3 MEM[base: previous_25,
offset: 12B]+0 S4 A32])))
        ]) t.c:31 1358 {*addsi3_carry1_cc}
     (expr_list:REG_DEAD (reg:SI 108)
        (nil)))
(note 136 135 64 7 NOTE_INSN_DELETED)
(insn 64 136 65 7 (set (mem:SI (plus:DI (reg:DI 88 [ ivtmp.10 ])
                (const_int 28 [0x1c])) [3 MEM[base: previous_25, offset: 28B]+0
S4 A32])
        (reg:SI 109)) t.c:31 1077 {*movsi_zarch}
     (nil))
(note 65 64 66 7 NOTE_INSN_DELETED)
(jump_insn 66 65 67 7 (set (pc)
        (if_then_else (geu (reg:CCU 33 %cc)
                (const_int 0 [0]))
            (label_ref 78)
            (pc))) t.c:31 1661 {*cjump_64}
     (int_list:REG_BR_PROB 9500 (expr_list:REG_DEAD (reg:CCZ 33 %cc)
            (nil)))


The failure disappears with:

commit bf7499197fbb065123257c374064f6bb715c951b
Author: Dominik Vogt <vogt@linux.vnet.ibm.com>
Date:   Mon Jul 4 14:25:22 2016 +0000

    S/390: Add support for z13 instructions lochi and locghi.

    The attached patch adds patterns to make use of the z13 LOCHI and
    LOCGHI instructions.
    ...


But that one only hides the problem. The mere presence of the lochi
alternatives lead to different RTL being emitted (although the alternative is
not enabled for -march=z196). The split then doesn't happen anymore.

Reverting the patch and continue bisecting. The failure finally disappears
with:

3f54004b095d1cd513e63753ee0f8f9f13698347 is the first bad commit
commit 3f54004b095d1cd513e63753ee0f8f9f13698347
Author: Bin Cheng <bin.cheng@arm.com>
Date:   Fri Jan 27 14:42:23 2017 +0000

    re PR rtl-optimization/78559 (wrong code due to tree if-conversion?)

            PR rtl-optimization/78559
            * combine.c (try_combine): Discard REG_EQUAL and REG_EQUIV for
            other_insn in combine.


This looks like the actual fix to me. The wrong CC mode survives as part of a
REG_EQUAL note:

Successfully matched this instruction:
(set (reg:SI 93 [ _27+4 ])
    (if_then_else:SI (geu (reg:CCL1 33 %cc)
            (const_int 0 [0]))
        (reg:SI 93 [ _27+4 ])
        (reg:SI 118)))
allowing combination of insns 56 and 135
original costs 4 + 4 = 16
replacement cost 8
deferring deletion of insn with uid = 56.
modifying other_insn   136: r93:SI={(geu(%cc:CCL1,0))?r93:SI:r118:SI}
      REG_DEAD %cc:CCU
      REG_EQUAL ltu(%cc:CCU,0)
deferring rescan insn with uid = 136.
modifying insn i3   135:
{%cc:CCL1=cmp(r108:SI+[r88:DI+0xc],r108:SI);r109:SI=r108:SI+[r88:DI+0xc];}
      REG_DEAD r108:SI
deferring rescan insn with uid = 135.

So we probably should mark it as duplicate of PR78559.

*** This bug has been marked as a duplicate of bug 78559 ***

[Bug c/98269] gcc 6.5.0 __builtin_add_overflow() with small uint32_t values incorrectly detects overflow

[stli at linux dot ibm.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98269

--- Comment #5 from stli at linux dot ibm.com <stli at linux dot ibm.com> ---
Just as information,
I've just committed this glibc patch:
"s390x: Require GCC 7.1 or later to build glibc."
https://sourceware.org/git/?p=glibc.git;a=commit;h=844b4d8b4b937fe6943d2c0c80ce7d871cdb1eb5