[PATCH] c++: Fix constexpr evaluation of self-modifying CONSTRUCTORs [PR94219]

[Patrick Palka <ppalka@redhat.com>]

This PR reveals that cxx_eval_bare_aggregate and cxx_eval_store_expression do
not anticipate that a constructor element's initializer could mutate the
underlying CONSTRUCTOR.  Evaluation of the initializer could add new elements to
the underlying CONSTRUCTOR, thereby potentially invalidating any pointers to
or assumptions about the CONSTRUCTOR's elements, and so these routines should be
prepared for that.

To fix this problem, this patch makes cxx_eval_bare_aggregate and
cxx_eval_store_expression recompute the pointer to the constructor_elt's through
which we're assigning, after it evaluates the initializer.  Care is taken to
to make the common case where the initializer does not modify the underlying
CONSTRUCTOR as fast as before.

Does this look OK to commit after testing?

gcc/cp/ChangeLog:

	PR c++/94205
	PR c++/94219
	* constexpr.c (get_or_insert_ctor_field): Split out (while adding
	support for VECTOR_TYPEs, and optimizations for the common case)
	from ...
	(cxx_eval_store_expression): ... here.  Rename local variable
	'changed_active_union_member_p' to 'activated_union_member_p'.  Record
	the sequence of indexes into 'indexes' that yields the subobject we're
	assigning to.  Record the integer offsets of the constructor indexes
	we're assigning through into 'index_pos_hints'.  After evaluating the
	initializer of the store expression, recompute 'valp' using 'indexes'
	and 'index_pos_hints' as hints.
	(cxx_eval_bare_aggregate): Tweak comments.  Use get_or_insert_ctor_field
	to recompute the pointer to the constructor_elt we're assigning through
	after evaluating each initializer.

gcc/testsuite/ChangeLog:

	PR c++/94205
	PR c++/94219
	* g++.dg/cpp1y/constexpr-nsdmi3.C: New test.
	* g++.dg/cpp1y/constexpr-nsdmi4.C: New test.
	* g++.dg/cpp1y/constexpr-nsdmi5.C: New test.
	* g++.dg/cpp1z/lambda-this5.C: New test.
---
 gcc/cp/constexpr.c                            | 252 +++++++++++-------
 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi3.C |  19 ++
 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi4.C |  21 ++
 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi5.C |  22 ++
 gcc/testsuite/g++.dg/cpp1z/lambda-this5.C     |  11 +
 5 files changed, 228 insertions(+), 97 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi3.C
 create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi4.C
 create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi5.C
 create mode 100644 gcc/testsuite/g++.dg/cpp1z/lambda-this5.C

diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
index 91f0c3ba269..b4173c595f0 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -3151,6 +3151,97 @@ find_array_ctor_elt (tree ary, tree dindex, bool insert)
   return -1;
 }
 
+/* Return a pointer to the constructor_elt of CTOR which matches INDEX.  If no
+   matching constructor_elt exists, then add one to CTOR.
+
+   As an optimization, if POS_HINT is non-negative then it is used as a guess
+   for the (integer) index of the matching constructor_elt within CTOR.  */
+
+static constructor_elt *
+get_or_insert_ctor_field (tree ctor, tree index, int pos_hint)
+{
+  tree type = TREE_TYPE (ctor);
+  if (TREE_CODE (type) == VECTOR_TYPE && index == NULL_TREE)
+    {
+      CONSTRUCTOR_APPEND_ELT (CONSTRUCTOR_ELTS (ctor), index, NULL_TREE);
+      return &CONSTRUCTOR_ELTS (ctor)->last();
+    }
+  else if (TREE_CODE (type) == ARRAY_TYPE || TREE_CODE (type) == VECTOR_TYPE)
+    {
+      HOST_WIDE_INT i = find_array_ctor_elt (ctor, index, /*insert*/true);
+      gcc_assert (i >= 0);
+      constructor_elt *cep = CONSTRUCTOR_ELT (ctor, i);
+      gcc_assert (cep->index == NULL_TREE
+		  || TREE_CODE (cep->index) != RANGE_EXPR);
+      return cep;
+    }
+  else
+    {
+      gcc_assert (TREE_CODE (index) == FIELD_DECL);
+
+      /* We must keep the CONSTRUCTOR's ELTS in FIELD order.
+	 Usually we meet initializers in that order, but it is
+	 possible for base types to be placed not in program
+	 order.  */
+      tree fields = TYPE_FIELDS (DECL_CONTEXT (index));
+      unsigned HOST_WIDE_INT idx = 0;
+      constructor_elt *cep = NULL;
+
+      /* First, check if we're changing the active member of a union.  */
+      if (TREE_CODE (type) == UNION_TYPE && CONSTRUCTOR_NELTS (ctor)
+	  && CONSTRUCTOR_ELT (ctor, 0)->index != index)
+	vec_safe_truncate (CONSTRUCTOR_ELTS (ctor), 0);
+      /* Next, check the hint.  */
+      else if (pos_hint >= 0 && (unsigned)pos_hint < CONSTRUCTOR_NELTS (ctor)
+	       && CONSTRUCTOR_ELT (ctor, pos_hint)->index == index)
+	{
+	  cep = CONSTRUCTOR_ELT (ctor, pos_hint);
+	  goto found;
+	}
+      /* If the hint was wrong, and if the bit offset of INDEX is larger than
+	 that of the last constructor_elt, then we can just immediately append a
+	 new constructor_elt to the end of CTOR.  */
+      else if (CONSTRUCTOR_NELTS (ctor)
+	       && tree_int_cst_compare (bit_position (index),
+					bit_position (CONSTRUCTOR_ELTS (ctor)
+						      ->last().index)) > 0)
+	{
+	  idx = CONSTRUCTOR_NELTS (ctor);
+	  goto insert;
+	}
+
+      /* Otherwise, we need to iterator over CTOR to find or insert INDEX
+	 appropriately.  */
+
+      for (; vec_safe_iterate (CONSTRUCTOR_ELTS (ctor), idx, &cep);
+	   idx++, fields = DECL_CHAIN (fields))
+	{
+	  if (index == cep->index)
+	    goto found;
+
+	  /* The field we're initializing must be on the field
+	     list.  Look to see if it is present before the
+	     field the current ELT initializes.  */
+	  for (; fields != cep->index; fields = DECL_CHAIN (fields))
+	    if (index == fields)
+	      goto insert;
+	}
+      /* We fell off the end of the CONSTRUCTOR, so insert a new
+	 entry at the end.  */
+
+    insert:
+      {
+	constructor_elt ce = { index, NULL_TREE };
+
+	vec_safe_insert (CONSTRUCTOR_ELTS (ctor), idx, ce);
+	cep = CONSTRUCTOR_ELT (ctor, idx);
+      }
+    found:;
+
+      return cep;
+    }
+}
+
 /* Under the control of CTX, issue a detailed diagnostic for
    an out-of-bounds subscript INDEX into the expression ARRAY.  */
 
@@ -3760,14 +3851,18 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx, tree t,
     {
       tree orig_value = value;
       init_subob_ctx (ctx, new_ctx, index, value);
+      int pos_hint = -1;
       if (new_ctx.ctor != ctx->ctor)
-	/* If we built a new CONSTRUCTOR, attach it now so that other
-	   initializers can refer to it.  */
-	CONSTRUCTOR_APPEND_ELT (*p, index, new_ctx.ctor);
+	{
+	  /* If we built a new CONSTRUCTOR, attach it now so that other
+	     initializers can refer to it.  */
+	  CONSTRUCTOR_APPEND_ELT (*p, index, new_ctx.ctor);
+	  pos_hint = vec_safe_length (*p) - 1;
+	}
       else if (TREE_CODE (type) == UNION_TYPE)
-	/* Otherwise if we're constructing a union, set the active union member
-	   anyway so that we can later detect if the initializer attempts to
-	   activate another member.  */
+	/* Otherwise if we're constructing a non-aggregate union member, set
+	   the active union member now so that we can later detect and diagnose
+	   if its initializer attempts to activate another member.  */
 	CONSTRUCTOR_APPEND_ELT (*p, index, NULL_TREE);
       tree elt = cxx_eval_constant_expression (&new_ctx, value,
 					       lval,
@@ -3804,18 +3899,18 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx, tree t,
 	{
 	  if (TREE_CODE (type) == UNION_TYPE
 	      && (*p)->last().index != index)
-	    /* The initializer may have erroneously changed the active union
-	       member that we're initializing.  */
+	    /* The initializer erroneously changed the active union member that
+	       we're initializing.  */
 	    gcc_assert (*non_constant_p);
-	  else if (new_ctx.ctor != ctx->ctor
-		   || TREE_CODE (type) == UNION_TYPE)
+	  else
 	    {
-	      /* We appended this element above; update the value.  */
-	      gcc_assert ((*p)->last().index == index);
-	      (*p)->last().value = elt;
+	      /* The initializer might have mutated the underlying CONSTRUCTOR,
+		 so recompute the location of the target constructer_elt.  */
+	      constructor_elt *cep
+		= get_or_insert_ctor_field (ctx->ctor, index, pos_hint);
+	      cep->value = elt;
 	    }
-	  else
-	    CONSTRUCTOR_APPEND_ELT (*p, index, elt);
+
 	  /* Adding or replacing an element might change the ctor's flags.  */
 	  TREE_CONSTANT (ctx->ctor) = constant_p;
 	  TREE_SIDE_EFFECTS (ctx->ctor) = side_effects_p;
@@ -4590,8 +4685,9 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
   type = TREE_TYPE (object);
   bool no_zero_init = true;
 
-  releasing_vec ctors;
-  bool changed_active_union_member_p = false;
+  releasing_vec ctors, indexes;
+  auto_vec<int> index_pos_hints;
+  bool activated_union_member_p = false;
   while (!refs->is_empty ())
     {
       if (*valp == NULL_TREE)
@@ -4632,94 +4728,49 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
 	 subobjects will also be zero-initialized.  */
       no_zero_init = CONSTRUCTOR_NO_CLEARING (*valp);
 
-      vec_safe_push (ctors, *valp);
-
       enum tree_code code = TREE_CODE (type);
       type = refs->pop();
       tree index = refs->pop();
 
-      constructor_elt *cep = NULL;
-      if (code == ARRAY_TYPE)
+      if (code == UNION_TYPE && CONSTRUCTOR_NELTS (*valp)
+	  && CONSTRUCTOR_ELT (*valp, 0)->index != index)
 	{
-	  HOST_WIDE_INT i
-	    = find_array_ctor_elt (*valp, index, /*insert*/true);
-	  gcc_assert (i >= 0);
-	  cep = CONSTRUCTOR_ELT (*valp, i);
-	  gcc_assert (cep->index == NULL_TREE
-		      || TREE_CODE (cep->index) != RANGE_EXPR);
-	}
-      else
-	{
-	  gcc_assert (TREE_CODE (index) == FIELD_DECL);
-
-	  /* We must keep the CONSTRUCTOR's ELTS in FIELD order.
-	     Usually we meet initializers in that order, but it is
-	     possible for base types to be placed not in program
-	     order.  */
-	  tree fields = TYPE_FIELDS (DECL_CONTEXT (index));
-	  unsigned HOST_WIDE_INT idx;
-
-	  if (code == UNION_TYPE && CONSTRUCTOR_NELTS (*valp)
-	      && CONSTRUCTOR_ELT (*valp, 0)->index != index)
+	  if (cxx_dialect < cxx2a)
 	    {
-	      if (cxx_dialect < cxx2a)
-		{
-		  if (!ctx->quiet)
-		    error_at (cp_expr_loc_or_input_loc (t),
-			      "change of the active member of a union "
-			      "from %qD to %qD",
-			      CONSTRUCTOR_ELT (*valp, 0)->index,
-			      index);
-		  *non_constant_p = true;
-		}
-	      else if (TREE_CODE (t) == MODIFY_EXPR
-		       && CONSTRUCTOR_NO_CLEARING (*valp))
-		{
-		  /* Diagnose changing the active union member while the union
-		     is in the process of being initialized.  */
-		  if (!ctx->quiet)
-		    error_at (cp_expr_loc_or_input_loc (t),
-			      "change of the active member of a union "
-			      "from %qD to %qD during initialization",
-			      CONSTRUCTOR_ELT (*valp, 0)->index,
-			      index);
-		  *non_constant_p = true;
-		}
-	      /* Changing active member.  */
-	      vec_safe_truncate (CONSTRUCTOR_ELTS (*valp), 0);
-	      no_zero_init = true;
+	      if (!ctx->quiet)
+		error_at (cp_expr_loc_or_input_loc (t),
+			  "change of the active member of a union "
+			  "from %qD to %qD",
+			  CONSTRUCTOR_ELT (*valp, 0)->index,
+			  index);
+	      *non_constant_p = true;
 	    }
-
-	  for (idx = 0;
-	       vec_safe_iterate (CONSTRUCTOR_ELTS (*valp), idx, &cep);
-	       idx++, fields = DECL_CHAIN (fields))
+	  else if (TREE_CODE (t) == MODIFY_EXPR
+		   && CONSTRUCTOR_NO_CLEARING (*valp))
 	    {
-	      if (index == cep->index)
-		goto found;
-
-	      /* The field we're initializing must be on the field
-		 list.  Look to see if it is present before the
-		 field the current ELT initializes.  */
-	      for (; fields != cep->index; fields = DECL_CHAIN (fields))
-		if (index == fields)
-		  goto insert;
+	      /* Diagnose changing the active union member while the union
+		 is in the process of being initialized.  */
+	      if (!ctx->quiet)
+		error_at (cp_expr_loc_or_input_loc (t),
+			  "change of the active member of a union "
+			  "from %qD to %qD during initialization",
+			  CONSTRUCTOR_ELT (*valp, 0)->index,
+			  index);
+	      *non_constant_p = true;
 	    }
+	  no_zero_init = true;
+	}
 
-	  /* We fell off the end of the CONSTRUCTOR, so insert a new
-	     entry at the end.  */
-	insert:
-	  {
-	    constructor_elt ce = { index, NULL_TREE };
+      vec_safe_push (ctors, *valp);
+      vec_safe_push (indexes, index);
 
-	    vec_safe_insert (CONSTRUCTOR_ELTS (*valp), idx, ce);
-	    cep = CONSTRUCTOR_ELT (*valp, idx);
+      constructor_elt *cep
+	= get_or_insert_ctor_field (*valp, index, /*pos_hint=*/-1);
+      index_pos_hints.safe_push (cep - CONSTRUCTOR_ELT (*valp, 0));
+
+      if (code == UNION_TYPE)
+	activated_union_member_p = true;
 
-	    if (code == UNION_TYPE)
-	      /* Record that we've changed an active union member.  */
-	      changed_active_union_member_p = true;
-	  }
-	found:;
-	}
       valp = &cep->value;
     }
 
@@ -4800,9 +4851,16 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
 	  init = tinit;
       init = cxx_eval_constant_expression (&new_ctx, init, false,
 					   non_constant_p, overflow_p);
-      if (ctors->is_empty())
-	/* The hash table might have moved since the get earlier.  */
-	valp = ctx->global->values.get (object);
+      /* The hash table might have moved since the get earlier.  */
+      valp = ctx->global->values.get (object);
+      /* The initializer might have mutated the underlying CONSTRUCTORs, so we
+	 must recompute VALP.  */
+      for (unsigned i = 0; i < vec_safe_length (indexes); i++)
+	{
+	  constructor_elt *cep
+	    = get_or_insert_ctor_field (*valp, indexes[i], index_pos_hints[i]);
+	  valp = &cep->value;
+	}
     }
 
   /* Don't share a CONSTRUCTOR that might be changed later.  */
@@ -4847,7 +4905,7 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
   unsigned i;
   bool c = TREE_CONSTANT (init);
   bool s = TREE_SIDE_EFFECTS (init);
-  if (!c || s || changed_active_union_member_p)
+  if (!c || s || activated_union_member_p)
     FOR_EACH_VEC_ELT (*ctors, i, elt)
       {
 	if (!c)
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi3.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi3.C
new file mode 100644
index 00000000000..0f91e93ca3f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi3.C
@@ -0,0 +1,19 @@
+// PR c++/94205
+// { dg-do compile { target c++14 } }
+
+struct S
+{
+  struct A
+  {
+    S *p;
+    constexpr A(S* p): p(p) {}
+    constexpr operator int() { p->a = 5; return 6; }
+  };
+  int a = A(this);
+};
+
+
+constexpr S s = {};
+
+#define SA(X) static_assert((X),#X)
+SA(s.a == 6);
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi4.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi4.C
new file mode 100644
index 00000000000..0ceef1bb29f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi4.C
@@ -0,0 +1,21 @@
+// PR c++/94219
+// { dg-do compile { target c++14 } }
+
+struct A { long x; };
+
+struct U;
+constexpr A foo(U *up);
+
+struct U {
+  A a = foo(this); int y;
+};
+
+constexpr A foo(U *up) {
+  up->y = 11;
+  return {42};
+}
+
+extern constexpr U u = {};
+
+static_assert(u.y == 11, "");
+static_assert(u.a.x == 42, "");
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi5.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi5.C
new file mode 100644
index 00000000000..59e7a10d6e8
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi5.C
@@ -0,0 +1,22 @@
+// PR c++/94219
+// { dg-do compile { target c++14 } }
+
+struct A { long x; };
+
+struct U;
+constexpr A foo(U *up);
+
+struct U {
+  U() = default;
+  int y; A a = foo(this);
+};
+
+constexpr A foo(U *up) {
+  up->y = 11;
+  return {42};
+}
+
+extern constexpr U u = {};
+
+static_assert(u.y == 11, "");
+static_assert(u.a.x == 42, "");
diff --git a/gcc/testsuite/g++.dg/cpp1z/lambda-this5.C b/gcc/testsuite/g++.dg/cpp1z/lambda-this5.C
new file mode 100644
index 00000000000..c6d44d7fd0b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1z/lambda-this5.C
@@ -0,0 +1,11 @@
+// PR c++/94205
+// { dg-do compile { target c++17 } }
+
+struct S
+{
+  int a = [this] { this->a = 5; return 6; } ();
+};
+
+constexpr S s = {};
+
+static_assert(s.a == 6);
-- 
2.26.0.106.g9fadedd637