This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

C++ PATCH for c++/91264 - detect modifying const objects in constexpr


One of the features of constexpr is that it doesn't allow UB; and such UB must
be detected at compile-time.  So running your code in a context that requires
a constant expression should ensure that the code in question is free of UB.
In effect, constexpr can serve as a sanitizer.  E.g. this article describes in
in more detail:
<https://shafik.github.io/c++/undefined%20behavior/2019/05/11/explporing_undefined_behavior_using_constexpr.html>

[dcl.type.cv]p4 says "Any attempt to modify a const object during its lifetime
results in undefined behavior." However, as the article above points out, we
aren't detecting that case in constexpr evaluation.

This patch fixes that.  It's not that easy, though, because we have to keep in
mind [class.ctor]p5:
"A constructor can be invoked for a const, volatile or const volatile object.
const and volatile semantics are not applied on an object under construction.
They come into effect when the constructor for the most derived object ends."

I handled this by keeping a hash set which tracks objects under construction.
I considered other options, such as going up call_stack, but that wouldn't
work with trivial constructor/op=.  It was also interesting to find out that
the definition of TREE_HAS_CONSTRUCTOR says "When appearing in a FIELD_DECL,
it means that this field has been duly initialized in its constructor" though
nowhere in the codebase do we set TREE_HAS_CONSTRUCTOR on a FIELD_DECL as far
as I can see.  Unfortunately, using this bit proved useless for my needs here.

Also, be mindful of mutable subobjects.

Does this approach look like an appropriate strategy for tracking objects'
construction?

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

2019-07-31  Marek Polacek  <polacek@redhat.com>

	PR c++/91264 - detect modifying const objects in constexpr.
	* constexpr.c (objects_under_construction): New hash set.
	(new_object_under_construction): New.
	(remove_object_under_construction): New.
	(object_under_construction_p): New.
	(modifying_const_object_error): New.
	(cxx_eval_call_expression): Save the objects being constructed
	into the hash set.
	(modifying_const_object_p): New.
	(cxx_eval_store_expression): Detect modifying a const object
	during constant expression evaluation.
	(cxx_eval_increment_expression): Use a better location when building
	up the store.

	* g++.dg/cpp1y/constexpr-tracking-const1.C: New test.
	* g++.dg/cpp1y/constexpr-tracking-const2.C: New test.
	* g++.dg/cpp1y/constexpr-tracking-const3.C: New test.
	* g++.dg/cpp1y/constexpr-tracking-const4.C: New test.
	* g++.dg/cpp1y/constexpr-tracking-const5.C: New test.
	* g++.dg/cpp1y/constexpr-tracking-const6.C: New test.
	* g++.dg/cpp1y/constexpr-tracking-const7.C: New test.
	* g++.dg/cpp1y/constexpr-tracking-const8.C: New test.

diff --git gcc/cp/constexpr.c gcc/cp/constexpr.c
index c1b8b9b8a5d..e1ee72f0343 100644
--- gcc/cp/constexpr.c
+++ gcc/cp/constexpr.c
@@ -1575,6 +1575,59 @@ clear_no_implicit_zero (tree ctor)
     }
 }
 
+/* A hash set used for tracking objects under construction.  This is used
+   for detecting modifying constant objects during constant expression
+   evaluation.  */
+static GTY(()) hash_set<tree> *objects_under_construction;
+
+/* Add OBJ to the hash set above.  */
+
+static void
+new_object_under_construction (const_tree obj)
+{
+  if (!CLASS_TYPE_P (TREE_TYPE (obj))
+      /* Modifying non-const objects is allowed any time.  */
+      || !CP_TYPE_CONST_P (TREE_TYPE (obj)))
+    return;
+
+  if (!objects_under_construction)
+    objects_under_construction = hash_set<tree>::create_ggc (3);
+  objects_under_construction->add (CONST_CAST_TREE (obj));
+}
+
+/* Remove OBJ from the hash set.  */
+
+static void
+remove_object_under_construction (const_tree obj)
+{
+  if (objects_under_construction)
+    objects_under_construction->remove (CONST_CAST_TREE (obj));
+}
+
+/* Return true if OBJ is an object currently under construction.  */
+
+static bool
+object_under_construction_p (const_tree obj)
+{
+  if (!CLASS_TYPE_P (TREE_TYPE (obj)))
+    return false;
+
+  return (objects_under_construction
+	  && objects_under_construction->contains (CONST_CAST_TREE (obj)));
+}
+
+/* Complain about a const object OBJ being modified in a constant expression.
+   EXPR is the MODIFY_EXPR expression performing the modification.  */
+
+static void
+modifying_const_object_error (tree expr, tree obj)
+{
+  location_t loc = cp_expr_loc_or_loc (expr, input_location);
+  error_at (loc, "modifying a const object %qE is not allowed in "
+	    "a constant expression", TREE_OPERAND (expr, 0));
+  inform (location_of (obj), "originally declared %<const%> here");
+}
+
 /* Subroutine of cxx_eval_constant_expression.
    Evaluate the call expression tree T in the context of OLD_CALL expression
    evaluation.  */
@@ -1694,8 +1747,12 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t,
 	    op = build1 (INDIRECT_REF, TREE_TYPE (TREE_TYPE (op)), op);
 	  tree set = build2 (MODIFY_EXPR, TREE_TYPE (op), op, init);
 	  new_ctx.call = &new_call;
-	  return cxx_eval_constant_expression (&new_ctx, set, lval,
+	  /* We're constructing object OP; save that.  */
+	  new_object_under_construction (op);
+	  init = cxx_eval_constant_expression (&new_ctx, set, lval,
 					       non_constant_p, overflow_p);
+	  remove_object_under_construction (op);
+	  return init;
 	}
     }
 
@@ -1775,6 +1832,20 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t,
 
   depth_ok = push_cx_call_context (t);
 
+  /* Remember the object we are constructing.  */
+  tree new_obj = NULL_TREE;
+  if (DECL_CONSTRUCTOR_P (fun))
+    {
+      /* In a constructor, it should be the first `this' argument.
+	 At this point it has already been evaluated in the call
+	 to cxx_bind_parameters_in_call.  */
+      new_obj = TREE_VEC_ELT (new_call.bindings, 0);
+      STRIP_NOPS (new_obj);
+      if (TREE_CODE (new_obj) == ADDR_EXPR)
+	new_obj = TREE_OPERAND (new_obj, 0);
+      new_object_under_construction (new_obj);
+    }
+
   tree result = NULL_TREE;
 
   constexpr_call *entry = NULL;
@@ -1910,6 +1981,11 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t,
 		}
 	    }
 
+	  /* At this point, the object's constructor will have run, so
+	     it's no longer under construction.  */
+	  if (new_obj)
+	    remove_object_under_construction (new_obj);
+
 	  /* Forget the saved values of the callee's SAVE_EXPRs.  */
 	  unsigned int i;
 	  tree save_expr;
@@ -3724,6 +3800,35 @@ maybe_simplify_trivial_copy (tree &target, tree &init)
     }
 }
 
+/* Return true if we are modifying something that is const during constant
+   expression evaluation.  CODE is the code of the statement, OBJ is the
+   object in question, MUTABLE_P is true if one of the subobjects were
+   declared mutable.  */
+
+static bool
+modifying_const_object_p (tree_code code, tree obj, bool mutable_p)
+{
+  /* If this is initialization, there's no problem.  */
+  if (code != MODIFY_EXPR)
+    return false;
+
+  tree type = TREE_TYPE (obj);
+
+  /* [basic.type.qualifier] "A const object is an object of type
+     const T or a non-mutable subobject of a const object."  */
+  return ((TREE_READONLY (obj) || CP_TYPE_CONST_P (type)
+	   /* If it's an aggregate and any field is const, then it is
+	      effectively const.  */
+	   || (CLASS_TYPE_P (type) && C_TYPE_FIELDS_READONLY (type)))
+	  && !mutable_p
+	  /* [class.ctor]p5 "A constructor can be invoked for a const,
+	     volatile, or const volatile object.  const and volatile
+	     semantics are not applied on an object under construction.
+	     They come into effect when the constructor for the most
+	     derived object ends."  */
+	  && !object_under_construction_p (obj));
+}
+
 /* Evaluate an INIT_EXPR or MODIFY_EXPR.  */
 
 static tree
@@ -3773,6 +3878,7 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
   /* Find the underlying variable.  */
   releasing_vec refs;
   tree object = NULL_TREE;
+  bool mutable_p = false;
   for (tree probe = target; object == NULL_TREE; )
     {
       switch (TREE_CODE (probe))
@@ -3783,6 +3889,16 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
 	  {
 	    tree ob = TREE_OPERAND (probe, 0);
 	    tree elt = TREE_OPERAND (probe, 1);
+	    if (DECL_P (elt) && DECL_MUTABLE_P (elt))
+	      mutable_p = true;
+	    if (evaluated
+		&& modifying_const_object_p (TREE_CODE (t), probe, mutable_p))
+	      {
+		if (!ctx->quiet)
+		  modifying_const_object_error (t, probe);
+		*non_constant_p = true;
+		return t;
+	      }
 	    if (TREE_CODE (probe) == ARRAY_REF)
 	      {
 		elt = eval_and_check_array_index (ctx, probe, false,
@@ -3811,6 +3927,14 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
 	}
     }
 
+  if (modifying_const_object_p (TREE_CODE (t), object, mutable_p))
+    {
+      if (!ctx->quiet)
+	modifying_const_object_error (t, object);
+      *non_constant_p = true;
+      return t;
+    }
+
   /* And then find/build up our initializer for the path to the subobject
      we're initializing.  */
   tree *valp;
@@ -4063,7 +4187,8 @@ cxx_eval_increment_expression (const constexpr_ctx *ctx, tree t,
     VERIFY_CONSTANT (mod);
 
   /* Storing the modified value.  */
-  tree store = build2 (MODIFY_EXPR, type, op, mod);
+  tree store = build2_loc (cp_expr_loc_or_loc (t, input_location),
+			   MODIFY_EXPR, type, op, mod);
   cxx_eval_constant_expression (ctx, store,
 				true, non_constant_p, overflow_p);
   ggc_free (store);
diff --git gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const1.C gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const1.C
new file mode 100644
index 00000000000..e081a535659
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const1.C
@@ -0,0 +1,72 @@
+// PR c++/91264
+// { dg-do compile { target c++14 } }
+
+constexpr void
+mod (int &r)
+{
+  r = 99; // { dg-error "modifying a const object" }
+}
+
+constexpr int
+fn1 ()
+{
+  const int i = 0; // { dg-message "originally declared" }
+  mod (const_cast<int &>(i)); // { dg-message "in .constexpr. expansion of " }
+  return i;
+}
+
+constexpr int i1 = fn1 (); // { dg-message "in .constexpr. expansion of " }
+
+constexpr int
+fn2 ()
+{
+  const int i = 5; // { dg-message "originally declared" }
+  const_cast<int &>(i) = 10; // { dg-error "modifying a const object" }
+  return i;
+}
+
+constexpr int i2 = fn2 (); // { dg-message "in .constexpr. expansion of " }
+
+constexpr int
+fn3 ()
+{
+  const int i = 5; // { dg-message "originally declared" }
+  ++const_cast<int &>(i); // { dg-error "modifying a const object" }
+  return i;
+}
+
+constexpr int i3 = fn3 (); // { dg-message "in .constexpr. expansion of " }
+
+constexpr int
+fn4 ()
+{
+  const int i = 5; // { dg-message "originally declared" }
+  const_cast<int &>(i)--; // { dg-error "modifying a const object" }
+  return i;
+}
+
+constexpr int i4 = fn4 (); // { dg-message "in .constexpr. expansion of " }
+
+constexpr int
+fn5 ()
+{
+  const int i = 5; // { dg-message "originally declared" }
+  const_cast<int &>(i) += 2; // { dg-error "modifying a const object" }
+  return i;
+}
+
+constexpr int i5 = fn5 (); // { dg-message "in .constexpr. expansion of " }
+
+constexpr int
+fn6 ()
+{
+  // This is OK.
+  int i = 3;
+  const int *cip = &i;
+  int *ip = const_cast<int *>(cip);
+  *ip = 4;
+  return i;
+}
+
+constexpr int i6 = fn6 ();
+static_assert(i6 == 4, "");
diff --git gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const2.C gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const2.C
new file mode 100644
index 00000000000..9803309cace
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const2.C
@@ -0,0 +1,23 @@
+// PR c++/91264
+// { dg-do compile { target c++14 } }
+
+struct X {
+  int j;
+  constexpr X() : j(0) { }
+};
+
+struct Y {
+  X x;
+  constexpr Y() : x{} { }
+};
+
+constexpr void
+g ()
+{
+  const Y y; // { dg-message "originally declared" }
+  Y *p = const_cast<Y *>(&y);
+  p->x.j = 99; // { dg-error "modifying a const object" }
+}
+
+static_assert((g() , 1), ""); // { dg-error "non-constant condition" }
+// { dg-message "in 'constexpr' expansion of" "" { target *-*-* } .-1 }
diff --git gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const3.C gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const3.C
new file mode 100644
index 00000000000..6853775c1e2
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const3.C
@@ -0,0 +1,22 @@
+// PR c++/91264
+// { dg-do compile { target c++14 } }
+
+struct A {
+  int n;
+  constexpr A() : n(1) { n = 2; }
+};
+
+struct B {
+  const A a;
+  constexpr B(bool b) {
+    if (b)
+      const_cast<A &>(a).n = 3; // { dg-error "modifying a const object" }
+    }
+};
+
+constexpr B b(false);
+static_assert(b.a.n == 2, "");
+
+constexpr B b2(true); // { dg-message "in .constexpr. expansion of " }
+// { dg-message "originally declared" "" { target *-*-* } .-1 } 
+static_assert((b2.a.n, 1), "");
diff --git gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const4.C gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const4.C
new file mode 100644
index 00000000000..8263a7cc505
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const4.C
@@ -0,0 +1,17 @@
+// PR c++/91264
+// { dg-do compile { target c++14 } }
+
+struct A {
+  const int n;
+  constexpr A() : n(1) { }
+};
+struct B {
+  A a;
+  constexpr B() {
+    int *p = const_cast<int *>(&a.n);
+    *p = 3; // { dg-error "modifying a const object" }
+  }
+};
+constexpr B b; // { dg-message "in .constexpr. expansion of " }
+// { dg-message "originally declared" "" { target *-*-* } .-1 }
+static_assert((b.a.n, 1), "");
diff --git gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const5.C gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const5.C
new file mode 100644
index 00000000000..bea54fb4fde
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const5.C
@@ -0,0 +1,17 @@
+// PR c++/91264
+// { dg-do compile { target c++14 } }
+
+struct A {
+  mutable int n;
+  constexpr A() : n(1) { n = 2; }
+};
+
+struct B {
+  const A a;
+  constexpr B() {
+    const_cast<A &>(a).n = 3;
+  }
+};
+
+constexpr B b{};
+static_assert((b.a.n, 1), "");
diff --git gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const6.C gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const6.C
new file mode 100644
index 00000000000..07a7f835e11
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const6.C
@@ -0,0 +1,22 @@
+// PR c++/91264
+// { dg-do compile { target c++14 } }
+
+struct X {
+  mutable int j;
+  constexpr X() : j(0) { }
+};
+
+struct Y {
+  X x;
+  constexpr Y() : x{} { }
+};
+
+constexpr void
+g ()
+{
+  const Y y;
+  Y *p = const_cast<Y *>(&y);
+  p->x.j = 99;
+}
+
+static_assert((g() , 1), "");
diff --git gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const7.C gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const7.C
new file mode 100644
index 00000000000..922e8ff126f
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const7.C
@@ -0,0 +1,23 @@
+// PR c++/91264
+// { dg-do compile { target c++14 } }
+
+struct D { int n; };
+
+struct C { const D d; };
+
+struct A {
+  C c;
+  constexpr A() : c{} { }
+};
+
+struct B {
+  A a;
+  constexpr B() {
+    int &r = const_cast<int &>(a.c.d.n);
+    r = 3; // { dg-error "modifying a const object" }
+  }
+};
+
+constexpr B b{}; // { dg-message "in .constexpr. expansion of " }
+// { dg-message "originally declared" "" { target *-*-* } .-1 }
+static_assert((b.a.c.d.n, 1), "");
diff --git gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const8.C gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const8.C
new file mode 100644
index 00000000000..2b3fe793f83
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const8.C
@@ -0,0 +1,23 @@
+// PR c++/91264
+// { dg-do compile { target c++14 } }
+
+struct B {
+  int i;
+  double d;
+};
+
+constexpr B bar()
+{
+    constexpr B b = {10,10.10}; // { dg-message "originally declared" }
+    B *p = const_cast<B*>(&b);
+
+    p->i = 11; // { dg-error "modifying a const object" }
+    p->d = 11.11;
+
+   return *p;
+}
+
+void foo()
+{  
+   constexpr B y = bar(); // { dg-message "in .constexpr. expansion of" }
+}


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]