Do not sanitize left shifts for -fwrapv (PR68418)

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.  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.

On 25/11/2015 14:55, 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.  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 230466)
> +++ c-family/c-ubsan.c	(working copy)
> @@ -128,7 +128,7 @@
>       (unsigned) x >> (uprecm1 - y)
>       if non-zero, is undefined.  */
>    if (code == LSHIFT_EXPR
> -      && !TYPE_UNSIGNED (type0)
> +      && !TYPE_OVERFLOW_WRAPS (type0)
>        && flag_isoc99)
>      {
>        tree x = fold_build2 (MINUS_EXPR, op1_utype, uprecm1,
> @@ -143,7 +143,7 @@
>       x < 0 || ((unsigned) x >> (uprecm1 - y))
>       if > 1, is undefined.  */
>    if (code == LSHIFT_EXPR
> -      && !TYPE_UNSIGNED (type0)
> +      && !TYPE_OVERFLOW_WRAPS (type0)
>        && (cxx_dialect >= cxx11))
>      {
>        tree x = fold_build2 (MINUS_EXPR, op1_utype, uprecm1,
> 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;
> +}
> 

Ping?

Paolo

On 12/04/2015 10:51 AM, Paolo Bonzini wrote:
>
>
> On 25/11/2015 14:55, 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.  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.
Doesn't this change how pointer types are handled?

jeff

> >> 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.
> Doesn't this change how pointer types are handled?

Why would pointer types be shifted at all (at the ubsan level,
which is basically the AST)?

Paolo

On 12/04/2015 01:48 PM, Paolo Bonzini wrote:
>
>>>> 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.
>> Doesn't this change how pointer types are handled?
>
> Why would pointer types be shifted at all (at the ubsan level,
> which is basically the AST)?
It's not really a question of why, it's a change in behaviour. 
Previously this code would emit instrumentation objects of pointer type 
if pointers are signed on the target.  After your change it will not, in 
fact, it may trigger a checking failure.

So you'd have to argue that we don't care about sanitization of these 
operations on pointers and verify that we don't trigger a checking 
failure.  I'm really not the best judge of whether or not we want to 
instrument pointer shifts -- they're not terribly useful in general, but 
I'm always [un]pleasantly surprised at what people actually do.


Jeff

On 12/04/2015 01:48 PM, Paolo Bonzini wrote:
>
>>>> 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.
>> Doesn't this change how pointer types are handled?
>
> Why would pointer types be shifted at all (at the ubsan level,
> which is basically the AST)?
BTW, if you argument is that we can never get into this code with a 
shift of a pointer object, I'd like to see some kind of analysis to back 
up that assertion -- which could be as simple as pointing to FE code 
that issues an error if the user tried to do something like shift a 
pointer object.

jeff

On 04/12/2015 23:48, Jeff Law wrote:
>>
>> Why would pointer types be shifted at all (at the ubsan level,
>> which is basically the AST)?
> BTW, if you argument is that we can never get into this code with a
> shift of a pointer object, I'd like to see some kind of analysis to back
> up that assertion -- which could be as simple as pointing to FE code
> that issues an error if the user tried to do something like shift a
> pointer object.

You're right, I should have qualified that better.  And actually there
is an issue with this patch, though it is not pointers.

There are only two call sites for ubsan_instrument_shift.
In c/c-typeck.c:

  /* Remember whether we're doing << or >>.  */
  bool doing_shift = false;

  /* The expression codes of the data types of the arguments tell us
     whether the arguments are integers, floating, pointers, etc.  */
  code0 = TREE_CODE (type0);
  code1 = TREE_CODE (type1);

  switch (code)
    {
    ...
    case RSHIFT_EXPR:
      ...
      else if ((code0 == INTEGER_TYPE || code0 == FIXED_POINT_TYPE)
               && code1 == INTEGER_TYPE)
        {
          doing_shift = true;
          ...
        }
      ...
    case LSHIFT_EXPR:
      ...
      else if ((code0 == INTEGER_TYPE || code0 == FIXED_POINT_TYPE)
               && code1 == INTEGER_TYPE)
        {
          doing_shift = true;
          ...
        }
      ...
    }
  ...
  if ((flag_sanitize & (SANITIZE_SHIFT | SANITIZE_DIVIDE
                        | SANITIZE_FLOAT_DIVIDE))
      && do_ubsan_in_current_function ()
      && (doing_div_or_mod || doing_shift)
      && !require_constant_value)
    {
      /* OP0 and/or OP1 might have side-effects.  */
      op0 = c_save_expr (op0);
      op1 = c_save_expr (op1);
      op0 = c_fully_fold (op0, false, NULL);
      op1 = c_fully_fold (op1, false, NULL);
      ...
      else if (doing_shift && (flag_sanitize & SANITIZE_SHIFT))
        instrument_expr = ubsan_instrument_shift (location, code, op0, op1);
    }

cp/typeck.c is the same but it doesn't handle code0 == FIXED_POINT_TYPE.

But FIXED_POINT_TYPE is not an integral type, and thus it would fail the
TYPE_OVERFLOW_WRAPS check with my patch.  I'll post an updated patch that
also removes all instrumentation in the case of fixed point types, similar
to instrument_si_overflow.

Thanks for the careful review!

Paolo