Re: Updating fixed-point support for new C constant expression handling

[Richard Sandiford <rdsandiford at googlemail dot com>]

"Joseph S. Myers" <joseph@codesourcery.com> writes:
> On Tue, 30 Jun 2009, Richard Sandiford wrote:
>> The 4.5 C constant expressions patch:
>> 
>>     http://gcc.gnu.org/ml/gcc-patches/2008-10/msg01061.html
>> 
>> stopped us from emitting the warnings expected by gcc.dg/fixed-point/addsub.c.
>> We used to handle the warnings in parser_build_binary_op, but now that
>> fixed-point constant folding is delayed until c_fully_fold, we no longer
>> know at that stage whether overflow has taken place.
>> 
>> What's the correct fix here?  Should build_binary_op treat fixed-point
>> constants like integer constants?  Should c_fully_fold emit warnings in
>> some cases?  Should we just drop the warnings?  Or something else?
>
> If the warnings are because this is a static initializer, 
> constant_expression_warning should deal with them and it should be 
> sufficient for TREE_OVERFLOW to be set.  (But such warnings are 
> deliberately conditioned on -pedantic.)
>
> If the warnings are simply a warning from overflow_warning desired whether 
> or not this is a static initializer, then the appropriate point to give 
> them would be the point of lowering - that is, c_fully_fold.
>
> Since TR 18037 says that fixed-point constants (like floating constants) 
> in integer constant expressions must be the immediate operands of casts, 
> folding expressions of fixed point constants into a single FIXED_CST the 
> way expressions of integer constants are folded into a single INTEGER_CST 
> would be inappropriate at parse time; it would lead to expressions 
> involving some more complicated expression of fixed-point constants, cast 
> to an integer type, wrongly being accepted as integer constant 
> expressions.
>
> In general it's preferable for the trees created at parse time to 
> correspond more closely to the source code, meaning that diagnostic checks 
> requiring lowering should happen at the time of lowering (or be changed 
> not to rely on lowering) if possible.

OK, how's this?  I tried to check all calls to c_fully_fold to
make sure they handled c_inhibit_evaluation_warnings appropriately.
As well as fixing the fixed-point problems, the patch restores the
4.4 behaviour for the attached testcase, but I'll leave you to tell
me whether that's a good thing. ;)  The testcase includes an example
of why each new c_inhibit_evaluation_warnings assignment is needed.

Bootstrapped & regression-tested on x86_64-linux-gnu.  Also
regression-tested on mipsisa64-elf.

Richard


gcc/
	* c-common.c (c_fully_fold_internal): Issue a warning if a binary
	operation overflows.  Likewise non-cast unary arithmetic.
	If one arm of a conditional expression is always taken,
	inhibit evaluation warnings for the other arm.  Likewise inhibit
	evaluation warnings for the second && or || operand if the first
	operand is enough to determine the result.
	* c-typeck.c (build_conditional_expr): Apply the same inhibition
	rules here.
	(build_binary_op): Prevent duplicate evaluation warnings.

gcc/testsuite/
	* gcc.dg/overflow-warn-8.c: New test.

Index: gcc/c-common.c
===================================================================
--- gcc/c-common.c	2009-08-09 09:43:26.000000000 +0100
+++ gcc/c-common.c	2009-08-09 11:00:07.000000000 +0100
@@ -1133,6 +1133,7 @@ c_fully_fold_internal (tree expr, bool i
   bool op0_const = true, op1_const = true, op2_const = true;
   bool op0_const_self = true, op1_const_self = true, op2_const_self = true;
   bool nowarning = TREE_NO_WARNING (expr);
+  int unused_p;
 
   /* This function is not relevant to C++ because C++ folds while
      parsing, and may need changes to be correct for C++ when C++
@@ -1308,6 +1309,10 @@ c_fully_fold_internal (tree expr, bool i
 	  : fold_build2_loc (loc, code, TREE_TYPE (expr), op0, op1);
       else
 	ret = fold (expr);
+      if (TREE_OVERFLOW_P (ret)
+	  && !TREE_OVERFLOW_P (op0)
+	  && !TREE_OVERFLOW_P (op1))
+	overflow_warning (EXPR_LOCATION (expr), ret);
       goto out;
 
     case INDIRECT_REF:
@@ -1342,6 +1347,20 @@ c_fully_fold_internal (tree expr, bool i
 	  TREE_SIDE_EFFECTS (ret) = TREE_SIDE_EFFECTS (expr);
 	  TREE_THIS_VOLATILE (ret) = TREE_THIS_VOLATILE (expr);
 	}
+      switch (code)
+	{
+	case FIX_TRUNC_EXPR:
+	case FLOAT_EXPR:
+	CASE_CONVERT:
+	  /* Don't warn about explicit conversions.  We will already
+	     have warned about suspect implicit conversions.  */
+	  break;
+
+	default:
+	  if (TREE_OVERFLOW_P (ret) && !TREE_OVERFLOW_P (op0))
+	    overflow_warning (EXPR_LOCATION (expr), ret);
+	  break;
+	}
       goto out;
 
     case TRUTH_ANDIF_EXPR:
@@ -1351,7 +1370,14 @@ c_fully_fold_internal (tree expr, bool i
       orig_op0 = op0 = TREE_OPERAND (expr, 0);
       orig_op1 = op1 = TREE_OPERAND (expr, 1);
       op0 = c_fully_fold_internal (op0, in_init, &op0_const, &op0_const_self);
+
+      unused_p = (op0 == (code == TRUTH_ANDIF_EXPR
+			  ? truthvalue_false_node
+			  : truthvalue_true_node));
+      c_inhibit_evaluation_warnings += unused_p;
       op1 = c_fully_fold_internal (op1, in_init, &op1_const, &op1_const_self);
+      c_inhibit_evaluation_warnings -= unused_p;
+
       if (op0 != orig_op0 || op1 != orig_op1 || in_init)
 	ret = in_init
 	  ? fold_build2_initializer_loc (loc, code, TREE_TYPE (expr), op0, op1)
@@ -1380,8 +1406,15 @@ c_fully_fold_internal (tree expr, bool i
       orig_op1 = op1 = TREE_OPERAND (expr, 1);
       orig_op2 = op2 = TREE_OPERAND (expr, 2);
       op0 = c_fully_fold_internal (op0, in_init, &op0_const, &op0_const_self);
+
+      c_inhibit_evaluation_warnings += (op0 == truthvalue_false_node);
       op1 = c_fully_fold_internal (op1, in_init, &op1_const, &op1_const_self);
+      c_inhibit_evaluation_warnings -= (op0 == truthvalue_false_node);
+
+      c_inhibit_evaluation_warnings += (op0 == truthvalue_true_node);
       op2 = c_fully_fold_internal (op2, in_init, &op2_const, &op2_const_self);
+      c_inhibit_evaluation_warnings -= (op0 == truthvalue_true_node);
+
       if (op0 != orig_op0 || op1 != orig_op1 || op2 != orig_op2)
 	ret = fold_build3_loc (loc, code, TREE_TYPE (expr), op0, op1, op2);
       else
Index: gcc/c-typeck.c
===================================================================
--- gcc/c-typeck.c	2009-08-09 09:43:26.000000000 +0100
+++ gcc/c-typeck.c	2009-08-09 10:53:55.000000000 +0100
@@ -3912,10 +3912,19 @@ build_conditional_expr (location_t colon
 		     that folding in this case even without
 		     warn_sign_compare to avoid warning options
 		     possibly affecting code generation.  */
+		  c_inhibit_evaluation_warnings
+		    += (ifexp == truthvalue_false_node);
 		  op1 = c_fully_fold (op1, require_constant_value,
 				      &op1_maybe_const);
+		  c_inhibit_evaluation_warnings
+		    -= (ifexp == truthvalue_false_node);
+
+		  c_inhibit_evaluation_warnings
+		    += (ifexp == truthvalue_true_node);
 		  op2 = c_fully_fold (op2, require_constant_value,
 				      &op2_maybe_const);
+		  c_inhibit_evaluation_warnings
+		    -= (ifexp == truthvalue_true_node);
 
 		  if (warn_sign_compare)
 		    {
@@ -9509,10 +9518,12 @@ build_binary_op (location_t location, en
 		     build_conditional_expr.  This requires the
 		     "original" values to be folded, not just op0 and
 		     op1.  */
+		  c_inhibit_evaluation_warnings++;
 		  op0 = c_fully_fold (op0, require_constant_value,
 				      &op0_maybe_const);
 		  op1 = c_fully_fold (op1, require_constant_value,
 				      &op1_maybe_const);
+		  c_inhibit_evaluation_warnings--;
 		  orig_op0_folded = c_fully_fold (orig_op0,
 						  require_constant_value,
 						  NULL);
Index: gcc/testsuite/gcc.dg/overflow-warn-8.c
===================================================================
--- /dev/null	2009-08-09 10:17:18.501841211 +0100
+++ gcc/testsuite/gcc.dg/overflow-warn-8.c	2009-08-09 10:58:23.000000000 +0100
@@ -0,0 +1,23 @@
+#include <limits.h>
+
+void foo (int j)
+{
+  int i1 = (int)(double)1.0 + INT_MAX; /* { dg-warning "integer overflow" } */
+  int i2 = (int)(double)1 + INT_MAX; /* { dg-warning "integer overflow" } */
+  int i3 = 1 + INT_MAX; /* { dg-warning "integer overflow" } */
+  int i4 = +1 + INT_MAX; /* { dg-warning "integer overflow" } */
+  int i5 = (int)((double)1.0 + INT_MAX);
+  int i6 = (double)1.0 + INT_MAX; /* { dg-warning "overflow in implicit constant" } */
+  int i7 = 0 ? (int)(double)1.0 + INT_MAX : 1;
+  int i8 = 1 ? 1 : (int)(double)1.0 + INT_MAX;
+  int i9 = j ? (int)(double)1.0 + INT_MAX : 1; /* { dg-warning "integer overflow" } */
+  unsigned int i10 = 0 ? (int)(double)1.0 + INT_MAX : 9U;
+  unsigned int i11 = 1 ? 9U : (int)(double)1.0 + INT_MAX;
+  unsigned int i12 = j ? (int)(double)1.0 + INT_MAX : 9U; /* { dg-warning "integer overflow" } */
+  int i13 = 1 || (int)(double)1.0 + INT_MAX < 0;
+  int i14 = 0 && (int)(double)1.0 + INT_MAX < 0;
+  int i15 = 0 || (int)(double)1.0 + INT_MAX < 0; /* { dg-warning "integer overflow" } */
+  int i16 = 1 && (int)(double)1.0 + INT_MAX < 0; /* { dg-warning "integer overflow" } */
+  int i17 = j || (int)(double)1.0 + INT_MAX < 0; /* { dg-warning "integer overflow" } */
+  int i18 = j && (int)(double)1.0 + INT_MAX < 0; /* { dg-warning "integer overflow" } */
+}