[PATCH] Fix PR68067

[PATCH] Fix PR68067

[Richard Biener]

The following patch adjusts negate_expr_p to account for the fact
that we can't generally change a - (b - c) to (c - b) + a because
-INF - 0 is ok while 0 - -INF not.  Similarly for a - (b + c).
While creating testcases I noticed that MULT_EXPR handling is bogus
as well as with -INF/2 * 2 neither operand can be negated safely.

I believe the division case is also still wrong but I refrained
from touching it with this patch.

Bootstrapped and tested on x86_64-unknown-linux-gnu.

Richard.

2015-10-27  Richard Biener  <rguenther@suse.de>

	PR middle-end/68067
	* fold-const.c (negate_expr_p): We cannot negate plus or minus
	if overflow is not wrapping.  Likewise multiplication unless
	one operand is constant and not power of two.
	(fold_negate_expr): Adjust accordingly.

	* gcc.dg/torture/pr68067-1.c: New testcase.
	* gcc.dg/torture/pr68067-2.c: Likewise.

Index: gcc/fold-const.c
===================================================================
--- gcc/fold-const.c	(revision 229404)
+++ gcc/fold-const.c	(working copy)
@@ -443,7 +443,9 @@ negate_expr_p (tree t)
 
     case PLUS_EXPR:
       if (HONOR_SIGN_DEPENDENT_ROUNDING (element_mode (type))
-	  || HONOR_SIGNED_ZEROS (element_mode (type)))
+	  || HONOR_SIGNED_ZEROS (element_mode (type))
+	  || (INTEGRAL_TYPE_P (type)
+	      && ! TYPE_OVERFLOW_WRAPS (type)))
 	return false;
       /* -(A + B) -> (-B) - A.  */
       if (negate_expr_p (TREE_OPERAND (t, 1))
@@ -457,12 +459,23 @@ negate_expr_p (tree t)
       /* We can't turn -(A-B) into B-A when we honor signed zeros.  */
       return !HONOR_SIGN_DEPENDENT_ROUNDING (element_mode (type))
 	     && !HONOR_SIGNED_ZEROS (element_mode (type))
+	     && (! INTEGRAL_TYPE_P (type)
+		 || TYPE_OVERFLOW_WRAPS (type))
 	     && reorder_operands_p (TREE_OPERAND (t, 0),
 				    TREE_OPERAND (t, 1));
 
     case MULT_EXPR:
-      if (TYPE_UNSIGNED (TREE_TYPE (t)))
-        break;
+      if (TYPE_UNSIGNED (type))
+	break;
+      /* INT_MIN/n * n doesn't overflow while negating one operand it does
+         if n is a power of two.  */
+      if (INTEGRAL_TYPE_P (TREE_TYPE (t))
+	  && ! TYPE_OVERFLOW_WRAPS (TREE_TYPE (t))
+	  && ! ((TREE_CODE (TREE_OPERAND (t, 0)) == INTEGER_CST
+		 && ! integer_pow2p (TREE_OPERAND (t, 0)))
+		|| (TREE_CODE (TREE_OPERAND (t, 1)) == INTEGER_CST
+		    && ! integer_pow2p (TREE_OPERAND (t, 1)))))
+	break;
 
       /* Fall through.  */
 
Index: gcc/testsuite/gcc.dg/torture/pr68067-1.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/pr68067-1.c	(revision 0)
+++ gcc/testsuite/gcc.dg/torture/pr68067-1.c	(working copy)
@@ -0,0 +1,12 @@
+/* { dg-do run } */
+
+int main()
+{
+  int a = -1;
+  static int b = -2147483647 - 1;
+  static int c = 0;
+  int t = a - (b - c);
+  if (t != 2147483647)
+    __builtin_abort();
+  return 0;
+}
Index: gcc/testsuite/gcc.dg/torture/pr68067-2.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/pr68067-2.c	(revision 0)
+++ gcc/testsuite/gcc.dg/torture/pr68067-2.c	(working copy)
@@ -0,0 +1,13 @@
+/* { dg-do run } */
+
+int main()
+{
+  int a = -1;
+  static int b = -2147483647 - 1;
+  static int c = 0;
+  int t = a - (b + c*-2);
+  if (t != 2147483647)
+    __builtin_abort();
+  return 0;
+}
+

Re: [PATCH] Fix PR68067

[Richard Biener]

On Tue, 27 Oct 2015, Richard Biener wrote:

> 
> The following patch adjusts negate_expr_p to account for the fact
> that we can't generally change a - (b - c) to (c - b) + a because
> -INF - 0 is ok while 0 - -INF not.  Similarly for a - (b + c).
> While creating testcases I noticed that MULT_EXPR handling is bogus
> as well as with -INF/2 * 2 neither operand can be negated safely.
> 
> I believe the division case is also still wrong but I refrained
> from touching it with this patch.

Here is the division part.

Bootstrap / regtest running on x86_64-unknown-linux-gnu.  A
related testcase went in with r202204.

Richard.

2015-10-27  Richard Biener  <rguenther@suse.de>

	* fold-const.c (negate_expr_p): Adjust the division case to
	properly avoid introducing undefined overflow.
	(fold_negate_expr): Likewise.

Index: gcc/fold-const.c
===================================================================
--- gcc/fold-const.c	(revision 229404)
+++ gcc/fold-const.c	(working copy)
@@ -475,29 +488,23 @@ negate_expr_p (tree t)
     case TRUNC_DIV_EXPR:
     case ROUND_DIV_EXPR:
     case EXACT_DIV_EXPR:
-      /* In general we can't negate A / B, because if A is INT_MIN and
-	 B is 1, we may turn this into INT_MIN / -1 which is undefined
-	 and actually traps on some architectures.  But if overflow is
-	 undefined, we can negate, because - (INT_MIN / 1) is an
-	 overflow.  */
       if (INTEGRAL_TYPE_P (TREE_TYPE (t)))
 	{
-	  if (!TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (t)))
-	    break;
-	  /* If overflow is undefined then we have to be careful because
-	     we ask whether it's ok to associate the negate with the
-	     division which is not ok for example for
-	     -((a - b) / c) where (-(a - b)) / c may invoke undefined
-	     overflow because of negating INT_MIN.  So do not use
-	     negate_expr_p here but open-code the two important cases.  */
-	  if (TREE_CODE (TREE_OPERAND (t, 0)) == NEGATE_EXPR
-	      || (TREE_CODE (TREE_OPERAND (t, 0)) == INTEGER_CST
-		  && may_negate_without_overflow_p (TREE_OPERAND (t, 0))))
+	  if (negate_expr_p (TREE_OPERAND (t, 0)))
 	    return true;
+
+	  /* In general we can't negate B in A / B, because if A is INT_MIN and
+	     B is 1, we may turn this into INT_MIN / -1 which is undefined
+	     and actually traps on some architectures.  */
+	  if (TYPE_OVERFLOW_WRAPS (TREE_TYPE (t))
+	      || (TREE_CODE (TREE_OPERAND (t, 1)) == INTEGER_CST
+		  && ! integer_onep (TREE_OPERAND (t, 1))))
+	    return negate_expr_p (TREE_OPERAND (t, 1));
 	}
-      else if (negate_expr_p (TREE_OPERAND (t, 0)))
-	return true;
-      return negate_expr_p (TREE_OPERAND (t, 1));
+      else
+	return (negate_expr_p (TREE_OPERAND (t, 0))
+		|| negate_expr_p (TREE_OPERAND (t, 1)));
+      break;
 
     case NOP_EXPR:
       /* Negate -((double)float) as (double)(-float).  */
@@ -667,40 +681,22 @@ fold_negate_expr (location_t loc, tree t
     case TRUNC_DIV_EXPR:
     case ROUND_DIV_EXPR:
     case EXACT_DIV_EXPR:
-      /* In general we can't negate A / B, because if A is INT_MIN and
+      /* In general we can't negate B in A / B, because if A is INT_MIN and
 	 B is 1, we may turn this into INT_MIN / -1 which is undefined
-	 and actually traps on some architectures.  But if overflow is
-	 undefined, we can negate, because - (INT_MIN / 1) is an
-	 overflow.  */
-      if (!INTEGRAL_TYPE_P (type) || TYPE_OVERFLOW_UNDEFINED (type))
-        {
-	  const char * const warnmsg = G_("assuming signed overflow does not "
-					  "occur when negating a division");
-          tem = TREE_OPERAND (t, 1);
-          if (negate_expr_p (tem))
-	    {
-	      if (INTEGRAL_TYPE_P (type)
-		  && (TREE_CODE (tem) != INTEGER_CST
-		      || integer_onep (tem)))
-		fold_overflow_warning (warnmsg, WARN_STRICT_OVERFLOW_MISC);
-	      return fold_build2_loc (loc, TREE_CODE (t), type,
-				  TREE_OPERAND (t, 0), negate_expr (tem));
-	    }
-	  /* If overflow is undefined then we have to be careful because
-	     we ask whether it's ok to associate the negate with the
-	     division which is not ok for example for
-	     -((a - b) / c) where (-(a - b)) / c may invoke undefined
-	     overflow because of negating INT_MIN.  So do not use
-	     negate_expr_p here but open-code the two important cases.  */
-          tem = TREE_OPERAND (t, 0);
-	  if ((INTEGRAL_TYPE_P (type)
-	       && (TREE_CODE (tem) == NEGATE_EXPR
-		   || (TREE_CODE (tem) == INTEGER_CST
-		       && may_negate_without_overflow_p (tem))))
-	      || !INTEGRAL_TYPE_P (type))
-	    return fold_build2_loc (loc, TREE_CODE (t), type,
-				    negate_expr (tem), TREE_OPERAND (t, 1));
-        }
+	 and actually traps on some architectures.  */
+      if ((! INTEGRAL_TYPE_P (type)
+	   || TYPE_OVERFLOW_WRAPS (TREE_TYPE (t))
+	   || (TREE_CODE (TREE_OPERAND (t, 1)) == INTEGER_CST
+	       && ! integer_onep (TREE_OPERAND (t, 1))))
+	  && negate_expr_p (TREE_OPERAND (t, 1)))
+	return fold_build2_loc (loc, TREE_CODE (t), type,
+				TREE_OPERAND (t, 0),
+				negate_expr (TREE_OPERAND (t, 1)));
+
+      if (negate_expr_p (TREE_OPERAND (t, 0)))
+	return fold_build2_loc (loc, TREE_CODE (t), type,
+				negate_expr (TREE_OPERAND (t, 0)),
+				TREE_OPERAND (t, 1));
       break;
 
     case NOP_EXPR:

Re: [PATCH] Fix PR68067

[Richard Biener]

On Tue, 27 Oct 2015, Richard Biener wrote:

> On Tue, 27 Oct 2015, Richard Biener wrote:
> 
> > 
> > The following patch adjusts negate_expr_p to account for the fact
> > that we can't generally change a - (b - c) to (c - b) + a because
> > -INF - 0 is ok while 0 - -INF not.  Similarly for a - (b + c).
> > While creating testcases I noticed that MULT_EXPR handling is bogus
> > as well as with -INF/2 * 2 neither operand can be negated safely.
> > 
> > I believe the division case is also still wrong but I refrained
> > from touching it with this patch.
> 
> Here is the division part.
> 
> Bootstrap / regtest running on x86_64-unknown-linux-gnu.  A
> related testcase went in with r202204.

Applied as follows.

Bootstrapped / tested on x86_64-unknown-linux-gnu.

Richard.

2015-10-28  Richard Biener  <rguenther@suse.de>

	* fold-const.c (negate_expr_p): Adjust the division case to
	properly avoid introducing undefined overflow.
	(fold_negate_expr): Likewise.


Index: gcc/fold-const.c
===================================================================
--- gcc/fold-const.c	(revision 229481)
+++ gcc/fold-const.c	(working copy)
@@ -488,29 +488,19 @@ negate_expr_p (tree t)
     case TRUNC_DIV_EXPR:
     case ROUND_DIV_EXPR:
     case EXACT_DIV_EXPR:
-      /* In general we can't negate A / B, because if A is INT_MIN and
-	 B is 1, we may turn this into INT_MIN / -1 which is undefined
-	 and actually traps on some architectures.  But if overflow is
-	 undefined, we can negate, because - (INT_MIN / 1) is an
-	 overflow.  */
-      if (INTEGRAL_TYPE_P (TREE_TYPE (t)))
-	{
-	  if (!TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (t)))
-	    break;
-	  /* If overflow is undefined then we have to be careful because
-	     we ask whether it's ok to associate the negate with the
-	     division which is not ok for example for
-	     -((a - b) / c) where (-(a - b)) / c may invoke undefined
-	     overflow because of negating INT_MIN.  So do not use
-	     negate_expr_p here but open-code the two important cases.  */
-	  if (TREE_CODE (TREE_OPERAND (t, 0)) == NEGATE_EXPR
-	      || (TREE_CODE (TREE_OPERAND (t, 0)) == INTEGER_CST
-		  && may_negate_without_overflow_p (TREE_OPERAND (t, 0))))
-	    return true;
-	}
-      else if (negate_expr_p (TREE_OPERAND (t, 0)))
+      if (TYPE_UNSIGNED (type))
+	break;
+      if (negate_expr_p (TREE_OPERAND (t, 0)))
 	return true;
-      return negate_expr_p (TREE_OPERAND (t, 1));
+      /* In general we can't negate B in A / B, because if A is INT_MIN and
+	 B is 1, we may turn this into INT_MIN / -1 which is undefined
+	 and actually traps on some architectures.  */
+      if (! INTEGRAL_TYPE_P (TREE_TYPE (t))
+	  || TYPE_OVERFLOW_WRAPS (TREE_TYPE (t))
+	  || (TREE_CODE (TREE_OPERAND (t, 1)) == INTEGER_CST
+	      && ! integer_onep (TREE_OPERAND (t, 1))))
+	return negate_expr_p (TREE_OPERAND (t, 1));
+      break;
 
     case NOP_EXPR:
       /* Negate -((double)float) as (double)(-float).  */
@@ -680,40 +677,23 @@ fold_negate_expr (location_t loc, tree t
     case TRUNC_DIV_EXPR:
     case ROUND_DIV_EXPR:
     case EXACT_DIV_EXPR:
-      /* In general we can't negate A / B, because if A is INT_MIN and
+      if (TYPE_UNSIGNED (type))
+	break;
+      if (negate_expr_p (TREE_OPERAND (t, 0)))
+	return fold_build2_loc (loc, TREE_CODE (t), type,
+				negate_expr (TREE_OPERAND (t, 0)),
+				TREE_OPERAND (t, 1));
+      /* In general we can't negate B in A / B, because if A is INT_MIN and
 	 B is 1, we may turn this into INT_MIN / -1 which is undefined
-	 and actually traps on some architectures.  But if overflow is
-	 undefined, we can negate, because - (INT_MIN / 1) is an
-	 overflow.  */
-      if (!INTEGRAL_TYPE_P (type) || TYPE_OVERFLOW_UNDEFINED (type))
-        {
-	  const char * const warnmsg = G_("assuming signed overflow does not "
-					  "occur when negating a division");
-          tem = TREE_OPERAND (t, 1);
-          if (negate_expr_p (tem))
-	    {
-	      if (INTEGRAL_TYPE_P (type)
-		  && (TREE_CODE (tem) != INTEGER_CST
-		      || integer_onep (tem)))
-		fold_overflow_warning (warnmsg, WARN_STRICT_OVERFLOW_MISC);
-	      return fold_build2_loc (loc, TREE_CODE (t), type,
-				  TREE_OPERAND (t, 0), negate_expr (tem));
-	    }
-	  /* If overflow is undefined then we have to be careful because
-	     we ask whether it's ok to associate the negate with the
-	     division which is not ok for example for
-	     -((a - b) / c) where (-(a - b)) / c may invoke undefined
-	     overflow because of negating INT_MIN.  So do not use
-	     negate_expr_p here but open-code the two important cases.  */
-          tem = TREE_OPERAND (t, 0);
-	  if ((INTEGRAL_TYPE_P (type)
-	       && (TREE_CODE (tem) == NEGATE_EXPR
-		   || (TREE_CODE (tem) == INTEGER_CST
-		       && may_negate_without_overflow_p (tem))))
-	      || !INTEGRAL_TYPE_P (type))
-	    return fold_build2_loc (loc, TREE_CODE (t), type,
-				    negate_expr (tem), TREE_OPERAND (t, 1));
-        }
+	 and actually traps on some architectures.  */
+      if ((! INTEGRAL_TYPE_P (TREE_TYPE (t))
+	   || TYPE_OVERFLOW_WRAPS (TREE_TYPE (t))
+	   || (TREE_CODE (TREE_OPERAND (t, 1)) == INTEGER_CST
+	       && ! integer_onep (TREE_OPERAND (t, 1))))
+	  && negate_expr_p (TREE_OPERAND (t, 1)))
+	return fold_build2_loc (loc, TREE_CODE (t), type,
+				TREE_OPERAND (t, 0),
+				negate_expr (TREE_OPERAND (t, 1)));
       break;
 
     case NOP_EXPR:

Re: [PATCH] Fix PR68067

[Alan Lawrence]

On 28/10/15 13:38, Richard Biener wrote:
>
> Applied as follows.
>
> Bootstrapped / tested on x86_64-unknown-linux-gnu.
>
> Richard.
>
> 2015-10-28  Richard Biener  <rguenther@suse.de>
>
> 	* fold-const.c (negate_expr_p): Adjust the division case to
> 	properly avoid introducing undefined overflow.
> 	(fold_negate_expr): Likewise.

Since this we've been seeing an ICE compiling polynom.c from 254.gap in SPEC2000 
on aarch64-linux-gnu with -O3 -ffast-math -mcpu=cortex-a53 (or -Ofast 
-mcpu=cortex-a53), on both native (bootstrapped and --disable-bootstrap) and 
cross-linux builds.

A number of options prevent the ICE, e.g. any of -fno-thread-jumps, 
-fno-strict-overflow, -fdump-tree-alias or -fdump-tree-ealias (!). Similarly, 
dropping the -mcpu=cortex-a53, or changing to -mcpu=cortex-a57.

(I have a recent build in a chroot for which -fno-strict-overflow does *not* fix 
the ICE but haven't yet figured out exactly what the difference in the chroot 
environment is.)

Moreover, preprocessing in a separate step (i.e. piping preprocessed output via 
a file with -E), also avoids the ICE. (This is hindering my efforts to reduce 
the testcase!).  So my hypothesis is that this is a front-end/preprocessor bug, 
rather than anything directly due to this commit.

The error message in full (line refs from that commit, r229479) is:
=====
../spec2000/benchspec/CINT2000/254.gap/src/polynom.c: In function 
‘NormalizeCoeffsListx’:
../spec2000/benchspec/CINT2000/254.gap/src/polynom.c:358:11: error: incompatible 
types in PHI argument 0
  TypHandle NormalizeCoeffsListx ( hdC )
            ^
long int

int

../spec2000/benchspec/CINT2000/254.gap/src/polynom.c:358:11: error: location 
references block not in block tree
l1_279 = PHI <1(28), l1_299(33)>
../spec2000/benchspec/CINT2000/254.gap/src/polynom.c:358:11: error: invalid PHI 
argument

../spec2000/benchspec/CINT2000/254.gap/src/polynom.c:358:11: internal compiler 
error: tree check: expected class ‘type’, have ‘declaration’ (namespace_decl) in 
useless_type_conversion_p, at gimple-expr.c:84
0xd110ef tree_class_check_failed(tree_node const*, tree_code_class, char const*, 
int, char const*)
         ../../gcc-fsf/gcc/tree.c:9643
0x82561b tree_class_check
         ../../gcc-fsf/gcc/tree.h:3042
0x82561b useless_type_conversion_p(tree_node*, tree_node*)
         ../../gcc-fsf/gcc/gimple-expr.c:84
0xaca043 verify_gimple_phi
         ../../gcc-fsf/gcc/tree-cfg.c:4673
0xaca043 verify_gimple_in_cfg(function*, bool)
         ../../gcc-fsf/gcc/tree-cfg.c:4967
0x9c2e0b execute_function_todo
         ../../gcc-fsf/gcc/passes.c:1967
0x9c360b do_per_function
         ../../gcc-fsf/gcc/passes.c:1659
0x9c3807 execute_todo
         ../../gcc-fsf/gcc/passes.c:2022
Please submit a full bug report,
with preprocessed source if appropriate.
=====
which looks like an "incompatible types from PHI argument" from a first call to 
verify_gimple_phi, then a second call to verify_gimple_phi prints "invalid phi 
argument" and ICEs in the test just before possibly printing a second 
incompatible_types message.


--Alan

Re: [PATCH] Fix PR68067

[Richard Biener]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3777 bytes --]

On Fri, 6 Nov 2015, Alan Lawrence wrote:

> On 28/10/15 13:38, Richard Biener wrote:
> > 
> > Applied as follows.
> > 
> > Bootstrapped / tested on x86_64-unknown-linux-gnu.
> > 
> > Richard.
> > 
> > 2015-10-28  Richard Biener  <rguenther@suse.de>
> > 
> > 	* fold-const.c (negate_expr_p): Adjust the division case to
> > 	properly avoid introducing undefined overflow.
> > 	(fold_negate_expr): Likewise.
> 
> Since this we've been seeing an ICE compiling polynom.c from 254.gap in
> SPEC2000 on aarch64-linux-gnu with -O3 -ffast-math -mcpu=cortex-a53 (or -Ofast
> -mcpu=cortex-a53), on both native (bootstrapped and --disable-bootstrap) and
> cross-linux builds.
> 
> A number of options prevent the ICE, e.g. any of -fno-thread-jumps,
> -fno-strict-overflow, -fdump-tree-alias or -fdump-tree-ealias (!). Similarly,
> dropping the -mcpu=cortex-a53, or changing to -mcpu=cortex-a57.
> 
> (I have a recent build in a chroot for which -fno-strict-overflow does *not*
> fix the ICE but haven't yet figured out exactly what the difference in the
> chroot environment is.)
> 
> Moreover, preprocessing in a separate step (i.e. piping preprocessed output
> via a file with -E), also avoids the ICE. (This is hindering my efforts to
> reduce the testcase!).  So my hypothesis is that this is a
> front-end/preprocessor bug, rather than anything directly due to this commit.
> 
> The error message in full (line refs from that commit, r229479) is:
> =====
> ../spec2000/benchspec/CINT2000/254.gap/src/polynom.c: In function
> ‘NormalizeCoeffsListx’:
> ../spec2000/benchspec/CINT2000/254.gap/src/polynom.c:358:11: error:
> incompatible types in PHI argument 0
>  TypHandle NormalizeCoeffsListx ( hdC )
>            ^
> long int
> 
> int
> 
> ../spec2000/benchspec/CINT2000/254.gap/src/polynom.c:358:11: error: location
> references block not in block tree
> l1_279 = PHI <1(28), l1_299(33)>

^^^

this is the error to look at!  It means that the GC heap will be corrupted
quite easily.

> ../spec2000/benchspec/CINT2000/254.gap/src/polynom.c:358:11: error: invalid
> PHI argument

which means this could be a followup error.  We do have a bugreport (or 
two) about similar issues that were tracked down to different patches.

Somebody needs to sit down and debug this properly ;)

> ../spec2000/benchspec/CINT2000/254.gap/src/polynom.c:358:11: internal compiler
> error: tree check: expected class ‘type’, have ‘declaration’ (namespace_decl)
> in useless_type_conversion_p, at gimple-expr.c:84
> 0xd110ef tree_class_check_failed(tree_node const*, tree_code_class, char
> const*, int, char const*)
>         ../../gcc-fsf/gcc/tree.c:9643
> 0x82561b tree_class_check
>         ../../gcc-fsf/gcc/tree.h:3042
> 0x82561b useless_type_conversion_p(tree_node*, tree_node*)
>         ../../gcc-fsf/gcc/gimple-expr.c:84
> 0xaca043 verify_gimple_phi
>         ../../gcc-fsf/gcc/tree-cfg.c:4673
> 0xaca043 verify_gimple_in_cfg(function*, bool)
>         ../../gcc-fsf/gcc/tree-cfg.c:4967
> 0x9c2e0b execute_function_todo
>         ../../gcc-fsf/gcc/passes.c:1967

Interesting would be for which pass this happens - just print
*pass at this point.

> 0x9c360b do_per_function
>         ../../gcc-fsf/gcc/passes.c:1659
> 0x9c3807 execute_todo
>         ../../gcc-fsf/gcc/passes.c:2022
> Please submit a full bug report,
> with preprocessed source if appropriate.
> =====
> which looks like an "incompatible types from PHI argument" from a first call
> to verify_gimple_phi, then a second call to verify_gimple_phi prints "invalid
> phi argument" and ICEs in the test just before possibly printing a second
> incompatible_types message.
> 
> 
> --Alan
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: [PATCH] Fix PR68067

[Alan Lawrence]

On 06/11/15 10:39, Richard Biener wrote:
>> ../spec2000/benchspec/CINT2000/254.gap/src/polynom.c:358:11: error: location
>> references block not in block tree
>> l1_279 = PHI <1(28), l1_299(33)>
>
> ^^^
>
> this is the error to look at!  It means that the GC heap will be corrupted
> quite easily.

Thanks, I'll have a go at that.

-fdump-tree-alias is also suspicious, hinting that that does more than just 
print. (How many bugs here??)

> Interesting would be for which pass this happens - just print
> *pass at this point.

FWIW - unswitch. (A long time after -fdump-tree-alias!)

Cheers, Alan

Re: [PATCH] Fix PR68067

[Richard Biener]

On Fri, 6 Nov 2015, Alan Lawrence wrote:

> On 06/11/15 10:39, Richard Biener wrote:
> > > ../spec2000/benchspec/CINT2000/254.gap/src/polynom.c:358:11: error:
> > > location
> > > references block not in block tree
> > > l1_279 = PHI <1(28), l1_299(33)>
> > 
> > ^^^
> > 
> > this is the error to look at!  It means that the GC heap will be corrupted
> > quite easily.
> 
> Thanks, I'll have a go at that.
> 
> -fdump-tree-alias is also suspicious, hinting that that does more than just
> print. (How many bugs here??)

Well, it only allocates some more heap memory for extra debug verbosity.
I think this can happen with other dumps as well.

> > Interesting would be for which pass this happens - just print
> > *pass at this point.
> 
> FWIW - unswitch. (A long time after -fdump-tree-alias!)

Ah, unswitch again... :/

> Cheers, Alan
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: [PATCH] Fix PR68067

[Jeff Law]

On 11/06/2015 05:26 AM, Richard Biener wrote:
> On Fri, 6 Nov 2015, Alan Lawrence wrote:
>
>> On 06/11/15 10:39, Richard Biener wrote:
>>>> ../spec2000/benchspec/CINT2000/254.gap/src/polynom.c:358:11: error:
>>>> location
>>>> references block not in block tree
>>>> l1_279 = PHI <1(28), l1_299(33)>
>>>
>>> ^^^
>>>
>>> this is the error to look at!  It means that the GC heap will be corrupted
>>> quite easily.
>>
>> Thanks, I'll have a go at that.
>>
>> -fdump-tree-alias is also suspicious, hinting that that does more than just
>> print. (How many bugs here??)
>
> Well, it only allocates some more heap memory for extra debug verbosity.
> I think this can happen with other dumps as well.
Right.  Anytime a -fdump-whatever changes behaviour, there's usually 
some underlying memory issue elsewhere it's tickling.

jeff

Re: [PATCH] Fix PR68067

[Alan Lawrence]

On 6 November 2015 at 10:39, Richard Biener <rguenther@suse.de> wrote:
>> ../spec2000/benchspec/CINT2000/254.gap/src/polynom.c:358:11: error: location
>> references block not in block tree
>> l1_279 = PHI <1(28), l1_299(33)>
>
> ^^^
>
> this is the error to look at!  It means that the GC heap will be corrupted
> quite easily.
>

This looked very similar to PR68117 - the invalid phi arg, and block
not in  block-tree, even if not the invalid tree code - and as the
posters there were having success with valgrind, whereas I wasn't, I
watched and waited. First observation is that it triggers the asserts
you suggested in comment 27
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D68117#c27). Indeed, it
fails those asserts, even after the patch in comment 25 (committed as
r230594) to tree-ssa.c (delete_tree_ssa), and the patch in comment#35
to function.c (set_cfun), and the patch in comment#30 (committed as
r230424) to cfgexpand.c (pass_expand::execute).

The patch in comment#29 (which replaces the asserts in comment#27 with
empties), however, fixes the problem - although I can't rule out, that
that's just by changing the memory allocation pattern.

Moreover, if I take those patches and rebase onto a recent trunk (onto
which the delete_tree_ssa and pass_expand::execute patches have
already been committed), i.e. just adding the assertions from
comment#27 and the call in function.c (set_cfun) - the assertions are
still failing on my testcase, whereas the original (assertionless)
failure was very erratic, and had since disappeared/been hidden on
trunk. Indeed those same assertions break in a few other places (even
in a --disable-bootstrap build after gcc/xgcc is built), so I feel I
have a good chance of producing a reasonable assertion-breaking
testcase.

So I have to ask, how sure are you that those assertions are(/should
be!) "correct"? :)

--Alan

Re: [PATCH] Fix PR68067

[Richard Biener]

On Fri, 20 Nov 2015, Alan Lawrence wrote:

> On 6 November 2015 at 10:39, Richard Biener <rguenther@suse.de> wrote:
> >> ../spec2000/benchspec/CINT2000/254.gap/src/polynom.c:358:11: error: location
> >> references block not in block tree
> >> l1_279 = PHI <1(28), l1_299(33)>
> >
> > ^^^
> >
> > this is the error to look at!  It means that the GC heap will be corrupted
> > quite easily.
> >
> 
> This looked very similar to PR68117 - the invalid phi arg, and block
> not in  block-tree, even if not the invalid tree code - and as the
> posters there were having success with valgrind, whereas I wasn't, I
> watched and waited. First observation is that it triggers the asserts
> you suggested in comment 27
> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D68117#c27). Indeed, it
> fails those asserts, even after the patch in comment 25 (committed as
> r230594) to tree-ssa.c (delete_tree_ssa), and the patch in comment#35
> to function.c (set_cfun), and the patch in comment#30 (committed as
> r230424) to cfgexpand.c (pass_expand::execute).
> 
> The patch in comment#29 (which replaces the asserts in comment#27 with
> empties), however, fixes the problem - although I can't rule out, that
> that's just by changing the memory allocation pattern.
> 
> Moreover, if I take those patches and rebase onto a recent trunk (onto
> which the delete_tree_ssa and pass_expand::execute patches have
> already been committed), i.e. just adding the assertions from
> comment#27 and the call in function.c (set_cfun) - the assertions are
> still failing on my testcase, whereas the original (assertionless)
> failure was very erratic, and had since disappeared/been hidden on
> trunk. Indeed those same assertions break in a few other places (even
> in a --disable-bootstrap build after gcc/xgcc is built), so I feel I
> have a good chance of producing a reasonable assertion-breaking
> testcase.
> 
> So I have to ask, how sure are you that those assertions are(/should
> be!) "correct"? :)

Ideally they should be correct but they happen to be not (and I think
the intent was that this should be harmless).  Basically I tried
to assert that nobody creates stale edge redirect data that is not
later consumed or cleared.  Happens to be too optimistic :/

Richard.

Re: [PATCH] Fix PR68067

[Alan Lawrence]

On 23/11/15 09:43, Richard Biener wrote:
> On Fri, 20 Nov 2015, Alan Lawrence wrote:
>
>> ...the asserts
>> you suggested in (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D68117#c27)...
 >>
>> So I have to ask, how sure are you that those assertions are(/should
>> be!) "correct"? :)
>
> Ideally they should be correct but they happen to be not (and I think
> the intent was that this should be harmless).  Basically I tried
> to assert that nobody creates stale edge redirect data that is not
> later consumed or cleared.  Happens to be too optimistic :/

Maybe so, but it looks like the edge_var_redirect_map is still suspect here. On 
the ~~28th call to loop_version, from tree_unswitch_loop, the call to 
lv_flush_pending_stmts executes (tree-cfg.c flush_pending_stmts):

            def = redirect_edge_var_map_def (vm);
            add_phi_arg (phi, def, e, redirect_edge_var_map_location(vm));

and BLOCK_LOCATION (redirect_edge_var_map_location(vm)) is

<<invalid tree code> 0x7fb7704a80 side-effects addressable asm_written used 
protected static visited tree_0 tree_2 tree_5>

so yeah, next question, how'd that get there...

A.

Re: [PATCH] Fix PR68067

[Alan Lawrence]

On 27/11/15 15:07, Alan Lawrence wrote:
> On 23/11/15 09:43, Richard Biener wrote:
>> On Fri, 20 Nov 2015, Alan Lawrence wrote:
>>
>>> ...the asserts
>>> you suggested in (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D68117#c27)...
>  >>
>>> So I have to ask, how sure are you that those assertions are(/should
>>> be!) "correct"? :)
>>
>> Ideally they should be correct but they happen to be not (and I think
>> the intent was that this should be harmless).  Basically I tried
>> to assert that nobody creates stale edge redirect data that is not
>> later consumed or cleared.  Happens to be too optimistic :/
>
> Maybe so, but it looks like the edge_var_redirect_map is still suspect here. On
> the ~~28th call to loop_version, from tree_unswitch_loop, the call to
> lv_flush_pending_stmts executes (tree-cfg.c flush_pending_stmts):
>
>             def = redirect_edge_var_map_def (vm);
>             add_phi_arg (phi, def, e, redirect_edge_var_map_location(vm));
>
> and BLOCK_LOCATION (redirect_edge_var_map_location(vm)) is
>
> <<invalid tree code> 0x7fb7704a80 side-effects addressable asm_written used
> protected static visited tree_0 tree_2 tree_5>
>
> so yeah, next question, how'd that get there...
>
> A.

Well, pass_dominator::execute calls redirect_edge_var_map with that edge 
pointer, at which time the edge is from from 32 (0x7fb79cc6e8) to block 20 
(0x7fb7485e38), and locus is 2147483884; and then again, with locus 0.

With no intervening calls to redirect_edge_var_map_clear for that edge, 
loop_version's call to flush_pending_statements then reads 
redirect_edge_var_map_vector for that edge pointer - which is now an edge from 
block 126 (0x7fb7485af8) to 117 (0x7fb74856e8). It sees those locations 
(2147483884 and 0)...

Clearing the edge redirect map at the end of pass_dominator fixes the ICE (as 
would clearing it at the end of each stage, or clearing it at the beginning of 
loop_unswitch, I guess).

I'll post a patch after more testing, but obviously I'm keen to hear if there 
are obvious problems with the approach?

And coming up with a testcase, well, heh - this broke because of identical 
pointers to structures allocated at different times, with intervening 
free...ideas welcome of course!

--Alan

Re: [PATCH] Fix PR68067

[Richard Biener]

On Fri, 27 Nov 2015, Alan Lawrence wrote:

> On 27/11/15 15:07, Alan Lawrence wrote:
> > On 23/11/15 09:43, Richard Biener wrote:
> > > On Fri, 20 Nov 2015, Alan Lawrence wrote:
> > > 
> > > > ...the asserts
> > > > you suggested in
> > > > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D68117#c27)...
> >  >>
> > > > So I have to ask, how sure are you that those assertions are(/should
> > > > be!) "correct"? :)
> > > 
> > > Ideally they should be correct but they happen to be not (and I think
> > > the intent was that this should be harmless).  Basically I tried
> > > to assert that nobody creates stale edge redirect data that is not
> > > later consumed or cleared.  Happens to be too optimistic :/
> > 
> > Maybe so, but it looks like the edge_var_redirect_map is still suspect here.
> > On
> > the ~~28th call to loop_version, from tree_unswitch_loop, the call to
> > lv_flush_pending_stmts executes (tree-cfg.c flush_pending_stmts):
> > 
> >             def = redirect_edge_var_map_def (vm);
> >             add_phi_arg (phi, def, e, redirect_edge_var_map_location(vm));
> > 
> > and BLOCK_LOCATION (redirect_edge_var_map_location(vm)) is
> > 
> > <<invalid tree code> 0x7fb7704a80 side-effects addressable asm_written used
> > protected static visited tree_0 tree_2 tree_5>
> > 
> > so yeah, next question, how'd that get there...
> > 
> > A.
> 
> Well, pass_dominator::execute calls redirect_edge_var_map with that edge
> pointer, at which time the edge is from from 32 (0x7fb79cc6e8) to block 20
> (0x7fb7485e38), and locus is 2147483884; and then again, with locus 0.
> 
> With no intervening calls to redirect_edge_var_map_clear for that edge,
> loop_version's call to flush_pending_statements then reads
> redirect_edge_var_map_vector for that edge pointer - which is now an edge from
> block 126 (0x7fb7485af8) to 117 (0x7fb74856e8). It sees those locations
> (2147483884 and 0)...
> 
> Clearing the edge redirect map at the end of pass_dominator fixes the ICE (as
> would clearing it at the end of each stage, or clearing it at the beginning of
> loop_unswitch, I guess).
> 
> I'll post a patch after more testing, but obviously I'm keen to hear if there
> are obvious problems with the approach?
> 
> And coming up with a testcase, well, heh - this broke because of identical
> pointers to structures allocated at different times, with intervening
> free...ideas welcome of course!

Yeah.  I've pondered with clearing the hashmap after each pass
(and hope no IPA pass would redirect edges).  Or even more aggressive,
clear the hashmap as well when we do set_cfun ().

Maybe you can try that?

And no, I don't think any pass expects this stuff to be live across
passes.

Richard.

Re: [PATCH] Fix PR68067

[Jeff Law]

On 11/30/2015 01:42 AM, Richard Biener wrote:

> Yeah.  I've pondered with clearing the hashmap after each pass
> (and hope no IPA pass would redirect edges).  Or even more aggressive,
> clear the hashmap as well when we do set_cfun ().
>
> Maybe you can try that?
>
> And no, I don't think any pass expects this stuff to be live across
> passes.
I'd argue that any pass that expects this stuff to be live across a pass 
is fundamentally broken.

jeff