[Bug c/100061] New: Comparison guarding undefined behaviour disappears

[Bug c/100061] New: Comparison guarding undefined behaviour disappears

[t.josefowitz at gmail dot com]

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

            Bug ID: 100061
           Summary: Comparison guarding undefined behaviour disappears
           Product: gcc
           Version: 10.3.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: t.josefowitz at gmail dot com
  Target Milestone: ---

Created attachment 50580
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50580&action=edit
test case

with int a = 0, b = -1, c = MAX_INT, d = 0,

        if ((a > d) && (a > d + 1)) {
                puts("YES");
                ret = 1;
        } else if ((b > c) && (b > c + 1)) {
                puts("NO NOT GREAT");
                ret = 0;
        } else {
                puts("OK");
                ret = 2;
        }

and optimization enabled, "else if" gets selected. While b > c + 1 is undefined
for c == MAX_INT, b > c should guard against it. The generated amd64 machine
code simply lacks the (b > c) check in the generated code, while curiously the
(a > d) from the first if is present.

with -fwrapv the (b > c + 1) becomes "more defined" and thus the bug then does
not trigger anymore (at least not in the same way), but I think e.g. optimizing
away (b > c) is not actually valid here as it seems to be a valid guard for the
undefined behaviour.

I reproduced this on GCC 10.2.0 and 10.3.0.

The attached cmp.c can be compiled:
gcc -O2 cmp.c -Wall -Wextra

and then executed:
./a.out 0 -1 2147483647 0

to trigger the issue.

Notably, if compiled with -fsanitize=undefined, no undefined behaviour is
reported but the issue goes away.


# gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/10/lto-wrapper
OFFLOAD_TARGET_NAMES=nvptx-none:amdgcn-amdhsa:hsa
OFFLOAD_TARGET_DEFAULT=1
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Ubuntu
10.2.0-13ubuntu1' --with-bugurl=file:///usr/share/doc/gcc-10/README.Bugs
--enable-languages=c,ada,c++,go,brig,d,fortran,objc,obj-c++,m2 --prefix=/usr
--with-gcc-major-version-only --program-suffix=-10
--program-prefix=x86_64-linux-gnu- --enable-shared --enable-linker-build-id
--libexecdir=/usr/lib --without-included-gettext --enable-threads=posix
--libdir=/usr/lib --enable-nls --enable-clocale=gnu --enable-libstdcxx-debug
--enable-libstdcxx-time=yes --with-default-libstdcxx-abi=new
--enable-gnu-unique-object --disable-vtable-verify --enable-plugin
--enable-default-pie --with-system-zlib --enable-libphobos-checking=release
--with-target-system-zlib=auto --enable-objc-gc=auto --enable-multiarch
--disable-werror --with-arch-32=i686 --with-abi=m64
--with-multilib-list=m32,m64,mx32 --enable-multilib --with-tune=generic
--enable-offload-targets=nvptx-none=/build/gcc-10-JvwpWM/gcc-10-10.2.0/debian/tmp-nvptx/usr,amdgcn-amdhsa=/build/gcc-10-JvwpWM/gcc-10-10.2.0/debian/tmp-gcn/usr,hsa
--without-cuda-driver --enable-checking=release --build=x86_64-linux-gnu
--host=x86_64-linux-gnu --target=x86_64-linux-gnu
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 10.2.0 (Ubuntu 10.2.0-13ubuntu1)

[Bug tree-optimization/100061] Comparison guarding undefined behaviour disappears

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2021-04-13
          Component|c                           |tree-optimization
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW
           Keywords|                            |wrong-code

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
Confirmed.  It's ifcombine doing

optimizing two comparisons to _1 < a_6(D)
optimizing two comparisons to _2 < b_7(D)

combining b > c && b > c + 1 to b > c + 1

[Bug tree-optimization/100061] Comparison guarding undefined behaviour disappears

[rguenth at gcc dot gnu.org]

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

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
It needs quite some refactoring to distinguish the short-circuiting case from
the non-short-circuiting one.

 if (b > c + 1)
   if (b > c)

is OK to combine while

 if (b > c)
   if (b > c + 1)

is not.

[Bug tree-optimization/100061] [10/11 Regression] Comparison guarding undefined behaviour disappears

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Comparison guarding         |[10/11 Regression]
                   |undefined behaviour         |Comparison guarding
                   |disappears                  |undefined behaviour
                   |                            |disappears
           Priority|P3                          |P2
   Target Milestone|---                         |10.4
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Started with r10-3156-gae9c3507829ca139749ac3f9cf4a78707a036d3b

[Bug tree-optimization/100061] [10/11 Regression] Comparison guarding undefined behaviour disappears

[jakub at gcc dot gnu.org]

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

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Reduced testcase:
__attribute__((noipa)) void
foo (int x, int y)
{
  if (x > y && x > y + 1)
    __builtin_abort ();
}

int
main ()
{
  foo (-1, __INT_MAX__);
  return 0;
}

[Bug tree-optimization/100061] [10/11 Regression] Comparison guarding undefined behaviour disappears

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
The two match.pd rules that trigger here are:
/* Transform (@0 < @1 and @0 < @2) to use min,
   (@0 > @1 and @0 > @2) to use max */
(for logic (bit_and bit_and bit_and bit_and bit_ior bit_ior bit_ior bit_ior)
     op    (lt      le      gt      ge      lt      le      gt      ge     )
     ext   (min     min     max     max     max     max     min     min    )
 (simplify
  (logic (op:cs @0 @1) (op:cs @0 @2))
  (if (INTEGRAL_TYPE_P (TREE_TYPE (@0))
       && TREE_CODE (@0) != INTEGER_CST)
   (op @0 (ext @1 @2)))))

and

/* max (a, a + CST) -> a + CST where CST is positive.  */
/* max (a, a + CST) -> a where CST is negative.  */
(simplify
 (max:c @0 (plus@2 @0 INTEGER_CST@1))
  (if (TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (@0)))
   (if (tree_int_cst_sgn (@1) > 0)
    @2
    @0)))

Those look fine.  What doesn't look fine is that maybe_fold_and_comparisons
and maybe_fold_or_comparisons when calling maybe_fold_comparisons_from_match_pd
pretend they are BIT_AND_EXPR or BIT_IOR_EXPR when they actually are
TRUTH_ANDIF_EXPR or TRUTH_ORIF_EXPR (unless we can prove both comparisons are
in the same bb, but we don't really tell).
Perhaps the cases that were only handled before Martin's patch,
i.e. when gimple_simplified_result_is_gimple_val, was ok because then we don't
really use a stmt that could trigger UB.
Or perhaps ifcombine should check what maybe_fold_and_comparisons has returned
and if it is a comparison where one of the operands is a SSA_NAMEs defining
stmt  doesn't dominate both original comparisons and that stmt or anything it
uses could trigger UB, punt?  Though I'd be afraid we could regress many cases
that old gimple-fold.c stuff got right (if it did).

[Bug tree-optimization/100061] [10/11 Regression] Comparison guarding undefined behaviour disappears

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

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

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
As said it needs thought and refactoring - the original intent to move all the
simplifications to match.pd works in the wrong direction unless we
"re-introduce" short-circuiting and/or into the match.pd patterns.

Tobias - did you create this case artificially or did you actually run into
a miscompilation in a real-world application?

[Bug tree-optimization/100061] [10/11 Regression] Comparison guarding undefined behaviour disappears

[rguenth at gcc dot gnu.org]

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

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #3)
> Started with r10-3156-gae9c3507829ca139749ac3f9cf4a78707a036d3b

Note the refactoring shouldn't have changed things and definitely
and_comparisons_1 isn't considering the 'and' to be short-circuiting
so it likely was a missed optimization there.  ifcombine uses
maybe_fold_and_comparisons which calls and_comparisons_1 with both
variants a & b and b & a.

[Bug tree-optimization/100061] [10/11 Regression] Comparison guarding undefined behaviour disappears

[jakub at gcc dot gnu.org]

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

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Sure, but perhaps what and_comparison_1 etc. did before didn't make a
difference between short-circuiting and non-short-circuiting cases, but now
that it uses match.pd it triggers there anything and we have at least one clear
case where it matters.

[Bug tree-optimization/100061] [10/11 Regression] Comparison guarding undefined behaviour disappears

[t.josefowitz at gmail dot com]

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

--- Comment #9 from Tobias S. Josefowitz <t.josefowitz at gmail dot com> ---
Richard - I ran into this in a real world application.

[Bug tree-optimization/100061] [10/11/12 Regression] Comparison guarding undefined behaviour disappears

[t.josefowitz at gmail dot com]

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

--- Comment #10 from Tobias S. Josefowitz <t.josefowitz at gmail dot com> ---
Is the 10.4 milestone really applicable here, or might that in practice be
something that would cause this bug to go somewhat unnoticed?

[Bug tree-optimization/100061] [10/11/12 Regression] Comparison guarding undefined behaviour disappears

[rguenth at gcc dot gnu.org]

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

--- Comment #11 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Tobias S. Josefowitz from comment #10)
> Is the 10.4 milestone really applicable here, or might that in practice be
> something that would cause this bug to go somewhat unnoticed?

The 10.4 milestone makes this bug appear in the list of important regressions
for the GCC 10, GCC 11 and GCC 12 (master) branches so it is exactly correct.

[Bug tree-optimization/100061] [10/11/12/13 Regression] Comparison guarding undefined behaviour disappears

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.4                        |10.5

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 10.4 is being released, retargeting bugs to GCC 10.5.

[Bug tree-optimization/100061] [10/11/12/13 Regression] Comparison guarding undefined behaviour disappears

[jakub at gcc dot gnu.org]

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

--- Comment #13 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Martin, could you please have a look into this?

[Bug tree-optimization/100061] [10/11 Regression] Comparison guarding undefined behaviour disappears

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[10/11/12/13 Regression]    |[10/11 Regression]
                   |Comparison guarding         |Comparison guarding
                   |undefined behaviour         |undefined behaviour
                   |disappears                  |disappears
      Known to fail|                            |11.3.1
      Known to work|                            |12.1.0, 13.0

--- Comment #14 from Richard Biener <rguenth at gcc dot gnu.org> ---
Btw, eventually the refactoring done for PR105142 in r12-8012-gfc8d9e4497032d
will help, at least the fixed problem is of similar nature.

The original testcase seems fixed by this at least as we are no longer
expanding the guarded c + 1.

[Bug tree-optimization/100061] [11 Regression] Comparison guarding undefined behaviour disappears

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.5                        |11.5

--- Comment #15 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 10 branch is being closed.