diff mbox

Reject boolean/enum types in last arg of __builtin_*_overflow_p

Message ID 20160614101602.GB7387@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek June 14, 2016, 10:16 a.m. UTC
On Tue, Jun 14, 2016 at 10:33:59AM +0200, Jakub Jelinek wrote:
> On Fri, Jun 10, 2016 at 01:28:55PM -0600, Martin Sebor wrote:
> > IMO, the right semantics for bool are to match what the standards
> > specify (i.e., no overflow, and the result is a logical OR of the
> > operands).
> 
> ???  It is far from that.
> The documentation says that say for __builtin_add_overflow (x, y, &z), you compute
> infinite precision signed result of x + y, cast the result to __typeof (z),
> extend again to infinite precision signed result and compare to the value
> before casting.  So, if z is bool/_Bool, then this means
> signed_intNNN_t t = (signed_intNNN_t) x + (signed_intNNN_t) y;
> z = (__typeof (z)) t;
> ovf = (signed_intNNN_t) z != t;
> 
> But, unlike normal signed or unsigned integral types or char, cast to
> bool/_Bool is special, it is actually a comparison != 0.
> So, I'd say if we want to support arith overflow to bool/_Bool, it should be
> above with:
> signed_intNNN_t t = (signed_intNNN_t) x + (signed_intNNN_t) y;
> z = t != 0;
> ovf = (signed_intNNN_t) z != t;
> so in the end, if we'd use
> movb x, r
> addb y, r
> seto ovf
> to compute whether the result overflowed 8-bits, we'd need to
> or in (r & 254) != 0 into the ovf and then or in ovf into r.
> That means changes everywhere where we handle the overflow builtins. :(

Here is an untested patch for that.  Except that the middle-end considers
conversions between BOOLEAN_TYPE and single bit unsigned type as useless,
so in theory this can't work well, and in practice only if we are lucky
enough (plus it generates terrible code right now), so we'd probably need
to come up with a different way of expressing whether the internal fn
should have a bool/_Bool-ish behavior or not (optional 3rd argument or
something ugly like that).  Plus add lots of testcases to cover the weirdo
cases.  Is it really worth it, even when we don't want to support overflow
into enumeration type and thus will not cover all integral types anyway?

Small parts of the patch will be needed even if we reject bool/_Bool,
the internal-fn.c change except for the BOOLEAN_TYPE handling (because
with __builtin_*_overflow_p we can end up with e.g. bitfield types in
there).



	Jakub

Comments

Martin Sebor June 14, 2016, 5:13 p.m. UTC | #1
> Here is an untested patch for that.  Except that the middle-end considers
> conversions between BOOLEAN_TYPE and single bit unsigned type as useless,
> so in theory this can't work well, and in practice only if we are lucky
> enough (plus it generates terrible code right now), so we'd probably need
> to come up with a different way of expressing whether the internal fn
> should have a bool/_Bool-ish behavior or not (optional 3rd argument or
> something ugly like that).  Plus add lots of testcases to cover the weirdo
> cases.  Is it really worth it, even when we don't want to support overflow
> into enumeration type and thus will not cover all integral types anyway?

If it's cumbersome to get to work I agree that it's not worth
the effort.  Thanks for taking the time to prototype it.

Martin
diff mbox

Patch

--- gcc/cp/constexpr.c.jj	2016-06-08 21:01:25.000000000 +0200
+++ gcc/cp/constexpr.c	2016-06-14 12:06:25.747105145 +0200
@@ -1303,11 +1303,21 @@  cxx_eval_internal_function (const conste
     {
       location_t loc = EXPR_LOC_OR_LOC (t, input_location);
       tree type = TREE_TYPE (TREE_TYPE (t));
-      tree result = fold_binary_loc (loc, opcode, type,
-				     fold_convert_loc (loc, type, arg0),
-				     fold_convert_loc (loc, type, arg1));
+      tree itype = type;
+      if (TREE_CODE (type) == BOOLEAN_TYPE)
+	itype = build_nonstandard_integer_type (1, 1);
+      tree result = fold_binary_loc (loc, opcode, itype,
+				     fold_convert_loc (loc, itype, arg0),
+				     fold_convert_loc (loc, itype, arg1));
       tree ovf
 	= build_int_cst (type, arith_overflowed_p (opcode, type, arg0, arg1));
+      if (TREE_CODE (type) == BOOLEAN_TYPE)
+	{
+	  result = fold_binary_loc (loc, NE_EXPR, type, result,
+				    build_int_cst (itype, 0));
+	  if (integer_onep (ovf))
+	    result = ovf;
+	}
       /* Reset TREE_OVERFLOW to avoid warnings for the overflow.  */
       if (TREE_OVERFLOW (result))
 	TREE_OVERFLOW (result) = 0;
--- gcc/internal-fn.c.jj	2016-06-13 20:45:05.042105674 +0200
+++ gcc/internal-fn.c	2016-06-14 10:17:23.257793021 +0200
@@ -407,7 +407,8 @@  get_min_precision (tree arg, signop sign
 
 /* Helper for expand_*_overflow.  Store RES into the __real__ part
    of TARGET.  If RES has larger MODE than __real__ part of TARGET,
-   set the __imag__ part to 1 if RES doesn't fit into it.  */
+   set the __imag__ part to 1 if RES doesn't fit into it.  Similarly
+   if LHS has smaller precision than its mode.  */
 
 static void
 expand_arith_overflow_result_store (tree lhs, rtx target,
@@ -427,6 +428,40 @@  expand_arith_overflow_result_store (tree
       write_complex_part (target, const1_rtx, true);
       emit_label (done_label);
     }
+  int prec = TYPE_PRECISION (TREE_TYPE (TREE_TYPE (lhs)));
+  int tgtprec = GET_MODE_PRECISION (tgtmode);
+  if (prec < tgtprec)
+    {
+      rtx_code_label *done_label = gen_label_rtx ();
+      int uns = TYPE_UNSIGNED (TREE_TYPE (TREE_TYPE (lhs)));
+      res = lres;
+      if (uns)
+	{
+	  rtx mask
+	    = immed_wide_int_const (wi::shifted_mask (0, prec, false, tgtprec),
+				    tgtmode);
+	  lres = expand_simple_binop (tgtmode, AND, res, mask, NULL_RTX,
+				      true, OPTAB_DIRECT);
+	}
+      else
+	{
+	  lres = expand_shift (LSHIFT_EXPR, tgtmode, res, tgtprec - prec,
+			       NULL_RTX, 1);
+	  lres = expand_shift (RSHIFT_EXPR, tgtmode, lres, tgtprec - prec,
+			       NULL_RTX, 0);
+	}
+      do_compare_rtx_and_jump (res, lres,
+			       EQ, true, tgtmode, NULL_RTX, NULL, done_label,
+			       PROB_VERY_LIKELY);
+      write_complex_part (target, const1_rtx, true);
+      emit_label (done_label);
+    }
+  /* For bool, make sure the result is 1 whenever overflow is 1, because
+     cast to bool is != comparison of the infinite precision result with 0.  */
+  if (TREE_CODE (TREE_TYPE (TREE_TYPE (lhs))) == BOOLEAN_TYPE)
+    lres = expand_simple_binop (tgtmode, IOR, lres,
+				read_complex_part (target, true), NULL_RTX,
+				true, OPTAB_DIRECT);
   write_complex_part (target, lres, false);
 }
 
--- gcc/tree-ssa-dce.c.jj	2016-05-24 16:09:39.000000000 +0200
+++ gcc/tree-ssa-dce.c	2016-06-14 11:25:53.630653255 +0200
@@ -1139,6 +1139,12 @@  maybe_optimize_arith_overflow (gimple_st
   if (lhs == NULL || TREE_CODE (lhs) != SSA_NAME)
     return;
 
+  tree type = TREE_TYPE (TREE_TYPE (lhs));
+  /* Punt on arith overflow to bool, there we always need the overflow
+     computation.  */
+  if (TREE_CODE (type) == BOOLEAN_TYPE)
+    return;
+
   imm_use_iterator imm_iter;
   use_operand_p use_p;
   bool has_debug_uses = false;
@@ -1166,7 +1172,6 @@  maybe_optimize_arith_overflow (gimple_st
   tree arg0 = gimple_call_arg (stmt, 0);
   tree arg1 = gimple_call_arg (stmt, 1);
   location_t loc = gimple_location (stmt);
-  tree type = TREE_TYPE (TREE_TYPE (lhs));
   tree utype = type;
   if (!TYPE_UNSIGNED (type))
     utype = build_nonstandard_integer_type (TYPE_PRECISION (type), 1);
--- gcc/testsuite/c-c++-common/builtin-arith-overflow-1.c.jj	2016-06-10 21:35:57.119137808 +0200
+++ gcc/testsuite/c-c++-common/builtin-arith-overflow-1.c	2016-06-14 09:41:06.030131384 +0200
@@ -118,14 +118,14 @@  generic_wrong_type (int a, int b)
 {
   void *p = 0;
   double d = 0;
-  int x = __builtin_add_overflow (a, b, p);   /* { dg-error "does not have pointer to integer type" } */
-  x += __builtin_sub_overflow (a, b, &p);     /* { dg-error "does not have pointer to integer type" } */
-  x += __builtin_mul_overflow (a, b, &d);     /* { dg-error "does not have pointer to integer type" } */
+  int x = __builtin_add_overflow (a, b, p);   /* { dg-error "does not have pointer to integral type" } */
+  x += __builtin_sub_overflow (a, b, &p);     /* { dg-error "does not have pointer to integral type" } */
+  x += __builtin_mul_overflow (a, b, &d);     /* { dg-error "does not have pointer to integral type" } */
 
   /* Also verify literal arguments.  */
-  x += __builtin_add_overflow (1, 1, p);   /* { dg-error "does not have pointer to integer type" } */
-  x += __builtin_sub_overflow (1, 1, &p);     /* { dg-error "does not have pointer to integer type" } */
-  x += __builtin_mul_overflow (1, 1, &d);     /* { dg-error "does not have pointer to integer type" } */
+  x += __builtin_add_overflow (1, 1, p);   /* { dg-error "does not have pointer to integral type" } */
+  x += __builtin_sub_overflow (1, 1, &p);     /* { dg-error "does not have pointer to integral type" } */
+  x += __builtin_mul_overflow (1, 1, &d);     /* { dg-error "does not have pointer to integral type" } */
   return x;
 }
 
@@ -236,7 +236,7 @@  f3 (float fa, int a, _Complex long int c
   x += __builtin_sub_overflow_p (ca, b, eb);	/* { dg-error "argument 1 in call to function\[^\n\r]*does not have integral type" } */
   x += __builtin_mul_overflow_p (a, fb, bb);	/* { dg-error "argument 2 in call to function\[^\n\r]*does not have integral type" } */
   x += __builtin_add_overflow_p (a, pb, a);	/* { dg-error "argument 2 in call to function\[^\n\r]*does not have integral type" } */
-  x += __builtin_sub_overflow_p (a, eb, eb);
+  x += __builtin_sub_overflow_p (a, eb, eb);	/* { dg-error "argument 3 in call to function\[^\n\r]*has enumerated type" } */
   x += __builtin_mul_overflow_p (a, bb, bb);
   x += __builtin_add_overflow_p (a, b, fa);	/* { dg-error "argument 3 in call to function\[^\n\r]*does not have integral type" } */
   x += __builtin_sub_overflow_p (a, b, ca);	/* { dg-error "argument 3 in call to function\[^\n\r]*does not have integral type" } */
@@ -247,11 +247,11 @@  f3 (float fa, int a, _Complex long int c
 int
 f4 (float *fp, double *dp, _Complex int *cp, enum E *ep, bool *bp, long long int *llp)
 {
-  int x = __builtin_add_overflow (1, 2, fp);	/* { dg-error "argument 3 in call to function\[^\n\r]*does not have pointer to integer type" } */
-  x += __builtin_sub_overflow (1, 2, dp);	/* { dg-error "argument 3 in call to function\[^\n\r]*does not have pointer to integer type" } */
-  x += __builtin_mul_overflow (1, 2, cp);	/* { dg-error "argument 3 in call to function\[^\n\r]*does not have pointer to integer type" } */
-  x += __builtin_add_overflow (1, 2, ep);	/* { dg-error "argument 3 in call to function\[^\n\r]*does not have pointer to integer type" } */
-  x += __builtin_sub_overflow (1, 2, bp);	/* { dg-error "argument 3 in call to function\[^\n\r]*does not have pointer to integer type" } */
+  int x = __builtin_add_overflow (1, 2, fp);	/* { dg-error "argument 3 in call to function\[^\n\r]*does not have pointer to integral type" } */
+  x += __builtin_sub_overflow (1, 2, dp);	/* { dg-error "argument 3 in call to function\[^\n\r]*does not have pointer to integral type" } */
+  x += __builtin_mul_overflow (1, 2, cp);	/* { dg-error "argument 3 in call to function\[^\n\r]*does not have pointer to integral type" } */
+  x += __builtin_add_overflow (1, 2, ep);	/* { dg-error "argument 3 in call to function\[^\n\r]*has pointer to enumerated type" } */
+  x += __builtin_sub_overflow (1, 2, bp);
   x += __builtin_mul_overflow (1, 2, llp);
   return x;
 }
--- gcc/testsuite/g++.dg/ext/builtin-arith-overflow-1.C.jj	2016-06-10 21:35:57.293135561 +0200
+++ gcc/testsuite/g++.dg/ext/builtin-arith-overflow-1.C	2016-06-14 09:08:33.440557080 +0200
@@ -1,11 +1,11 @@ 
 // { dg-do compile }
 
-enum A { B = 1, C = 2, D = __builtin_add_overflow_p (B, C, C) };
-int e[__builtin_add_overflow_p (B, C, C) + 1];
+enum A { B = 1, C = 2, D = __builtin_add_overflow_p (B, C, 0) };
+int e[__builtin_add_overflow_p (B, C, 0) + 1];
 template <int N> int foo (int);
 
 void
 bar ()
 {
-  foo <__builtin_add_overflow_p (B, C, C) + 1> (0);
+  foo <__builtin_add_overflow_p (B, C, 0) + 1> (0);
 }
--- gcc/tree-ssa-math-opts.c.jj	2016-06-07 14:56:43.000000000 +0200
+++ gcc/tree-ssa-math-opts.c	2016-06-14 11:43:04.673151569 +0200
@@ -3707,7 +3707,7 @@  match_uaddsub_overflow (gimple_stmt_iter
   gimple *use_stmt;
 
   gcc_checking_assert (code == PLUS_EXPR || code == MINUS_EXPR);
-  if (!INTEGRAL_TYPE_P (type)
+  if (TREE_CODE (type) != INTEGER_TYPE
       || !TYPE_UNSIGNED (type)
       || has_zero_uses (lhs)
       || has_single_use (lhs)
--- gcc/gimple-fold.c.jj	2016-06-10 20:24:02.000000000 +0200
+++ gcc/gimple-fold.c	2016-06-14 11:14:50.606230043 +0200
@@ -3226,15 +3226,28 @@  gimple_fold_call (gimple_stmt_iterator *
 	  else if (TREE_CODE (arg0) == INTEGER_CST
 		   && TREE_CODE (arg1) == INTEGER_CST)
 	    {
-	      if (cplx_result)
+	      if (!cplx_result)
+		result = int_const_binop (subcode, arg0, arg1);
+	      else if (TREE_CODE (type) == BOOLEAN_TYPE)
+		{
+		  tree itype = build_nonstandard_integer_type (1, 1);
+		  result = int_const_binop (subcode,
+					    fold_convert (itype, arg0),
+					    fold_convert (itype, arg1));
+		  result = fold_build2 (NE_EXPR, type, result,
+					build_int_cst (itype, 0));
+		}
+	      else
 		result = int_const_binop (subcode, fold_convert (type, arg0),
 					  fold_convert (type, arg1));
-	      else
-		result = int_const_binop (subcode, arg0, arg1);
 	      if (result && arith_overflowed_p (subcode, type, arg0, arg1))
 		{
 		  if (cplx_result)
-		    overflow = build_one_cst (type);
+		    {
+		      overflow = build_one_cst (type);
+		      if (TREE_CODE (type) == BOOLEAN_TYPE)
+			result = overflow;
+		    }
 		  else
 		    result = NULL_TREE;
 		}
@@ -3249,7 +3262,11 @@  gimple_fold_call (gimple_stmt_iterator *
 		    {
 		      if (arith_overflowed_p (PLUS_EXPR, type, result,
 					      integer_zero_node))
-			overflow = build_one_cst (type);
+			{
+			  overflow = build_one_cst (type);
+			  if (TREE_CODE (type) == BOOLEAN_TYPE)
+			    result = overflow;
+			}
 		    }
 		  else if ((!TYPE_UNSIGNED (TREE_TYPE (result))
 			    && TYPE_UNSIGNED (type))
@@ -3259,7 +3276,16 @@  gimple_fold_call (gimple_stmt_iterator *
 				     && !TYPE_UNSIGNED (type)))))
 		    result = NULL_TREE;
 		  if (result)
-		    result = fold_convert (type, result);
+		    {
+		      if (TREE_CODE (type) != BOOLEAN_TYPE
+			  || TREE_CODE (TREE_TYPE (result)) == BOOLEAN_TYPE)
+			result = fold_convert (type, result);
+		      else
+			result
+                          = fold_build2 (NE_EXPR, type, result,
+                                         build_int_cst (TREE_TYPE (result),
+                                                        0));
+		    }
 		}
 	    }
 	}
--- gcc/c-family/c-common.c.jj	2016-06-10 21:35:57.367134605 +0200
+++ gcc/c-family/c-common.c	2016-06-14 09:13:47.398439543 +0200
@@ -9983,10 +9983,16 @@  check_builtin_function_arguments (locati
 		return false;
 	      }
 	  if (TREE_CODE (TREE_TYPE (args[2])) != POINTER_TYPE
-	      || TREE_CODE (TREE_TYPE (TREE_TYPE (args[2]))) != INTEGER_TYPE)
+	      || !INTEGRAL_TYPE_P (TREE_TYPE (TREE_TYPE (args[2]))))
 	    {
 	      error_at (ARG_LOCATION (2), "argument 3 in call to function %qE "
-			"does not have pointer to integer type", fndecl);
+			"does not have pointer to integral type", fndecl);
+	      return false;
+	    }
+          else if (TREE_CODE (TREE_TYPE (TREE_TYPE (args[2]))) == ENUMERAL_TYPE)
+	    {
+	      error_at (ARG_LOCATION (2), "argument 3 in call to function %qE "
+			"has pointer to enumerated type", fndecl);
 	      return false;
 	    }
 	  return true;
@@ -10006,6 +10012,12 @@  check_builtin_function_arguments (locati
 			  "%qE does not have integral type", i + 1, fndecl);
 		return false;
 	      }
+	  if (TREE_CODE (TREE_TYPE (args[2])) == ENUMERAL_TYPE)
+	    {
+	      error_at (ARG_LOCATION (2), "argument 3 in call to function "
+			"%qE has enumerated type", fndecl);
+	      return false;
+	    }
 	  return true;
 	}
       return false;
--- gcc/doc/extend.texi.jj	2016-06-13 20:45:09.000000000 +0200
+++ gcc/doc/extend.texi	2016-06-14 09:25:59.806863118 +0200
@@ -9833,8 +9833,8 @@  performed in infinite signed precision,
 behavior for all argument values.
 
 The first built-in function allows arbitrary integral types for operands and
-the result type must be pointer to some integer type, the rest of the built-in
-functions have explicit integer types.
+the result type must be pointer to some integral type other than enumerated type,
+the rest of the built-in functions have explicit integer types.
 
 The compiler will attempt to use hardware instructions to implement
 these built-in functions where possible, like conditional jump on overflow
@@ -9879,7 +9879,8 @@  would overflow.
 These built-in functions are similar to @code{__builtin_add_overflow},
 @code{__builtin_sub_overflow}, or @code{__builtin_mul_overflow}, except that
 they don't store the result of the arithmetic operation anywhere and the
-last argument is not a pointer, but some integral expression.
+last argument is not a pointer, but some expression with integral type other
+than enumerated type.
 
 The built-in functions promote the first two operands into infinite precision signed type
 and perform addition on those promoted operands. The result is then
--- gcc/tree-vrp.c.jj	2016-05-22 12:20:37.000000000 +0200
+++ gcc/tree-vrp.c	2016-06-14 11:59:48.549504122 +0200
@@ -4019,6 +4019,8 @@  extract_range_basic (value_range *vr, gi
 			set_value_range (vr, VR_RANGE, build_int_cst (type, 0),
 					 build_int_cst (type, 1), NULL);
 		    }
+		  else if (TREE_CODE (type) == BOOLEAN_TYPE)
+		    return;
 		  else if (types_compatible_p (type, TREE_TYPE (op0))
 			   && types_compatible_p (type, TREE_TYPE (op1)))
 		    {