[PATCH v2] Do not sanitize left shifts for -fwrapv (PR68418)

[PATCH v2] Do not sanitize left shifts for -fwrapv (PR68418)

[Paolo Bonzini]

Left shifts into the sign bit is a kind of overflow, and the
standard chooses to treat left shifts of negative values the
same way.

However, the -fwrapv option modifies the language to one where
integers are defined as two's complement---which also defines
entirely the behavior of shifts.  Disable sanitization of left
shifts when -fwrapv is in effect, using the same logic as
instrument_si_overflow.  The same change was proposed
for LLVM at https://llvm.org/bugs/show_bug.cgi?id=25552.

Bootstrapped/regtested x86_64-pc-linux-gnu.  Ok for trunk, and for
GCC 5 branch after 5.3 is released?

Thanks,

Paolo

gcc:
	PR sanitizer/68418
	* c-family/c-ubsan.c (ubsan_instrument_shift): Disable
	sanitization of left shifts for wrapping signed types as well.

gcc/testsuite:
	PR sanitizer/68418
	* gcc.dg/ubsan/c99-wrapv-shift-1.c,
	gcc.dg/ubsan/c99-wrapv-shift-2.c: New testcases.

Index: c-family/c-ubsan.c
===================================================================
--- c-family/c-ubsan.c	(revision 231289)
+++ c-family/c-ubsan.c	(working copy)
@@ -124,12 +124,17 @@ ubsan_instrument_shift (location_t loc, enum tree_
   t = fold_convert_loc (loc, op1_utype, op1);
   t = fold_build2 (GT_EXPR, boolean_type_node, t, uprecm1);
 
+  /* If this is not a signed operation, don't perform overflow checks.
+     Also punt on bit-fields.  */
+  if (!INTEGRAL_TYPE_P (type0)
+      || TYPE_OVERFLOW_WRAPS (type0)
+      || GET_MODE_BITSIZE (TYPE_MODE (type0)) != TYPE_PRECISION (type0))
+    ;
+
   /* For signed x << y, in C99/C11, the following:
      (unsigned) x >> (uprecm1 - y)
      if non-zero, is undefined.  */
-  if (code == LSHIFT_EXPR
-      && !TYPE_UNSIGNED (type0)
-      && flag_isoc99)
+  else if (code == LSHIFT_EXPR && flag_isoc99 && cxx_dialect < cxx11)
     {
       tree x = fold_build2 (MINUS_EXPR, op1_utype, uprecm1,
 			    fold_convert (op1_utype, unshare_expr (op1)));
@@ -142,9 +147,7 @@ ubsan_instrument_shift (location_t loc, enum tree_
   /* For signed x << y, in C++11 and later, the following:
      x < 0 || ((unsigned) x >> (uprecm1 - y))
      if > 1, is undefined.  */
-  if (code == LSHIFT_EXPR
-      && !TYPE_UNSIGNED (type0)
-      && (cxx_dialect >= cxx11))
+  else if (code == LSHIFT_EXPR && cxx_dialect >= cxx11)
     {
       tree x = fold_build2 (MINUS_EXPR, op1_utype, uprecm1,
 			    fold_convert (op1_utype, unshare_expr (op1)));
Index: testsuite/gcc.dg/ubsan/c99-wrapv-shift-1.c
===================================================================
--- testsuite/gcc.dg/ubsan/c99-wrapv-shift-1.c	(revision 0)
+++ testsuite/gcc.dg/ubsan/c99-wrapv-shift-1.c	(working copy)
@@ -0,0 +1,9 @@
+/* { dg-do run } */
+/* { dg-options "-fsanitize=shift -fwrapv -w -std=c99" } */
+
+int
+main (void)
+{
+  int a = -42;
+  a << 1;
+}
Index: testsuite/gcc.dg/ubsan/c99-wrapv-shift-2.c
===================================================================
--- testsuite/gcc.dg/ubsan/c99-wrapv-shift-2.c	(revision 0)
+++ testsuite/gcc.dg/ubsan/c99-wrapv-shift-2.c	(working copy)
@@ -0,0 +1,9 @@
+/* { dg-do run } */
+/* { dg-options "-fsanitize=shift -fwrapv -w -std=c99" } */
+
+int
+main (void)
+{
+  int a = 1;
+  a <<= 31;
+}

Re: [PATCH v2] Do not sanitize left shifts for -fwrapv (PR68418)

[Jeff Law]

On 12/09/2015 10:08 AM, Paolo Bonzini wrote:
> Left shifts into the sign bit is a kind of overflow, and the
> standard chooses to treat left shifts of negative values the
> same way.
>
> However, the -fwrapv option modifies the language to one where
> integers are defined as two's complement---which also defines
> entirely the behavior of shifts.  Disable sanitization of left
> shifts when -fwrapv is in effect, using the same logic as
> instrument_si_overflow.  The same change was proposed
> for LLVM at https://llvm.org/bugs/show_bug.cgi?id=25552.
>
> Bootstrapped/regtested x86_64-pc-linux-gnu.  Ok for trunk, and for
> GCC 5 branch after 5.3 is released?
>
> Thanks,
>
> Paolo
>
> gcc:
> 	PR sanitizer/68418
> 	* c-family/c-ubsan.c (ubsan_instrument_shift): Disable
> 	sanitization of left shifts for wrapping signed types as well.
>
> gcc/testsuite:
> 	PR sanitizer/68418
> 	* gcc.dg/ubsan/c99-wrapv-shift-1.c,
> 	gcc.dg/ubsan/c99-wrapv-shift-2.c: New testcases.
Thanks for the pointers to the earlier code that constrains the types.

FWIW Jan Beulich is twiddling the code leading to the 
ubsan_instrument_shift call.  In fact, your change may make Jan's change 
safe :-)


OK for the trunk.

Thanks,
Jeff

Re: [PATCH v2] Do not sanitize left shifts for -fwrapv (PR68418)

[Eric Botcazou]

> gcc:
> 	PR sanitizer/68418
> 	* c-family/c-ubsan.c (ubsan_instrument_shift): Disable
> 	sanitization of left shifts for wrapping signed types as well.

That's incorrect, it should be put into c-family/ChangeLog without prefix.

-- 
Eric Botcazou