[Bug tree-optimization/108625] New: forwprop introduces new UB

[Bug tree-optimization/108625] New: forwprop introduces new UB

[kristerw at gcc dot gnu.org]

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

            Bug ID: 108625
           Summary: forwprop introduces new UB
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: kristerw at gcc dot gnu.org
  Target Milestone: ---

Consider the function

  unsigned char foo(int x)
  {
    int t = -x;
    unsigned char t1 = t;
    unsigned char t2 = t;
    return t1 + t2;
  }

This does not invoke undefined behavior when called as foo(0x40000001),
but forwprop1 optimizes this to

  unsigned char foo (int x)
  {
    int t;
    unsigned char _5;
    int _7;

    <bb 2> :
    t_2 = -x_1(D);
    _7 = t_2 - x_1(D);
    _5 = (unsigned char) _7;
    return _5;
  }

where _7 has signed overflow for x = 0x40000001.

[Bug tree-optimization/108625] [11/12/13 Regression] forwprop introduces new UB

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2023-02-01
   Target Milestone|---                         |11.4
            Summary|forwprop introduces new UB  |[11/12/13 Regression]
                   |                            |forwprop introduces new UB
             Status|UNCONFIRMED                 |NEW

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
GCC 12.1.0:
Applying pattern match.pd:2763, gimple-match.cc:40998
Applying pattern match.pd:2532, gimple-match.cc:63517
Applying pattern match.pd:3647, gimple-match.cc:69622
gimple_simplified to _7 = t_2 - x_1(D);
_5 = (unsigned char) _7;


Introduced by r11-3207-g8f0d743c2dee6a .

Confirmed.

[Bug tree-optimization/108625] [11/12/13 Regression] forwprop introduces new UB

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |rguenth at gcc dot gnu.org

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
we come from

  t_2 = -x_1(D);
  t1_3 = (unsigned char) t_2;
  t2_4 = (unsigned char) t_2;
  _5 = t1_3 + t2_4;

where

  t_2 + t_2

"simplifies", but somehow the ! constraint isn't honored.  We do

                res_op->set_op (NOP_EXPR, type, 1);
                {
                  tree _o1[2], _r1;
                  _o1[0] = captures[0];
                  _o1[1] = captures[1];
                  gimple_match_op tem_op (res_op->cond.any_else (), op,
TREE_TYPE (_o1[0]), _o1[0], _o1[1]); 
                  tem_op.resimplify (lseq, valueize);
                  _r1 = maybe_push_res_to_seq (&tem_op, NULL);
                  if (!_r1) goto next_after_fail1082;
                  res_op->ops[0] = _r1;

so the intent is that the maybe_push_res_to_seq (..., NULL) will catch
not simplified operands of the (convert).

The issue is that we go through

 /* A + (-B) -> A - B */
 (simplify
  (plus:c @0 (convert? (negate @1))) 
  /* Apply STRIP_NOPS on the negate.  */
  (if (tree_nop_conversion_p (type, TREE_TYPE (@1))
       && !TYPE_OVERFLOW_SANITIZED (type))
   (with
    {
     tree t1 = type;
     if (INTEGRAL_TYPE_P (type)
         && TYPE_OVERFLOW_WRAPS (type) != TYPE_OVERFLOW_WRAPS (TREE_TYPE (@1)))
       t1 = TYPE_OVERFLOW_WRAPS (type) ? type : TREE_TYPE (@1);
    }  
    (convert (minus (convert:t1 @0) (convert:t1 @1))))))

and

/* Basic strip-useless-type-conversions / strip_nops.  */
(for cvt (convert view_convert float fix_trunc)
 (simplify
  (cvt @0)
  (if ((GIMPLE && useless_type_conversion_p (type, TREE_TYPE (@0)))
       || (GENERIC && type == TREE_TYPE (@0)))
   @0)))

which defeats the check because the

                  tem_op.resimplify (lseq, valueize);

ends up with

  _7 = t_2 - x_1(D);

in the 'lseq' and plain _7 in tem_op.  We could fix this by using a
NULL lseq in the resimplification as well at the expense of not
catching some more complicated simplifications to "leafs" or
alternatively do a more complicated check for the force_leaf case,
checking the actual leaf and if SSA name, verify the definition is
not in 'lseq'.

I'm going to test the simpler NULL lseq to resimplify variant.

[Bug tree-optimization/108625] [11/12/13 Regression] forwprop introduces new UB

[cvs-commit at gcc dot gnu.org]

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

--- Comment #3 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Richard Biener <rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:605d1297b91c2c7c23ccfe669e66dda5791d1f55

commit r13-5651-g605d1297b91c2c7c23ccfe669e66dda5791d1f55
Author: Richard Biener <rguenther@suse.de>
Date:   Thu Feb 2 11:09:26 2023 +0100

    middle-end/108625 - wrong folding due to misinterpreted !

    The following fixes a problem with ! handling in genmatch which isn't
    conservative enough when intermediate simplifications push to the
    sequence but the final operation appears to just pick an existing
    (but in this case newly defined in the sequence) operand.  The easiest
    fix is to disallow adding to the sequence when processing !.

            PR middle-end/108625
            * genmatch.cc (expr::gen_transform): Also disallow resimplification
            from pushing to lseq with force_leaf.
            (dt_simplify::gen_1): Likewise.

            * gcc.dg/pr108625.c: New testcase.

[Bug tree-optimization/108625] [11/12 Regression] forwprop introduces new UB

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2
            Summary|[11/12/13 Regression]       |[11/12 Regression] forwprop
                   |forwprop introduces new UB  |introduces new UB
      Known to work|                            |13.0

[Bug tree-optimization/108625] [11/12 Regression] forwprop introduces new UB

[cvs-commit at gcc dot gnu.org]

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

--- Comment #4 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-12 branch has been updated by Richard Biener
<rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:e19e9c36abc7fff9c628cf744e55e4c0e686ea53

commit r12-9257-ge19e9c36abc7fff9c628cf744e55e4c0e686ea53
Author: Richard Biener <rguenther@suse.de>
Date:   Thu Feb 2 11:09:26 2023 +0100

    middle-end/108625 - wrong folding due to misinterpreted !

    The following fixes a problem with ! handling in genmatch which isn't
    conservative enough when intermediate simplifications push to the
    sequence but the final operation appears to just pick an existing
    (but in this case newly defined in the sequence) operand.  The easiest
    fix is to disallow adding to the sequence when processing !.

            PR middle-end/108625
            * genmatch.cc (expr::gen_transform): Also disallow resimplification
            from pushing to lseq with force_leaf.
            (dt_simplify::gen_1): Likewise.

            * gcc.dg/pr108625.c: New testcase.

    (cherry picked from commit 605d1297b91c2c7c23ccfe669e66dda5791d1f55)

[Bug tree-optimization/108625] [11 Regression] forwprop introduces new UB

[cvs-commit at gcc dot gnu.org]

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

--- Comment #5 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Richard Biener
<rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:a0a1905b26751acea523a3bf1bca68d4ddc55c09

commit r11-10675-ga0a1905b26751acea523a3bf1bca68d4ddc55c09
Author: Richard Biener <rguenther@suse.de>
Date:   Thu Feb 2 11:09:26 2023 +0100

    middle-end/108625 - wrong folding due to misinterpreted !

    The following fixes a problem with ! handling in genmatch which isn't
    conservative enough when intermediate simplifications push to the
    sequence but the final operation appears to just pick an existing
    (but in this case newly defined in the sequence) operand.  The easiest
    fix is to disallow adding to the sequence when processing !.

            PR middle-end/108625
            * genmatch.c (expr::gen_transform): Also disallow resimplification
            from pushing to lseq with force_leaf.
            (dt_simplify::gen_1): Likewise.

            * gcc.dg/pr108625.c: New testcase.

    (cherry picked from commit 605d1297b91c2c7c23ccfe669e66dda5791d1f55)

[Bug tree-optimization/108625] [11 Regression] forwprop introduces new UB

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
      Known to fail|                            |11.3.0
         Resolution|---                         |FIXED
      Known to work|                            |11.3.1

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed.