diff mbox

[C/C++] Don't convert RHS of a shift-expression to int (PR c/63862)

Message ID 20141127192403.GD15555@redhat.com
State New
Headers show

Commit Message

Marek Polacek Nov. 27, 2014, 7:24 p.m. UTC
On Thu, Nov 27, 2014 at 10:56:06AM +0100, Richard Biener wrote:
> But then rather than using integer_type_node I'd convert to
> unsigned_type_node (if that doesn't work you know we have some nasty
> dependence on negative shift counts "working")

Yeah, I think adding the conversion into c_gimplify_expr is the best
thing we can do until we have type demotion/promotion pass.  Seems
that unsigned_type_node there works well.
(What happened to Kai's type elevation pass?)

Bootstrapped/regtested on ppc64-linux, ok for trunk?

2014-11-27  Marek Polacek  <polacek@redhat.com>

	PR c/63862
c-family/
	* c-ubsan.c (ubsan_instrument_shift): Change the type of a MINUS_EXPR
	to op1_utype.
	* c-gimplify.c (c_gimplify_expr): Convert right operand of a shift
	expression to unsigned_type_node.
c/
	* c-typeck.c (build_binary_op) <RSHIFT_EXPR, LSHIFT_EXPR>: Don't
	convert the right operand to integer type.
cp/
	* typeck.c (cp_build_binary_op) <RSHIFT_EXPR, LSHIFT_EXPR>: Don't
	convert the right operand to integer type.
testsuite/
	* gcc.c-torture/execute/shiftopt-1.c: Don't XFAIL anymore.
	* c-c++-common/ubsan/shift-7.c: New test.


	Marek

Comments

Jakub Jelinek Nov. 27, 2014, 7:42 p.m. UTC | #1
On Thu, Nov 27, 2014 at 08:24:03PM +0100, Marek Polacek wrote:
> On Thu, Nov 27, 2014 at 10:56:06AM +0100, Richard Biener wrote:
> > But then rather than using integer_type_node I'd convert to
> > unsigned_type_node (if that doesn't work you know we have some nasty
> > dependence on negative shift counts "working")
> 
> Yeah, I think adding the conversion into c_gimplify_expr is the best
> thing we can do until we have type demotion/promotion pass.  Seems
> that unsigned_type_node there works well.
> (What happened to Kai's type elevation pass?)
> 
> Bootstrapped/regtested on ppc64-linux, ok for trunk?

LGTM.

> 2014-11-27  Marek Polacek  <polacek@redhat.com>
> 
> 	PR c/63862
> c-family/
> 	* c-ubsan.c (ubsan_instrument_shift): Change the type of a MINUS_EXPR
> 	to op1_utype.
> 	* c-gimplify.c (c_gimplify_expr): Convert right operand of a shift
> 	expression to unsigned_type_node.
> c/
> 	* c-typeck.c (build_binary_op) <RSHIFT_EXPR, LSHIFT_EXPR>: Don't
> 	convert the right operand to integer type.
> cp/
> 	* typeck.c (cp_build_binary_op) <RSHIFT_EXPR, LSHIFT_EXPR>: Don't
> 	convert the right operand to integer type.
> testsuite/
> 	* gcc.c-torture/execute/shiftopt-1.c: Don't XFAIL anymore.
> 	* c-c++-common/ubsan/shift-7.c: New test.

	Jakub
diff mbox

Patch

diff --git gcc/c-family/c-gimplify.c gcc/c-family/c-gimplify.c
index 85b4223..8e0eb4c 100644
--- gcc/c-family/c-gimplify.c
+++ gcc/c-family/c-gimplify.c
@@ -242,6 +242,24 @@  c_gimplify_expr (tree *expr_p, gimple_seq *pre_p ATTRIBUTE_UNUSED,
 
   switch (code)
     {
+    case LSHIFT_EXPR:
+    case RSHIFT_EXPR:
+      {
+	/* We used to convert the right operand of a shift-expression
+	   to an integer_type_node in the FEs.  But it is unnecessary
+	   and not desirable for diagnostics and sanitizers.  We keep
+	   this here to not pessimize the code, but we convert to an
+	   unsigned type, because negative shift counts are undefined
+	   anyway.
+	   We should get rid of this conversion when we have a proper
+	   type demotion/promotion pass.  */
+	tree *op1_p = &TREE_OPERAND (*expr_p, 1);
+	if (TREE_CODE (TREE_TYPE (*op1_p)) != VECTOR_TYPE
+	    && TYPE_MAIN_VARIANT (TREE_TYPE (*op1_p)) != unsigned_type_node)
+	  *op1_p = convert (unsigned_type_node, *op1_p);
+	break;
+      }
+
     case DECL_EXPR:
       /* This is handled mostly by gimplify.c, but we have to deal with
 	 not warning about int x = x; as it is a GCC extension to turn off
diff --git gcc/c-family/c-ubsan.c gcc/c-family/c-ubsan.c
index 90b03f2..96afc67 100644
--- gcc/c-family/c-ubsan.c
+++ gcc/c-family/c-ubsan.c
@@ -151,7 +151,7 @@  ubsan_instrument_shift (location_t loc, enum tree_code code,
       && !TYPE_UNSIGNED (type0)
       && flag_isoc99)
     {
-      tree x = fold_build2 (MINUS_EXPR, unsigned_type_node, uprecm1,
+      tree x = fold_build2 (MINUS_EXPR, op1_utype, uprecm1,
 			    fold_convert (op1_utype, op1));
       tt = fold_convert_loc (loc, unsigned_type_for (type0), op0);
       tt = fold_build2 (RSHIFT_EXPR, TREE_TYPE (tt), tt, x);
diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c
index 67efb46..bf0f306 100644
--- gcc/c/c-typeck.c
+++ gcc/c/c-typeck.c
@@ -10513,11 +10513,6 @@  build_binary_op (location_t location, enum tree_code code,
 
 	  /* Use the type of the value to be shifted.  */
 	  result_type = type0;
-	  /* Convert the non vector shift-count to an integer, regardless
-	     of size of value being shifted.  */
-	  if (TREE_CODE (TREE_TYPE (op1)) != VECTOR_TYPE
-	      && TYPE_MAIN_VARIANT (TREE_TYPE (op1)) != integer_type_node)
-	    op1 = convert (integer_type_node, op1);
 	  /* Avoid converting op1 to result_type later.  */
 	  converted = 1;
 	}
@@ -10563,11 +10558,6 @@  build_binary_op (location_t location, enum tree_code code,
 
 	  /* Use the type of the value to be shifted.  */
 	  result_type = type0;
-	  /* Convert the non vector shift-count to an integer, regardless
-	     of size of value being shifted.  */
-	  if (TREE_CODE (TREE_TYPE (op1)) != VECTOR_TYPE
-	      && TYPE_MAIN_VARIANT (TREE_TYPE (op1)) != integer_type_node)
-	    op1 = convert (integer_type_node, op1);
 	  /* Avoid converting op1 to result_type later.  */
 	  converted = 1;
 	}
diff --git gcc/cp/typeck.c gcc/cp/typeck.c
index 8b66acc..6ca346b 100644
--- gcc/cp/typeck.c
+++ gcc/cp/typeck.c
@@ -4295,10 +4295,6 @@  cp_build_binary_op (location_t location,
 			     "right shift count >= width of type");
 		}
 	    }
-	  /* Convert the shift-count to an integer, regardless of
-	     size of value being shifted.  */
-	  if (TYPE_MAIN_VARIANT (TREE_TYPE (op1)) != integer_type_node)
-	    op1 = cp_convert (integer_type_node, op1, complain);
 	  /* Avoid converting op1 to result_type later.  */
 	  converted = 1;
 	}
@@ -4344,10 +4340,6 @@  cp_build_binary_op (location_t location,
 			     "left shift count >= width of type");
 		}
 	    }
-	  /* Convert the shift-count to an integer, regardless of
-	     size of value being shifted.  */
-	  if (TYPE_MAIN_VARIANT (TREE_TYPE (op1)) != integer_type_node)
-	    op1 = cp_convert (integer_type_node, op1, complain);
 	  /* Avoid converting op1 to result_type later.  */
 	  converted = 1;
 	}
diff --git gcc/testsuite/c-c++-common/ubsan/shift-7.c gcc/testsuite/c-c++-common/ubsan/shift-7.c
index e69de29..1e33273 100644
--- gcc/testsuite/c-c++-common/ubsan/shift-7.c
+++ gcc/testsuite/c-c++-common/ubsan/shift-7.c
@@ -0,0 +1,27 @@ 
+/* PR c/63862 */
+/* { dg-do run } */
+/* { dg-options "-fsanitize=undefined" } */
+
+unsigned long long int __attribute__ ((noinline, noclone))
+foo (unsigned long long int i, unsigned long long int j)
+{
+  asm ("");
+  return i >> j;
+}
+
+unsigned long long int __attribute__ ((noinline, noclone))
+bar (unsigned long long int i, unsigned long long int j)
+{
+  asm ("");
+  return i << j;
+}
+
+int
+main ()
+{
+  foo (1ULL, 0x100000000ULL);
+  bar (1ULL, 0x100000000ULL);
+}
+
+/* { dg-output "shift exponent 4294967296 is too large for \[^\n\r]*-bit type 'long long unsigned int'\[^\n\r]*(\n|\r\n|\r)" } */
+/* { dg-output "\[^\n\r]*shift exponent 4294967296 is too large for \[^\n\r]*-bit type 'long long unsigned int'\[^\n\r]*(\n|\r\n|\r)" } */
diff --git gcc/testsuite/gcc.c-torture/execute/shiftopt-1.c gcc/testsuite/gcc.c-torture/execute/shiftopt-1.c
index 3ff714d..8c855b8 100644
--- gcc/testsuite/gcc.c-torture/execute/shiftopt-1.c
+++ gcc/testsuite/gcc.c-torture/execute/shiftopt-1.c
@@ -22,16 +22,11 @@  utest (unsigned int x)
   if (0 >> x != 0)
     link_error ();
 
-  /* XFAIL: the C frontend converts the shift amount to 'int'
-     thus we get -1 >> (int)x which means the shift amount may
-     be negative.  See PR63862.  */
-#if 0
   if (-1 >> x != -1)
     link_error ();
 
   if (~0 >> x != ~0)
     link_error ();
-#endif
 }
 
 void