c, c++, v3: Avoid incorrect shortening of divisions [PR108365]

Message ID Y8GY0+AOA8qXDm8h@tucnak
State Unresolved
Headers
Series c, c++, v3: Avoid incorrect shortening of divisions [PR108365] |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Jakub Jelinek Jan. 13, 2023, 5:45 p.m. UTC
  On Fri, Jan 13, 2023 at 11:58:06AM -0500, Jason Merrill wrote:
> LGTM, though we might put that condition in c-common somewhere?

So like this then?  Just tested on the new testcases, full bootstrap/regtest
queued?

2023-01-13  Jakub Jelinek  <jakub@redhat.com>

	PR c++/108365
	* c-common.h (may_shorten_divmod): New static inline function.

	* c-typeck.cc (build_binary_op): Use may_shorten_divmod for integral
	division or modulo.

	* typeck.cc (cp_build_binary_op): Use may_shorten_divmod for integral
	division or modulo.

	* c-c++-common/pr108365.c: New test.
	* g++.dg/opt/pr108365.C: New test.
	* g++.dg/warn/pr108365.C: New test.



	Jakub
  

Comments

Jason Merrill Jan. 13, 2023, 7:08 p.m. UTC | #1
On 1/13/23 12:45, Jakub Jelinek wrote:
> On Fri, Jan 13, 2023 at 11:58:06AM -0500, Jason Merrill wrote:
>> LGTM, though we might put that condition in c-common somewhere?
> 
> So like this then?  Just tested on the new testcases, full bootstrap/regtest
> queued?

OK..

> 2023-01-13  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/108365
> 	* c-common.h (may_shorten_divmod): New static inline function.
> 
> 	* c-typeck.cc (build_binary_op): Use may_shorten_divmod for integral
> 	division or modulo.
> 
> 	* typeck.cc (cp_build_binary_op): Use may_shorten_divmod for integral
> 	division or modulo.
> 
> 	* c-c++-common/pr108365.c: New test.
> 	* g++.dg/opt/pr108365.C: New test.
> 	* g++.dg/warn/pr108365.C: New test.
> 
> --- gcc/c-family/c-common.h.jj	2022-11-14 13:35:34.195160199 +0100
> +++ gcc/c-family/c-common.h	2023-01-13 18:35:08.130362228 +0100
> @@ -918,6 +918,30 @@ extern tree convert_init (tree, tree);
>   /* Subroutine of build_binary_op, used for certain operations.  */
>   extern tree shorten_binary_op (tree result_type, tree op0, tree op1, bool bitwise);
>   
> +/* Return true if division or modulo op0 / op1 or op0 % op1 may be shortened.
> +   We can shorten only if we can guarantee that op0 is not signed integral
> +   minimum or op1 is not -1, because e.g. (long long) INT_MIN / -1 is
> +   well defined INT_MAX + 1LL if long long is wider than int, but INT_MIN / -1
> +   is UB.  */
> +static inline bool
> +may_shorten_divmod (tree op0, tree op1)
> +{
> +  tree type0 = TREE_TYPE (op0);
> +  if (TYPE_UNSIGNED (type0))
> +    return true;
> +  /* A cast from narrower unsigned won't be signed integral minimum,
> +     but cast from same or wider precision unsigned could be.  */
> +  if (TREE_CODE (op0) == NOP_EXPR
> +      && INTEGRAL_TYPE_P (TREE_TYPE (TREE_OPERAND (op0, 0)))
> +      && TYPE_UNSIGNED (TREE_TYPE (TREE_OPERAND (op0, 0)))
> +      && (TYPE_PRECISION (TREE_TYPE (TREE_OPERAND (op0, 0)))
> +	  < TYPE_PRECISION (type0)))
> +    return true;
> +  if (TREE_CODE (op1) == INTEGER_CST && !integer_all_onesp (op1))
> +    return true;
> +  return false;
> +}
> +
>   /* Subroutine of build_binary_op, used for comparison operations.
>      See if the operands have both been converted from subword integer types
>      and, if so, perhaps change them both back to their original type.  */
> --- gcc/c/c-typeck.cc.jj	2023-01-13 11:11:45.368016437 +0100
> +++ gcc/c/c-typeck.cc	2023-01-13 18:38:25.919538847 +0100
> @@ -12431,9 +12431,7 @@ build_binary_op (location_t location, en
>   	       undefined if the quotient can't be represented in the
>   	       computation mode.  We shorten only if unsigned or if
>   	       dividing by something we know != -1.  */
> -	    shorten = (TYPE_UNSIGNED (TREE_TYPE (orig_op0))
> -		       || (TREE_CODE (op1) == INTEGER_CST
> -			   && !integer_all_onesp (op1)));
> +	    shorten = may_shorten_divmod (op0, op1);
>   	  common = 1;
>   	}
>         break;
> @@ -12467,9 +12465,7 @@ build_binary_op (location_t location, en
>   	     on some targets, since the modulo instruction is undefined if the
>   	     quotient can't be represented in the computation mode.  We shorten
>   	     only if unsigned or if dividing by something we know != -1.  */
> -	  shorten = (TYPE_UNSIGNED (TREE_TYPE (orig_op0))
> -		     || (TREE_CODE (op1) == INTEGER_CST
> -			 && !integer_all_onesp (op1)));
> +	  shorten = may_shorten_divmod (op0, op1);
>   	  common = 1;
>   	}
>         break;
> --- gcc/cp/typeck.cc.jj	2023-01-13 11:11:45.418015716 +0100
> +++ gcc/cp/typeck.cc	2023-01-13 18:38:40.754327078 +0100
> @@ -5455,10 +5455,7 @@ cp_build_binary_op (const op_location_t
>   		 point, so we have to dig out the original type to find out if
>   		 it was unsigned.  */
>   	      tree stripped_op1 = tree_strip_any_location_wrapper (op1);
> -	      shorten = ((TREE_CODE (op0) == NOP_EXPR
> -			  && TYPE_UNSIGNED (TREE_TYPE (TREE_OPERAND (op0, 0))))
> -			 || (TREE_CODE (stripped_op1) == INTEGER_CST
> -			     && ! integer_all_onesp (stripped_op1)));
> +	      shorten = may_shorten_divmod (op0, stripped_op1);
>   	    }
>   
>   	  common = 1;
> @@ -5491,10 +5488,7 @@ cp_build_binary_op (const op_location_t
>   	     quotient can't be represented in the computation mode.  We shorten
>   	     only if unsigned or if dividing by something we know != -1.  */
>   	  tree stripped_op1 = tree_strip_any_location_wrapper (op1);
> -	  shorten = ((TREE_CODE (op0) == NOP_EXPR
> -		      && TYPE_UNSIGNED (TREE_TYPE (TREE_OPERAND (op0, 0))))
> -		     || (TREE_CODE (stripped_op1) == INTEGER_CST
> -			 && ! integer_all_onesp (stripped_op1)));
> +	  shorten = may_shorten_divmod (op0, stripped_op1);
>   	  common = 1;
>   	}
>         break;
> --- gcc/testsuite/c-c++-common/pr108365.c.jj	2023-01-13 18:25:09.163911212 +0100
> +++ gcc/testsuite/c-c++-common/pr108365.c	2023-01-13 18:25:09.163911212 +0100
> @@ -0,0 +1,16 @@
> +/* PR c++/108365 */
> +/* { dg-do compile { target { { ilp32 || lp64 } || llp64 } } } */
> +/* { dg-options "-O2 -fdump-tree-gimple" } */
> +/* { dg-final { scan-tree-dump-not " \\\((int|unsigned short int|long long int|unsigned int)\\\) " "gimple" } } */
> +
> +unsigned short
> +foo (unsigned short x, unsigned short y)
> +{
> +  return (unsigned) x / y;
> +}
> +
> +unsigned int
> +bar (unsigned int x, unsigned int y)
> +{
> +  return (long long) x / y;
> +}
> --- gcc/testsuite/g++.dg/opt/pr108365.C.jj	2023-01-13 18:25:09.163911212 +0100
> +++ gcc/testsuite/g++.dg/opt/pr108365.C	2023-01-13 18:25:09.163911212 +0100
> @@ -0,0 +1,13 @@
> +// PR c++/108365
> +// { dg-do run }
> +
> +char b = 1;
> +
> +int
> +main ()
> +{
> +#if __CHAR_BIT__ == 8 && __SIZEOF_SHORT__ == 2 && __SIZEOF_INT__ == 4 && __SIZEOF_LONG_LONG__ == 8
> +  while ((short) ((long long) (unsigned long long) (-__INT_MAX__ - 1) / (long long) (b ? -1 : 0)))
> +    ;
> +#endif
> +}
> --- gcc/testsuite/g++.dg/warn/pr108365.C.jj	2023-01-13 18:25:09.163911212 +0100
> +++ gcc/testsuite/g++.dg/warn/pr108365.C	2023-01-13 18:25:09.163911212 +0100
> @@ -0,0 +1,5 @@
> +// PR c++/108365
> +// { dg-do compile { target { { { ilp32 || lp64 } || llp64 } && c++11 } } }
> +
> +constexpr char b = 1;
> +long t = (short) ((long long) (unsigned long long) (-__INT_MAX__ - 1) / (long long) (b ? -1 : 0)); // { dg-bogus "integer overflow in expression of type" }
> 
> 
> 	Jakub
>
  

Patch

--- gcc/c-family/c-common.h.jj	2022-11-14 13:35:34.195160199 +0100
+++ gcc/c-family/c-common.h	2023-01-13 18:35:08.130362228 +0100
@@ -918,6 +918,30 @@  extern tree convert_init (tree, tree);
 /* Subroutine of build_binary_op, used for certain operations.  */
 extern tree shorten_binary_op (tree result_type, tree op0, tree op1, bool bitwise);
 
+/* Return true if division or modulo op0 / op1 or op0 % op1 may be shortened.
+   We can shorten only if we can guarantee that op0 is not signed integral
+   minimum or op1 is not -1, because e.g. (long long) INT_MIN / -1 is
+   well defined INT_MAX + 1LL if long long is wider than int, but INT_MIN / -1
+   is UB.  */
+static inline bool
+may_shorten_divmod (tree op0, tree op1)
+{
+  tree type0 = TREE_TYPE (op0);
+  if (TYPE_UNSIGNED (type0))
+    return true;
+  /* A cast from narrower unsigned won't be signed integral minimum,
+     but cast from same or wider precision unsigned could be.  */
+  if (TREE_CODE (op0) == NOP_EXPR
+      && INTEGRAL_TYPE_P (TREE_TYPE (TREE_OPERAND (op0, 0)))
+      && TYPE_UNSIGNED (TREE_TYPE (TREE_OPERAND (op0, 0)))
+      && (TYPE_PRECISION (TREE_TYPE (TREE_OPERAND (op0, 0)))
+	  < TYPE_PRECISION (type0)))
+    return true;
+  if (TREE_CODE (op1) == INTEGER_CST && !integer_all_onesp (op1))
+    return true;
+  return false;
+}
+
 /* Subroutine of build_binary_op, used for comparison operations.
    See if the operands have both been converted from subword integer types
    and, if so, perhaps change them both back to their original type.  */
--- gcc/c/c-typeck.cc.jj	2023-01-13 11:11:45.368016437 +0100
+++ gcc/c/c-typeck.cc	2023-01-13 18:38:25.919538847 +0100
@@ -12431,9 +12431,7 @@  build_binary_op (location_t location, en
 	       undefined if the quotient can't be represented in the
 	       computation mode.  We shorten only if unsigned or if
 	       dividing by something we know != -1.  */
-	    shorten = (TYPE_UNSIGNED (TREE_TYPE (orig_op0))
-		       || (TREE_CODE (op1) == INTEGER_CST
-			   && !integer_all_onesp (op1)));
+	    shorten = may_shorten_divmod (op0, op1);
 	  common = 1;
 	}
       break;
@@ -12467,9 +12465,7 @@  build_binary_op (location_t location, en
 	     on some targets, since the modulo instruction is undefined if the
 	     quotient can't be represented in the computation mode.  We shorten
 	     only if unsigned or if dividing by something we know != -1.  */
-	  shorten = (TYPE_UNSIGNED (TREE_TYPE (orig_op0))
-		     || (TREE_CODE (op1) == INTEGER_CST
-			 && !integer_all_onesp (op1)));
+	  shorten = may_shorten_divmod (op0, op1);
 	  common = 1;
 	}
       break;
--- gcc/cp/typeck.cc.jj	2023-01-13 11:11:45.418015716 +0100
+++ gcc/cp/typeck.cc	2023-01-13 18:38:40.754327078 +0100
@@ -5455,10 +5455,7 @@  cp_build_binary_op (const op_location_t
 		 point, so we have to dig out the original type to find out if
 		 it was unsigned.  */
 	      tree stripped_op1 = tree_strip_any_location_wrapper (op1);
-	      shorten = ((TREE_CODE (op0) == NOP_EXPR
-			  && TYPE_UNSIGNED (TREE_TYPE (TREE_OPERAND (op0, 0))))
-			 || (TREE_CODE (stripped_op1) == INTEGER_CST
-			     && ! integer_all_onesp (stripped_op1)));
+	      shorten = may_shorten_divmod (op0, stripped_op1);
 	    }
 
 	  common = 1;
@@ -5491,10 +5488,7 @@  cp_build_binary_op (const op_location_t
 	     quotient can't be represented in the computation mode.  We shorten
 	     only if unsigned or if dividing by something we know != -1.  */
 	  tree stripped_op1 = tree_strip_any_location_wrapper (op1);
-	  shorten = ((TREE_CODE (op0) == NOP_EXPR
-		      && TYPE_UNSIGNED (TREE_TYPE (TREE_OPERAND (op0, 0))))
-		     || (TREE_CODE (stripped_op1) == INTEGER_CST
-			 && ! integer_all_onesp (stripped_op1)));
+	  shorten = may_shorten_divmod (op0, stripped_op1);
 	  common = 1;
 	}
       break;
--- gcc/testsuite/c-c++-common/pr108365.c.jj	2023-01-13 18:25:09.163911212 +0100
+++ gcc/testsuite/c-c++-common/pr108365.c	2023-01-13 18:25:09.163911212 +0100
@@ -0,0 +1,16 @@ 
+/* PR c++/108365 */
+/* { dg-do compile { target { { ilp32 || lp64 } || llp64 } } } */
+/* { dg-options "-O2 -fdump-tree-gimple" } */
+/* { dg-final { scan-tree-dump-not " \\\((int|unsigned short int|long long int|unsigned int)\\\) " "gimple" } } */
+
+unsigned short
+foo (unsigned short x, unsigned short y)
+{
+  return (unsigned) x / y;
+}
+
+unsigned int
+bar (unsigned int x, unsigned int y)
+{
+  return (long long) x / y;
+}
--- gcc/testsuite/g++.dg/opt/pr108365.C.jj	2023-01-13 18:25:09.163911212 +0100
+++ gcc/testsuite/g++.dg/opt/pr108365.C	2023-01-13 18:25:09.163911212 +0100
@@ -0,0 +1,13 @@ 
+// PR c++/108365
+// { dg-do run }
+
+char b = 1;
+
+int
+main ()
+{
+#if __CHAR_BIT__ == 8 && __SIZEOF_SHORT__ == 2 && __SIZEOF_INT__ == 4 && __SIZEOF_LONG_LONG__ == 8
+  while ((short) ((long long) (unsigned long long) (-__INT_MAX__ - 1) / (long long) (b ? -1 : 0)))
+    ;
+#endif
+}
--- gcc/testsuite/g++.dg/warn/pr108365.C.jj	2023-01-13 18:25:09.163911212 +0100
+++ gcc/testsuite/g++.dg/warn/pr108365.C	2023-01-13 18:25:09.163911212 +0100
@@ -0,0 +1,5 @@ 
+// PR c++/108365
+// { dg-do compile { target { { { ilp32 || lp64 } || llp64 } && c++11 } } }
+
+constexpr char b = 1;
+long t = (short) ((long long) (unsigned long long) (-__INT_MAX__ - 1) / (long long) (b ? -1 : 0)); // { dg-bogus "integer overflow in expression of type" }