Re: [C++ PATCH] Detect UB in shifts in constexpr functions

[Marek Polacek <polacek at redhat dot com>]

On Mon, Nov 24, 2014 at 04:49:25PM +0100, Jakub Jelinek wrote:
> On Mon, Nov 24, 2014 at 02:51:14PM +0100, Marek Polacek wrote:
> > --- gcc/cp/constexpr.c
> > +++ gcc/cp/constexpr.c
> > @@ -1451,6 +1451,43 @@ verify_constant (tree t, bool allow_non_constant, bool *non_constant_p,
> >    return *non_constant_p;
> >  }
> >  
> > +/* Return true if the shift operation on LHS and RHS is undefined.  */
> > +
> > +static bool
> > +cxx_eval_check_shift_p (enum tree_code code, tree lhs, tree rhs)
> > +{
> > +  if (code != LSHIFT_EXPR && code != RSHIFT_EXPR)
> > +    return false;
> > +
> > +  tree lhstype = TREE_TYPE (lhs);
> > +  unsigned HOST_WIDE_INT uprec = TYPE_PRECISION (TREE_TYPE (lhs));
> > +
> > +  /* [expr.shift] The behavior is undefined if the right operand
> > +     is negative, or greater than or equal to the length in bits
> > +     of the promoted left operand.  */
> > +  if (tree_int_cst_sgn (rhs) == -1 || compare_tree_int (rhs, uprec) >= 0)
> > +    return true;
> 
> I think VERIFY_CONSTANT doesn't guarantee both operands are INTEGER_CSTs.

Oh well.  I ran the testsuite with an assert checking that I always have
INTEGER_CSTs and didn't see any ICEs.

> Consider say:
> 
> constexpr int p = 1;
> constexpr int foo (int a)
> {
>   return a << (int) &p;
> }
> constexpr int bar (int a)
> {
>   return ((int) &p) << a;
> }
> constexpr int q = foo (5);
> constexpr int r = bar (2);
> constexpr int s = bar (0);
> 
> Now, for foo (5) and bar (2) fold_binary_loc returns NULL and thus
> your cxx_eval_check_shift_p is not called, for bar (0) it returns
> non-NULL and while the result still is not a constant expression and
> right now is diagnosed, with your patch it would ICE.
> 
> So, I'd just return false if either lhs or rhs are not INTEGER_CSTs.
 
Ok, I'll add that.  Thank for pointing that out.

> > +
> > +  /* The value of E1 << E2 is E1 left-shifted E2 bit positions; [...]
> > +     if E1 has a signed type and non-negative value, and E1x2^E2 is
> > +     representable in the corresponding unsigned type of the result type,
> > +     then that value, converted to the result type, is the resulting value;
> > +     otherwise, the behavior is undefined.  */
> > +  if (code == LSHIFT_EXPR && !TYPE_UNSIGNED (lhstype))
> > +    {
> > +      if (tree_int_cst_sgn (lhs) == -1)
> > +	return true;
> > +      tree t = build_int_cst (unsigned_type_node, uprec - 1);
> > +      t = fold_build2 (MINUS_EXPR, unsigned_type_node, t, rhs);
> > +      tree ulhs = fold_convert (unsigned_type_for (lhstype), lhs);
> > +      t = fold_build2 (RSHIFT_EXPR, TREE_TYPE (ulhs), ulhs, t);
> > +      if (tree_int_cst_lt (integer_one_node, t))
> > +	return true;
> 
> I'll leave to Jason whether this shouldn't be using the various
> cxx_eval_*_expression calls instead, or perhaps int_const_binop or wide_int
> stuff directly.

ISTR int_const_binop calls wide_int routines wi::rshift/wi::lshift and these
return 0 and do not have any overflow flag, so that might not help (?).

	Marek