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

[Paolo Bonzini <bonzini@gnu.org>]

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