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

In this PR, we are performing constexpr evaluation of a CONSTRUCTOR of type
union U which looks like

  {.a=foo (&<PLACEHOLDER_EXPR union U>)}.

Since the function foo takes a reference to the CONSTRUCTOR we're building, it
could potentially modify the CONSTRUCTOR from under us.  In particular since U
is a union, the evaluation of a's initializer could change the active member
from a to another member -- something which cxx_eval_bare_aggregate doesn't
expect to happen.

Upon further investigation, it turns out this issue is not limited to
constructors of UNION_TYPE and not limited to cxx_eval_bare_aggregate either.
For example, within cxx_eval_store_expression we may be evaluating an assignment
such as (this comes from the test pr94066-2.C):

  ((union U *) this)->a = TARGET_EXPR <D.2163, foo ((union U *) this)>;

where evaluation of foo could change the active member of *this, which was set
earlier in cxx_eval_store_expression to 'a'.  And if U is a RECORD_TYPE, then
evaluation of foo could add new fields to *this, thereby making stale the 'valp'
pointer to the target constructor_elt through which we're later assigning.

So in short, it seems that both cxx_eval_bare_aggregate and
cxx_eval_store_expression do not anticipate that a constructor_elt's initializer
could modify the underlying CONSTRUCTOR as a side-effect.

To fix this problem, this patch makes cxx_eval_bare_aggregate and
cxx_eval_store_expression recompute the pointer to the constructor_elt through
which we're assigning, after the initializer has been evaluated.

I am worried that the use of get_or_insert_ctor_field in cxx_eval_bare_aggregate
might introduce quadratic behavior where there wasn't before.  I wonder if
there's a cheap heuristic we can use in cxx_eval_bare_aggregate to determine
whether "self-modification" took place, and in which case we could avoid calling
get_or_insert_ctor_field and do the fast thing we that we were doing before the
patch.

Tested on x86_64-pc-linux-gnu, but a full bootstrap is in progress.  Does this
look like the right approach?

gcc/cp/ChangeLog:

	PR c++/94066
	* constexpr.c (get_or_insert_ctor_field): Split out (while adding
	handling for VECTOR_TYPEs) from ...
	(cxx_eval_store_expression): ... here.  Record the sequence of indexes
	into INDEXES that yields the suboject we're assigning to.  After
	evaluating the initializer of the store expression, recompute valp using
	INDEXES.
	(cxx_eval_bare_aggregate): 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++/94066
	* g++.dg/cpp1y/pr94066.C: New test.
	* g++.dg/cpp1y/pr94066-2.C: New test.
	* g++.dg/cpp1y/pr94066-3.C: New test.
	* g++.dg/cpp1y/pr94066-4.C: New test.
	* g++.dg/cpp1y/pr94066-5.C: New test.
---
 gcc/cp/constexpr.c                     | 184 +++++++++++++++----------
 gcc/testsuite/g++.dg/cpp1y/pr94066-2.C |  21 +++
 gcc/testsuite/g++.dg/cpp1y/pr94066-3.C |  21 +++
 gcc/testsuite/g++.dg/cpp1y/pr94066-4.C |  22 +++
 gcc/testsuite/g++.dg/cpp1y/pr94066-5.C |  18 +++
 gcc/testsuite/g++.dg/cpp1y/pr94066.C   |  20 +++
 6 files changed, 210 insertions(+), 76 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr94066-2.C
 create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr94066-3.C
 create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr94066-4.C
 create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr94066-5.C
 create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr94066.C

On 3/16/20 1:39 PM, Patrick Palka wrote:
> In this PR, we are performing constexpr evaluation of a CONSTRUCTOR of type
> union U which looks like
> 
>    {.a=foo (&<PLACEHOLDER_EXPR union U>)}.
> 
> Since the function foo takes a reference to the CONSTRUCTOR we're building, it
> could potentially modify the CONSTRUCTOR from under us.  In particular since U
> is a union, the evaluation of a's initializer could change the active member
> from a to another member -- something which cxx_eval_bare_aggregate doesn't
> expect to happen.
> 
> Upon further investigation, it turns out this issue is not limited to
> constructors of UNION_TYPE and not limited to cxx_eval_bare_aggregate either.
> For example, within cxx_eval_store_expression we may be evaluating an assignment
> such as (this comes from the test pr94066-2.C):
> 
>    ((union U *) this)->a = TARGET_EXPR <D.2163, foo ((union U *) this)>;

I assume this is actually an INIT_EXPR, or we would have preevaluated 
and not had this problem.

> where evaluation of foo could change the active member of *this, which was set
> earlier in cxx_eval_store_expression to 'a'.  And if U is a RECORD_TYPE, then
> evaluation of foo could add new fields to *this, thereby making stale the 'valp'
> pointer to the target constructor_elt through which we're later assigning.
> 
> So in short, it seems that both cxx_eval_bare_aggregate and
> cxx_eval_store_expression do not anticipate that a constructor_elt's initializer
> could modify the underlying CONSTRUCTOR as a side-effect.

Oof.  If this is well-formed, it's because initialization of a doesn't 
actually start until the return statement of foo, so we're probably 
wrong to create a CONSTRUCTOR to hold the value of 'a' before evaluating 
foo.  Perhaps init_subob_ctx shouldn't preemptively create a 
CONSTRUCTOR, and similarly for the cxx_eval_store_expression !preeval code.

> To fix this problem, this patch makes cxx_eval_bare_aggregate and
> cxx_eval_store_expression recompute the pointer to the constructor_elt through
> which we're assigning, after the initializer has been evaluated.
> 
> I am worried that the use of get_or_insert_ctor_field in cxx_eval_bare_aggregate
> might introduce quadratic behavior where there wasn't before.  I wonder if
> there's a cheap heuristic we can use in cxx_eval_bare_aggregate to determine
> whether "self-modification" took place, and in which case we could avoid calling
> get_or_insert_ctor_field and do the fast thing we that we were doing before the
> patch.

We could remember the (integer) index of the constructor element and 
verify that our sub-ctors are still at those indices before doing a new 
search.

> Tested on x86_64-pc-linux-gnu, but a full bootstrap is in progress.  Does this
> look like the right approach?
> 
> gcc/cp/ChangeLog:
> 
> 	PR c++/94066
> 	* constexpr.c (get_or_insert_ctor_field): Split out (while adding
> 	handling for VECTOR_TYPEs) from ...
> 	(cxx_eval_store_expression): ... here.  Record the sequence of indexes
> 	into INDEXES that yields the suboject we're assigning to.  After
> 	evaluating the initializer of the store expression, recompute valp using
> 	INDEXES.
> 	(cxx_eval_bare_aggregate): 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++/94066
> 	* g++.dg/cpp1y/pr94066.C: New test.
> 	* g++.dg/cpp1y/pr94066-2.C: New test.
> 	* g++.dg/cpp1y/pr94066-3.C: New test.
> 	* g++.dg/cpp1y/pr94066-4.C: New test.
> 	* g++.dg/cpp1y/pr94066-5.C: New test.
> ---
>   gcc/cp/constexpr.c                     | 184 +++++++++++++++----------
>   gcc/testsuite/g++.dg/cpp1y/pr94066-2.C |  21 +++
>   gcc/testsuite/g++.dg/cpp1y/pr94066-3.C |  21 +++
>   gcc/testsuite/g++.dg/cpp1y/pr94066-4.C |  22 +++
>   gcc/testsuite/g++.dg/cpp1y/pr94066-5.C |  18 +++
>   gcc/testsuite/g++.dg/cpp1y/pr94066.C   |  20 +++
>   6 files changed, 210 insertions(+), 76 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr94066-2.C
>   create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr94066-3.C
>   create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr94066-4.C
>   create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr94066-5.C
>   create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr94066.C
> 
> diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
> index 192face9a3a..9fcf9f33457 100644
> --- a/gcc/cp/constexpr.c
> +++ b/gcc/cp/constexpr.c
> @@ -3144,6 +3144,88 @@ 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 it to CTOR.  Emit an appropriate
> +   diagnostic if CTOR constructs a UNION_TYPE and adding INDEX would change the
> +   active member of the union, unless QUIET is true.   */
> +
> +static constructor_elt *
> +get_or_insert_ctor_field (tree ctor, tree index,
> +			  bool quiet, bool *non_constant_p)
> +{
> +  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;
> +
> +      if (TREE_CODE (type) == UNION_TYPE && CONSTRUCTOR_NELTS (ctor)
> +	  && CONSTRUCTOR_ELT (ctor, 0)->index != index)
> +	{
> +	  if (cxx_dialect < cxx2a)
> +	    {
> +	      if (!quiet)
> +		error ("change of the active member of a union from %qD to %qD",
> +			CONSTRUCTOR_ELT (ctor, 0)->index,
> +			index);
> +	      *non_constant_p = true;
> +	    }
> +	  /* Changing active member.  */
> +	  vec_safe_truncate (CONSTRUCTOR_ELTS (ctor), 0);
> +	}
> +
> +      constructor_elt *cep = NULL;
> +      for (idx = 0;
> +	   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.  */
>   
> @@ -3784,14 +3866,12 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx, tree t,
>   	}
>         else
>   	{
> -	  if (new_ctx.ctor != ctx->ctor)
> -	    {
> -	      /* We appended this element above; update the value.  */
> -	      gcc_assert ((*p)->last().index == index);
> -	      (*p)->last().value = elt;
> -	    }
> -	  else
> -	    CONSTRUCTOR_APPEND_ELT (*p, index, elt);
> +	  iloc_sentinel ils (cp_expr_location (t));
> +	  constructor_elt *cep
> +	    = get_or_insert_ctor_field (ctx->ctor, index,
> +					ctx->quiet, non_constant_p);
> +	  cep->value = 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;
> @@ -4566,7 +4646,7 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
>     type = TREE_TYPE (object);
>     bool no_zero_init = true;
>   
> -  releasing_vec ctors;
> +  releasing_vec ctors, indexes;
>     while (!refs->is_empty ())
>       {
>         if (*valp == NULL_TREE)
> @@ -4607,77 +4687,21 @@ 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)
> -	{
> -	  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 (!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;
> -		}
> -	      /* Changing active member.  */
> -	      vec_safe_truncate (CONSTRUCTOR_ELTS (*valp), 0);
> -	      no_zero_init = true;
> -	    }
> +      vec_safe_push (ctors, *valp);
> +      vec_safe_push (indexes, index);
>   
> -	  for (idx = 0;
> -	       vec_safe_iterate (CONSTRUCTOR_ELTS (*valp), 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;
> -	    }
> +      if (code == UNION_TYPE && CONSTRUCTOR_NELTS (*valp)
> +	  && CONSTRUCTOR_ELT (*valp, 0)->index != index)
> +	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 };
> +      iloc_sentinel ils (cp_expr_location (t));
> +      constructor_elt *cep
> +	= get_or_insert_ctor_field (*valp, index, ctx->quiet, non_constant_p);
>   
> -	    vec_safe_insert (CONSTRUCTOR_ELTS (*valp), idx, ce);
> -	    cep = CONSTRUCTOR_ELT (*valp, idx);
> -	  }
> -	found:;
> -	}
>         valp = &cep->value;
>       }
>   
> @@ -4758,9 +4782,17 @@ 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);
> +      /* And the underlying CONSTRUCTORs may have been modified by the
> +	 initializer.  */
> +      for (unsigned i = 0; i < vec_safe_length (indexes); i++)
> +	{
> +	  constructor_elt *cep
> +	    = get_or_insert_ctor_field (*valp, indexes[i],
> +					/*quiet=*/true, non_constant_p);
> +	  valp = &cep->value;
> +	}
>       }
>   
>     /* Don't share a CONSTRUCTOR that might be changed later.  */
> diff --git a/gcc/testsuite/g++.dg/cpp1y/pr94066-2.C b/gcc/testsuite/g++.dg/cpp1y/pr94066-2.C
> new file mode 100644
> index 00000000000..ec8210a9d73
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-2.C
> @@ -0,0 +1,21 @@
> +// PR c++/94066
> +// { dg-do compile { target c++14 } }
> +
> +struct A { long x; };
> +
> +union U;
> +constexpr A foo(U *up);
> +
> +union U {
> +  U() = default;
> +  A a = foo(this); int y;
> +};
> +
> +constexpr A foo(U *up) {
> +  up->y = 11; // { dg-error ".U::a. to .U::y." "" { target c++17_down } }
> +  return {42};
> +}
> +
> +extern constexpr U u = {}; // { dg-error ".U::y. to .U::a." "" { target c++17_down } }
> +
> +static_assert(u.a.x == 42, ""); // { dg-error "non-constant" "" { target c++17_down } }
> diff --git a/gcc/testsuite/g++.dg/cpp1y/pr94066-3.C b/gcc/testsuite/g++.dg/cpp1y/pr94066-3.C
> new file mode 100644
> index 00000000000..b68c975d569
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-3.C
> @@ -0,0 +1,21 @@
> +// PR c++/94066
> +// { 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/pr94066-4.C b/gcc/testsuite/g++.dg/cpp1y/pr94066-4.C
> new file mode 100644
> index 00000000000..0362c939faf
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-4.C
> @@ -0,0 +1,22 @@
> +// PR c++/94066
> +// { 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/cpp1y/pr94066-5.C b/gcc/testsuite/g++.dg/cpp1y/pr94066-5.C
> new file mode 100644
> index 00000000000..63069f3ea5f
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-5.C
> @@ -0,0 +1,18 @@
> +// PR c++/94066
> +// { dg-do compile { target c++14 } }
> +
> +union U;
> +constexpr int foo(U *up);
> +
> +union U {
> +  int a = foo(this); int y;
> +};
> +
> +constexpr int foo(U *up) {
> +  up->y = 11;
> +  return 42;
> +}
> +
> +extern constexpr U u = {}; // { dg-error ".U::y. to .U::a." "" { target c++17_down } }
> +
> +static_assert(u.a == 42, ""); // { dg-error "non-constant" "" { target c++17_down } }
> diff --git a/gcc/testsuite/g++.dg/cpp1y/pr94066.C b/gcc/testsuite/g++.dg/cpp1y/pr94066.C
> new file mode 100644
> index 00000000000..11912199406
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/pr94066.C
> @@ -0,0 +1,20 @@
> +// PR c++/94066
> +// { dg-do compile { target c++14 } }
> +
> +struct A { long x; };
> +
> +union U;
> +constexpr A foo(U *up);
> +
> +union U {
> +  A a = foo(this); int y;
> +};
> +
> +constexpr A foo(U *up) {
> +  up->y = 11; // { dg-error ".U::a. to .U::y." "" { target c++17_down } }
> +  return {42};
> +}
> +
> +extern constexpr U u = {}; // { dg-error ".U::y. to .U::a." "" { target c++17_down } }
> +
> +static_assert(u.a.x == 42, ""); // { dg-error "non-constant" "" { target c++17_down } }
>

On Tue, 17 Mar 2020, Jason Merrill wrote:

> On 3/16/20 1:39 PM, Patrick Palka wrote:
> > In this PR, we are performing constexpr evaluation of a CONSTRUCTOR of type
> > union U which looks like
> > 
> >    {.a=foo (&<PLACEHOLDER_EXPR union U>)}.
> > 
> > Since the function foo takes a reference to the CONSTRUCTOR we're building,
> > it
> > could potentially modify the CONSTRUCTOR from under us.  In particular since
> > U
> > is a union, the evaluation of a's initializer could change the active member
> > from a to another member -- something which cxx_eval_bare_aggregate doesn't
> > expect to happen.
> > 
> > Upon further investigation, it turns out this issue is not limited to
> > constructors of UNION_TYPE and not limited to cxx_eval_bare_aggregate
> > either.
> > For example, within cxx_eval_store_expression we may be evaluating an
> > assignment
> > such as (this comes from the test pr94066-2.C):
> > 
> >    ((union U *) this)->a = TARGET_EXPR <D.2163, foo ((union U *) this)>;
> 
> I assume this is actually an INIT_EXPR, or we would have preevaluated and not
> had this problem.

Yes exactly, I should have specified that the above is an INIT_EXPR and
not a MODIFY_EXPR.

> 
> > where evaluation of foo could change the active member of *this, which was
> > set
> > earlier in cxx_eval_store_expression to 'a'.  And if U is a RECORD_TYPE,
> > then
> > evaluation of foo could add new fields to *this, thereby making stale the
> > 'valp'
> > pointer to the target constructor_elt through which we're later assigning.
> > 
> > So in short, it seems that both cxx_eval_bare_aggregate and
> > cxx_eval_store_expression do not anticipate that a constructor_elt's
> > initializer
> > could modify the underlying CONSTRUCTOR as a side-effect.
> 
> Oof.  If this is well-formed, it's because initialization of a doesn't
> actually start until the return statement of foo, so we're probably wrong to
> create a CONSTRUCTOR to hold the value of 'a' before evaluating foo.  Perhaps
> init_subob_ctx shouldn't preemptively create a CONSTRUCTOR, and similarly for
> the cxx_eval_store_expression !preeval code.

Hmm, I think I see what you mean.  I'll look into this.

> 
> > To fix this problem, this patch makes cxx_eval_bare_aggregate and
> > cxx_eval_store_expression recompute the pointer to the constructor_elt
> > through
> > which we're assigning, after the initializer has been evaluated.
> > 
> > I am worried that the use of get_or_insert_ctor_field in
> > cxx_eval_bare_aggregate
> > might introduce quadratic behavior where there wasn't before.  I wonder if
> > there's a cheap heuristic we can use in cxx_eval_bare_aggregate to determine
> > whether "self-modification" took place, and in which case we could avoid
> > calling
> > get_or_insert_ctor_field and do the fast thing we that we were doing before
> > the
> > patch.
> 
> We could remember the (integer) index of the constructor element and verify
> that our sub-ctors are still at those indices before doing a new search.

Good idea, this should essentially eliminate the overhead of the second
set of calls to get_or_insert_ctor_fields in cxx_eval_store_expression
in the common case where the initializer doesn't mutate the underlying
CONSTRUCTORs.  I added this optimization to v2 of the patch, through a
new 'pos_hint' parameter of get_or_insert_ctor_fields.

As another optimization to get_or_insert_ctor_fields, we can first check
whether the bit_position of INDEX is greater than the bit_position of
the last constructor element of CTOR.  In his case we can immediately
append a new constructor element to CTOR, and avoid iterating over the
whole TYPE_FIELDS chain and over CTOR.  I think this optimization
eliminates the potential quadratic behavior in cxx_eval_bare_aggregate
in the common case.

With these two optimizations, the added overhead of the recomputations
added by this patch should be negligible.

-- >8 --

gcc/cp/ChangeLog:

	PR c++/94066
	* constexpr.c (get_or_insert_ctor_field): Split out (while adding
	handling for VECTOR_TYPEs, and common-case optimizations) from ...
	(cxx_eval_store_expression): ... here.  Record the sequence of indexes
	into INDEXES that yields the suboject we're assigning to.  Record the
	integer offsets of the constructor indexes we're assigning through into
	INDEX_POSITIONS.  After evaluating the initializer of the store
	expression, recompute valp using INDEXES and using INDEX_POSITIONS as
	hints.
	(cxx_eval_bare_aggregate): 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++/94066
	* g++.dg/cpp1y/pr94066.C: New test.
	* g++.dg/cpp1y/pr94066-2.C: New test.
	* g++.dg/cpp1y/pr94066-3.C: New test.
	* g++.dg/cpp1y/pr94066-4.C: New test.
	* g++.dg/cpp1y/pr94066-5.C: New test.
---
 gcc/cp/constexpr.c                     | 219 ++++++++++++++++---------
 gcc/testsuite/g++.dg/cpp1y/pr94066-2.C |  21 +++
 gcc/testsuite/g++.dg/cpp1y/pr94066-3.C |  21 +++
 gcc/testsuite/g++.dg/cpp1y/pr94066-4.C |  22 +++
 gcc/testsuite/g++.dg/cpp1y/pr94066-5.C |  18 ++
 gcc/testsuite/g++.dg/cpp1y/pr94066.C   |  20 +++
 6 files changed, 242 insertions(+), 79 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr94066-2.C
 create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr94066-3.C
 create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr94066-4.C
 create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr94066-5.C
 create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr94066.C

diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
index 192face9a3a..9ea64467661 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -3144,6 +3144,107 @@ 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.  Emit an appropriate
+   diagnostic if CTOR constructs a UNION_TYPE and adding INDEX would change the
+   active member of the union, unless QUIET is true.  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,
+			  bool quiet, bool *non_constant_p)
+{
+  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;
+
+      if (TREE_CODE (type) == UNION_TYPE && CONSTRUCTOR_NELTS (ctor)
+	  && CONSTRUCTOR_ELT (ctor, 0)->index != index)
+	{
+	  if (cxx_dialect < cxx2a)
+	    {
+	      if (!quiet)
+		error ("change of the active member of a union from %qD to %qD",
+			CONSTRUCTOR_ELT (ctor, 0)->index,
+			index);
+	      *non_constant_p = true;
+	    }
+	  /* Changing active member.  */
+	  vec_safe_truncate (CONSTRUCTOR_ELTS (ctor), 0);
+	}
+      /* 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 bit offset of INDEX is at larger than that of the endmost
+	 constructor_elt, then just immediately append a new constructor_elt to
+	 the end of CTOR.  */
+      else if (TREE_CODE (type) == RECORD_TYPE && CONSTRUCTOR_NELTS (ctor)
+	       && tree_int_cst_compare (bit_position (index),
+					bit_position (CONSTRUCTOR_ELTS (ctor)
+						      ->last().index)) > 0)
+	{
+	  idx = CONSTRUCTOR_NELTS (ctor);
+	  goto insert;
+	}
+
+      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.  */
 
@@ -3747,10 +3848,14 @@ 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;
+	}
       tree elt = cxx_eval_constant_expression (&new_ctx, value,
 					       lval,
 					       non_constant_p, overflow_p);
@@ -3784,14 +3889,15 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx, tree t,
 	}
       else
 	{
-	  if (new_ctx.ctor != ctx->ctor)
-	    {
-	      /* We appended this element above; update the value.  */
-	      gcc_assert ((*p)->last().index == index);
-	      (*p)->last().value = elt;
-	    }
-	  else
-	    CONSTRUCTOR_APPEND_ELT (*p, index, elt);
+	  /* The underlying CONSTRUCTOR might have been mutated by the
+	     initializer, so recompute the location of the target
+	     constructor_elt.  */
+	  iloc_sentinel ils (cp_expr_location (t));
+	  constructor_elt *cep
+	    = get_or_insert_ctor_field (ctx->ctor, index, pos_hint,
+					ctx->quiet, non_constant_p);
+	  cep->value = 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;
@@ -4566,7 +4672,8 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
   type = TREE_TYPE (object);
   bool no_zero_init = true;
 
-  releasing_vec ctors;
+  releasing_vec ctors, indexes;
+  auto_vec<int> index_positions;
   while (!refs->is_empty ())
     {
       if (*valp == NULL_TREE)
@@ -4607,77 +4714,23 @@ 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)
-	{
-	  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 (!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;
-		}
-	      /* Changing active member.  */
-	      vec_safe_truncate (CONSTRUCTOR_ELTS (*valp), 0);
-	      no_zero_init = true;
-	    }
+      vec_safe_push (ctors, *valp);
+      vec_safe_push (indexes, index);
 
-	  for (idx = 0;
-	       vec_safe_iterate (CONSTRUCTOR_ELTS (*valp), 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;
-	    }
+      if (code == UNION_TYPE && CONSTRUCTOR_NELTS (*valp)
+	  && CONSTRUCTOR_ELT (*valp, 0)->index != index)
+	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 };
+      iloc_sentinel ils (cp_expr_location (t));
+      constructor_elt *cep
+	= get_or_insert_ctor_field (*valp, index, /*pos_hint=*/-1,
+				    ctx->quiet, non_constant_p);
+      index_positions.safe_push (cep - CONSTRUCTOR_ELTS (*valp)->address ());
 
-	    vec_safe_insert (CONSTRUCTOR_ELTS (*valp), idx, ce);
-	    cep = CONSTRUCTOR_ELT (*valp, idx);
-	  }
-	found:;
-	}
       valp = &cep->value;
     }
 
@@ -4758,9 +4811,17 @@ 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);
+      /* And the underlying CONSTRUCTORs might have been mutated by the
+	 initializer, 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_positions[i],
+					/*quiet=*/true, non_constant_p);
+	  valp = &cep->value;
+	}
     }
 
   /* Don't share a CONSTRUCTOR that might be changed later.  */
diff --git a/gcc/testsuite/g++.dg/cpp1y/pr94066-2.C b/gcc/testsuite/g++.dg/cpp1y/pr94066-2.C
new file mode 100644
index 00000000000..ec8210a9d73
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-2.C
@@ -0,0 +1,21 @@
+// PR c++/94066
+// { dg-do compile { target c++14 } }
+
+struct A { long x; };
+
+union U;
+constexpr A foo(U *up);
+
+union U {
+  U() = default;
+  A a = foo(this); int y;
+};
+
+constexpr A foo(U *up) {
+  up->y = 11; // { dg-error ".U::a. to .U::y." "" { target c++17_down } }
+  return {42};
+}
+
+extern constexpr U u = {}; // { dg-error ".U::y. to .U::a." "" { target c++17_down } }
+
+static_assert(u.a.x == 42, ""); // { dg-error "non-constant" "" { target c++17_down } }
diff --git a/gcc/testsuite/g++.dg/cpp1y/pr94066-3.C b/gcc/testsuite/g++.dg/cpp1y/pr94066-3.C
new file mode 100644
index 00000000000..b68c975d569
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-3.C
@@ -0,0 +1,21 @@
+// PR c++/94066
+// { 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/pr94066-4.C b/gcc/testsuite/g++.dg/cpp1y/pr94066-4.C
new file mode 100644
index 00000000000..0362c939faf
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-4.C
@@ -0,0 +1,22 @@
+// PR c++/94066
+// { 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/cpp1y/pr94066-5.C b/gcc/testsuite/g++.dg/cpp1y/pr94066-5.C
new file mode 100644
index 00000000000..63069f3ea5f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-5.C
@@ -0,0 +1,18 @@
+// PR c++/94066
+// { dg-do compile { target c++14 } }
+
+union U;
+constexpr int foo(U *up);
+
+union U {
+  int a = foo(this); int y;
+};
+
+constexpr int foo(U *up) {
+  up->y = 11;
+  return 42;
+}
+
+extern constexpr U u = {}; // { dg-error ".U::y. to .U::a." "" { target c++17_down } }
+
+static_assert(u.a == 42, ""); // { dg-error "non-constant" "" { target c++17_down } }
diff --git a/gcc/testsuite/g++.dg/cpp1y/pr94066.C b/gcc/testsuite/g++.dg/cpp1y/pr94066.C
new file mode 100644
index 00000000000..11912199406
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/pr94066.C
@@ -0,0 +1,20 @@
+// PR c++/94066
+// { dg-do compile { target c++14 } }
+
+struct A { long x; };
+
+union U;
+constexpr A foo(U *up);
+
+union U {
+  A a = foo(this); int y;
+};
+
+constexpr A foo(U *up) {
+  up->y = 11; // { dg-error ".U::a. to .U::y." "" { target c++17_down } }
+  return {42};
+}
+
+extern constexpr U u = {}; // { dg-error ".U::y. to .U::a." "" { target c++17_down } }
+
+static_assert(u.a.x == 42, ""); // { dg-error "non-constant" "" { target c++17_down } }

On Wed, 18 Mar 2020, Patrick Palka wrote:

> On Tue, 17 Mar 2020, Jason Merrill wrote:
> 
> > On 3/16/20 1:39 PM, Patrick Palka wrote:
> > > In this PR, we are performing constexpr evaluation of a CONSTRUCTOR of type
> > > union U which looks like
> > > 
> > >    {.a=foo (&<PLACEHOLDER_EXPR union U>)}.
> > > 
> > > Since the function foo takes a reference to the CONSTRUCTOR we're building,
> > > it
> > > could potentially modify the CONSTRUCTOR from under us.  In particular since
> > > U
> > > is a union, the evaluation of a's initializer could change the active member
> > > from a to another member -- something which cxx_eval_bare_aggregate doesn't
> > > expect to happen.
> > > 
> > > Upon further investigation, it turns out this issue is not limited to
> > > constructors of UNION_TYPE and not limited to cxx_eval_bare_aggregate
> > > either.
> > > For example, within cxx_eval_store_expression we may be evaluating an
> > > assignment
> > > such as (this comes from the test pr94066-2.C):
> > > 
> > >    ((union U *) this)->a = TARGET_EXPR <D.2163, foo ((union U *) this)>;
> > 
> > I assume this is actually an INIT_EXPR, or we would have preevaluated and not
> > had this problem.
> 
> Yes exactly, I should have specified that the above is an INIT_EXPR and
> not a MODIFY_EXPR.
> 
> > 
> > > where evaluation of foo could change the active member of *this, which was
> > > set
> > > earlier in cxx_eval_store_expression to 'a'.  And if U is a RECORD_TYPE,
> > > then
> > > evaluation of foo could add new fields to *this, thereby making stale the
> > > 'valp'
> > > pointer to the target constructor_elt through which we're later assigning.
> > > 
> > > So in short, it seems that both cxx_eval_bare_aggregate and
> > > cxx_eval_store_expression do not anticipate that a constructor_elt's
> > > initializer
> > > could modify the underlying CONSTRUCTOR as a side-effect.
> > 
> > Oof.  If this is well-formed, it's because initialization of a doesn't
> > actually start until the return statement of foo, so we're probably wrong to
> > create a CONSTRUCTOR to hold the value of 'a' before evaluating foo.  Perhaps
> > init_subob_ctx shouldn't preemptively create a CONSTRUCTOR, and similarly for
> > the cxx_eval_store_expression !preeval code.
> 
> Hmm, I think I see what you mean.  I'll look into this.

In cpp0x/constexpr-array12.C we have

    struct A { int ar[3]; };
    constexpr A a1 = { 0, a1.ar[0] };

the initializer for a1 is a CONSTRUCTOR with the form

    {.ar={0, (int) VIEW_CONVERT_EXPR<const struct A>(a1).ar[0]}}

If we don't preemptively create a CONSTRUCTOR in cxx_eval_bare_aggregate
to hold the value of 'ar' before evaluating its initializer, then we
won't be able to resolve the 'a1.ar[0]' later on, and we will reject
this otherwise valid test case with an "accessing an uninitialized array
element" diagnostic.  So it seems we need to continue creating a
CONSTRUCTOR in cxx_eval_bare_aggregate before evaluating the initializer
of an aggregate sub-object to handle self-referential CONSTRUCTORs like
the one above.

Then again, clang is going with rejecting the original testcase with the
following justification: https://bugs.llvm.org/show_bug.cgi?id=45133#c1
Should we follow suit?

> 
> > 
> > > To fix this problem, this patch makes cxx_eval_bare_aggregate and
> > > cxx_eval_store_expression recompute the pointer to the constructor_elt
> > > through
> > > which we're assigning, after the initializer has been evaluated.
> > > 
> > > I am worried that the use of get_or_insert_ctor_field in
> > > cxx_eval_bare_aggregate
> > > might introduce quadratic behavior where there wasn't before.  I wonder if
> > > there's a cheap heuristic we can use in cxx_eval_bare_aggregate to determine
> > > whether "self-modification" took place, and in which case we could avoid
> > > calling
> > > get_or_insert_ctor_field and do the fast thing we that we were doing before
> > > the
> > > patch.
> > 
> > We could remember the (integer) index of the constructor element and verify
> > that our sub-ctors are still at those indices before doing a new search.
> 
> Good idea, this should essentially eliminate the overhead of the second
> set of calls to get_or_insert_ctor_fields in cxx_eval_store_expression
> in the common case where the initializer doesn't mutate the underlying
> CONSTRUCTORs.  I added this optimization to v2 of the patch, through a
> new 'pos_hint' parameter of get_or_insert_ctor_fields.
> 
> As another optimization to get_or_insert_ctor_fields, we can first check
> whether the bit_position of INDEX is greater than the bit_position of
> the last constructor element of CTOR.  In his case we can immediately
> append a new constructor element to CTOR, and avoid iterating over the
> whole TYPE_FIELDS chain and over CTOR.  I think this optimization
> eliminates the potential quadratic behavior in cxx_eval_bare_aggregate
> in the common case.
> 
> With these two optimizations, the added overhead of the recomputations
> added by this patch should be negligible.
> 
> -- >8 --
> 
> gcc/cp/ChangeLog:
> 
> 	PR c++/94066
> 	* constexpr.c (get_or_insert_ctor_field): Split out (while adding
> 	handling for VECTOR_TYPEs, and common-case optimizations) from ...
> 	(cxx_eval_store_expression): ... here.  Record the sequence of indexes
> 	into INDEXES that yields the suboject we're assigning to.  Record the
> 	integer offsets of the constructor indexes we're assigning through into
> 	INDEX_POSITIONS.  After evaluating the initializer of the store
> 	expression, recompute valp using INDEXES and using INDEX_POSITIONS as
> 	hints.
> 	(cxx_eval_bare_aggregate): 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++/94066
> 	* g++.dg/cpp1y/pr94066.C: New test.
> 	* g++.dg/cpp1y/pr94066-2.C: New test.
> 	* g++.dg/cpp1y/pr94066-3.C: New test.
> 	* g++.dg/cpp1y/pr94066-4.C: New test.
> 	* g++.dg/cpp1y/pr94066-5.C: New test.
> ---
>  gcc/cp/constexpr.c                     | 219 ++++++++++++++++---------
>  gcc/testsuite/g++.dg/cpp1y/pr94066-2.C |  21 +++
>  gcc/testsuite/g++.dg/cpp1y/pr94066-3.C |  21 +++
>  gcc/testsuite/g++.dg/cpp1y/pr94066-4.C |  22 +++
>  gcc/testsuite/g++.dg/cpp1y/pr94066-5.C |  18 ++
>  gcc/testsuite/g++.dg/cpp1y/pr94066.C   |  20 +++
>  6 files changed, 242 insertions(+), 79 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr94066-2.C
>  create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr94066-3.C
>  create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr94066-4.C
>  create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr94066-5.C
>  create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr94066.C
> 
> diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
> index 192face9a3a..9ea64467661 100644
> --- a/gcc/cp/constexpr.c
> +++ b/gcc/cp/constexpr.c
> @@ -3144,6 +3144,107 @@ 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.  Emit an appropriate
> +   diagnostic if CTOR constructs a UNION_TYPE and adding INDEX would change the
> +   active member of the union, unless QUIET is true.  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,
> +			  bool quiet, bool *non_constant_p)
> +{
> +  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;
> +
> +      if (TREE_CODE (type) == UNION_TYPE && CONSTRUCTOR_NELTS (ctor)
> +	  && CONSTRUCTOR_ELT (ctor, 0)->index != index)
> +	{
> +	  if (cxx_dialect < cxx2a)
> +	    {
> +	      if (!quiet)
> +		error ("change of the active member of a union from %qD to %qD",
> +			CONSTRUCTOR_ELT (ctor, 0)->index,
> +			index);
> +	      *non_constant_p = true;
> +	    }
> +	  /* Changing active member.  */
> +	  vec_safe_truncate (CONSTRUCTOR_ELTS (ctor), 0);
> +	}
> +      /* 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 bit offset of INDEX is at larger than that of the endmost
> +	 constructor_elt, then just immediately append a new constructor_elt to
> +	 the end of CTOR.  */
> +      else if (TREE_CODE (type) == RECORD_TYPE && CONSTRUCTOR_NELTS (ctor)
> +	       && tree_int_cst_compare (bit_position (index),
> +					bit_position (CONSTRUCTOR_ELTS (ctor)
> +						      ->last().index)) > 0)
> +	{
> +	  idx = CONSTRUCTOR_NELTS (ctor);
> +	  goto insert;
> +	}
> +
> +      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.  */
>  
> @@ -3747,10 +3848,14 @@ 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;
> +	}
>        tree elt = cxx_eval_constant_expression (&new_ctx, value,
>  					       lval,
>  					       non_constant_p, overflow_p);
> @@ -3784,14 +3889,15 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx, tree t,
>  	}
>        else
>  	{
> -	  if (new_ctx.ctor != ctx->ctor)
> -	    {
> -	      /* We appended this element above; update the value.  */
> -	      gcc_assert ((*p)->last().index == index);
> -	      (*p)->last().value = elt;
> -	    }
> -	  else
> -	    CONSTRUCTOR_APPEND_ELT (*p, index, elt);
> +	  /* The underlying CONSTRUCTOR might have been mutated by the
> +	     initializer, so recompute the location of the target
> +	     constructor_elt.  */
> +	  iloc_sentinel ils (cp_expr_location (t));
> +	  constructor_elt *cep
> +	    = get_or_insert_ctor_field (ctx->ctor, index, pos_hint,
> +					ctx->quiet, non_constant_p);
> +	  cep->value = 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;
> @@ -4566,7 +4672,8 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
>    type = TREE_TYPE (object);
>    bool no_zero_init = true;
>  
> -  releasing_vec ctors;
> +  releasing_vec ctors, indexes;
> +  auto_vec<int> index_positions;
>    while (!refs->is_empty ())
>      {
>        if (*valp == NULL_TREE)
> @@ -4607,77 +4714,23 @@ 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)
> -	{
> -	  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 (!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;
> -		}
> -	      /* Changing active member.  */
> -	      vec_safe_truncate (CONSTRUCTOR_ELTS (*valp), 0);
> -	      no_zero_init = true;
> -	    }
> +      vec_safe_push (ctors, *valp);
> +      vec_safe_push (indexes, index);
>  
> -	  for (idx = 0;
> -	       vec_safe_iterate (CONSTRUCTOR_ELTS (*valp), 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;
> -	    }
> +      if (code == UNION_TYPE && CONSTRUCTOR_NELTS (*valp)
> +	  && CONSTRUCTOR_ELT (*valp, 0)->index != index)
> +	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 };
> +      iloc_sentinel ils (cp_expr_location (t));
> +      constructor_elt *cep
> +	= get_or_insert_ctor_field (*valp, index, /*pos_hint=*/-1,
> +				    ctx->quiet, non_constant_p);
> +      index_positions.safe_push (cep - CONSTRUCTOR_ELTS (*valp)->address ());
>  
> -	    vec_safe_insert (CONSTRUCTOR_ELTS (*valp), idx, ce);
> -	    cep = CONSTRUCTOR_ELT (*valp, idx);
> -	  }
> -	found:;
> -	}
>        valp = &cep->value;
>      }
>  
> @@ -4758,9 +4811,17 @@ 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);
> +      /* And the underlying CONSTRUCTORs might have been mutated by the
> +	 initializer, 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_positions[i],
> +					/*quiet=*/true, non_constant_p);
> +	  valp = &cep->value;
> +	}
>      }
>  
>    /* Don't share a CONSTRUCTOR that might be changed later.  */
> diff --git a/gcc/testsuite/g++.dg/cpp1y/pr94066-2.C b/gcc/testsuite/g++.dg/cpp1y/pr94066-2.C
> new file mode 100644
> index 00000000000..ec8210a9d73
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-2.C
> @@ -0,0 +1,21 @@
> +// PR c++/94066
> +// { dg-do compile { target c++14 } }
> +
> +struct A { long x; };
> +
> +union U;
> +constexpr A foo(U *up);
> +
> +union U {
> +  U() = default;
> +  A a = foo(this); int y;
> +};
> +
> +constexpr A foo(U *up) {
> +  up->y = 11; // { dg-error ".U::a. to .U::y." "" { target c++17_down } }
> +  return {42};
> +}
> +
> +extern constexpr U u = {}; // { dg-error ".U::y. to .U::a." "" { target c++17_down } }
> +
> +static_assert(u.a.x == 42, ""); // { dg-error "non-constant" "" { target c++17_down } }
> diff --git a/gcc/testsuite/g++.dg/cpp1y/pr94066-3.C b/gcc/testsuite/g++.dg/cpp1y/pr94066-3.C
> new file mode 100644
> index 00000000000..b68c975d569
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-3.C
> @@ -0,0 +1,21 @@
> +// PR c++/94066
> +// { 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/pr94066-4.C b/gcc/testsuite/g++.dg/cpp1y/pr94066-4.C
> new file mode 100644
> index 00000000000..0362c939faf
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-4.C
> @@ -0,0 +1,22 @@
> +// PR c++/94066
> +// { 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/cpp1y/pr94066-5.C b/gcc/testsuite/g++.dg/cpp1y/pr94066-5.C
> new file mode 100644
> index 00000000000..63069f3ea5f
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-5.C
> @@ -0,0 +1,18 @@
> +// PR c++/94066
> +// { dg-do compile { target c++14 } }
> +
> +union U;
> +constexpr int foo(U *up);
> +
> +union U {
> +  int a = foo(this); int y;
> +};
> +
> +constexpr int foo(U *up) {
> +  up->y = 11;
> +  return 42;
> +}
> +
> +extern constexpr U u = {}; // { dg-error ".U::y. to .U::a." "" { target c++17_down } }
> +
> +static_assert(u.a == 42, ""); // { dg-error "non-constant" "" { target c++17_down } }
> diff --git a/gcc/testsuite/g++.dg/cpp1y/pr94066.C b/gcc/testsuite/g++.dg/cpp1y/pr94066.C
> new file mode 100644
> index 00000000000..11912199406
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/pr94066.C
> @@ -0,0 +1,20 @@
> +// PR c++/94066
> +// { dg-do compile { target c++14 } }
> +
> +struct A { long x; };
> +
> +union U;
> +constexpr A foo(U *up);
> +
> +union U {
> +  A a = foo(this); int y;
> +};
> +
> +constexpr A foo(U *up) {
> +  up->y = 11; // { dg-error ".U::a. to .U::y." "" { target c++17_down } }
> +  return {42};
> +}
> +
> +extern constexpr U u = {}; // { dg-error ".U::y. to .U::a." "" { target c++17_down } }
> +
> +static_assert(u.a.x == 42, ""); // { dg-error "non-constant" "" { target c++17_down } }
> -- 
> 2.26.0.rc1.11.g30e9940356
> 
>

On 3/18/20 11:58 AM, Patrick Palka wrote:
> On Wed, 18 Mar 2020, Patrick Palka wrote:
> 
>> On Tue, 17 Mar 2020, Jason Merrill wrote:
>>
>>> On 3/16/20 1:39 PM, Patrick Palka wrote:
>>>> In this PR, we are performing constexpr evaluation of a CONSTRUCTOR of type
>>>> union U which looks like
>>>>
>>>>     {.a=foo (&<PLACEHOLDER_EXPR union U>)}.
>>>>
>>>> Since the function foo takes a reference to the CONSTRUCTOR we're building,
>>>> it
>>>> could potentially modify the CONSTRUCTOR from under us.  In particular since
>>>> U
>>>> is a union, the evaluation of a's initializer could change the active member
>>>> from a to another member -- something which cxx_eval_bare_aggregate doesn't
>>>> expect to happen.
>>>>
>>>> Upon further investigation, it turns out this issue is not limited to
>>>> constructors of UNION_TYPE and not limited to cxx_eval_bare_aggregate
>>>> either.
>>>> For example, within cxx_eval_store_expression we may be evaluating an
>>>> assignment
>>>> such as (this comes from the test pr94066-2.C):
>>>>
>>>>     ((union U *) this)->a = TARGET_EXPR <D.2163, foo ((union U *) this)>;
>>>
>>> I assume this is actually an INIT_EXPR, or we would have preevaluated and not
>>> had this problem.
>>
>> Yes exactly, I should have specified that the above is an INIT_EXPR and
>> not a MODIFY_EXPR.
>>
>>>
>>>> where evaluation of foo could change the active member of *this, which was
>>>> set
>>>> earlier in cxx_eval_store_expression to 'a'.  And if U is a RECORD_TYPE,
>>>> then
>>>> evaluation of foo could add new fields to *this, thereby making stale the
>>>> 'valp'
>>>> pointer to the target constructor_elt through which we're later assigning.
>>>>
>>>> So in short, it seems that both cxx_eval_bare_aggregate and
>>>> cxx_eval_store_expression do not anticipate that a constructor_elt's
>>>> initializer
>>>> could modify the underlying CONSTRUCTOR as a side-effect.
>>>
>>> Oof.  If this is well-formed, it's because initialization of a doesn't
>>> actually start until the return statement of foo, so we're probably wrong to
>>> create a CONSTRUCTOR to hold the value of 'a' before evaluating foo.  Perhaps
>>> init_subob_ctx shouldn't preemptively create a CONSTRUCTOR, and similarly for
>>> the cxx_eval_store_expression !preeval code.
>>
>> Hmm, I think I see what you mean.  I'll look into this.
> 
> In cpp0x/constexpr-array12.C we have
> 
>      struct A { int ar[3]; };
>      constexpr A a1 = { 0, a1.ar[0] };
> 
> the initializer for a1 is a CONSTRUCTOR with the form
> 
>      {.ar={0, (int) VIEW_CONVERT_EXPR<const struct A>(a1).ar[0]}}
> 
> If we don't preemptively create a CONSTRUCTOR in cxx_eval_bare_aggregate
> to hold the value of 'ar' before evaluating its initializer, then we
> won't be able to resolve the 'a1.ar[0]' later on, and we will reject
> this otherwise valid test case with an "accessing an uninitialized array
> element" diagnostic.  So it seems we need to continue creating a
> CONSTRUCTOR in cxx_eval_bare_aggregate before evaluating the initializer
> of an aggregate sub-object to handle self-referential CONSTRUCTORs like
> the one above.
> 
> Then again, clang is going with rejecting the original testcase with the
> following justification: https://bugs.llvm.org/show_bug.cgi?id=45133#c1
> Should we follow suit?

Yes, let's.  There's no need to bend over backward to allow this kind of 
pathological testcase.  Hubert is proposing that the initialization of 
u.a starts with the call to foo, and the testcase has undefined behavior 
because it ends the lifetime of u.a in the middle of its initialization.

Jason

On Wed, 18 Mar 2020, Jason Merrill wrote:

> On 3/18/20 11:58 AM, Patrick Palka wrote:
> > On Wed, 18 Mar 2020, Patrick Palka wrote:
> > 
> > > On Tue, 17 Mar 2020, Jason Merrill wrote:
> > > 
> > > > On 3/16/20 1:39 PM, Patrick Palka wrote:
> > > > > In this PR, we are performing constexpr evaluation of a CONSTRUCTOR of
> > > > > type
> > > > > union U which looks like
> > > > > 
> > > > >     {.a=foo (&<PLACEHOLDER_EXPR union U>)}.
> > > > > 
> > > > > Since the function foo takes a reference to the CONSTRUCTOR we're
> > > > > building,
> > > > > it
> > > > > could potentially modify the CONSTRUCTOR from under us.  In particular
> > > > > since
> > > > > U
> > > > > is a union, the evaluation of a's initializer could change the active
> > > > > member
> > > > > from a to another member -- something which cxx_eval_bare_aggregate
> > > > > doesn't
> > > > > expect to happen.
> > > > > 
> > > > > Upon further investigation, it turns out this issue is not limited to
> > > > > constructors of UNION_TYPE and not limited to cxx_eval_bare_aggregate
> > > > > either.
> > > > > For example, within cxx_eval_store_expression we may be evaluating an
> > > > > assignment
> > > > > such as (this comes from the test pr94066-2.C):
> > > > > 
> > > > >     ((union U *) this)->a = TARGET_EXPR <D.2163, foo ((union U *)
> > > > > this)>;
> > > > 
> > > > I assume this is actually an INIT_EXPR, or we would have preevaluated
> > > > and not
> > > > had this problem.
> > > 
> > > Yes exactly, I should have specified that the above is an INIT_EXPR and
> > > not a MODIFY_EXPR.
> > > 
> > > > 
> > > > > where evaluation of foo could change the active member of *this, which
> > > > > was
> > > > > set
> > > > > earlier in cxx_eval_store_expression to 'a'.  And if U is a
> > > > > RECORD_TYPE,
> > > > > then
> > > > > evaluation of foo could add new fields to *this, thereby making stale
> > > > > the
> > > > > 'valp'
> > > > > pointer to the target constructor_elt through which we're later
> > > > > assigning.
> > > > > 
> > > > > So in short, it seems that both cxx_eval_bare_aggregate and
> > > > > cxx_eval_store_expression do not anticipate that a constructor_elt's
> > > > > initializer
> > > > > could modify the underlying CONSTRUCTOR as a side-effect.
> > > > 
> > > > Oof.  If this is well-formed, it's because initialization of a doesn't
> > > > actually start until the return statement of foo, so we're probably
> > > > wrong to
> > > > create a CONSTRUCTOR to hold the value of 'a' before evaluating foo.
> > > > Perhaps
> > > > init_subob_ctx shouldn't preemptively create a CONSTRUCTOR, and
> > > > similarly for
> > > > the cxx_eval_store_expression !preeval code.
> > > 
> > > Hmm, I think I see what you mean.  I'll look into this.
> > 
> > In cpp0x/constexpr-array12.C we have
> > 
> >      struct A { int ar[3]; };
> >      constexpr A a1 = { 0, a1.ar[0] };
> > 
> > the initializer for a1 is a CONSTRUCTOR with the form
> > 
> >      {.ar={0, (int) VIEW_CONVERT_EXPR<const struct A>(a1).ar[0]}}
> > 
> > If we don't preemptively create a CONSTRUCTOR in cxx_eval_bare_aggregate
> > to hold the value of 'ar' before evaluating its initializer, then we
> > won't be able to resolve the 'a1.ar[0]' later on, and we will reject
> > this otherwise valid test case with an "accessing an uninitialized array
> > element" diagnostic.  So it seems we need to continue creating a
> > CONSTRUCTOR in cxx_eval_bare_aggregate before evaluating the initializer
> > of an aggregate sub-object to handle self-referential CONSTRUCTORs like
> > the one above.
> > 
> > Then again, clang is going with rejecting the original testcase with the
> > following justification: https://bugs.llvm.org/show_bug.cgi?id=45133#c1
> > Should we follow suit?
> 
> Yes, let's.  There's no need to bend over backward to allow this kind of
> pathological testcase.  Hubert is proposing that the initialization of u.a
> starts with the call to foo, and the testcase has undefined behavior because
> it ends the lifetime of u.a in the middle of its initialization.

Got it.  I filed PR c++/94219 to track the testcase that uses a struct
in place of the union, on which we also ICE but this ICE is not a
regression from what I can tell.  For GCC 10 I'll work on a minimal
patch to reject the original testcase from PR c++/94066.