Re: [C/C++ PATCH] Implement -Wshift-overflow (PR c++/55095)

[Richard Sandiford <rdsandiford at googlemail dot com>]

Richard Biener <richard.guenther@gmail.com> writes:
> On Mon, Jun 1, 2015 at 10:06 PM, Richard Sandiford
> <rdsandiford@googlemail.com> wrote:
>> Marek Polacek <polacek@redhat.com> writes:
>>> +  /* Left-hand operand must be signed.  */
>>> +  if (TYPE_UNSIGNED (type0))
>>> +    return false;
>>> +
>>> +  /* Compute the result in infinite precision math (sort of).  */
>>> +  widest_int w = wi::lshift (wi::to_widest (op0), wi::to_widest (op1));
>>> +  unsigned int min_prec = wi::min_precision (w, SIGNED);
>>> +  /* Watch out for shifting a negative value.  */
>>> +  tree r = wide_int_to_tree (tree_int_cst_sgn (op0) >= 0
>>> +                          ? unsigned_type_for (type0)
>>> +                          : type0, w);
>>> +  bool overflowed = wi::cmps (w, wi::to_widest (r));
>>> +  if (overflowed && c_inhibit_evaluation_warnings == 0)
>>> +    warning_at (loc, OPT_Wshift_overflow,
>>> +             "result of %qE requires %u bits to represent, "
>>> +             "but %qT only has %u bits",
>>> +             build2_loc (loc, LSHIFT_EXPR, type0, op0, op1),
>>> +             min_prec, type0, TYPE_PRECISION (type0));
>>> +
>>> +  return overflowed;
>>
>> Yeah, this "sort of" is a bit worrying :-)  Especially as the number
>> of bits in a widest_int depends on the number of bits in the target's
>> widest integer mode.  E.g. for i386 it's forced to 128, but for ARM
>> it's 512 (IIRC).
>>
>> Could you do the check based on the wi::min_precision of the unshifted
>> value?  I.e. see whether adding the shift amount to that gives a value
>> greater than type's precision.
>
> You could always use a FIXED_WIDE_INT like VRP does for its overflow
> detection stuff.

That would work too, but why impose an arbitrary limit?  Unless I'm
missing something, the code above should be equivalent to:

  unsigned int min_prec = (wi::min_precision (op0, SIGNED)
  			   + TREE_INT_CST_LOW (op1));
  bool overflowed = min_prec > TYPE_PRECISION (type0);
  if (overflowed && c_inhibit_evaluation_warnings == 0)
    warning_at (loc, OPT_Wshift_overflow,
             "result of %qE requires %u bits to represent, "
             "but %qT only has %u bits",
             build2_loc (loc, LSHIFT_EXPR, type0, op0, op1),
             min_prec, type0, TYPE_PRECISION (type0));

which seems simpler than anything involving wider precision.

Thanks,
Richard