This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[4.5 C] Fix bugs with constant expressions changes


I've applied this patch to c-4_5-branch to fix some bugs arising from
my constant expressions changes.  Bootstrapped with no regressions on
i686-pc-linux-gnu.

This originates in observing some -Wsign-compare warnings (errors with
-Werror) when bootstrapping the branch on sparc-sun-solaris2.8.  Those
I investigated arose from comparisons of unsigned quantities with
BITS_PER_WORD, a macro whose expansion could only be deduced to be
nonnegative after folding.

The basic fix is thus to fold the operands involved in such warnings
before warning.  (If most folding leaves the front ends altogether to
be run on GIMPLE, the warning code and the tests for what is
nonnegative may need to become smarter to handle unfolded
expressions.  It might also be possible to represent the implicit
conversions unambiguously in the front end's IR and do the warnings at
the time of later recursive folding.  But earlier folding in these
cases is the simplest solution in the context of the constant
expressions changes.)

When folding early, it's necessary to avoid the result of folding
being wrongly treated as constant, which can involve inserting a
C_MAYBE_CONST_EXPR to mark it non-constant, and to avoid overheads
from folding the same expressions many times in certain code it's also
desirable to avoid the previously folded tree being folded again,
which can also be achieved with a C_MAYBE_CONST_EXPR.  (Some other
cases do not need such an expression inserted; e.g., c_fully_fold
knows the RHS of a MODIFY_EXPR was always fully folded and does not
need folding again, and a MODIFY_EXPR cannot be constant.)

So this leads to C_MAYBE_CONST_EXPRs being generated in places they
were not previously.  This shows up certain cases where they could be
generated in expressions that did not get subsequently folded.  One
such case was function designators, a clear bug for which I've added a
test that would have failed before this patch (without the extra
C_MAYBE_CONST_EXPRs newly added with this patch).  Another group was
convert_for_assignment converting to bool; the value being converted
was folded before calling convert_for_assignment (this being needed
for some diagnostic purposes) with the result not being folded.  For
this, and a further group involving ObjC, telling build_binary_op when
it is being called at such late stages so it knows it need not create
C_MAYBE_CONST_EXPRs seemed the simplest approach.  Again, some
failures would occur without the changes to add more such
C_MAYBE_CONST_EXPRs and some depended on the extra such expressions.

2008-11-09  Joseph Myers  <joseph@codesourcery.com>

	* c-tree.h (in_late_binary_op): Declare.
	* c-typeck.c (in_late_binary_op): New.
	(note_integer_operands): Create overflowed INTEGER_CST if possible
	and in_late_binary_op.
	(build_function_call): Fold the function designator.
	(build_conditional_expr): Fold operands before possibly doing
	-Wsign-compare warnings.
	(convert_for_assignment): Set in_late_binary_op for conversions to
	boolean.
	(build_binary_op): Fold operands for -Wsign-compare warnings
	unless in_late_binary_op.  Avoid creating C_MAYBE_CONST_EXPR if
	in_late_binary_op.

objc:
2008-11-09  Joseph Myers  <joseph@codesourcery.com>

	* objc-act.c (objc_finish_try_stmt): Set in_late_binary_op.

testsuite:
2008-11-09  Joseph Myers  <joseph@codesourcery.com>

	* gcc.c-torture/compile/20081108-1.c,
	gcc.c-torture/compile/20081108-2.c,
	gcc.c-torture/compile/20081108-3.c, gcc.dg/compare10.c: New tests.
	* gcc.dg/c99-const-expr-7.c: Add another test.

Index: c-tree.h
===================================================================
--- c-tree.h	(revision 141697)
+++ c-tree.h	(working copy)
@@ -555,6 +555,7 @@ extern bool c_vla_unspec_p (tree x, tree
 			  ((VOLATILE_P) ? TYPE_QUAL_VOLATILE : 0))
 
 /* in c-typeck.c */
+extern bool in_late_binary_op;
 extern int in_alignof;
 extern int in_sizeof;
 extern int in_typeof;
Index: c-typeck.c
===================================================================
--- c-typeck.c	(revision 141697)
+++ c-typeck.c	(working copy)
@@ -54,6 +54,13 @@ enum impl_conv {
   ic_return
 };
 
+/* Whether we are building a boolean conversion inside
+   convert_for_assignment, or some other late binary operation.  If
+   build_binary_op is called (from code shared with C++) in this case,
+   then the operands have already been folded and the result will not
+   be folded again, so C_MAYBE_CONST_EXPR should not be generated.  */
+bool in_late_binary_op;
+
 /* The level of nesting inside "__alignof__".  */
 int in_alignof;
 
@@ -485,14 +492,23 @@ c_fully_fold_internal (tree expr, bool i
 
 /* EXPR may appear in an unevaluated part of an integer constant
    expression, but not in an evaluated part.  Wrap it in a
-   C_MAYBE_CONST_EXPR.  */
+   C_MAYBE_CONST_EXPR, or mark it with TREE_OVERFLOW if it is just an
+   INTEGER_CST and we cannot create a C_MAYBE_CONST_EXPR.  */
 
 static tree
 note_integer_operands (tree expr)
 {
   tree ret;
-  ret = build2 (C_MAYBE_CONST_EXPR, TREE_TYPE (expr), NULL_TREE, expr);
-  C_MAYBE_CONST_EXPR_INT_OPERANDS (ret) = 1;
+  if (TREE_CODE (expr) == INTEGER_CST && in_late_binary_op)
+    {
+      ret = copy_node (expr);
+      TREE_OVERFLOW (ret) = 1;
+    }
+  else
+    {
+      ret = build2 (C_MAYBE_CONST_EXPR, TREE_TYPE (expr), NULL_TREE, expr);
+      C_MAYBE_CONST_EXPR_INT_OPERANDS (ret) = 1;
+    }
   return ret;
 }
 
@@ -2761,6 +2777,8 @@ build_function_call (tree function, tree
      expressions, like those used for ObjC messenger dispatches.  */
   function = objc_rewrite_function_call (function, params);
 
+  function = c_fully_fold (function, false, NULL);
+
   fntype = TREE_TYPE (function);
 
   if (TREE_CODE (fntype) == ERROR_MARK)
@@ -3987,7 +4005,7 @@ build_conditional_expr (tree ifexp, bool
 	 and later code won't know it used to be different.
 	 Do this check on the original types, so that explicit casts
 	 will be considered, but default promotions won't.  */
-      if (warn_sign_compare && !skip_evaluation)
+      if (!skip_evaluation)
 	{
 	  int unsigned_op1 = TYPE_UNSIGNED (TREE_TYPE (orig_op1));
 	  int unsigned_op2 = TYPE_UNSIGNED (TREE_TYPE (orig_op2));
@@ -4001,16 +4019,47 @@ build_conditional_expr (tree ifexp, bool
 		 all the values of the unsigned type.  */
 	      if (!TYPE_UNSIGNED (result_type))
 		/* OK */;
-	      /* Do not warn if the signed quantity is an unsuffixed
-		 integer literal (or some static constant expression
-		 involving such literals) and it is non-negative.  */
-	      else if ((unsigned_op2
-			&& tree_expr_nonnegative_warnv_p (op1, &ovf))
-		       || (unsigned_op1
-			   && tree_expr_nonnegative_warnv_p (op2, &ovf)))
-		/* OK */;
 	      else
-		warning (OPT_Wsign_compare, "signed and unsigned type in conditional expression");
+		{
+		  bool op1_maybe_const = true;
+		  bool op2_maybe_const = true;
+
+		  /* Do not warn if the signed quantity is an
+		     unsuffixed integer literal (or some static
+		     constant expression involving such literals) and
+		     it is non-negative.  This warning requires the
+		     operands to be folded for best results, so do
+		     that folding in this case even without
+		     warn_sign_compare to avoid warning options
+		     possibly affecting code generation.  */
+		  op1 = c_fully_fold (op1, require_constant_value,
+				      &op1_maybe_const);
+		  op2 = c_fully_fold (op2, require_constant_value,
+				      &op2_maybe_const);
+
+		  if (warn_sign_compare)
+		    {
+		      if ((unsigned_op2
+			   && tree_expr_nonnegative_warnv_p (op1, &ovf))
+			  || (unsigned_op1
+			      && tree_expr_nonnegative_warnv_p (op2, &ovf)))
+			/* OK */;
+		      else
+			warning (OPT_Wsign_compare, "signed and unsigned type in conditional expression");
+		    }
+		  if (!op1_maybe_const || TREE_CODE (op1) != INTEGER_CST)
+		    {
+		      op1 = build2 (C_MAYBE_CONST_EXPR, TREE_TYPE (op1),
+				    NULL, op1);
+		      C_MAYBE_CONST_EXPR_NON_CONST (op1) = !op1_maybe_const;
+		    }
+		  if (!op2_maybe_const || TREE_CODE (op2) != INTEGER_CST)
+		    {
+		      op2 = build2 (C_MAYBE_CONST_EXPR, TREE_TYPE (op2),
+				    NULL, op2);
+		      C_MAYBE_CONST_EXPR_NON_CONST (op2) = !op2_maybe_const;
+		    }
+		}
 	    }
 	}
     }
@@ -4752,7 +4801,16 @@ convert_for_assignment (tree type, tree 
 	       || coder == FIXED_POINT_TYPE
 	       || coder == ENUMERAL_TYPE || coder == COMPLEX_TYPE
 	       || coder == BOOLEAN_TYPE))
-    return convert_and_check (type, orig_rhs);
+    {
+      tree ret;
+      bool save = in_late_binary_op;
+      if (codel == BOOLEAN_TYPE)
+	in_late_binary_op = true;
+      ret = convert_and_check (type, orig_rhs);
+      if (codel == BOOLEAN_TYPE)
+	in_late_binary_op = save;
+      return ret;
+    }
 
   /* Aggregates in different TUs might need conversion.  */
   if ((codel == RECORD_TYPE || codel == UNION_TYPE)
@@ -5067,7 +5125,14 @@ convert_for_assignment (tree type, tree 
       return convert (type, rhs);
     }
   else if (codel == BOOLEAN_TYPE && coder == POINTER_TYPE)
-    return convert (type, rhs);
+    {
+      tree ret;
+      bool save = in_late_binary_op;
+      in_late_binary_op = true;
+      ret = convert (type, rhs);
+      in_late_binary_op = save;
+      return ret;
+    }
 
   switch (errtype)
     {
@@ -9315,10 +9380,54 @@ build_binary_op (location_t location, en
 	  converted = 1;
 	  resultcode = xresultcode;
 
-	  if (warn_sign_compare && !skip_evaluation)
-            {
-              warn_for_sign_compare (location, orig_op0, orig_op1, op0, op1, 
-                                     result_type, resultcode);
+	  if (!skip_evaluation)
+	    {
+	      bool op0_maybe_const = true;
+	      bool op1_maybe_const = true;
+	      tree orig_op0_folded, orig_op1_folded;
+
+	      if (in_late_binary_op)
+		{
+		  orig_op0_folded = orig_op0;
+		  orig_op1_folded = orig_op1;
+		}
+	      else
+		{
+		  /* Fold for the sake of possible warnings, as in
+		     build_conditional_expr.  This requires the
+		     "original" values to be folded, not just op0 and
+		     op1.  */
+		  op0 = c_fully_fold (op0, require_constant_value,
+				      &op0_maybe_const);
+		  op1 = c_fully_fold (op1, require_constant_value,
+				      &op1_maybe_const);
+		  orig_op0_folded = c_fully_fold (orig_op0,
+						  require_constant_value,
+						  NULL);
+		  orig_op1_folded = c_fully_fold (orig_op1,
+						  require_constant_value,
+						  NULL);
+		}
+
+	      if (warn_sign_compare)
+		warn_for_sign_compare (location, orig_op0_folded,
+				       orig_op1_folded, op0, op1,
+				       result_type, resultcode);
+	      if (!in_late_binary_op)
+		{
+		  if (!op0_maybe_const || TREE_CODE (op0) != INTEGER_CST)
+		    {
+		      op0 = build2 (C_MAYBE_CONST_EXPR, TREE_TYPE (op0),
+				    NULL, op0);
+		      C_MAYBE_CONST_EXPR_NON_CONST (op0) = !op0_maybe_const;
+		    }
+		  if (!op1_maybe_const || TREE_CODE (op1) != INTEGER_CST)
+		    {
+		      op1 = build2 (C_MAYBE_CONST_EXPR, TREE_TYPE (op1),
+				    NULL, op1);
+		      C_MAYBE_CONST_EXPR_NON_CONST (op1) = !op1_maybe_const;
+		    }
+		}
 	    }
 	}
     }
@@ -9374,7 +9483,8 @@ build_binary_op (location_t location, en
     ret = (int_operands
 	   ? note_integer_operands (ret)
 	   : build1 (NOP_EXPR, TREE_TYPE (ret), ret));
-  else if (TREE_CODE (ret) != INTEGER_CST && int_operands)
+  else if (TREE_CODE (ret) != INTEGER_CST && int_operands
+	   && !in_late_binary_op)
     ret = note_integer_operands (ret);
   if (real_result_type)
     ret = build1 (EXCESS_PRECISION_EXPR, real_result_type, ret);
Index: objc/objc-act.c
===================================================================
--- objc/objc-act.c	(revision 141697)
+++ objc/objc-act.c	(working copy)
@@ -3885,12 +3885,15 @@ objc_finish_try_stmt (void)
   /* If we're doing Darwin setjmp exceptions, build the big nasty.  */
   if (flag_objc_sjlj_exceptions)
     {
+      bool save = in_late_binary_op;
+      in_late_binary_op = true;
       if (!cur_try_context->finally_body)
 	{
 	  cur_try_context->finally_locus = input_location;
 	  cur_try_context->end_finally_locus = input_location;
 	}
       stmt = next_sjlj_build_try_catch_finally ();
+      in_late_binary_op = save;
     }
   else
     {
Index: testsuite/gcc.c-torture/compile/20081108-1.c
===================================================================
--- testsuite/gcc.c-torture/compile/20081108-1.c	(revision 0)
+++ testsuite/gcc.c-torture/compile/20081108-1.c	(revision 0)
@@ -0,0 +1,11 @@
+/* Test function call with function designator involving VLA
+   side-effects does not lead to an ICE.  */
+
+void f (void);
+void g (void);
+
+void
+h (int a, void *b)
+{
+  ((void *)(int (*)[++a])b ? f : g) ();
+}
Index: testsuite/gcc.c-torture/compile/20081108-2.c
===================================================================
--- testsuite/gcc.c-torture/compile/20081108-2.c	(revision 0)
+++ testsuite/gcc.c-torture/compile/20081108-2.c	(revision 0)
@@ -0,0 +1,4 @@
+/* Test boolean conversion as part of returning unsigned value does
+   not lead to an ICE.  */
+
+_Bool f(unsigned a) { return a & 1; }
Index: testsuite/gcc.c-torture/compile/20081108-3.c
===================================================================
--- testsuite/gcc.c-torture/compile/20081108-3.c	(revision 0)
+++ testsuite/gcc.c-torture/compile/20081108-3.c	(revision 0)
@@ -0,0 +1,4 @@
+/* Test boolean conversion of an overflowing integer return value does
+   not lead to an ICE.  */
+
+_Bool f(void) { return __INT_MAX__ + 1; }
Index: testsuite/gcc.dg/compare10.c
===================================================================
--- testsuite/gcc.dg/compare10.c	(revision 0)
+++ testsuite/gcc.dg/compare10.c	(revision 0)
@@ -0,0 +1,16 @@
+/* Test for bogus -Wsign-compare warnings that appeared when not
+   folding operands before warning.  */
+/* { dg-do compile } */
+/* { dg-options "-Wsign-compare" } */
+
+int
+test_compare (int a, unsigned b)
+{
+  return (b > 8 * (a ? 4 : 8));
+}
+
+unsigned int
+test_conditional (int a, unsigned b, int c)
+{
+  return (c ? b : 8 * (a ? 4 : 8));
+}
Index: testsuite/gcc.dg/c99-const-expr-7.c
===================================================================
--- testsuite/gcc.dg/c99-const-expr-7.c	(revision 141697)
+++ testsuite/gcc.dg/c99-const-expr-7.c	(working copy)
@@ -38,3 +38,6 @@ int j[1] = { DBL_MAX }; /* { dg-warning 
 
 int array[2] = { [0 * (INT_MAX + 1)] = 0 }; /* { dg-warning "integer overflow in expression" } */
 /* { dg-error "overflow in constant expression" "constant" { target *-*-* } 39 } */
+
+_Bool k = INT_MAX + 1; /* { dg-warning "integer overflow in expression" } */
+/* { dg-error "overflow in constant expression" "constant" { target *-*-* } 42 } */

-- 
Joseph S. Myers
joseph@codesourcery.com


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]