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

[Jakub Jelinek <jakub at redhat dot com>]

On Mon, Nov 24, 2014 at 04:58:22PM +0100, Marek Polacek wrote:
> > 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.

Note, the above only with -m32 obviously, or supposedly for bar
you could change return type and the cast and type of r/s to
__PTRDIFF_TYPE__ or so.  foo, as gcc always casts the shift count to int,
would supposedly need to stay the way it is and might produce different
diagnostics between -m32 and -m64.
> 
> > > +
> > > +  /* 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 (?).

I meant for the above computation, there you don't check any overflow.

	Jakub