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

[Jason Merrill <jason@redhat.com>]

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
>