diff mbox

[C++] Detect UB in shifts in constexpr functions

Message ID 20141202141426.GQ15555@redhat.com
State New
Headers show

Commit Message

Marek Polacek Dec. 2, 2014, 2:14 p.m. UTC
On Fri, Nov 28, 2014 at 09:28:03AM -0500, Jason Merrill wrote:
> I was thinking even more detailed: one diagnostic for negative count, one
> diagnostic for count larger than the precision of the lhs, and then a third
> for overflow.
 
Alright, done.

> >+      /* For signed x << y the following:
> >+	 (unsigned) x >> ((prec (lhs) - 1) - y)
> >+	 if > 1, is undefined.  */
> 
> I meant briefly explaining where that formula comes from.

This was a little bit problematic, because I don't really know where
that formula comes from.  We use it in c-ubsan.c, so I've just taken
it from there.  It was Jakub who came with that formula, so maybe he
remembers its origin.  I wrote something more to the comment describing
it, even though that might not be enough.

Another change I made is that I've added missing cxx11 check for the
signed x << case.

Bootstrapped/regtested on ppc64-linux, ok for trunk?

2014-12-02  Marek Polacek  <polacek@redhat.com>

	* constexpr.c (cxx_eval_check_shift_p): New function.
	(cxx_eval_binary_expression): Call it.

	* g++.dg/cpp0x/constexpr-shift1.C: New test.
	* g++.dg/cpp1y/constexpr-shift1.C: New test.
	* g++.dg/ubsan/pr63956.C: Add dg-errors.


	Marek

Comments

Jason Merrill Dec. 2, 2014, 2:59 p.m. UTC | #1
On 12/02/2014 09:14 AM, Marek Polacek wrote:
> On Fri, Nov 28, 2014 at 09:28:03AM -0500, Jason Merrill wrote:
>> I was thinking even more detailed: one diagnostic for negative count, one
>> diagnostic for count larger than the precision of the lhs, and then a third
>> for overflow.
>
> Alright, done.

Thanks.  These errors also need to be conditional on (!ctx->quiet), and 
if we see one of these conditions we need to set *non_constant_p.

> This was a little bit problematic, because I don't really know where
> that formula comes from.  We use it in c-ubsan.c, so I've just taken
> it from there.  It was Jakub who came with that formula, so maybe he
> remembers its origin.  I wrote something more to the comment describing
> it, even though that might not be enough.

Looks good.

Jason
diff mbox

Patch

diff --git gcc/cp/constexpr.c gcc/cp/constexpr.c
index 2184a2f..556c422 100644
--- gcc/cp/constexpr.c
+++ gcc/cp/constexpr.c
@@ -1470,6 +1470,73 @@  verify_constant (tree t, bool allow_non_constant, bool *non_constant_p,
   return *non_constant_p;
 }
 
+/* Check whether the shift operation with code CODE and type TYPE on LHS
+   and RHS is undefined.  If it is, explain why and give an error.  */
+
+static void
+cxx_eval_check_shift_p (location_t loc, enum tree_code code, tree type,
+			tree lhs, tree rhs)
+{
+  if ((code != LSHIFT_EXPR && code != RSHIFT_EXPR)
+      || TREE_CODE (lhs) != INTEGER_CST
+      || TREE_CODE (rhs) != INTEGER_CST)
+    return;
+
+  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)
+    {
+      error_at (loc, "right operand of shift expression %q+E is negative",
+		build2_loc (loc, code, type, lhs, rhs));
+      return;
+    }
+  if (compare_tree_int (rhs, uprec) >= 0)
+    {
+      error_at (loc, "right operand of shift expression %q+E is >= than "
+		"the precision of the left operand",
+		build2_loc (loc, code, type, lhs, rhs));
+      return;
+    }
+
+  /* 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)
+      && (cxx_dialect >= cxx11))
+    {
+      if (tree_int_cst_sgn (lhs) == -1)
+	{
+	  error_at (loc, "left operand of shift expression %q+E is negative",
+		    build2_loc (loc, code, type, lhs, rhs));
+	  return;
+	}
+      /* For signed x << y the following:
+	 (unsigned) x >> ((prec (lhs) - 1) - y)
+	 if > 1, is undefined.  The right-hand side of this formula
+	 is the highest bit of the LHS that can be set (starting from 0),
+	 so that the shift doesn't overflow.  We then right-shift the LHS
+	 to see whether any other bit is set making the original shift
+	 undefined -- the result is not representable in the corresponding
+	 unsigned type.  */
+      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))
+	{
+	  error_at (loc, "shift expression %q+E overflows",
+		    build2_loc (loc, code, type, lhs, rhs));
+	  return;
+	}
+    }
+}
+
 /* Subroutine of cxx_eval_constant_expression.
    Attempt to reduce the unary expression tree T to a compile time value.
    If successful, return the value.  Otherwise issue a diagnostic
@@ -1532,6 +1599,8 @@  cxx_eval_binary_expression (const constexpr_ctx *ctx, tree t,
       else
 	r = build2_loc (loc, code, type, lhs, rhs);
     }
+  else
+    cxx_eval_check_shift_p (loc, code, type, lhs, rhs);
   VERIFY_CONSTANT (r);
   return r;
 }
diff --git gcc/testsuite/g++.dg/cpp0x/constexpr-shift1.C gcc/testsuite/g++.dg/cpp0x/constexpr-shift1.C
index e69de29..1f4ee73 100644
--- gcc/testsuite/g++.dg/cpp0x/constexpr-shift1.C
+++ gcc/testsuite/g++.dg/cpp0x/constexpr-shift1.C
@@ -0,0 +1,73 @@ 
+// { dg-do compile { target c++11 } }
+
+constexpr int
+fn1 (int i, int j)
+{
+  return i << j; // { dg-error "is negative" }
+}
+
+constexpr int i1 = fn1 (1, -1);
+
+constexpr int
+fn2 (int i, int j)
+{
+  return i << j; // { dg-error "is >= than the precision of the left operand" }
+}
+
+constexpr int i2 = fn2 (1, 200);
+
+constexpr int
+fn3 (int i, int j)
+{
+  return i << j; // { dg-error "is negative" }
+}
+
+constexpr int i3 = fn3 (-1, 2);
+
+constexpr int
+fn4 (int i, int j)
+{
+  return i << j; // { dg-error "overflows" }
+}
+
+constexpr int i4 = fn4 (__INT_MAX__, 2);
+
+constexpr int
+fn5 (int i, int j)
+{
+  return i << j;
+}
+
+constexpr int i5 = fn5 (__INT_MAX__, 1);
+
+constexpr int
+fn6 (unsigned int i, unsigned int j)
+{
+  return i << j; // { dg-error "is >= than the precision of the left operand" }
+}
+
+constexpr int i6 = fn6 (1, -1);
+
+constexpr int
+fn7 (int i, int j)
+{
+  return i >> j; // { dg-error "is negative" }
+}
+
+constexpr int i7 = fn7 (1, -1);
+
+constexpr int
+fn8 (int i, int j)
+{
+  return i >> j;
+}
+
+constexpr int i8 = fn8 (-1, 1);
+
+constexpr int
+fn9 (int i, int j)
+{
+  return i >> j;  // { dg-error "is >= than the precision of the left operand" }
+}
+
+constexpr int i9 = fn9 (1, 200);
diff --git gcc/testsuite/g++.dg/cpp1y/constexpr-shift1.C gcc/testsuite/g++.dg/cpp1y/constexpr-shift1.C
index e69de29..a739fd2 100644
--- gcc/testsuite/g++.dg/cpp1y/constexpr-shift1.C
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-shift1.C
@@ -0,0 +1,9 @@ 
+// { dg-do compile { target c++14 } }
+
+constexpr int p = 1;
+constexpr __PTRDIFF_TYPE__ bar (int a)
+{
+  return ((__PTRDIFF_TYPE__) &p) << a; // { dg-error "is not a constant expression" }
+}
+constexpr __PTRDIFF_TYPE__ r = bar (2);
+constexpr __PTRDIFF_TYPE__ s = bar (0); // { dg-error "conversion from pointer" }
diff --git gcc/testsuite/g++.dg/ubsan/pr63956.C gcc/testsuite/g++.dg/ubsan/pr63956.C
index adfb55f..185a719 100644
--- gcc/testsuite/g++.dg/ubsan/pr63956.C
+++ gcc/testsuite/g++.dg/ubsan/pr63956.C
@@ -14,11 +14,11 @@  fn1 (int a, int b)
 }
 
 constexpr int i1 = fn1 (5, 3);
-constexpr int i2 = fn1 (5, -2);
-constexpr int i3 = fn1 (5, sizeof (int) * __CHAR_BIT__);
-constexpr int i4 = fn1 (5, 256);
+constexpr int i2 = fn1 (5, -2); // { dg-error "is negative" }
+constexpr int i3 = fn1 (5, sizeof (int) * __CHAR_BIT__); // { dg-error "is >= than the precision of the left operand" }
+constexpr int i4 = fn1 (5, 256); // { dg-error "is >= than the precision of the left operand" }
 constexpr int i5 = fn1 (5, 2);
-constexpr int i6 = fn1 (-2, 4);
+constexpr int i6 = fn1 (-2, 4); // { dg-error "is negative" }
 constexpr int i7 = fn1 (0, 2);
 
 SA (i1 == 40);
@@ -34,9 +34,9 @@  fn2 (int a, int b)
 }
 
 constexpr int j1 = fn2 (4, 1);
-constexpr int j2 = fn2 (4, -1);
-constexpr int j3 = fn2 (10, sizeof (int) * __CHAR_BIT__);
-constexpr int j4 = fn2 (1, 256);
+constexpr int j2 = fn2 (4, -1); // { dg-error "is negative" }
+constexpr int j3 = fn2 (10, sizeof (int) * __CHAR_BIT__); // { dg-error "is >= than the precision of the left operand" }
+constexpr int j4 = fn2 (1, 256); // { dg-error "is >= than the precision of the left operand" }
 constexpr int j5 = fn2 (5, 2);
 constexpr int j6 = fn2 (-2, 4);
 constexpr int j7 = fn2 (0, 4);