diff mbox series

Fix PR90394, [10 Regression] ICE in is_value_included_in, at tree-ssa-uninit.c:1055

Message ID 87pnojfh7v.fsf@ispras.ru
State New
Headers show
Series Fix PR90394, [10 Regression] ICE in is_value_included_in, at tree-ssa-uninit.c:1055 | expand

Commit Message

Vladislav Ivanishin May 15, 2019, 11:09 p.m. UTC 
Hi!

Here is a simple patch fixing the regression introduced in r270660.
Regtested and bootstrapped with `BOOT_CFLAGS="-O
-Wno-error=maybe-uninitialized -Wuninitialized"` on x86_64-pc-linux-gnu.
OK for trunk?

It is not the ideal solution, but it is the simplest (and safest).  So
let's roll with it and then I've already prepared an improvement, but
need your input.

The case where code2 == NE_EXPR and code1 == BIT_AND_EXPR is now (with
this patch) caught by

  if (code1 != code2)
    return false;

This means a false positive in some cases: e.g. if we change 5u to 16u
in the gimple test case above, we'll have

  if (v != 16)
    u = sth;
   ...
  if (v & 3)
    use (u);

for which a warning will be generated (and it's an FP: u is uninit iff
v == 16, but in that case u is not used).

This can be fixed with something like this:

 if (code2 == NE_EXPR)
  {
    if (code1 == BIT_AND_EXPR)
      return !((wi::to_wide (expr1.pred_rhs) & wi::to_wide (expr2.pred_rhs))
               .to_uhwi());
    else
      return !is_value_included_in (expr2.pred_rhs, expr1.pred_rhs, code1);
  }

But actually I would rather extend is_value_included_in to handle
BIT_AND_EXPR.  OTOH, its other callers don't seem to need it.  My
concern is that handling BIT_AND_EXPR in is_value_included_in as I would
like to do would make the assertion in is_value_included_in weaker.

Frankly, I don't completely understand where BIT_AND_EXPR can and cannot
appear.

Would you have any suggestions?

Thank you.
Vlad

Comments

Richard Biener May 16, 2019, 11:09 a.m. UTC | #1 
On Thu, May 16, 2019 at 1:09 AM Vladislav Ivanishin <vlad@ispras.ru> wrote:
>
> Hi!
>
> Here is a simple patch fixing the regression introduced in r270660.
>
>
> Regtested and bootstrapped with `BOOT_CFLAGS="-O
> -Wno-error=maybe-uninitialized -Wuninitialized"` on x86_64-pc-linux-gnu.
> OK for trunk?

OK.

> It is not the ideal solution, but it is the simplest (and safest).  So
> let's roll with it and then I've already prepared an improvement, but
> need your input.
>
> The case where code2 == NE_EXPR and code1 == BIT_AND_EXPR is now (with
> this patch) caught by
>
>   if (code1 != code2)
>     return false;
>
> This means a false positive in some cases: e.g. if we change 5u to 16u
> in the gimple test case above, we'll have
>
>   if (v != 16)
>     u = sth;
>    ...
>   if (v & 3)
>     use (u);
>
> for which a warning will be generated (and it's an FP: u is uninit iff
> v == 16, but in that case u is not used).
>
> This can be fixed with something like this:
>
>  if (code2 == NE_EXPR)
>   {
>     if (code1 == BIT_AND_EXPR)
>       return !((wi::to_wide (expr1.pred_rhs) & wi::to_wide (expr2.pred_rhs))
>                .to_uhwi());
>     else
>       return !is_value_included_in (expr2.pred_rhs, expr1.pred_rhs, code1);
>   }
>
> But actually I would rather extend is_value_included_in to handle
> BIT_AND_EXPR.  OTOH, its other callers don't seem to need it.  My
> concern is that handling BIT_AND_EXPR in is_value_included_in as I would
> like to do would make the assertion in is_value_included_in weaker.
>
> Frankly, I don't completely understand where BIT_AND_EXPR can and cannot
> appear.
>
> Would you have any suggestions?

I don't remember enough around this code to give an advice without digging in
myself.  svn blame might help to uncover reasoning from the original patch
submission adding BIT_AND_EXPR support.

Richard.

> Thank you.
> Vlad
diff mbox series

Patch

gcc/testsuite/ChangeLog:

	  PR tree-optimization/90394
	  * gcc.dg/uninit-pr90394-1-gimple.c: New test.
	  * gcc.dg/uninit-pr90394.c: New test.

gcc/ChangeLog:

	  PR tree-optimization/90394
	  * tree-ssa-uninit.c (is_pred_expr_subset_of): Potentially give false
	  positives rather than ICE for cases where (code2 == NE_EXPR
	  && code1 == BIT_AND_EXPR).
---
 .../gcc.dg/uninit-pr90394-1-gimple.c          | 47 +++++++++++++++++++
 gcc/testsuite/gcc.dg/uninit-pr90394.c         | 33 +++++++++++++
 gcc/tree-ssa-uninit.c                         |  2 +-
 3 files changed, 81 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.dg/uninit-pr90394-1-gimple.c
 create mode 100644 gcc/testsuite/gcc.dg/uninit-pr90394.c

diff --git a/gcc/testsuite/gcc.dg/uninit-pr90394-1-gimple.c b/gcc/testsuite/gcc.dg/uninit-pr90394-1-gimple.c
new file mode 100644
index 00000000000..f8feb6b8967
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/uninit-pr90394-1-gimple.c
@@ -0,0 +1,47 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fgimple -O -Wmaybe-uninitialized" } */
+
+unsigned int __GIMPLE (ssa,startwith("uninit1"))
+foo (unsigned int v)
+{
+  /* The warning is not bogus, because (5 & 3) != 0 and therefore if v == 5,
+     the value of undef is used without being initialized.  */
+  unsigned int undef;        /* { dg-warning "may be used uninitialized" } */
+  unsigned int _2;
+  unsigned int _9;
+  unsigned int _10;
+  unsigned pred;
+
+  __BB(2):
+  if (v_4(D) != 5u)
+    goto __BB3;
+  else
+    goto __BB4;
+
+  /* 'undef' is defined conditionally (under 'v != 5' predicate)  */
+  __BB(3):
+  undef_8 = 8u;
+  goto __BB4;
+
+  /* An undef value flows into a phi.  */
+  __BB(4):
+  undef_1 = __PHI (__BB2: undef_5(D), __BB3: undef_8);
+  pred = v_4(D) & 3u;
+  if (pred != 0u)
+    goto __BB5;
+  else
+    goto __BB6;
+
+  /* The phi value is used here (under 'v & 3' predicate).  */
+  __BB(5):
+  _9 = undef_1;
+  goto __BB7;
+
+  __BB(6):
+  _10 = v_4(D);
+  goto __BB7;
+
+  __BB(7):
+  _2 = __PHI (__BB5: _9, __BB6: _10);
+  return _2;
+}
diff --git a/gcc/testsuite/gcc.dg/uninit-pr90394.c b/gcc/testsuite/gcc.dg/uninit-pr90394.c
new file mode 100644
index 00000000000..16e750d6b33
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/uninit-pr90394.c
@@ -0,0 +1,33 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O1 -fno-tree-fre -Wuninitialized" } */
+int pz;
+int zi;
+
+void
+uk (void)
+{
+  int th = 1;
+  int *gw = &zi;
+
+  for (zi = 0; zi < 2; ++zi)
+    {
+      int a2 = 0;
+
+      for (zi = 0; zi < 1; ++zi)
+        {
+          th = a2 * 2;
+
+ og:
+          for (pz = 0; pz < 1; ++pz)
+            {
+            }
+        }
+
+      pz = !!*gw ? *gw : pz;
+      pz = (!!th ? (pz & 1) : 0);
+      if (pz == 0)
+        ++a2;
+    }
+
+  goto og;
+}
diff --git a/gcc/tree-ssa-uninit.c b/gcc/tree-ssa-uninit.c
index 7362e374dea..b89da4017e8 100644
--- a/gcc/tree-ssa-uninit.c
+++ b/gcc/tree-ssa-uninit.c
@@ -1471,7 +1471,7 @@  is_pred_expr_subset_of (pred_info expr1, pred_info expr2)
   if (code2 == NE_EXPR && code1 == NE_EXPR)
     return false;
 
-  if (code2 == NE_EXPR)
+  if (code2 == NE_EXPR && code1 != BIT_AND_EXPR)
     return !is_value_included_in (expr2.pred_rhs, expr1.pred_rhs, code1);
 
   if ((code1 == EQ_EXPR || code1 == BIT_AND_EXPR) && code2 == BIT_AND_EXPR)
-- 
2.20.1