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

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

[Marek Polacek]

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" }
+}

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

[Jason Merrill]

On 7/31/19 3:26 PM, Marek Polacek wrote:
> 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?

For scalar objects, we should be able to rely on INIT_EXPR vs. 
MODIFY_EXPR to distinguish between initialization and modification; for 
class objects, I wonder about setting a flag on the CONSTRUCTOR after 
initialization is complete to indicate that the value is now constant.

Jason

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

[Marek Polacek]

On Mon, Aug 05, 2019 at 03:54:19PM -0400, Jason Merrill wrote:
> On 7/31/19 3:26 PM, Marek Polacek wrote:
> > 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?
> 
> For scalar objects, we should be able to rely on INIT_EXPR vs. MODIFY_EXPR
> to distinguish between initialization and modification; for class objects, I

This is already true: only class object go into the hash set.

> wonder about setting a flag on the CONSTRUCTOR after initialization is
> complete to indicate that the value is now constant.

But here we're not dealing with CONSTRUCTORs in the gcc sense (i.e. exprs with
TREE_CODE == CONSTRUCTOR).  We have a CALL_EXPR like Y::Y ((struct Y *) &y),
which initializes the object "y".  Setting a flag on the CALL_EXPR or its underlying
function decl wouldn't help.  (Also, all 6 TREE_LANG_FLAGs for a CONSTRUCTOR
are used.)

Am I missing something?

Marek

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

[Jason Merrill]

On 8/6/19 3:20 PM, Marek Polacek wrote:
> On Mon, Aug 05, 2019 at 03:54:19PM -0400, Jason Merrill wrote:
>> On 7/31/19 3:26 PM, Marek Polacek wrote:
>>> 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?
>>
>> For scalar objects, we should be able to rely on INIT_EXPR vs. MODIFY_EXPR
>> to distinguish between initialization and modification; for class objects, I
> 
> This is already true: only class object go into the hash set.
> 
>> wonder about setting a flag on the CONSTRUCTOR after initialization is
>> complete to indicate that the value is now constant.
> 
> But here we're not dealing with CONSTRUCTORs in the gcc sense (i.e. exprs with
> TREE_CODE == CONSTRUCTOR).  We have a CALL_EXPR like Y::Y ((struct Y *) &y),
> which initializes the object "y".  Setting a flag on the CALL_EXPR or its underlying
> function decl wouldn't help.
> 
> Am I missing something?

I was thinking that where in your current patch you call 
remove_object_under_construction, we could instead mark the object's 
value CONSTRUCTOR as immutable.

>   (Also, all 6 TREE_LANG_FLAGs for a CONSTRUCTOR are used.)

TREE_READONLY seems suitable.

Jason

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

[Marek Polacek]

On Thu, Aug 08, 2019 at 11:06:17AM -0400, Jason Merrill wrote:
> On 8/6/19 3:20 PM, Marek Polacek wrote:
> > On Mon, Aug 05, 2019 at 03:54:19PM -0400, Jason Merrill wrote:
> > > On 7/31/19 3:26 PM, Marek Polacek wrote:
> > > > 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?
> > > 
> > > For scalar objects, we should be able to rely on INIT_EXPR vs. MODIFY_EXPR
> > > to distinguish between initialization and modification; for class objects, I
> > 
> > This is already true: only class object go into the hash set.
> > 
> > > wonder about setting a flag on the CONSTRUCTOR after initialization is
> > > complete to indicate that the value is now constant.
> > 
> > But here we're not dealing with CONSTRUCTORs in the gcc sense (i.e. exprs with
> > TREE_CODE == CONSTRUCTOR).  We have a CALL_EXPR like Y::Y ((struct Y *) &y),
> > which initializes the object "y".  Setting a flag on the CALL_EXPR or its underlying
> > function decl wouldn't help.
> > 
> > Am I missing something?
> 
> I was thinking that where in your current patch you call
> remove_object_under_construction, we could instead mark the object's value
> CONSTRUCTOR as immutable.

Ah, what you meant was to look at DECL_INITIAL of the object we're
constructing, which could be a CONSTRUCTOR.  Unfortunately, this
DECL_INITIAL is null (in all the new tests when doing
remove_object_under_construction), so there's nothing to mark as TREE_READONLY :/.

Marek

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

[Jason Merrill]

On Thu, Aug 8, 2019 at 3:25 PM Marek Polacek <polacek@redhat.com> wrote:
>
> On Thu, Aug 08, 2019 at 11:06:17AM -0400, Jason Merrill wrote:
> > On 8/6/19 3:20 PM, Marek Polacek wrote:
> > > On Mon, Aug 05, 2019 at 03:54:19PM -0400, Jason Merrill wrote:
> > > > On 7/31/19 3:26 PM, Marek Polacek wrote:
> > > > > 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?
> > > >
> > > > For scalar objects, we should be able to rely on INIT_EXPR vs. MODIFY_EXPR
> > > > to distinguish between initialization and modification; for class objects, I
> > >
> > > This is already true: only class object go into the hash set.
> > >
> > > > wonder about setting a flag on the CONSTRUCTOR after initialization is
> > > > complete to indicate that the value is now constant.
> > >
> > > But here we're not dealing with CONSTRUCTORs in the gcc sense (i.e. exprs with
> > > TREE_CODE == CONSTRUCTOR).  We have a CALL_EXPR like Y::Y ((struct Y *) &y),
> > > which initializes the object "y".  Setting a flag on the CALL_EXPR or its underlying
> > > function decl wouldn't help.
> > >
> > > Am I missing something?
> >
> > I was thinking that where in your current patch you call
> > remove_object_under_construction, we could instead mark the object's value
> > CONSTRUCTOR as immutable.
>
> Ah, what you meant was to look at DECL_INITIAL of the object we're
> constructing, which could be a CONSTRUCTOR.  Unfortunately, this
> DECL_INITIAL is null (in all the new tests when doing
> remove_object_under_construction), so there's nothing to mark as TREE_READONLY :/.

There's a value in ctx->values, isn't there?

Jason

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

[Marek Polacek]

On Wed, Aug 14, 2019 at 02:50:13PM -0400, Jason Merrill wrote:
> On Thu, Aug 8, 2019 at 3:25 PM Marek Polacek <polacek@redhat.com> wrote:
> >
> > On Thu, Aug 08, 2019 at 11:06:17AM -0400, Jason Merrill wrote:
> > > On 8/6/19 3:20 PM, Marek Polacek wrote:
> > > > On Mon, Aug 05, 2019 at 03:54:19PM -0400, Jason Merrill wrote:
> > > > > On 7/31/19 3:26 PM, Marek Polacek wrote:
> > > > > > 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?
> > > > >
> > > > > For scalar objects, we should be able to rely on INIT_EXPR vs. MODIFY_EXPR
> > > > > to distinguish between initialization and modification; for class objects, I
> > > >
> > > > This is already true: only class object go into the hash set.
> > > >
> > > > > wonder about setting a flag on the CONSTRUCTOR after initialization is
> > > > > complete to indicate that the value is now constant.
> > > >
> > > > But here we're not dealing with CONSTRUCTORs in the gcc sense (i.e. exprs with
> > > > TREE_CODE == CONSTRUCTOR).  We have a CALL_EXPR like Y::Y ((struct Y *) &y),
> > > > which initializes the object "y".  Setting a flag on the CALL_EXPR or its underlying
> > > > function decl wouldn't help.
> > > >
> > > > Am I missing something?
> > >
> > > I was thinking that where in your current patch you call
> > > remove_object_under_construction, we could instead mark the object's value
> > > CONSTRUCTOR as immutable.
> >
> > Ah, what you meant was to look at DECL_INITIAL of the object we're
> > constructing, which could be a CONSTRUCTOR.  Unfortunately, this
> > DECL_INITIAL is null (in all the new tests when doing
> > remove_object_under_construction), so there's nothing to mark as TREE_READONLY :/.
> 
> There's a value in ctx->values, isn't there?

Doesn't seem to be the case for e.g.

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, "");

Here we're constructing "b", its ctx->values->get(new_obj) is initially
"{}".  In the middle of constructing "b", we construct "b.a", but that
has nothing in ctx->values.

Marek

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

[Jason Merrill]

On 8/15/19 5:34 PM, Marek Polacek wrote:
> On Wed, Aug 14, 2019 at 02:50:13PM -0400, Jason Merrill wrote:
>> On Thu, Aug 8, 2019 at 3:25 PM Marek Polacek <polacek@redhat.com> wrote:
>>>
>>> On Thu, Aug 08, 2019 at 11:06:17AM -0400, Jason Merrill wrote:
>>>> On 8/6/19 3:20 PM, Marek Polacek wrote:
>>>>> On Mon, Aug 05, 2019 at 03:54:19PM -0400, Jason Merrill wrote:
>>>>>> On 7/31/19 3:26 PM, Marek Polacek wrote:
>>>>>>> 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?
>>>>>>
>>>>>> For scalar objects, we should be able to rely on INIT_EXPR vs. MODIFY_EXPR
>>>>>> to distinguish between initialization and modification; for class objects, I
>>>>>
>>>>> This is already true: only class object go into the hash set.
>>>>>
>>>>>> wonder about setting a flag on the CONSTRUCTOR after initialization is
>>>>>> complete to indicate that the value is now constant.
>>>>>
>>>>> But here we're not dealing with CONSTRUCTORs in the gcc sense (i.e. exprs with
>>>>> TREE_CODE == CONSTRUCTOR).  We have a CALL_EXPR like Y::Y ((struct Y *) &y),
>>>>> which initializes the object "y".  Setting a flag on the CALL_EXPR or its underlying
>>>>> function decl wouldn't help.
>>>>>
>>>>> Am I missing something?
>>>>
>>>> I was thinking that where in your current patch you call
>>>> remove_object_under_construction, we could instead mark the object's value
>>>> CONSTRUCTOR as immutable.
>>>
>>> Ah, what you meant was to look at DECL_INITIAL of the object we're
>>> constructing, which could be a CONSTRUCTOR.  Unfortunately, this
>>> DECL_INITIAL is null (in all the new tests when doing
>>> remove_object_under_construction), so there's nothing to mark as TREE_READONLY :/.
>>
>> There's a value in ctx->values, isn't there?
> 
> Doesn't seem to be the case for e.g.
> 
> 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, "");
> 
> Here we're constructing "b", its ctx->values->get(new_obj) is initially
> "{}".  In the middle of constructing "b", we construct "b.a", but that
> has nothing in ctx->values.

Right, subobjects aren't in ctx->values.  In cxx_eval_call_expression we 
have

           if (DECL_CONSTRUCTOR_P (fun))
             /* This can be null for a subobject constructor call, in 

                which case what we care about is the initialization 

                side-effects rather than the value.  We could get at the 

                value by evaluating *this, but we don't bother; there's 

                no need to put such a call in the hash table.  */
             result = lval ? ctx->object : ctx->ctor;

Your patch already finds *this (b.a) and puts it in new_obj; if it's 
const we can evaluate it to get the CONSTRUCTOR to set TREE_READONLY on.

Jason

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

[Marek Polacek]

On Thu, Aug 15, 2019 at 08:21:25PM -0400, Jason Merrill wrote:
> On 8/15/19 5:34 PM, Marek Polacek wrote:
> > On Wed, Aug 14, 2019 at 02:50:13PM -0400, Jason Merrill wrote:
> > > On Thu, Aug 8, 2019 at 3:25 PM Marek Polacek <polacek@redhat.com> wrote:
> > > > 
> > > > On Thu, Aug 08, 2019 at 11:06:17AM -0400, Jason Merrill wrote:
> > > > > On 8/6/19 3:20 PM, Marek Polacek wrote:
> > > > > > On Mon, Aug 05, 2019 at 03:54:19PM -0400, Jason Merrill wrote:
> > > > > > > On 7/31/19 3:26 PM, Marek Polacek wrote:
> > > > > > > > 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?
> > > > > > > 
> > > > > > > For scalar objects, we should be able to rely on INIT_EXPR vs. MODIFY_EXPR
> > > > > > > to distinguish between initialization and modification; for class objects, I
> > > > > > 
> > > > > > This is already true: only class object go into the hash set.
> > > > > > 
> > > > > > > wonder about setting a flag on the CONSTRUCTOR after initialization is
> > > > > > > complete to indicate that the value is now constant.
> > > > > > 
> > > > > > But here we're not dealing with CONSTRUCTORs in the gcc sense (i.e. exprs with
> > > > > > TREE_CODE == CONSTRUCTOR).  We have a CALL_EXPR like Y::Y ((struct Y *) &y),
> > > > > > which initializes the object "y".  Setting a flag on the CALL_EXPR or its underlying
> > > > > > function decl wouldn't help.
> > > > > > 
> > > > > > Am I missing something?
> > > > > 
> > > > > I was thinking that where in your current patch you call
> > > > > remove_object_under_construction, we could instead mark the object's value
> > > > > CONSTRUCTOR as immutable.
> > > > 
> > > > Ah, what you meant was to look at DECL_INITIAL of the object we're
> > > > constructing, which could be a CONSTRUCTOR.  Unfortunately, this
> > > > DECL_INITIAL is null (in all the new tests when doing
> > > > remove_object_under_construction), so there's nothing to mark as TREE_READONLY :/.
> > > 
> > > There's a value in ctx->values, isn't there?
> > 
> > Doesn't seem to be the case for e.g.
> > 
> > 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, "");
> > 
> > Here we're constructing "b", its ctx->values->get(new_obj) is initially
> > "{}".  In the middle of constructing "b", we construct "b.a", but that
> > has nothing in ctx->values.
> 
> Right, subobjects aren't in ctx->values.  In cxx_eval_call_expression we
> have
> 
>           if (DECL_CONSTRUCTOR_P (fun))
>             /* This can be null for a subobject constructor call, in
> 
>                which case what we care about is the initialization
> 
>                side-effects rather than the value.  We could get at the
> 
>                value by evaluating *this, but we don't bother; there's
> 
>                no need to put such a call in the hash table.  */
>             result = lval ? ctx->object : ctx->ctor;
> 
> Your patch already finds *this (b.a) and puts it in new_obj; if it's const
> we can evaluate it to get the CONSTRUCTOR to set TREE_READONLY on.

Ah, got it!  This patch uses setting TREE_READONLY to achieve what I was after.
I also needed to set TREE_READONLY in cxx_eval_constant_expression/DECL_EXPR.
The additional evaluating will only happen for const-qual objects so I hope not
very often.

Any further comments?  Thanks,

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

2019-08-16  Marek Polacek  <polacek@redhat.com>

	PR c++/91264 - detect modifying const objects in constexpr.
	* constexpr.c (object_under_construction_p): New function.
	(modifying_const_object_error): Likewise.
	(cxx_eval_call_expression): Set TREE_READONLY on a CONSTRUCTOR of
	a const-qualified object after it's been fully constructed.
	(modifying_const_object_p): New function.
	(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.
	(cxx_eval_constant_expression) <case DECL_EXPR>: Mark a constant
	object's constructor TREE_READONLY.

	* 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.
	* g++.dg/cpp1y/constexpr-tracking-const9.C: New test.
	* g++.dg/cpp1y/constexpr-tracking-const10.C: New test.
	* g++.dg/cpp1y/constexpr-tracking-const11.C: New test.

diff --git gcc/cp/constexpr.c gcc/cp/constexpr.c
index 23f2a027e2f..0bd190cc366 100644
--- gcc/cp/constexpr.c
+++ gcc/cp/constexpr.c
@@ -1575,6 +1575,41 @@ clear_no_implicit_zero (tree ctor)
     }
 }
 
+/* Return true if OBJ is an object currently under construction.  */
+
+static bool
+object_under_construction_p (const constexpr_ctx *ctx, tree obj)
+{
+  if (!CLASS_TYPE_P (TREE_TYPE (obj)))
+    return false;
+
+  tree *ctor = ctx->values->get (obj);
+  /* Subobjects won't be stored in ctx->values but we can get
+     its CONSTRUCTOR by evaluating *this.  */
+  if (ctor == NULL)
+    {
+      bool dummy1 = false, dummy2 = false;
+      tree e = cxx_eval_constant_expression (ctx, obj, /*lval*/false, &dummy1,
+					     &dummy2);
+      return !TREE_READONLY (e);
+    }
+  else
+    return !TREE_READONLY (*ctor);
+}
+
+/* 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_input_loc (expr);
+  auto_diagnostic_group d;
+  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.  */
@@ -1775,6 +1810,19 @@ 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);
+    }
+
   tree result = NULL_TREE;
 
   constexpr_call *entry = NULL;
@@ -1910,6 +1958,29 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t,
 		}
 	    }
 
+	  /* At this point, the object's constructor will have run, so
+	     the object is no longer under construction, and its possible
+	     'const' semantics now apply.  Make a note of this fact by
+	     marking the CONSTRUCTOR TREE_READONLY.  */
+	  if (new_obj
+	      && CLASS_TYPE_P (TREE_TYPE (new_obj))
+	      && CP_TYPE_CONST_P (TREE_TYPE (new_obj)))
+	    {
+	      tree *ctor = ctx->values->get (new_obj);
+	      /* Subobjects won't be stored in ctx->values but we can get
+		 its CONSTRUCTOR by evaluating *this.  */
+	      if (ctor == NULL)
+		{
+		  tree e = cxx_eval_constant_expression (ctx, new_obj,
+							 /*lval*/false,
+							 non_constant_p,
+							 overflow_p);
+		  TREE_READONLY (e) = true;
+		}
+	      else
+		TREE_READONLY (*ctor) = true;
+	    }
+
 	  /* Forget the saved values of the callee's SAVE_EXPRs.  */
 	  unsigned int i;
 	  tree save_expr;
@@ -3724,6 +3795,36 @@ 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 (const constexpr_ctx *ctx, 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 (ctx, obj));
+}
+
 /* Evaluate an INIT_EXPR or MODIFY_EXPR.  */
 
 static tree
@@ -3773,6 +3874,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 +3885,17 @@ 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 (ctx, 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 +3924,14 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
 	}
     }
 
+  if (modifying_const_object_p (ctx, 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 +4184,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);
@@ -4650,6 +4772,11 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t,
 						 non_constant_p, overflow_p);
 	    /* Don't share a CONSTRUCTOR that might be changed.  */
 	    init = unshare_constructor (init);
+	    /* Remember that a constant object's constructor has already
+	       ran.  */
+	    if (CLASS_TYPE_P (TREE_TYPE (r))
+		&& CP_TYPE_CONST_P (TREE_TYPE (r)))
+	      TREE_READONLY (init) = true;
 	    ctx->values->put (r, init);
 	  }
 	else if (ctx == &new_ctx)
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-const10.C gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const10.C
new file mode 100644
index 00000000000..53acb37beb8
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const10.C
@@ -0,0 +1,22 @@
+// PR c++/91264
+// { dg-do compile { target c++14 } }
+
+struct B {
+  B() = default;
+  int i;
+};
+
+constexpr B bar()
+{
+    constexpr B b = B(); // { dg-message "originally declared" }
+    B *p = const_cast<B*>(&b);
+
+    p->i = 11; // { dg-error "modifying a const object" }
+
+   return *p;
+}
+
+void foo()
+{  
+   constexpr B y = bar(); // { dg-message "in .constexpr. expansion of" }
+}
diff --git gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const11.C gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const11.C
new file mode 100644
index 00000000000..2b351cd013a
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const11.C
@@ -0,0 +1,16 @@
+// PR c++/91264
+// { dg-do compile { target c++14 } }
+
+struct S {
+    int a = 1;
+    int * ptr = &a;
+};
+
+constexpr bool f() {
+    auto const s = S{}; // { dg-message "originally declared" }
+    *s.ptr = 2; // { dg-error "modifying a const object" }
+    return s.a == 2;
+}
+
+static_assert(f(), ""); // { dg-error "non-constant condition" }
+// { dg-message "in 'constexpr' expansion of" "" { target *-*-* } .-1 }
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" }
+}
diff --git gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const9.C gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const9.C
new file mode 100644
index 00000000000..0edec4d05cf
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const9.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{}; // { 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" }
+}

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

[Jason Merrill]

On 8/16/19 5:11 AM, Marek Polacek wrote:
> On Thu, Aug 15, 2019 at 08:21:25PM -0400, Jason Merrill wrote:
>> On 8/15/19 5:34 PM, Marek Polacek wrote:
>>> On Wed, Aug 14, 2019 at 02:50:13PM -0400, Jason Merrill wrote:
>>>> On Thu, Aug 8, 2019 at 3:25 PM Marek Polacek <polacek@redhat.com> wrote:
>>>>>
>>>>> On Thu, Aug 08, 2019 at 11:06:17AM -0400, Jason Merrill wrote:
>>>>>> On 8/6/19 3:20 PM, Marek Polacek wrote:
>>>>>>> On Mon, Aug 05, 2019 at 03:54:19PM -0400, Jason Merrill wrote:
>>>>>>>> On 7/31/19 3:26 PM, Marek Polacek wrote:
>>>>>>>>> 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?
>>>>>>>>
>>>>>>>> For scalar objects, we should be able to rely on INIT_EXPR vs. MODIFY_EXPR
>>>>>>>> to distinguish between initialization and modification; for class objects, I
>>>>>>>
>>>>>>> This is already true: only class object go into the hash set.
>>>>>>>
>>>>>>>> wonder about setting a flag on the CONSTRUCTOR after initialization is
>>>>>>>> complete to indicate that the value is now constant.
>>>>>>>
>>>>>>> But here we're not dealing with CONSTRUCTORs in the gcc sense (i.e. exprs with
>>>>>>> TREE_CODE == CONSTRUCTOR).  We have a CALL_EXPR like Y::Y ((struct Y *) &y),
>>>>>>> which initializes the object "y".  Setting a flag on the CALL_EXPR or its underlying
>>>>>>> function decl wouldn't help.
>>>>>>>
>>>>>>> Am I missing something?
>>>>>>
>>>>>> I was thinking that where in your current patch you call
>>>>>> remove_object_under_construction, we could instead mark the object's value
>>>>>> CONSTRUCTOR as immutable.
>>>>>
>>>>> Ah, what you meant was to look at DECL_INITIAL of the object we're
>>>>> constructing, which could be a CONSTRUCTOR.  Unfortunately, this
>>>>> DECL_INITIAL is null (in all the new tests when doing
>>>>> remove_object_under_construction), so there's nothing to mark as TREE_READONLY :/.
>>>>
>>>> There's a value in ctx->values, isn't there?
>>>
>>> Doesn't seem to be the case for e.g.
>>>
>>> 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, "");
>>>
>>> Here we're constructing "b", its ctx->values->get(new_obj) is initially
>>> "{}".  In the middle of constructing "b", we construct "b.a", but that
>>> has nothing in ctx->values.
>>
>> Right, subobjects aren't in ctx->values.  In cxx_eval_call_expression we
>> have
>>
>>            if (DECL_CONSTRUCTOR_P (fun))
>>              /* This can be null for a subobject constructor call, in
>>
>>                 which case what we care about is the initialization
>>
>>                 side-effects rather than the value.  We could get at the
>>
>>                 value by evaluating *this, but we don't bother; there's
>>
>>                 no need to put such a call in the hash table.  */
>>              result = lval ? ctx->object : ctx->ctor;
>>
>> Your patch already finds *this (b.a) and puts it in new_obj; if it's const
>> we can evaluate it to get the CONSTRUCTOR to set TREE_READONLY on.
> 
> Ah, got it!  This patch uses setting TREE_READONLY to achieve what I was after.
> I also needed to set TREE_READONLY in cxx_eval_constant_expression/DECL_EXPR.
> The additional evaluating will only happen for const-qual objects so I hope not
> very often.
> 
> Any further comments?  Thanks,
> 
> @@ -1910,6 +1958,29 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t,
>
> +	  /* At this point, the object's constructor will have run, so
> +	     the object is no longer under construction, and its possible
> +	     'const' semantics now apply.  Make a note of this fact by
> +	     marking the CONSTRUCTOR TREE_READONLY.  */
> +	  if (new_obj
> +	      && CLASS_TYPE_P (TREE_TYPE (new_obj))
> +	      && CP_TYPE_CONST_P (TREE_TYPE (new_obj)))
> +	    {
> +	      tree *ctor = ctx->values->get (new_obj);

I don't think trying ctx->values->get first is a win, let's go straight 
to cxx_eval_constant_expression.

> +/* 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 (const constexpr_ctx *ctx, 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)))

This seems wrong; if one field is const, we can still modify other 
fields. I don't see a test for that case.

> @@ -3783,6 +3885,17 @@ 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 (ctx, TREE_CODE (t), probe,
> +					     mutable_p))
> +	      {
> +		if (!ctx->quiet)
> +		  modifying_const_object_error (t, probe);
> +		*non_constant_p = true;
> +		return t;
> +	      }

What if there's a mutable member further down, i.e.

struct A { mutable int i; };
struct B { A a; };
const B b;
b.a.i = 42;

?  And also...

> @@ -3811,6 +3924,14 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
>   
> +  if (modifying_const_object_p (ctx, TREE_CODE (t), object, mutable_p))
> +    {
> +      if (!ctx->quiet)
> +	modifying_const_object_error (t, object);
> +      *non_constant_p = true;
> +      return t;
> +    }

...we are already collecting the CONSTRUCTORs that we're dealing with in 
the "ctors" stack, we shouldn't need to evaluate object at this point. 
I'd expect the topmost class-type CONSTRUCTOR on the stack (if any) to 
be the one we want to look at. I'd think you could do away with much of 
modifying_const_object_p.

> @@ -4650,6 +4772,11 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t,
>   						 non_constant_p, overflow_p);
>   	    /* Don't share a CONSTRUCTOR that might be changed.  */
>   	    init = unshare_constructor (init);
> +	    /* Remember that a constant object's constructor has already
> +	       ran.  */

"has...run"

Jason

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

[Marek Polacek]

On Fri, Aug 16, 2019 at 05:40:39PM -0700, Jason Merrill wrote:
> On 8/16/19 5:11 AM, Marek Polacek wrote:
> > On Thu, Aug 15, 2019 at 08:21:25PM -0400, Jason Merrill wrote:
> > > On 8/15/19 5:34 PM, Marek Polacek wrote:
> > > > On Wed, Aug 14, 2019 at 02:50:13PM -0400, Jason Merrill wrote:
> > > > > On Thu, Aug 8, 2019 at 3:25 PM Marek Polacek <polacek@redhat.com> wrote:
> > > > > > 
> > > > > > On Thu, Aug 08, 2019 at 11:06:17AM -0400, Jason Merrill wrote:
> > > > > > > On 8/6/19 3:20 PM, Marek Polacek wrote:
> > > > > > > > On Mon, Aug 05, 2019 at 03:54:19PM -0400, Jason Merrill wrote:
> > > > > > > > > On 7/31/19 3:26 PM, Marek Polacek wrote:
> > > > > > > > > > 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?
> > > > > > > > > 
> > > > > > > > > For scalar objects, we should be able to rely on INIT_EXPR vs. MODIFY_EXPR
> > > > > > > > > to distinguish between initialization and modification; for class objects, I
> > > > > > > > 
> > > > > > > > This is already true: only class object go into the hash set.
> > > > > > > > 
> > > > > > > > > wonder about setting a flag on the CONSTRUCTOR after initialization is
> > > > > > > > > complete to indicate that the value is now constant.
> > > > > > > > 
> > > > > > > > But here we're not dealing with CONSTRUCTORs in the gcc sense (i.e. exprs with
> > > > > > > > TREE_CODE == CONSTRUCTOR).  We have a CALL_EXPR like Y::Y ((struct Y *) &y),
> > > > > > > > which initializes the object "y".  Setting a flag on the CALL_EXPR or its underlying
> > > > > > > > function decl wouldn't help.
> > > > > > > > 
> > > > > > > > Am I missing something?
> > > > > > > 
> > > > > > > I was thinking that where in your current patch you call
> > > > > > > remove_object_under_construction, we could instead mark the object's value
> > > > > > > CONSTRUCTOR as immutable.
> > > > > > 
> > > > > > Ah, what you meant was to look at DECL_INITIAL of the object we're
> > > > > > constructing, which could be a CONSTRUCTOR.  Unfortunately, this
> > > > > > DECL_INITIAL is null (in all the new tests when doing
> > > > > > remove_object_under_construction), so there's nothing to mark as TREE_READONLY :/.
> > > > > 
> > > > > There's a value in ctx->values, isn't there?
> > > > 
> > > > Doesn't seem to be the case for e.g.
> > > > 
> > > > 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, "");
> > > > 
> > > > Here we're constructing "b", its ctx->values->get(new_obj) is initially
> > > > "{}".  In the middle of constructing "b", we construct "b.a", but that
> > > > has nothing in ctx->values.
> > > 
> > > Right, subobjects aren't in ctx->values.  In cxx_eval_call_expression we
> > > have
> > > 
> > >            if (DECL_CONSTRUCTOR_P (fun))
> > >              /* This can be null for a subobject constructor call, in
> > > 
> > >                 which case what we care about is the initialization
> > > 
> > >                 side-effects rather than the value.  We could get at the
> > > 
> > >                 value by evaluating *this, but we don't bother; there's
> > > 
> > >                 no need to put such a call in the hash table.  */
> > >              result = lval ? ctx->object : ctx->ctor;
> > > 
> > > Your patch already finds *this (b.a) and puts it in new_obj; if it's const
> > > we can evaluate it to get the CONSTRUCTOR to set TREE_READONLY on.
> > 
> > Ah, got it!  This patch uses setting TREE_READONLY to achieve what I was after.
> > I also needed to set TREE_READONLY in cxx_eval_constant_expression/DECL_EXPR.
> > The additional evaluating will only happen for const-qual objects so I hope not
> > very often.
> > 
> > Any further comments?  Thanks,
> > 
> > @@ -1910,6 +1958,29 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t,
> > 
> > +	  /* At this point, the object's constructor will have run, so
> > +	     the object is no longer under construction, and its possible
> > +	     'const' semantics now apply.  Make a note of this fact by
> > +	     marking the CONSTRUCTOR TREE_READONLY.  */
> > +	  if (new_obj
> > +	      && CLASS_TYPE_P (TREE_TYPE (new_obj))
> > +	      && CP_TYPE_CONST_P (TREE_TYPE (new_obj)))
> > +	    {
> > +	      tree *ctor = ctx->values->get (new_obj);
> 
> I don't think trying ctx->values->get first is a win, let's go straight to
> cxx_eval_constant_expression.

Done.

> > +/* 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 (const constexpr_ctx *ctx, 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)))
> 
> This seems wrong; if one field is const, we can still modify other fields. I
> don't see a test for that case.

I took it from cp_build_modify_expr but it's not really needed, so dropped.

> > @@ -3783,6 +3885,17 @@ 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 (ctx, TREE_CODE (t), probe,
> > +					     mutable_p))
> > +	      {
> > +		if (!ctx->quiet)
> > +		  modifying_const_object_error (t, probe);
> > +		*non_constant_p = true;
> > +		return t;
> > +	      }
> 
> What if there's a mutable member further down, i.e.
> 
> struct A { mutable int i; };
> struct B { A a; };
> const B b;
> b.a.i = 42;
> 
> ?  And also...

This works.  We check mutable going from the innermost object to the outermost,
so if we have b.a.n, we check n first.  Some of the new tests are exercising this.

> > @@ -3811,6 +3924,14 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
> > +  if (modifying_const_object_p (ctx, TREE_CODE (t), object, mutable_p))
> > +    {
> > +      if (!ctx->quiet)
> > +	modifying_const_object_error (t, object);
> > +      *non_constant_p = true;
> > +      return t;
> > +    }
> 
> ...we are already collecting the CONSTRUCTORs that we're dealing with in the
> "ctors" stack, we shouldn't need to evaluate object at this point. I'd
> expect the topmost class-type CONSTRUCTOR on the stack (if any) to be the
> one we want to look at. I'd think you could do away with much of
> modifying_const_object_p.

Oh, got it; wish I'd thought of that sooner.  We can't just check the topmost
CONSTRUCTOR though, the yuge comment I've added has an example when that would
not be enough.

And yes, I could 86 most of modifying_const_object_p.

> > @@ -4650,6 +4772,11 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t,
> >   						 non_constant_p, overflow_p);
> >   	    /* Don't share a CONSTRUCTOR that might be changed.  */
> >   	    init = unshare_constructor (init);
> > +	    /* Remember that a constant object's constructor has already
> > +	       ran.  */
> 
> "has...run"

Yikes.

Thanks for the review, it allowed me to make the patch more concise.

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

2019-08-18  Marek Polacek  <polacek@redhat.com>

	PR c++/91264 - detect modifying const objects in constexpr.
	* constexpr.c (modifying_const_object_error): New function.
	(cxx_eval_call_expression): Set TREE_READONLY on a CONSTRUCTOR of
	a const-qualified object after it's been fully constructed.
	(modifying_const_object_p): New function.
	(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.
	(cxx_eval_constant_expression) <case DECL_EXPR>: Mark a constant
	object's constructor TREE_READONLY.

	* 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.
	* g++.dg/cpp1y/constexpr-tracking-const9.C: New test.
	* g++.dg/cpp1y/constexpr-tracking-const10.C: New test.
	* g++.dg/cpp1y/constexpr-tracking-const11.C: New test.
	* g++.dg/cpp1y/constexpr-tracking-const12.C: New test.
	* g++.dg/cpp1y/constexpr-tracking-const13.C: New test.
	* g++.dg/cpp1y/constexpr-tracking-const14.C: New test.

diff --git gcc/cp/constexpr.c gcc/cp/constexpr.c
index 23f2a027e2f..e5ed326efca 100644
--- gcc/cp/constexpr.c
+++ gcc/cp/constexpr.c
@@ -1575,6 +1575,19 @@ clear_no_implicit_zero (tree ctor)
     }
 }
 
+/* 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_input_loc (expr);
+  auto_diagnostic_group d;
+  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.  */
@@ -1775,6 +1788,19 @@ 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);
+    }
+
   tree result = NULL_TREE;
 
   constexpr_call *entry = NULL;
@@ -1910,6 +1936,23 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t,
 		}
 	    }
 
+	  /* At this point, the object's constructor will have run, so
+	     the object is no longer under construction, and its possible
+	     'const' semantics now apply.  Make a note of this fact by
+	     marking the CONSTRUCTOR TREE_READONLY.  */
+	  if (new_obj
+	      && CLASS_TYPE_P (TREE_TYPE (new_obj))
+	      && CP_TYPE_CONST_P (TREE_TYPE (new_obj)))
+	    {
+	      /* Subobjects might not be stored in ctx->values but we can
+		 get its CONSTRUCTOR by evaluating *this.  */
+	      tree e = cxx_eval_constant_expression (ctx, new_obj,
+						     /*lval*/false,
+						     non_constant_p,
+						     overflow_p);
+	      TREE_READONLY (e) = true;
+	    }
+
 	  /* Forget the saved values of the callee's SAVE_EXPRs.  */
 	  unsigned int i;
 	  tree save_expr;
@@ -3724,6 +3767,26 @@ 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;
+
+  /* [basic.type.qualifier] "A const object is an object of type
+     const T or a non-mutable subobject of a const object."  */
+  if (mutable_p)
+    return false;
+
+  return (TREE_READONLY (obj) || CP_TYPE_CONST_P (TREE_TYPE (obj)));
+}
+
 /* Evaluate an INIT_EXPR or MODIFY_EXPR.  */
 
 static tree
@@ -3773,6 +3836,9 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
   /* Find the underlying variable.  */
   releasing_vec refs;
   tree object = NULL_TREE;
+  /* If we're modifying a const object, save it.  */
+  tree const_object_being_modified = NULL_TREE;
+  bool mutable_p = false;
   for (tree probe = target; object == NULL_TREE; )
     {
       switch (TREE_CODE (probe))
@@ -3783,6 +3849,12 @@ 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)
+		&& const_object_being_modified == NULL_TREE)
+	      const_object_being_modified = probe;
 	    if (TREE_CODE (probe) == ARRAY_REF)
 	      {
 		elt = eval_and_check_array_index (ctx, probe, false,
@@ -3811,6 +3883,10 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
 	}
     }
 
+  if (modifying_const_object_p (TREE_CODE (t), object, mutable_p)
+      && const_object_being_modified == NULL_TREE)
+    const_object_being_modified = object;
+
   /* And then find/build up our initializer for the path to the subobject
      we're initializing.  */
   tree *valp;
@@ -3950,6 +4026,61 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
       valp = &cep->value;
     }
 
+  /* Detect modifying a constant object in constexpr evaluation.
+     We have found a const object that is being modified.  Figure out
+     if we need to issue an error.  Consider
+
+     struct A {
+       int n;
+       constexpr A() : n(1) { n = 2; } // #1
+     };
+     struct B {
+       const A a;
+       constexpr B() { a.n = 3; } // #2
+     };
+    constexpr B b{};
+
+    #1 is OK, since we're modifying an object under construction, but
+    #2 is wrong, since "a" is const and has been fully constructed.
+    We track the "under construction" state by using the TREE_READONLY
+    bit in the object's CONSTRUCTOR.  For the example above, the *ctors
+    stack at the point of #2 will look like:
+
+      ctors[0] = {.a={.n=2}}  TREE_READONLY = 0
+      ctors[1] = {.n=2}       TREE_READONLY = 1
+
+    and we're modifying "b.a", so we search the stack and see that the
+    constructor for "b.a" has already run.  */
+  if (const_object_being_modified)
+    {
+      bool fail = false;
+      if (!CLASS_TYPE_P (TREE_TYPE (const_object_being_modified)))
+	fail = true;
+      else
+	{
+	  /* [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."  */
+	  tree elt;
+	  unsigned int i;
+	  FOR_EACH_VEC_ELT (*ctors, i, elt)
+	    if (CLASS_TYPE_P (TREE_TYPE (elt)) && TREE_READONLY (elt))
+	      {
+		fail = true;
+		break;
+	      }
+	}
+      if (fail)
+	{
+	  if (!ctx->quiet)
+	    modifying_const_object_error (t, const_object_being_modified);
+	  *non_constant_p = true;
+	  return t;
+	}
+    }
+
   if (!preeval)
     {
       /* Create a new CONSTRUCTOR in case evaluation of the initializer
@@ -4063,7 +4194,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);
@@ -4650,6 +4782,11 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t,
 						 non_constant_p, overflow_p);
 	    /* Don't share a CONSTRUCTOR that might be changed.  */
 	    init = unshare_constructor (init);
+	    /* Remember that a constant object's constructor has already
+	       run.  */
+	    if (CLASS_TYPE_P (TREE_TYPE (r))
+		&& CP_TYPE_CONST_P (TREE_TYPE (r)))
+	      TREE_READONLY (init) = true;
 	    ctx->values->put (r, init);
 	  }
 	else if (ctx == &new_ctx)
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-const10.C gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const10.C
new file mode 100644
index 00000000000..53acb37beb8
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const10.C
@@ -0,0 +1,22 @@
+// PR c++/91264
+// { dg-do compile { target c++14 } }
+
+struct B {
+  B() = default;
+  int i;
+};
+
+constexpr B bar()
+{
+    constexpr B b = B(); // { dg-message "originally declared" }
+    B *p = const_cast<B*>(&b);
+
+    p->i = 11; // { dg-error "modifying a const object" }
+
+   return *p;
+}
+
+void foo()
+{  
+   constexpr B y = bar(); // { dg-message "in .constexpr. expansion of" }
+}
diff --git gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const11.C gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const11.C
new file mode 100644
index 00000000000..2b351cd013a
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const11.C
@@ -0,0 +1,16 @@
+// PR c++/91264
+// { dg-do compile { target c++14 } }
+
+struct S {
+    int a = 1;
+    int * ptr = &a;
+};
+
+constexpr bool f() {
+    auto const s = S{}; // { dg-message "originally declared" }
+    *s.ptr = 2; // { dg-error "modifying a const object" }
+    return s.a == 2;
+}
+
+static_assert(f(), ""); // { dg-error "non-constant condition" }
+// { dg-message "in 'constexpr' expansion of" "" { target *-*-* } .-1 }
diff --git gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const12.C gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const12.C
new file mode 100644
index 00000000000..d83e2794f7f
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const12.C
@@ -0,0 +1,17 @@
+// PR c++/91264
+// { dg-do compile { target c++14 } }
+
+struct A {
+  const int n;
+  int m;
+  constexpr A() : n(1), m(2) { }
+};
+struct B {
+  A a;
+  constexpr B() {
+    int *p = &a.m;
+    *p = 3;
+  }
+};
+constexpr B b;
+static_assert(b.a.m == 3, "");
diff --git gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const13.C gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const13.C
new file mode 100644
index 00000000000..bc7faa3c250
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const13.C
@@ -0,0 +1,20 @@
+// PR c++/91264
+// { dg-do compile { target c++14 } }
+
+struct A {
+  mutable int i;
+  constexpr A() : i(0) { }
+};
+struct B {
+  A a;
+  constexpr B() : a{} { }
+};
+
+constexpr void
+g ()
+{
+  const B b;
+  b.a.i = 42;
+}
+
+static_assert((g(), 1), "");
diff --git gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const14.C gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const14.C
new file mode 100644
index 00000000000..45c4fcf50be
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const14.C
@@ -0,0 +1,38 @@
+// PR c++/91264
+// { dg-do compile { target c++14 } }
+
+struct F {
+  const int f;
+  constexpr F() : f(9) { }
+};
+
+struct C {
+  int n;
+  const F f;
+  constexpr C() : n(1) { n = 66; }
+};
+
+struct A {
+  int r;
+  const C c;
+  constexpr A() : r(11) { r = 14; const_cast<C &>(c).n = 42; } // { dg-error "modifying a const object" }
+};
+
+struct D {
+  const A a;
+  constexpr D() { } // { dg-message "in .constexpr. expansion of" }
+};
+
+struct E {
+  const D d;
+  constexpr E() { } // { dg-message "in .constexpr. expansion of" }
+};
+
+struct B {
+  const E e;
+  constexpr B(bool) { } // { dg-message "in .constexpr. expansion of" }
+};
+
+constexpr B b(false); // { dg-message "in .constexpr. expansion of" }
+// { dg-message "originally declared" "" { target *-*-* } .-1 }
+static_assert(b.e.d.a.c.n == 2, ""); // { dg-error "non-constant condition" }
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..54d83b17d39
--- /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" }
+}
diff --git gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const9.C gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const9.C
new file mode 100644
index 00000000000..0edec4d05cf
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const9.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{}; // { 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" }
+}

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

[Jason Merrill]

On 8/18/19 6:52 AM, Marek Polacek wrote:
> On Fri, Aug 16, 2019 at 05:40:39PM -0700, Jason Merrill wrote:
>> On 8/16/19 5:11 AM, Marek Polacek wrote:
>>> On Thu, Aug 15, 2019 at 08:21:25PM -0400, Jason Merrill wrote:
>>>> On 8/15/19 5:34 PM, Marek Polacek wrote:
>>>>> On Wed, Aug 14, 2019 at 02:50:13PM -0400, Jason Merrill wrote:
>>>>>> On Thu, Aug 8, 2019 at 3:25 PM Marek Polacek <polacek@redhat.com> wrote:
>>>>>>>
>>>>>>> On Thu, Aug 08, 2019 at 11:06:17AM -0400, Jason Merrill wrote:
>>>>>>>> On 8/6/19 3:20 PM, Marek Polacek wrote:
>>>>>>>>> On Mon, Aug 05, 2019 at 03:54:19PM -0400, Jason Merrill wrote:
>>>>>>>>>> On 7/31/19 3:26 PM, Marek Polacek wrote:
>>>>>>>>>>> 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?
>>>>>>>>>>
>>>>>>>>>> For scalar objects, we should be able to rely on INIT_EXPR vs. MODIFY_EXPR
>>>>>>>>>> to distinguish between initialization and modification; for class objects, I
>>>>>>>>>
>>>>>>>>> This is already true: only class object go into the hash set.
>>>>>>>>>
>>>>>>>>>> wonder about setting a flag on the CONSTRUCTOR after initialization is
>>>>>>>>>> complete to indicate that the value is now constant.
>>>>>>>>>
>>>>>>>>> But here we're not dealing with CONSTRUCTORs in the gcc sense (i.e. exprs with
>>>>>>>>> TREE_CODE == CONSTRUCTOR).  We have a CALL_EXPR like Y::Y ((struct Y *) &y),
>>>>>>>>> which initializes the object "y".  Setting a flag on the CALL_EXPR or its underlying
>>>>>>>>> function decl wouldn't help.
>>>>>>>>>
>>>>>>>>> Am I missing something?
>>>>>>>>
>>>>>>>> I was thinking that where in your current patch you call
>>>>>>>> remove_object_under_construction, we could instead mark the object's value
>>>>>>>> CONSTRUCTOR as immutable.
>>>>>>>
>>>>>>> Ah, what you meant was to look at DECL_INITIAL of the object we're
>>>>>>> constructing, which could be a CONSTRUCTOR.  Unfortunately, this
>>>>>>> DECL_INITIAL is null (in all the new tests when doing
>>>>>>> remove_object_under_construction), so there's nothing to mark as TREE_READONLY :/.
>>>>>>
>>>>>> There's a value in ctx->values, isn't there?
>>>>>
>>>>> Doesn't seem to be the case for e.g.
>>>>>
>>>>> 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, "");
>>>>>
>>>>> Here we're constructing "b", its ctx->values->get(new_obj) is initially
>>>>> "{}".  In the middle of constructing "b", we construct "b.a", but that
>>>>> has nothing in ctx->values.
>>>>
>>>> Right, subobjects aren't in ctx->values.  In cxx_eval_call_expression we
>>>> have
>>>>
>>>>             if (DECL_CONSTRUCTOR_P (fun))
>>>>               /* This can be null for a subobject constructor call, in
>>>>
>>>>                  which case what we care about is the initialization
>>>>
>>>>                  side-effects rather than the value.  We could get at the
>>>>
>>>>                  value by evaluating *this, but we don't bother; there's
>>>>
>>>>                  no need to put such a call in the hash table.  */
>>>>               result = lval ? ctx->object : ctx->ctor;
>>>>
>>>> Your patch already finds *this (b.a) and puts it in new_obj; if it's const
>>>> we can evaluate it to get the CONSTRUCTOR to set TREE_READONLY on.
>>>
>>> Ah, got it!  This patch uses setting TREE_READONLY to achieve what I was after.
>>> I also needed to set TREE_READONLY in cxx_eval_constant_expression/DECL_EXPR.
>>> The additional evaluating will only happen for const-qual objects so I hope not
>>> very often.
>>>
>>> Any further comments?  Thanks,
>>>
>>> @@ -1910,6 +1958,29 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t,
>>>
>>> +	  /* At this point, the object's constructor will have run, so
>>> +	     the object is no longer under construction, and its possible
>>> +	     'const' semantics now apply.  Make a note of this fact by
>>> +	     marking the CONSTRUCTOR TREE_READONLY.  */
>>> +	  if (new_obj
>>> +	      && CLASS_TYPE_P (TREE_TYPE (new_obj))
>>> +	      && CP_TYPE_CONST_P (TREE_TYPE (new_obj)))
>>> +	    {
>>> +	      tree *ctor = ctx->values->get (new_obj);
>>
>> I don't think trying ctx->values->get first is a win, let's go straight to
>> cxx_eval_constant_expression.
> 
> Done.
> 
>>> +/* 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 (const constexpr_ctx *ctx, 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)))
>>
>> This seems wrong; if one field is const, we can still modify other fields. I
>> don't see a test for that case.
> 
> I took it from cp_build_modify_expr but it's not really needed, so dropped.
> 
>>> @@ -3783,6 +3885,17 @@ 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 (ctx, TREE_CODE (t), probe,
>>> +					     mutable_p))
>>> +	      {
>>> +		if (!ctx->quiet)
>>> +		  modifying_const_object_error (t, probe);
>>> +		*non_constant_p = true;
>>> +		return t;
>>> +	      }
>>
>> What if there's a mutable member further down, i.e.
>>
>> struct A { mutable int i; };
>> struct B { A a; };
>> const B b;
>> b.a.i = 42;
>>
>> ?  And also...
> 
> This works.  We check mutable going from the innermost object to the outermost,
> so if we have b.a.n, we check n first.  Some of the new tests are exercising this.
> 
>>> @@ -3811,6 +3924,14 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
>>> +  if (modifying_const_object_p (ctx, TREE_CODE (t), object, mutable_p))
>>> +    {
>>> +      if (!ctx->quiet)
>>> +	modifying_const_object_error (t, object);
>>> +      *non_constant_p = true;
>>> +      return t;
>>> +    }
>>
>> ...we are already collecting the CONSTRUCTORs that we're dealing with in the
>> "ctors" stack, we shouldn't need to evaluate object at this point. I'd
>> expect the topmost class-type CONSTRUCTOR on the stack (if any) to be the
>> one we want to look at. I'd think you could do away with much of
>> modifying_const_object_p.
> 
> Oh, got it; wish I'd thought of that sooner.  We can't just check the topmost
> CONSTRUCTOR though, the yuge comment I've added has an example when that would
> not be enough.
> 
> And yes, I could 86 most of modifying_const_object_p.
> 
>>> @@ -4650,6 +4772,11 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t,
>>>    						 non_constant_p, overflow_p);
>>>    	    /* Don't share a CONSTRUCTOR that might be changed.  */
>>>    	    init = unshare_constructor (init);
>>> +	    /* Remember that a constant object's constructor has already
>>> +	       ran.  */
>>
>> "has...run"
> 
> Yikes.
> 
> Thanks for the review, it allowed me to make the patch more concise.
> 
> Bootstrapped/regtested on x86_64-linux, ok for trunk?

Looking pretty good, just a couple of nits:

> +    We track the "under construction" state by using the TREE_READONLY
> +    bit in the object's CONSTRUCTOR.

Only incidentally; what we track using the TREE_READONLY bit is whether 
the object is read-only.

> +	  /* [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."  */
> +	  tree elt;
> +	  unsigned int i;
> +	  FOR_EACH_VEC_ELT (*ctors, i, elt)
> +	    if (CLASS_TYPE_P (TREE_TYPE (elt)) && TREE_READONLY (elt))

Would it make sense to look here for the CONSTRUCTOR with the same type 
as const_object_being_modified and set "fail" (or not) based only on 
that CONSTRUCTOR?

Jason

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

[Marek Polacek]

On Sun, Aug 18, 2019 at 05:08:07PM -0700, Jason Merrill wrote:
> On 8/18/19 6:52 AM, Marek Polacek wrote:
> > On Fri, Aug 16, 2019 at 05:40:39PM -0700, Jason Merrill wrote:
> > > On 8/16/19 5:11 AM, Marek Polacek wrote:
> > > > On Thu, Aug 15, 2019 at 08:21:25PM -0400, Jason Merrill wrote:
> > > > > On 8/15/19 5:34 PM, Marek Polacek wrote:
> > > > > > On Wed, Aug 14, 2019 at 02:50:13PM -0400, Jason Merrill wrote:
> > > > > > > On Thu, Aug 8, 2019 at 3:25 PM Marek Polacek <polacek@redhat.com> wrote:
> > > > > > > > 
> > > > > > > > On Thu, Aug 08, 2019 at 11:06:17AM -0400, Jason Merrill wrote:
> > > > > > > > > On 8/6/19 3:20 PM, Marek Polacek wrote:
> > > > > > > > > > On Mon, Aug 05, 2019 at 03:54:19PM -0400, Jason Merrill wrote:
> > > > > > > > > > > On 7/31/19 3:26 PM, Marek Polacek wrote:
> > > > > > > > > > > > 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?
> > > > > > > > > > > 
> > > > > > > > > > > For scalar objects, we should be able to rely on INIT_EXPR vs. MODIFY_EXPR
> > > > > > > > > > > to distinguish between initialization and modification; for class objects, I
> > > > > > > > > > 
> > > > > > > > > > This is already true: only class object go into the hash set.
> > > > > > > > > > 
> > > > > > > > > > > wonder about setting a flag on the CONSTRUCTOR after initialization is
> > > > > > > > > > > complete to indicate that the value is now constant.
> > > > > > > > > > 
> > > > > > > > > > But here we're not dealing with CONSTRUCTORs in the gcc sense (i.e. exprs with
> > > > > > > > > > TREE_CODE == CONSTRUCTOR).  We have a CALL_EXPR like Y::Y ((struct Y *) &y),
> > > > > > > > > > which initializes the object "y".  Setting a flag on the CALL_EXPR or its underlying
> > > > > > > > > > function decl wouldn't help.
> > > > > > > > > > 
> > > > > > > > > > Am I missing something?
> > > > > > > > > 
> > > > > > > > > I was thinking that where in your current patch you call
> > > > > > > > > remove_object_under_construction, we could instead mark the object's value
> > > > > > > > > CONSTRUCTOR as immutable.
> > > > > > > > 
> > > > > > > > Ah, what you meant was to look at DECL_INITIAL of the object we're
> > > > > > > > constructing, which could be a CONSTRUCTOR.  Unfortunately, this
> > > > > > > > DECL_INITIAL is null (in all the new tests when doing
> > > > > > > > remove_object_under_construction), so there's nothing to mark as TREE_READONLY :/.
> > > > > > > 
> > > > > > > There's a value in ctx->values, isn't there?
> > > > > > 
> > > > > > Doesn't seem to be the case for e.g.
> > > > > > 
> > > > > > 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, "");
> > > > > > 
> > > > > > Here we're constructing "b", its ctx->values->get(new_obj) is initially
> > > > > > "{}".  In the middle of constructing "b", we construct "b.a", but that
> > > > > > has nothing in ctx->values.
> > > > > 
> > > > > Right, subobjects aren't in ctx->values.  In cxx_eval_call_expression we
> > > > > have
> > > > > 
> > > > >             if (DECL_CONSTRUCTOR_P (fun))
> > > > >               /* This can be null for a subobject constructor call, in
> > > > > 
> > > > >                  which case what we care about is the initialization
> > > > > 
> > > > >                  side-effects rather than the value.  We could get at the
> > > > > 
> > > > >                  value by evaluating *this, but we don't bother; there's
> > > > > 
> > > > >                  no need to put such a call in the hash table.  */
> > > > >               result = lval ? ctx->object : ctx->ctor;
> > > > > 
> > > > > Your patch already finds *this (b.a) and puts it in new_obj; if it's const
> > > > > we can evaluate it to get the CONSTRUCTOR to set TREE_READONLY on.
> > > > 
> > > > Ah, got it!  This patch uses setting TREE_READONLY to achieve what I was after.
> > > > I also needed to set TREE_READONLY in cxx_eval_constant_expression/DECL_EXPR.
> > > > The additional evaluating will only happen for const-qual objects so I hope not
> > > > very often.
> > > > 
> > > > Any further comments?  Thanks,
> > > > 
> > > > @@ -1910,6 +1958,29 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t,
> > > > 
> > > > +	  /* At this point, the object's constructor will have run, so
> > > > +	     the object is no longer under construction, and its possible
> > > > +	     'const' semantics now apply.  Make a note of this fact by
> > > > +	     marking the CONSTRUCTOR TREE_READONLY.  */
> > > > +	  if (new_obj
> > > > +	      && CLASS_TYPE_P (TREE_TYPE (new_obj))
> > > > +	      && CP_TYPE_CONST_P (TREE_TYPE (new_obj)))
> > > > +	    {
> > > > +	      tree *ctor = ctx->values->get (new_obj);
> > > 
> > > I don't think trying ctx->values->get first is a win, let's go straight to
> > > cxx_eval_constant_expression.
> > 
> > Done.
> > 
> > > > +/* 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 (const constexpr_ctx *ctx, 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)))
> > > 
> > > This seems wrong; if one field is const, we can still modify other fields. I
> > > don't see a test for that case.
> > 
> > I took it from cp_build_modify_expr but it's not really needed, so dropped.
> > 
> > > > @@ -3783,6 +3885,17 @@ 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 (ctx, TREE_CODE (t), probe,
> > > > +					     mutable_p))
> > > > +	      {
> > > > +		if (!ctx->quiet)
> > > > +		  modifying_const_object_error (t, probe);
> > > > +		*non_constant_p = true;
> > > > +		return t;
> > > > +	      }
> > > 
> > > What if there's a mutable member further down, i.e.
> > > 
> > > struct A { mutable int i; };
> > > struct B { A a; };
> > > const B b;
> > > b.a.i = 42;
> > > 
> > > ?  And also...
> > 
> > This works.  We check mutable going from the innermost object to the outermost,
> > so if we have b.a.n, we check n first.  Some of the new tests are exercising this.
> > 
> > > > @@ -3811,6 +3924,14 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
> > > > +  if (modifying_const_object_p (ctx, TREE_CODE (t), object, mutable_p))
> > > > +    {
> > > > +      if (!ctx->quiet)
> > > > +	modifying_const_object_error (t, object);
> > > > +      *non_constant_p = true;
> > > > +      return t;
> > > > +    }
> > > 
> > > ...we are already collecting the CONSTRUCTORs that we're dealing with in the
> > > "ctors" stack, we shouldn't need to evaluate object at this point. I'd
> > > expect the topmost class-type CONSTRUCTOR on the stack (if any) to be the
> > > one we want to look at. I'd think you could do away with much of
> > > modifying_const_object_p.
> > 
> > Oh, got it; wish I'd thought of that sooner.  We can't just check the topmost
> > CONSTRUCTOR though, the yuge comment I've added has an example when that would
> > not be enough.
> > 
> > And yes, I could 86 most of modifying_const_object_p.
> > 
> > > > @@ -4650,6 +4772,11 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t,
> > > >    						 non_constant_p, overflow_p);
> > > >    	    /* Don't share a CONSTRUCTOR that might be changed.  */
> > > >    	    init = unshare_constructor (init);
> > > > +	    /* Remember that a constant object's constructor has already
> > > > +	       ran.  */
> > > 
> > > "has...run"
> > 
> > Yikes.
> > 
> > Thanks for the review, it allowed me to make the patch more concise.
> > 
> > Bootstrapped/regtested on x86_64-linux, ok for trunk?
> 
> Looking pretty good, just a couple of nits:
> 
> > +    We track the "under construction" state by using the TREE_READONLY
> > +    bit in the object's CONSTRUCTOR.
> 
> Only incidentally; what we track using the TREE_READONLY bit is whether the
> object is read-only.

Indeed, that wording was imprecise.  I've reworded it.

> > +	  /* [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."  */
> > +	  tree elt;
> > +	  unsigned int i;
> > +	  FOR_EACH_VEC_ELT (*ctors, i, elt)
> > +	    if (CLASS_TYPE_P (TREE_TYPE (elt)) && TREE_READONLY (elt))
> 
> Would it make sense to look here for the CONSTRUCTOR with the same type as
> const_object_being_modified and set "fail" (or not) based only on that
> CONSTRUCTOR?

That works too!  It's the only contstructor we care about, because if it's
readonly, we know we have to fail, and if it isn't, we know that the
outer constructor can't have been constructed yet, since it subsumes the
current object's constructor.

Regtest/bootstrap still running, but all the new tests still pass as before.

Ok if that passes?

2019-08-18  Marek Polacek  <polacek@redhat.com>

	PR c++/91264 - detect modifying const objects in constexpr.
	* constexpr.c (modifying_const_object_error): New function.
	(cxx_eval_call_expression): Set TREE_READONLY on a CONSTRUCTOR of
	a const-qualified object after it's been fully constructed.
	(modifying_const_object_p): New function.
	(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.
	(cxx_eval_constant_expression) <case DECL_EXPR>: Mark a constant
	object's constructor TREE_READONLY.

	* 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.
	* g++.dg/cpp1y/constexpr-tracking-const9.C: New test.
	* g++.dg/cpp1y/constexpr-tracking-const10.C: New test.
	* g++.dg/cpp1y/constexpr-tracking-const11.C: New test.
	* g++.dg/cpp1y/constexpr-tracking-const12.C: New test.
	* g++.dg/cpp1y/constexpr-tracking-const13.C: New test.
	* g++.dg/cpp1y/constexpr-tracking-const14.C: New test.

diff --git gcc/cp/constexpr.c gcc/cp/constexpr.c
index 23f2a027e2f..dbd0dc3b445 100644
--- gcc/cp/constexpr.c
+++ gcc/cp/constexpr.c
@@ -1575,6 +1575,19 @@ clear_no_implicit_zero (tree ctor)
     }
 }
 
+/* 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_input_loc (expr);
+  auto_diagnostic_group d;
+  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.  */
@@ -1775,6 +1788,19 @@ 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);
+    }
+
   tree result = NULL_TREE;
 
   constexpr_call *entry = NULL;
@@ -1910,6 +1936,23 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t,
 		}
 	    }
 
+	  /* At this point, the object's constructor will have run, so
+	     the object is no longer under construction, and its possible
+	     'const' semantics now apply.  Make a note of this fact by
+	     marking the CONSTRUCTOR TREE_READONLY.  */
+	  if (new_obj
+	      && CLASS_TYPE_P (TREE_TYPE (new_obj))
+	      && CP_TYPE_CONST_P (TREE_TYPE (new_obj)))
+	    {
+	      /* Subobjects might not be stored in ctx->values but we can
+		 get its CONSTRUCTOR by evaluating *this.  */
+	      tree e = cxx_eval_constant_expression (ctx, new_obj,
+						     /*lval*/false,
+						     non_constant_p,
+						     overflow_p);
+	      TREE_READONLY (e) = true;
+	    }
+
 	  /* Forget the saved values of the callee's SAVE_EXPRs.  */
 	  unsigned int i;
 	  tree save_expr;
@@ -3724,6 +3767,26 @@ 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;
+
+  /* [basic.type.qualifier] "A const object is an object of type
+     const T or a non-mutable subobject of a const object."  */
+  if (mutable_p)
+    return false;
+
+  return (TREE_READONLY (obj) || CP_TYPE_CONST_P (TREE_TYPE (obj)));
+}
+
 /* Evaluate an INIT_EXPR or MODIFY_EXPR.  */
 
 static tree
@@ -3773,6 +3836,9 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
   /* Find the underlying variable.  */
   releasing_vec refs;
   tree object = NULL_TREE;
+  /* If we're modifying a const object, save it.  */
+  tree const_object_being_modified = NULL_TREE;
+  bool mutable_p = false;
   for (tree probe = target; object == NULL_TREE; )
     {
       switch (TREE_CODE (probe))
@@ -3783,6 +3849,12 @@ 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)
+		&& const_object_being_modified == NULL_TREE)
+	      const_object_being_modified = probe;
 	    if (TREE_CODE (probe) == ARRAY_REF)
 	      {
 		elt = eval_and_check_array_index (ctx, probe, false,
@@ -3811,6 +3883,10 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
 	}
     }
 
+  if (modifying_const_object_p (TREE_CODE (t), object, mutable_p)
+      && const_object_being_modified == NULL_TREE)
+    const_object_being_modified = object;
+
   /* And then find/build up our initializer for the path to the subobject
      we're initializing.  */
   tree *valp;
@@ -3950,6 +4026,62 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
       valp = &cep->value;
     }
 
+  /* Detect modifying a constant object in constexpr evaluation.
+     We have found a const object that is being modified.  Figure out
+     if we need to issue an error.  Consider
+
+     struct A {
+       int n;
+       constexpr A() : n(1) { n = 2; } // #1
+     };
+     struct B {
+       const A a;
+       constexpr B() { a.n = 3; } // #2
+     };
+    constexpr B b{};
+
+    #1 is OK, since we're modifying an object under construction, but
+    #2 is wrong, since "a" is const and has been fully constructed.
+    To track it, we use the TREE_READONLY bit in the object's CONSTRUCTOR
+    which means that the object is read-only.  For the example above, the
+    *ctors stack at the point of #2 will look like:
+
+      ctors[0] = {.a={.n=2}}  TREE_READONLY = 0
+      ctors[1] = {.n=2}       TREE_READONLY = 1
+
+    and we're modifying "b.a", so we search the stack and see if the
+    constructor for "b.a" has already run.  */
+  if (const_object_being_modified)
+    {
+      bool fail = false;
+      if (!CLASS_TYPE_P (TREE_TYPE (const_object_being_modified)))
+	fail = true;
+      else
+	{
+	  /* [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."  */
+	  tree elt;
+	  unsigned int i;
+	  FOR_EACH_VEC_ELT (*ctors, i, elt)
+	    if (same_type_ignoring_top_level_qualifiers_p
+		(TREE_TYPE (const_object_being_modified), TREE_TYPE (elt)))
+	      {
+		fail = TREE_READONLY (elt);
+		break;
+	      }
+	}
+      if (fail)
+	{
+	  if (!ctx->quiet)
+	    modifying_const_object_error (t, const_object_being_modified);
+	  *non_constant_p = true;
+	  return t;
+	}
+    }
+
   if (!preeval)
     {
       /* Create a new CONSTRUCTOR in case evaluation of the initializer
@@ -4063,7 +4195,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);
@@ -4650,6 +4783,11 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t,
 						 non_constant_p, overflow_p);
 	    /* Don't share a CONSTRUCTOR that might be changed.  */
 	    init = unshare_constructor (init);
+	    /* Remember that a constant object's constructor has already
+	       run.  */
+	    if (CLASS_TYPE_P (TREE_TYPE (r))
+		&& CP_TYPE_CONST_P (TREE_TYPE (r)))
+	      TREE_READONLY (init) = true;
 	    ctx->values->put (r, init);
 	  }
 	else if (ctx == &new_ctx)
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-const10.C gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const10.C
new file mode 100644
index 00000000000..53acb37beb8
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const10.C
@@ -0,0 +1,22 @@
+// PR c++/91264
+// { dg-do compile { target c++14 } }
+
+struct B {
+  B() = default;
+  int i;
+};
+
+constexpr B bar()
+{
+    constexpr B b = B(); // { dg-message "originally declared" }
+    B *p = const_cast<B*>(&b);
+
+    p->i = 11; // { dg-error "modifying a const object" }
+
+   return *p;
+}
+
+void foo()
+{  
+   constexpr B y = bar(); // { dg-message "in .constexpr. expansion of" }
+}
diff --git gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const11.C gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const11.C
new file mode 100644
index 00000000000..2b351cd013a
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const11.C
@@ -0,0 +1,16 @@
+// PR c++/91264
+// { dg-do compile { target c++14 } }
+
+struct S {
+    int a = 1;
+    int * ptr = &a;
+};
+
+constexpr bool f() {
+    auto const s = S{}; // { dg-message "originally declared" }
+    *s.ptr = 2; // { dg-error "modifying a const object" }
+    return s.a == 2;
+}
+
+static_assert(f(), ""); // { dg-error "non-constant condition" }
+// { dg-message "in 'constexpr' expansion of" "" { target *-*-* } .-1 }
diff --git gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const12.C gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const12.C
new file mode 100644
index 00000000000..d83e2794f7f
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const12.C
@@ -0,0 +1,17 @@
+// PR c++/91264
+// { dg-do compile { target c++14 } }
+
+struct A {
+  const int n;
+  int m;
+  constexpr A() : n(1), m(2) { }
+};
+struct B {
+  A a;
+  constexpr B() {
+    int *p = &a.m;
+    *p = 3;
+  }
+};
+constexpr B b;
+static_assert(b.a.m == 3, "");
diff --git gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const13.C gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const13.C
new file mode 100644
index 00000000000..bc7faa3c250
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const13.C
@@ -0,0 +1,20 @@
+// PR c++/91264
+// { dg-do compile { target c++14 } }
+
+struct A {
+  mutable int i;
+  constexpr A() : i(0) { }
+};
+struct B {
+  A a;
+  constexpr B() : a{} { }
+};
+
+constexpr void
+g ()
+{
+  const B b;
+  b.a.i = 42;
+}
+
+static_assert((g(), 1), "");
diff --git gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const14.C gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const14.C
new file mode 100644
index 00000000000..45c4fcf50be
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const14.C
@@ -0,0 +1,38 @@
+// PR c++/91264
+// { dg-do compile { target c++14 } }
+
+struct F {
+  const int f;
+  constexpr F() : f(9) { }
+};
+
+struct C {
+  int n;
+  const F f;
+  constexpr C() : n(1) { n = 66; }
+};
+
+struct A {
+  int r;
+  const C c;
+  constexpr A() : r(11) { r = 14; const_cast<C &>(c).n = 42; } // { dg-error "modifying a const object" }
+};
+
+struct D {
+  const A a;
+  constexpr D() { } // { dg-message "in .constexpr. expansion of" }
+};
+
+struct E {
+  const D d;
+  constexpr E() { } // { dg-message "in .constexpr. expansion of" }
+};
+
+struct B {
+  const E e;
+  constexpr B(bool) { } // { dg-message "in .constexpr. expansion of" }
+};
+
+constexpr B b(false); // { dg-message "in .constexpr. expansion of" }
+// { dg-message "originally declared" "" { target *-*-* } .-1 }
+static_assert(b.e.d.a.c.n == 2, ""); // { dg-error "non-constant condition" }
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..54d83b17d39
--- /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" }
+}
diff --git gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const9.C gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const9.C
new file mode 100644
index 00000000000..0edec4d05cf
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const9.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{}; // { 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" }
+}

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

[Marek Polacek]

On Sun, Aug 18, 2019 at 08:37:14PM -0400, Marek Polacek wrote:
> Ok if that passes?

Which it did.

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

[Jason Merrill]

Ok, thanks.

On Sun, Aug 18, 2019, 6:26 PM Marek Polacek <polacek@redhat.com> wrote:

> On Sun, Aug 18, 2019 at 08:37:14PM -0400, Marek Polacek wrote:
> > Ok if that passes?
>
> Which it did.
>

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

[Paolo Carlini]

Hi,

On 31/07/19 21:26, Marek Polacek wrote:
> +static void
> +modifying_const_object_error (tree expr, tree obj)
> +{
> +  location_t loc = cp_expr_loc_or_loc (expr, input_location);

Nit: now we have cp_expr_loc_input_loc

Paolo.

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

[Marek Polacek]

On Tue, Aug 06, 2019 at 09:38:35PM +0200, Paolo Carlini wrote:
> Hi,
> 
> On 31/07/19 21:26, Marek Polacek wrote:
> > +static void
> > +modifying_const_object_error (tree expr, tree obj)
> > +{
> > +  location_t loc = cp_expr_loc_or_loc (expr, input_location);
> 
> Nit: now we have cp_expr_loc_input_loc

Ah yeah, and I was meaning to update it!  Will fix.

Marek