c++: Reject changing active member of union during initialization [PR94066]

This patch adds a check to detect changing the active union member during
initialization of the union.  It uses the CONSTRUCTOR_NO_CLEARING flag as a
proxy for whether the non-empty CONSTRUCTOR of UNION_TYPE we're assigning to in
cxx_eval_store_expression is in the process of being initialized, which seems to
work well.

In order for this check to work reliably, we also have to adjust
cxx_eval_bare_aggregate to set the active union member before processing the
initializer.

Does this look OK to commit after testing?

gcc/cp/ChangeLog:

	PR c++/94066
	* constexpr.c (cxx_eval_bare_aggregate): When constructing a union,
	always set the active union member before evaluating the initializer.
	Relax assertion that verifies the index of the constructor element we're
	initializing hasn't been changed.
	(cxx_eval_store_expression): Diagnose changing the active union member
	while the union is in the process of being initialized.

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.
---
 gcc/cp/constexpr.c                     | 25 ++++++++++++++++++++++++-
 gcc/testsuite/g++.dg/cpp1y/pr94066-2.C | 19 +++++++++++++++++++
 gcc/testsuite/g++.dg/cpp1y/pr94066-3.C | 18 ++++++++++++++++++
 gcc/testsuite/g++.dg/cpp1y/pr94066.C   | 18 ++++++++++++++++++
 4 files changed, 79 insertions(+), 1 deletion(-)
 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.C

On Thu, 19 Mar 2020, Patrick Palka wrote:

> This patch adds a check to detect changing the active union member during
> initialization of the union.  It uses the CONSTRUCTOR_NO_CLEARING flag as a
> proxy for whether the non-empty CONSTRUCTOR of UNION_TYPE we're assigning to in
> cxx_eval_store_expression is in the process of being initialized, which seems to
> work well.

If we can't rely on CONSTRUCTOR_NO_CLEARING to be set iff a CONSTRUCTOR
is in the process of being initialized, then here's an alternative patch
for consideration, that detects this UB in an indirect way and after the
fact.

-- >8 --

This patch adds checks to detect whether an active member of a union has been
changed during initialization of the union.

It add checks in three places, one in cxx_eval_bare_aggregate and two in
cxx_eval_store_expression (for preeval and !preeval).  These checks are indirect
and only detect this UB after the whole initializer has been evaluated and not
at e.g. the exact assignment in the initializer through which the active union
member gets changed, so the source locations attached to the error messages are
suboptimal.

gcc/cp/ChangeLog:

	PR c++/94066
	* constexpr.c (cxx_eval_bare_aggregate): Check whether the active
	member of the union has been changed during evaluation of its
	initializer, and diagnose this if so.
	(cxx_eval_store_expression): Likewise for both preeval and !preeval
	cases.

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.
---
 gcc/cp/constexpr.c                     | 45 +++++++++++++++++++++++++-
 gcc/testsuite/g++.dg/cpp1y/pr94066-2.C | 19 +++++++++++
 gcc/testsuite/g++.dg/cpp1y/pr94066-3.C | 21 ++++++++++++
 gcc/testsuite/g++.dg/cpp1y/pr94066.C   | 18 +++++++++++
 4 files changed, 102 insertions(+), 1 deletion(-)
 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.C

diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
index 192face9a3a..bcb6b758980 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -3784,7 +3784,19 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx, tree t,
 	}
       else
 	{
-	  if (new_ctx.ctor != ctx->ctor)
+	  if (TREE_CODE (type) == UNION_TYPE
+	      && vec_safe_length (*p)
+	      && (**p)[0].index != index)
+	    {
+	      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",
+			  index,
+			  (**p)[0].index);
+	      *non_constant_p = true;
+	    }
+	  else if (new_ctx.ctor != ctx->ctor)
 	    {
 	      /* We appended this element above; update the value.  */
 	      gcc_assert ((*p)->last().index == index);
@@ -4647,6 +4659,16 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
 			      index);
 		  *non_constant_p = true;
 		}
+	      else if (preeval && TREE_CODE (t) == INIT_EXPR)
+		{
+		  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",
+			      index,
+			      CONSTRUCTOR_ELT (*valp, 0)->index);
+		  *non_constant_p = true;
+		}
 	      /* Changing active member.  */
 	      vec_safe_truncate (CONSTRUCTOR_ELTS (*valp), 0);
 	      no_zero_init = true;
@@ -4761,6 +4783,27 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
       if (ctors->is_empty())
 	/* The hash table might have moved since the get earlier.  */
 	valp = ctx->global->values.get (object);
+      else if (TREE_CODE (t) == INIT_EXPR)
+	{
+	  tree ctor = ctors->last();
+	  if (TREE_CODE (TREE_TYPE (ctor)) == UNION_TYPE)
+	    {
+	      tree index = TREE_OPERAND (target, 1);
+	      gcc_assert (CONSTRUCTOR_NELTS (ctor));
+	      gcc_assert (TREE_CODE (index) == FIELD_DECL
+			  && DECL_CONTEXT (index) == TREE_TYPE (ctor));
+	      if (CONSTRUCTOR_ELT (ctor, 0)->index != index)
+		{
+		  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",
+			      index,
+			      CONSTRUCTOR_ELT (ctor, 0)->index);
+		  *non_constant_p = true;
+		}
+	    }
+	}
     }
 
   /* 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..438e2e436d8
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-2.C
@@ -0,0 +1,19 @@
+// 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::a' to 'U::y' during initialization" }
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..d74e222b421
--- /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; };
+
+union U;
+constexpr int foo(U *up);
+
+union U {
+  U() = default;
+  // { dg-error "'U::a' to 'U::y' during initialization" "" { target c++2a } .-1 }
+  int a = foo(this); int y;
+};
+
+constexpr int foo(U *up) {
+  up->y = 11;
+  return {42};
+}
+
+extern constexpr U u = {};
+// { dg-error "'U::a' to 'U::y' during initialization" "" { target c++17_down } .-1 }
diff --git a/gcc/testsuite/g++.dg/cpp1y/pr94066.C b/gcc/testsuite/g++.dg/cpp1y/pr94066.C
new file mode 100644
index 00000000000..3d981b5540e
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/pr94066.C
@@ -0,0 +1,18 @@
+// 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::a' to 'U::y' during initialization" }

On Thu, Mar 19, 2020 at 01:06:35PM -0400, Patrick Palka via Gcc-patches wrote:
> On Thu, 19 Mar 2020, Patrick Palka wrote:
> 
> > This patch adds a check to detect changing the active union member during
> > initialization of the union.  It uses the CONSTRUCTOR_NO_CLEARING flag as a
> > proxy for whether the non-empty CONSTRUCTOR of UNION_TYPE we're assigning to in
> > cxx_eval_store_expression is in the process of being initialized, which seems to
> > work well.
> 
> If we can't rely on CONSTRUCTOR_NO_CLEARING to be set iff a CONSTRUCTOR
> is in the process of being initialized, then here's an alternative patch
> for consideration, that detects this UB in an indirect way and after the
> fact.

Yeah, I'm not sure if that would work well, especially in C++20 where we
sometimes don't clear it:

  /* The result of a constexpr function must be completely initialized.

     However, in C++20, a constexpr constructor doesn't necessarily have
     to initialize all the fields, so we don't clear CONSTRUCTOR_NO_CLEARING
     in order to detect reading an unitialized object in constexpr instead
     of value-initializing it.  (reduced_constant_expression_p is expected to
     take care of clearing the flag.)  */
  if (TREE_CODE (result) == CONSTRUCTOR
      && (cxx_dialect < cxx2a
          || !DECL_CONSTRUCTOR_P (fun)))
    clear_no_implicit_zero (result);

and rely on reduced_constant_expression_p to clear it.

Marek

On Thu, 19 Mar 2020, Marek Polacek wrote:

> On Thu, Mar 19, 2020 at 01:06:35PM -0400, Patrick Palka via Gcc-patches wrote:
> > On Thu, 19 Mar 2020, Patrick Palka wrote:
> > 
> > > This patch adds a check to detect changing the active union member during
> > > initialization of the union.  It uses the CONSTRUCTOR_NO_CLEARING flag as a
> > > proxy for whether the non-empty CONSTRUCTOR of UNION_TYPE we're assigning to in
> > > cxx_eval_store_expression is in the process of being initialized, which seems to
> > > work well.
> > 
> > If we can't rely on CONSTRUCTOR_NO_CLEARING to be set iff a CONSTRUCTOR
> > is in the process of being initialized, then here's an alternative patch
> > for consideration, that detects this UB in an indirect way and after the
> > fact.
> 
> Yeah, I'm not sure if that would work well, especially in C++20 where we
> sometimes don't clear it:
> 
>   /* The result of a constexpr function must be completely initialized.
> 
>      However, in C++20, a constexpr constructor doesn't necessarily have
>      to initialize all the fields, so we don't clear CONSTRUCTOR_NO_CLEARING
>      in order to detect reading an unitialized object in constexpr instead
>      of value-initializing it.  (reduced_constant_expression_p is expected to
>      take care of clearing the flag.)  */
>   if (TREE_CODE (result) == CONSTRUCTOR
>       && (cxx_dialect < cxx2a
>           || !DECL_CONSTRUCTOR_P (fun)))
>     clear_no_implicit_zero (result);
> 
> and rely on reduced_constant_expression_p to clear it.

I see, thanks.  Here's a reproducer for the issue you pointed out, which
is a valid testcase but gets rejected with the proposed patch:

    union U
    {
      int x;
      char y;
    };

    constexpr bool
    baz ()
    {
      U u;
      u.x = 3;
      u.y = 7;
      return (u.y == 7);
    }

    static_assert (baz ());

CONSTRUCTOR_NO_CLEARING is set for 'u' and is not cleared after its
constructor returns, and so the check yields a false positive for the
assignment to u.y.  That's unfortunate...

On 3/19/20 12:35 PM, Patrick Palka wrote:
> This patch adds a check to detect changing the active union member during
> initialization of the union.  It uses the CONSTRUCTOR_NO_CLEARING flag as a
> proxy for whether the non-empty CONSTRUCTOR of UNION_TYPE we're assigning to in
> cxx_eval_store_expression is in the process of being initialized, which seems to
> work well.

> In order for this check to work reliably, we also have to adjust
> cxx_eval_bare_aggregate to set the active union member before processing the
> initializer.
> 
> Does this look OK to commit after testing?

That makes sense.  OK.

> gcc/cp/ChangeLog:
> 
> 	PR c++/94066
> 	* constexpr.c (cxx_eval_bare_aggregate): When constructing a union,
> 	always set the active union member before evaluating the initializer.
> 	Relax assertion that verifies the index of the constructor element we're
> 	initializing hasn't been changed.
> 	(cxx_eval_store_expression): Diagnose changing the active union member
> 	while the union is in the process of being initialized.
> 
> 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.
> ---
>   gcc/cp/constexpr.c                     | 25 ++++++++++++++++++++++++-
>   gcc/testsuite/g++.dg/cpp1y/pr94066-2.C | 19 +++++++++++++++++++
>   gcc/testsuite/g++.dg/cpp1y/pr94066-3.C | 18 ++++++++++++++++++
>   gcc/testsuite/g++.dg/cpp1y/pr94066.C   | 18 ++++++++++++++++++
>   4 files changed, 79 insertions(+), 1 deletion(-)
>   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.C
> 
> diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
> index 192face9a3a..97fe5572f71 100644
> --- a/gcc/cp/constexpr.c
> +++ b/gcc/cp/constexpr.c
> @@ -3751,6 +3751,11 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx, tree t,
>   	/* 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);
> +      else if (TREE_CODE (type) == UNION_TYPE)
> +	/* If we're constructing a union, set the active union member now so
> +	   that we can later detect if the initializer attempts to activate
> +	   another member.  */
> +	CONSTRUCTOR_APPEND_ELT (*p, index, NULL_TREE);
>         tree elt = cxx_eval_constant_expression (&new_ctx, value,
>   					       lval,
>   					       non_constant_p, overflow_p);
> @@ -3784,7 +3789,12 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx, tree t,
>   	}
>         else
>   	{
> -	  if (new_ctx.ctor != ctx->ctor)
> +	  if (TREE_CODE (type) == UNION_TYPE
> +	      && (*p)->last().index != index)
> +	    /* The initializer may have erroneously changed the active union
> +	       member we were initializing.  */
> +	    gcc_assert (*non_constant_p);
> +	  else if (new_ctx.ctor != ctx->ctor)
>   	    {
>   	      /* We appended this element above; update the value.  */
>   	      gcc_assert ((*p)->last().index == index);
> @@ -4647,6 +4657,19 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
>   			      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;
> 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..1c00b650961
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-2.C
> @@ -0,0 +1,19 @@
> +// 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'" }
> +  return {42};
> +}
> +
> +extern constexpr U u = {};
> 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..6bf1ec81885
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-3.C
> @@ -0,0 +1,18 @@
> +// PR c++/94066
> +// { dg-do compile { target c++14 } }
> +
> +struct A { long x; };
> +
> +union U;
> +constexpr int foo(U *up);
> +
> +union U {
> +  int a = foo(this); int y;
> +};
> +
> +constexpr int foo(U *up) {
> +  up->y = 11; // { dg-error "'U::a' to 'U::y'" }
> +  return {42};
> +}
> +
> +extern constexpr U u = {};
> diff --git a/gcc/testsuite/g++.dg/cpp1y/pr94066.C b/gcc/testsuite/g++.dg/cpp1y/pr94066.C
> new file mode 100644
> index 00000000000..6725c8c737f
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/pr94066.C
> @@ -0,0 +1,18 @@
> +// 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'" }
> +  return {42};
> +}
> +
> +extern constexpr U u = {};
>

On Thu, 19 Mar 2020, Jason Merrill wrote:

> On 3/19/20 12:35 PM, Patrick Palka wrote:
> > This patch adds a check to detect changing the active union member during
> > initialization of the union.  It uses the CONSTRUCTOR_NO_CLEARING flag as a
> > proxy for whether the non-empty CONSTRUCTOR of UNION_TYPE we're assigning to
> > in
> > cxx_eval_store_expression is in the process of being initialized, which
> > seems to
> > work well.
> 
> > In order for this check to work reliably, we also have to adjust
> > cxx_eval_bare_aggregate to set the active union member before processing the
> > initializer.
> > 
> > Does this look OK to commit after testing?
> 
> That makes sense.  OK.

Thanks.  The issue Marek points out unfortunately makes this approach of
using CONSTRUCTOR_NO_CLEARING unreliable and regresses other testcases,
and I'm afraid I don't know how to salvage this approach...

> 
> > gcc/cp/ChangeLog:
> > 
> > 	PR c++/94066
> > 	* constexpr.c (cxx_eval_bare_aggregate): When constructing a union,
> > 	always set the active union member before evaluating the initializer.
> > 	Relax assertion that verifies the index of the constructor element
> > we're
> > 	initializing hasn't been changed.
> > 	(cxx_eval_store_expression): Diagnose changing the active union member
> > 	while the union is in the process of being initialized.
> > 
> > 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.
> > ---
> >   gcc/cp/constexpr.c                     | 25 ++++++++++++++++++++++++-
> >   gcc/testsuite/g++.dg/cpp1y/pr94066-2.C | 19 +++++++++++++++++++
> >   gcc/testsuite/g++.dg/cpp1y/pr94066-3.C | 18 ++++++++++++++++++
> >   gcc/testsuite/g++.dg/cpp1y/pr94066.C   | 18 ++++++++++++++++++
> >   4 files changed, 79 insertions(+), 1 deletion(-)
> >   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.C
> > 
> > diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
> > index 192face9a3a..97fe5572f71 100644
> > --- a/gcc/cp/constexpr.c
> > +++ b/gcc/cp/constexpr.c
> > @@ -3751,6 +3751,11 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx,
> > tree t,
> >   	/* 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);
> > +      else if (TREE_CODE (type) == UNION_TYPE)
> > +	/* If we're constructing a union, set the active union member now so
> > +	   that we can later detect if the initializer attempts to activate
> > +	   another member.  */
> > +	CONSTRUCTOR_APPEND_ELT (*p, index, NULL_TREE);
> >         tree elt = cxx_eval_constant_expression (&new_ctx, value,
> >   					       lval,
> >   					       non_constant_p, overflow_p);
> > @@ -3784,7 +3789,12 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx,
> > tree t,
> >   	}
> >         else
> >   	{
> > -	  if (new_ctx.ctor != ctx->ctor)
> > +	  if (TREE_CODE (type) == UNION_TYPE
> > +	      && (*p)->last().index != index)
> > +	    /* The initializer may have erroneously changed the active union
> > +	       member we were initializing.  */
> > +	    gcc_assert (*non_constant_p);
> > +	  else if (new_ctx.ctor != ctx->ctor)
> >   	    {
> >   	      /* We appended this element above; update the value.  */
> >   	      gcc_assert ((*p)->last().index == index);
> > @@ -4647,6 +4657,19 @@ cxx_eval_store_expression (const constexpr_ctx *ctx,
> > tree t,
> >   			      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;
> > 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..1c00b650961
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-2.C
> > @@ -0,0 +1,19 @@
> > +// 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'" }
> > +  return {42};
> > +}
> > +
> > +extern constexpr U u = {};
> > 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..6bf1ec81885
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-3.C
> > @@ -0,0 +1,18 @@
> > +// PR c++/94066
> > +// { dg-do compile { target c++14 } }
> > +
> > +struct A { long x; };
> > +
> > +union U;
> > +constexpr int foo(U *up);
> > +
> > +union U {
> > +  int a = foo(this); int y;
> > +};
> > +
> > +constexpr int foo(U *up) {
> > +  up->y = 11; // { dg-error "'U::a' to 'U::y'" }
> > +  return {42};
> > +}
> > +
> > +extern constexpr U u = {};
> > diff --git a/gcc/testsuite/g++.dg/cpp1y/pr94066.C
> > b/gcc/testsuite/g++.dg/cpp1y/pr94066.C
> > new file mode 100644
> > index 00000000000..6725c8c737f
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/cpp1y/pr94066.C
> > @@ -0,0 +1,18 @@
> > +// 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'" }
> > +  return {42};
> > +}
> > +
> > +extern constexpr U u = {};
> > 
> 
>

On 3/19/20 2:06 PM, Patrick Palka via Gcc-patches wrote:
> On Thu, 19 Mar 2020, Marek Polacek wrote:
> 
>> On Thu, Mar 19, 2020 at 01:06:35PM -0400, Patrick Palka via Gcc-patches wrote:
>>> On Thu, 19 Mar 2020, Patrick Palka wrote:
>>>
>>>> This patch adds a check to detect changing the active union member during
>>>> initialization of the union.  It uses the CONSTRUCTOR_NO_CLEARING flag as a
>>>> proxy for whether the non-empty CONSTRUCTOR of UNION_TYPE we're assigning to in
>>>> cxx_eval_store_expression is in the process of being initialized, which seems to
>>>> work well.
>>>
>>> If we can't rely on CONSTRUCTOR_NO_CLEARING to be set iff a CONSTRUCTOR
>>> is in the process of being initialized, then here's an alternative patch
>>> for consideration, that detects this UB in an indirect way and after the
>>> fact.
>>
>> Yeah, I'm not sure if that would work well, especially in C++20 where we
>> sometimes don't clear it:
>>
>>    /* The result of a constexpr function must be completely initialized.
>>
>>       However, in C++20, a constexpr constructor doesn't necessarily have
>>       to initialize all the fields, so we don't clear CONSTRUCTOR_NO_CLEARING
>>       in order to detect reading an unitialized object in constexpr instead
>>       of value-initializing it.  (reduced_constant_expression_p is expected to
>>       take care of clearing the flag.)  */
>>    if (TREE_CODE (result) == CONSTRUCTOR
>>        && (cxx_dialect < cxx2a
>>            || !DECL_CONSTRUCTOR_P (fun)))
>>      clear_no_implicit_zero (result);
>>
>> and rely on reduced_constant_expression_p to clear it.
> 
> I see, thanks.  Here's a reproducer for the issue you pointed out, which
> is a valid testcase but gets rejected with the proposed patch:
> 
>      union U
>      {
>        int x;
>        char y;
>      };
> 
>      constexpr bool
>      baz ()
>      {
>        U u;
>        u.x = 3;
>        u.y = 7;
>        return (u.y == 7);
>      }
> 
>      static_assert (baz ());
> 
> CONSTRUCTOR_NO_CLEARING is set for 'u' and is not cleared after its
> constructor returns, and so the check yields a false positive for the
> assignment to u.y.  That's unfortunate...

We should probably clear the flag when we assign to u.x because once we 
give a value to one union member, the union has a value.

Jason

On Thu, 19 Mar 2020, Jason Merrill wrote:

> On 3/19/20 2:06 PM, Patrick Palka via Gcc-patches wrote:
> > On Thu, 19 Mar 2020, Marek Polacek wrote:
> > 
> > > On Thu, Mar 19, 2020 at 01:06:35PM -0400, Patrick Palka via Gcc-patches
> > > wrote:
> > > > On Thu, 19 Mar 2020, Patrick Palka wrote:
> > > > 
> > > > > This patch adds a check to detect changing the active union member
> > > > > during
> > > > > initialization of the union.  It uses the CONSTRUCTOR_NO_CLEARING flag
> > > > > as a
> > > > > proxy for whether the non-empty CONSTRUCTOR of UNION_TYPE we're
> > > > > assigning to in
> > > > > cxx_eval_store_expression is in the process of being initialized,
> > > > > which seems to
> > > > > work well.
> > > > 
> > > > If we can't rely on CONSTRUCTOR_NO_CLEARING to be set iff a CONSTRUCTOR
> > > > is in the process of being initialized, then here's an alternative patch
> > > > for consideration, that detects this UB in an indirect way and after the
> > > > fact.
> > > 
> > > Yeah, I'm not sure if that would work well, especially in C++20 where we
> > > sometimes don't clear it:
> > > 
> > >    /* The result of a constexpr function must be completely initialized.
> > > 
> > >       However, in C++20, a constexpr constructor doesn't necessarily have
> > >       to initialize all the fields, so we don't clear
> > > CONSTRUCTOR_NO_CLEARING
> > >       in order to detect reading an unitialized object in constexpr
> > > instead
> > >       of value-initializing it.  (reduced_constant_expression_p is
> > > expected to
> > >       take care of clearing the flag.)  */
> > >    if (TREE_CODE (result) == CONSTRUCTOR
> > >        && (cxx_dialect < cxx2a
> > >            || !DECL_CONSTRUCTOR_P (fun)))
> > >      clear_no_implicit_zero (result);
> > > 
> > > and rely on reduced_constant_expression_p to clear it.
> > 
> > I see, thanks.  Here's a reproducer for the issue you pointed out, which
> > is a valid testcase but gets rejected with the proposed patch:
> > 
> >      union U
> >      {
> >        int x;
> >        char y;
> >      };
> > 
> >      constexpr bool
> >      baz ()
> >      {
> >        U u;
> >        u.x = 3;
> >        u.y = 7;
> >        return (u.y == 7);
> >      }
> > 
> >      static_assert (baz ());
> > 
> > CONSTRUCTOR_NO_CLEARING is set for 'u' and is not cleared after its
> > constructor returns, and so the check yields a false positive for the
> > assignment to u.y.  That's unfortunate...
> 
> We should probably clear the flag when we assign to u.x because once we give a
> value to one union member, the union has a value.

That works well.  Further testing revealed a couple of more issues
caused by the new hunk

@@ -3751,6 +3751,11 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx, tree t,
 	/* 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);
+      else if (TREE_CODE (type) == UNION_TYPE)
+	/* If we're constructing a union, set the active union member now so
+	   that we can later detect if the initializer attempts to activate
+	   another member.  */
+	CONSTRUCTOR_APPEND_ELT (*p, index, NULL_TREE);
       tree elt = cxx_eval_constant_expression (&new_ctx, value,
 					       lval,
 					       non_constant_p, overflow_p);

where other routines aren't prepared to handle a NULL constructor union
element.   They are cxx_eval_component_reference (fixed by emitting a
different error message given a NULL constructor elt) and
cxx_eval_bare_aggregate itself (fixed by not re-reducing a CONSTRUCTOR
returned by lookup_placeholder).

As a drive-by this patch adds a check in reduced_constant_expression_p
to correctly return false for an empty CONSTRUCTOR of UNION_TYPE.  This
allows us to correctly diagnose the uninitialized use in
constexpr-union4.C where we weren't before.

-- >8 --

gcc/cp/ChangeLog:

	PR c++/94066
	* constexpr.c (reduced_constant_expression_p) [CONSTRUCTOR]: Properly
	handle unions without an initializer.
	(cxx_eval_component_reference): Emit a different diagnostic when the
	constructor element corresponding to a union member is NULL.
	(cxx_eval_bare_aggregate): When constructing a union, always set the
	active union member before evaluating the initializer.  Relax assertion
	that verifies the index of the constructor element we're initializing
	hasn't been changed.
	(cxx_eval_store_expression): Diagnose changing the active union member
	while the union is in the process of being initialized.  After setting
	an active union member, clear CONSTRUCTOR_NO_CLEARING on the underlying
	CONSTRUCTOR.
	(cxx_eval_constant_expression) [PLACEHOLDER_EXPR]: Don't re-reduce a
	CONSTRUCTOR returned by lookup_placeholder.

gcc/testsuite/ChangeLog:

	PR c++/94066
	* g++.dg/cpp1y/constexpr-union2.C: New test.
	* g++.dg/cpp1y/constexpr-union3.C: New test.
	* g++.dg/cpp1y/constexpr-union4.C: New test.
	* g++.dg/cpp1y/constexpr-union5.C: New test.
	* 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/cpp2a/constexpr-union1.C: New test.
---
 gcc/cp/constexpr.c                            | 69 ++++++++++++++++---
 gcc/testsuite/g++.dg/cpp1y/constexpr-union2.C |  9 +++
 gcc/testsuite/g++.dg/cpp1y/constexpr-union3.C |  9 +++
 gcc/testsuite/g++.dg/cpp1y/constexpr-union4.C |  9 +++
 gcc/testsuite/g++.dg/cpp1y/constexpr-union5.C | 15 ++++
 gcc/testsuite/g++.dg/cpp1y/pr94066-2.C        | 19 +++++
 gcc/testsuite/g++.dg/cpp1y/pr94066-3.C        | 16 +++++
 gcc/testsuite/g++.dg/cpp1y/pr94066.C          | 18 +++++
 gcc/testsuite/g++.dg/cpp2a/constexpr-union1.C | 18 +++++
 9 files changed, 173 insertions(+), 9 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-union2.C
 create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-union3.C
 create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-union4.C
 create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-union5.C
 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.C
 create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-union1.C

diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
index 192face9a3a..6284ab25b7d 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -2591,10 +2591,17 @@ reduced_constant_expression_p (tree t)
 	    return false;
 	  else if (cxx_dialect >= cxx2a
 		   /* An ARRAY_TYPE doesn't have any TYPE_FIELDS.  */
-		   && (TREE_CODE (TREE_TYPE (t)) == ARRAY_TYPE
-		       /* A union only initializes one member.  */
-		       || TREE_CODE (TREE_TYPE (t)) == UNION_TYPE))
+		   && TREE_CODE (TREE_TYPE (t)) == ARRAY_TYPE)
 	    field = NULL_TREE;
+	  else if (cxx_dialect >= cxx2a
+		   && TREE_CODE (TREE_TYPE (t)) == UNION_TYPE)
+	    {
+	      if (CONSTRUCTOR_NELTS (t) == 0)
+		/* An initialized union has a constructor element.  */
+		return false;
+	      /* And it only initializes one member.  */
+	      field = NULL_TREE;
+	    }
 	  else
 	    field = next_initializable_field (TYPE_FIELDS (TREE_TYPE (t)));
 	}
@@ -3446,8 +3453,14 @@ cxx_eval_component_reference (const constexpr_ctx *ctx, tree t,
     {
       /* DR 1188 says we don't have to deal with this.  */
       if (!ctx->quiet)
-	error ("accessing %qD member instead of initialized %qD member in "
-	       "constant expression", part, CONSTRUCTOR_ELT (whole, 0)->index);
+	{
+	  constructor_elt *cep = CONSTRUCTOR_ELT (whole, 0);
+	  if (cep->value == NULL_TREE)
+	    error ("accessing uninitialized member %qD", part);
+	  else
+	    error ("accessing %qD member instead of initialized %qD member in "
+		   "constant expression", part, cep->index);
+	}
       *non_constant_p = true;
       return t;
     }
@@ -3751,6 +3764,11 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx, tree t,
 	/* 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);
+      else if (TREE_CODE (type) == UNION_TYPE)
+	/* If we're constructing a union, set the active union member now so
+	   that we can later detect if the initializer attempts to activate
+	   another member.  */
+	CONSTRUCTOR_APPEND_ELT (*p, index, NULL_TREE);
       tree elt = cxx_eval_constant_expression (&new_ctx, value,
 					       lval,
 					       non_constant_p, overflow_p);
@@ -3784,7 +3802,13 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx, tree t,
 	}
       else
 	{
-	  if (new_ctx.ctor != ctx->ctor)
+	  if (TREE_CODE (type) == UNION_TYPE
+	      && (*p)->last().index != index)
+	    /* The initializer may have 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)
 	    {
 	      /* We appended this element above; update the value.  */
 	      gcc_assert ((*p)->last().index == index);
@@ -4567,6 +4591,7 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
   bool no_zero_init = true;
 
   releasing_vec ctors;
+  bool changed_active_union_member_p = false;
   while (!refs->is_empty ())
     {
       if (*valp == NULL_TREE)
@@ -4647,6 +4672,19 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
 			      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;
@@ -4675,6 +4713,10 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
 
 	    vec_safe_insert (CONSTRUCTOR_ELTS (*valp), idx, ce);
 	    cep = CONSTRUCTOR_ELT (*valp, idx);
+
+	    if (code == UNION_TYPE)
+	      /* Record that we've changed an active union member.  */
+	      changed_active_union_member_p = true;
 	  }
 	found:;
 	}
@@ -4805,13 +4847,17 @@ 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)
+  if (!c || s || changed_active_union_member_p)
     FOR_EACH_VEC_ELT (*ctors, i, elt)
       {
 	if (!c)
 	  TREE_CONSTANT (elt) = false;
 	if (s)
 	  TREE_SIDE_EFFECTS (elt) = true;
+	/* Clear CONSTRUCTOR_NO_CLEARING since we've activated a member of
+	   this union.  */
+	if (TREE_CODE (TREE_TYPE (elt)) == UNION_TYPE)
+	  CONSTRUCTOR_NO_CLEARING (elt) = false;
       }
 
   if (*non_constant_p)
@@ -6133,8 +6179,13 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t,
     case PLACEHOLDER_EXPR:
       /* Use of the value or address of the current object.  */
       if (tree ctor = lookup_placeholder (ctx, lval, TREE_TYPE (t)))
-	return cxx_eval_constant_expression (ctx, ctor, lval,
-					     non_constant_p, overflow_p);
+	{
+	  if (TREE_CODE (ctor) == CONSTRUCTOR)
+	    return ctor;
+	  else
+	    return cxx_eval_constant_expression (ctx, ctor, lval,
+						 non_constant_p, overflow_p);
+	}
       /* A placeholder without a referent.  We can get here when
 	 checking whether NSDMIs are noexcept, or in massage_init_elt;
 	 just say it's non-constant for now.  */
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-union2.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-union2.C
new file mode 100644
index 00000000000..7a6a818742b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-union2.C
@@ -0,0 +1,9 @@
+// { dg-do compile { target c++14 } }
+
+union U
+{
+  char *x = &y;
+  char y;
+};
+
+constexpr U u = {};
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-union3.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-union3.C
new file mode 100644
index 00000000000..5cf62e46cb5
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-union3.C
@@ -0,0 +1,9 @@
+// { dg-do compile { target c++14 } }
+
+union U
+{
+  int x = (x = x + 1);
+  char y;
+};
+
+constexpr U u = {}; // { dg-error "accessing uninitialized member" }
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-union4.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-union4.C
new file mode 100644
index 00000000000..3e44a1378f3
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-union4.C
@@ -0,0 +1,9 @@
+// { dg-do compile { target c++14 } }
+
+union U
+{
+  int x = y;
+  char y;
+};
+
+constexpr U u = {}; // { dg-error "accessing uninitialized member" }
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-union5.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-union5.C
new file mode 100644
index 00000000000..55fe9fa2f0b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-union5.C
@@ -0,0 +1,15 @@
+// { 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->a++;
+  return {42};
+}
+
+extern constexpr U u = {}; // { dg-error "accessing uninitialized member" }
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..1c00b650961
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-2.C
@@ -0,0 +1,19 @@
+// 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'" }
+  return {42};
+}
+
+extern constexpr U u = {};
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..175018acf86
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-3.C
@@ -0,0 +1,16 @@
+// 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; // { dg-error "'U::a' to 'U::y'" }
+  return {42};
+}
+
+extern constexpr U u = {};
diff --git a/gcc/testsuite/g++.dg/cpp1y/pr94066.C b/gcc/testsuite/g++.dg/cpp1y/pr94066.C
new file mode 100644
index 00000000000..6725c8c737f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/pr94066.C
@@ -0,0 +1,18 @@
+// 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'" }
+  return {42};
+}
+
+extern constexpr U u = {};
diff --git a/gcc/testsuite/g++.dg/cpp2a/constexpr-union1.C b/gcc/testsuite/g++.dg/cpp2a/constexpr-union1.C
new file mode 100644
index 00000000000..c38167ad798
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/constexpr-union1.C
@@ -0,0 +1,18 @@
+// { dg-do compile { target c++2a } }
+
+union U
+{
+  int x;
+  char y;
+};
+
+constexpr bool
+baz ()
+{
+  U u;
+  u.x = 3;
+  u.y = 7;
+  return (u.y == 7);
+}
+
+static_assert (baz ());

On 3/20/20 9:49 AM, Patrick Palka wrote:
> On Thu, 19 Mar 2020, Jason Merrill wrote:
> 
>> On 3/19/20 2:06 PM, Patrick Palka via Gcc-patches wrote:
>>> On Thu, 19 Mar 2020, Marek Polacek wrote:
>>>
>>>> On Thu, Mar 19, 2020 at 01:06:35PM -0400, Patrick Palka via Gcc-patches
>>>> wrote:
>>>>> On Thu, 19 Mar 2020, Patrick Palka wrote:
>>>>>
>>>>>> This patch adds a check to detect changing the active union member
>>>>>> during
>>>>>> initialization of the union.  It uses the CONSTRUCTOR_NO_CLEARING flag
>>>>>> as a
>>>>>> proxy for whether the non-empty CONSTRUCTOR of UNION_TYPE we're
>>>>>> assigning to in
>>>>>> cxx_eval_store_expression is in the process of being initialized,
>>>>>> which seems to
>>>>>> work well.
>>>>>
>>>>> If we can't rely on CONSTRUCTOR_NO_CLEARING to be set iff a CONSTRUCTOR
>>>>> is in the process of being initialized, then here's an alternative patch
>>>>> for consideration, that detects this UB in an indirect way and after the
>>>>> fact.
>>>>
>>>> Yeah, I'm not sure if that would work well, especially in C++20 where we
>>>> sometimes don't clear it:
>>>>
>>>>     /* The result of a constexpr function must be completely initialized.
>>>>
>>>>        However, in C++20, a constexpr constructor doesn't necessarily have
>>>>        to initialize all the fields, so we don't clear
>>>> CONSTRUCTOR_NO_CLEARING
>>>>        in order to detect reading an unitialized object in constexpr
>>>> instead
>>>>        of value-initializing it.  (reduced_constant_expression_p is
>>>> expected to
>>>>        take care of clearing the flag.)  */
>>>>     if (TREE_CODE (result) == CONSTRUCTOR
>>>>         && (cxx_dialect < cxx2a
>>>>             || !DECL_CONSTRUCTOR_P (fun)))
>>>>       clear_no_implicit_zero (result);
>>>>
>>>> and rely on reduced_constant_expression_p to clear it.
>>>
>>> I see, thanks.  Here's a reproducer for the issue you pointed out, which
>>> is a valid testcase but gets rejected with the proposed patch:
>>>
>>>       union U
>>>       {
>>>         int x;
>>>         char y;
>>>       };
>>>
>>>       constexpr bool
>>>       baz ()
>>>       {
>>>         U u;
>>>         u.x = 3;
>>>         u.y = 7;
>>>         return (u.y == 7);
>>>       }
>>>
>>>       static_assert (baz ());
>>>
>>> CONSTRUCTOR_NO_CLEARING is set for 'u' and is not cleared after its
>>> constructor returns, and so the check yields a false positive for the
>>> assignment to u.y.  That's unfortunate...
>>
>> We should probably clear the flag when we assign to u.x because once we give a
>> value to one union member, the union has a value.
> 
> That works well.  Further testing revealed a couple of more issues
> caused by the new hunk
> 
> @@ -3751,6 +3751,11 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx, tree t,
>   	/* 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);
> +      else if (TREE_CODE (type) == UNION_TYPE)
> +	/* If we're constructing a union, set the active union member now so
> +	   that we can later detect if the initializer attempts to activate
> +	   another member.  */
> +	CONSTRUCTOR_APPEND_ELT (*p, index, NULL_TREE);
>         tree elt = cxx_eval_constant_expression (&new_ctx, value,
>   					       lval,
>   					       non_constant_p, overflow_p);
> 
> where other routines aren't prepared to handle a NULL constructor union
> element.   They are cxx_eval_component_reference (fixed by emitting a
> different error message given a NULL constructor elt) and
> cxx_eval_bare_aggregate itself (fixed by not re-reducing a CONSTRUCTOR
> returned by lookup_placeholder).
> 
> As a drive-by this patch adds a check in reduced_constant_expression_p
> to correctly return false for an empty CONSTRUCTOR of UNION_TYPE.  This
> allows us to correctly diagnose the uninitialized use in
> constexpr-union4.C where we weren't before.
> 
> -- >8 --
> 
> gcc/cp/ChangeLog:
> 
> 	PR c++/94066
> 	* constexpr.c (reduced_constant_expression_p) [CONSTRUCTOR]: Properly
> 	handle unions without an initializer.
> 	(cxx_eval_component_reference): Emit a different diagnostic when the
> 	constructor element corresponding to a union member is NULL.
> 	(cxx_eval_bare_aggregate): When constructing a union, always set the
> 	active union member before evaluating the initializer.  Relax assertion
> 	that verifies the index of the constructor element we're initializing
> 	hasn't been changed.
> 	(cxx_eval_store_expression): Diagnose changing the active union member
> 	while the union is in the process of being initialized.  After setting
> 	an active union member, clear CONSTRUCTOR_NO_CLEARING on the underlying
> 	CONSTRUCTOR.
> 	(cxx_eval_constant_expression) [PLACEHOLDER_EXPR]: Don't re-reduce a
> 	CONSTRUCTOR returned by lookup_placeholder.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR c++/94066
> 	* g++.dg/cpp1y/constexpr-union2.C: New test.
> 	* g++.dg/cpp1y/constexpr-union3.C: New test.
> 	* g++.dg/cpp1y/constexpr-union4.C: New test.
> 	* g++.dg/cpp1y/constexpr-union5.C: New test.
> 	* 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/cpp2a/constexpr-union1.C: New test.
> ---
>   gcc/cp/constexpr.c                            | 69 ++++++++++++++++---
>   gcc/testsuite/g++.dg/cpp1y/constexpr-union2.C |  9 +++
>   gcc/testsuite/g++.dg/cpp1y/constexpr-union3.C |  9 +++
>   gcc/testsuite/g++.dg/cpp1y/constexpr-union4.C |  9 +++
>   gcc/testsuite/g++.dg/cpp1y/constexpr-union5.C | 15 ++++
>   gcc/testsuite/g++.dg/cpp1y/pr94066-2.C        | 19 +++++
>   gcc/testsuite/g++.dg/cpp1y/pr94066-3.C        | 16 +++++
>   gcc/testsuite/g++.dg/cpp1y/pr94066.C          | 18 +++++
>   gcc/testsuite/g++.dg/cpp2a/constexpr-union1.C | 18 +++++
>   9 files changed, 173 insertions(+), 9 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-union2.C
>   create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-union3.C
>   create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-union4.C
>   create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-union5.C
>   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.C
>   create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-union1.C
> 
> diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
> index 192face9a3a..6284ab25b7d 100644
> --- a/gcc/cp/constexpr.c
> +++ b/gcc/cp/constexpr.c
> @@ -2591,10 +2591,17 @@ reduced_constant_expression_p (tree t)
>   	    return false;
>   	  else if (cxx_dialect >= cxx2a
>   		   /* An ARRAY_TYPE doesn't have any TYPE_FIELDS.  */
> -		   && (TREE_CODE (TREE_TYPE (t)) == ARRAY_TYPE
> -		       /* A union only initializes one member.  */
> -		       || TREE_CODE (TREE_TYPE (t)) == UNION_TYPE))
> +		   && TREE_CODE (TREE_TYPE (t)) == ARRAY_TYPE)
>   	    field = NULL_TREE;
> +	  else if (cxx_dialect >= cxx2a
> +		   && TREE_CODE (TREE_TYPE (t)) == UNION_TYPE)
> +	    {
> +	      if (CONSTRUCTOR_NELTS (t) == 0)
> +		/* An initialized union has a constructor element.  */
> +		return false;
> +	      /* And it only initializes one member.  */
> +	      field = NULL_TREE;
> +	    }
>   	  else
>   	    field = next_initializable_field (TYPE_FIELDS (TREE_TYPE (t)));
>   	}
> @@ -3446,8 +3453,14 @@ cxx_eval_component_reference (const constexpr_ctx *ctx, tree t,
>       {
>         /* DR 1188 says we don't have to deal with this.  */
>         if (!ctx->quiet)
> -	error ("accessing %qD member instead of initialized %qD member in "
> -	       "constant expression", part, CONSTRUCTOR_ELT (whole, 0)->index);
> +	{
> +	  constructor_elt *cep = CONSTRUCTOR_ELT (whole, 0);
> +	  if (cep->value == NULL_TREE)
> +	    error ("accessing uninitialized member %qD", part);
> +	  else
> +	    error ("accessing %qD member instead of initialized %qD member in "
> +		   "constant expression", part, cep->index);
> +	}
>         *non_constant_p = true;
>         return t;
>       }
> @@ -3751,6 +3764,11 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx, tree t,
>   	/* 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);
> +      else if (TREE_CODE (type) == UNION_TYPE)
> +	/* If we're constructing a union, set the active union member now so
> +	   that we can later detect if the initializer attempts to activate
> +	   another member.  */
> +	CONSTRUCTOR_APPEND_ELT (*p, index, NULL_TREE);
>         tree elt = cxx_eval_constant_expression (&new_ctx, value,
>   					       lval,
>   					       non_constant_p, overflow_p);
> @@ -3784,7 +3802,13 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx, tree t,
>   	}
>         else
>   	{
> -	  if (new_ctx.ctor != ctx->ctor)
> +	  if (TREE_CODE (type) == UNION_TYPE
> +	      && (*p)->last().index != index)
> +	    /* The initializer may have 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)
>   	    {
>   	      /* We appended this element above; update the value.  */
>   	      gcc_assert ((*p)->last().index == index);
> @@ -4567,6 +4591,7 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
>     bool no_zero_init = true;
>   
>     releasing_vec ctors;
> +  bool changed_active_union_member_p = false;
>     while (!refs->is_empty ())
>       {
>         if (*valp == NULL_TREE)
> @@ -4647,6 +4672,19 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
>   			      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.  */

Hmm, could we also detect this situation by noticing that the current 
active constructor element has NULL_TREE value?

> +		  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;
> @@ -4675,6 +4713,10 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
>   
>   	    vec_safe_insert (CONSTRUCTOR_ELTS (*valp), idx, ce);
>   	    cep = CONSTRUCTOR_ELT (*valp, idx);
> +
> +	    if (code == UNION_TYPE)
> +	      /* Record that we've changed an active union member.  */
> +	      changed_active_union_member_p = true;
>   	  }
>   	found:;
>   	}
> @@ -4805,13 +4847,17 @@ 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)
> +  if (!c || s || changed_active_union_member_p)
>       FOR_EACH_VEC_ELT (*ctors, i, elt)
>         {
>   	if (!c)
>   	  TREE_CONSTANT (elt) = false;
>   	if (s)
>   	  TREE_SIDE_EFFECTS (elt) = true;
> +	/* Clear CONSTRUCTOR_NO_CLEARING since we've activated a member of
> +	   this union.  */
> +	if (TREE_CODE (TREE_TYPE (elt)) == UNION_TYPE)
> +	  CONSTRUCTOR_NO_CLEARING (elt) = false;
>         }
>   
>     if (*non_constant_p)
> @@ -6133,8 +6179,13 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t,
>       case PLACEHOLDER_EXPR:
>         /* Use of the value or address of the current object.  */
>         if (tree ctor = lookup_placeholder (ctx, lval, TREE_TYPE (t)))
> -	return cxx_eval_constant_expression (ctx, ctor, lval,
> -					     non_constant_p, overflow_p);
> +	{
> +	  if (TREE_CODE (ctor) == CONSTRUCTOR)
> +	    return ctor;
> +	  else
> +	    return cxx_eval_constant_expression (ctx, ctor, lval,
> +						 non_constant_p, overflow_p);
> +	}
>         /* A placeholder without a referent.  We can get here when
>   	 checking whether NSDMIs are noexcept, or in massage_init_elt;
>   	 just say it's non-constant for now.  */
> diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-union2.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-union2.C
> new file mode 100644
> index 00000000000..7a6a818742b
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-union2.C
> @@ -0,0 +1,9 @@
> +// { dg-do compile { target c++14 } }
> +
> +union U
> +{
> +  char *x = &y;
> +  char y;
> +};
> +
> +constexpr U u = {};
> diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-union3.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-union3.C
> new file mode 100644
> index 00000000000..5cf62e46cb5
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-union3.C
> @@ -0,0 +1,9 @@
> +// { dg-do compile { target c++14 } }
> +
> +union U
> +{
> +  int x = (x = x + 1);
> +  char y;
> +};
> +
> +constexpr U u = {}; // { dg-error "accessing uninitialized member" }
> diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-union4.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-union4.C
> new file mode 100644
> index 00000000000..3e44a1378f3
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-union4.C
> @@ -0,0 +1,9 @@
> +// { dg-do compile { target c++14 } }
> +
> +union U
> +{
> +  int x = y;
> +  char y;
> +};
> +
> +constexpr U u = {}; // { dg-error "accessing uninitialized member" }
> diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-union5.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-union5.C
> new file mode 100644
> index 00000000000..55fe9fa2f0b
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-union5.C
> @@ -0,0 +1,15 @@
> +// { 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->a++;
> +  return {42};
> +}
> +
> +extern constexpr U u = {}; // { dg-error "accessing uninitialized member" }
> 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..1c00b650961
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-2.C
> @@ -0,0 +1,19 @@
> +// 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'" }
> +  return {42};
> +}
> +
> +extern constexpr U u = {};
> 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..175018acf86
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-3.C
> @@ -0,0 +1,16 @@
> +// 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; // { dg-error "'U::a' to 'U::y'" }
> +  return {42};
> +}
> +
> +extern constexpr U u = {};
> diff --git a/gcc/testsuite/g++.dg/cpp1y/pr94066.C b/gcc/testsuite/g++.dg/cpp1y/pr94066.C
> new file mode 100644
> index 00000000000..6725c8c737f
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/pr94066.C
> @@ -0,0 +1,18 @@
> +// 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'" }
> +  return {42};
> +}
> +
> +extern constexpr U u = {};
> diff --git a/gcc/testsuite/g++.dg/cpp2a/constexpr-union1.C b/gcc/testsuite/g++.dg/cpp2a/constexpr-union1.C
> new file mode 100644
> index 00000000000..c38167ad798
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp2a/constexpr-union1.C
> @@ -0,0 +1,18 @@
> +// { dg-do compile { target c++2a } }
> +
> +union U
> +{
> +  int x;
> +  char y;
> +};
> +
> +constexpr bool
> +baz ()
> +{
> +  U u;
> +  u.x = 3;
> +  u.y = 7;
> +  return (u.y == 7);
> +}
> +
> +static_assert (baz ());
>

On Fri, 20 Mar 2020, Jason Merrill wrote:

> On 3/20/20 9:49 AM, Patrick Palka wrote:
> > On Thu, 19 Mar 2020, Jason Merrill wrote:
> > 
> > > On 3/19/20 2:06 PM, Patrick Palka via Gcc-patches wrote:
> > > > On Thu, 19 Mar 2020, Marek Polacek wrote:
> > > > 
> > > > > On Thu, Mar 19, 2020 at 01:06:35PM -0400, Patrick Palka via
> > > > > Gcc-patches
> > > > > wrote:
> > > > > > On Thu, 19 Mar 2020, Patrick Palka wrote:
> > > > > > 
> > > > > > > This patch adds a check to detect changing the active union member
> > > > > > > during
> > > > > > > initialization of the union.  It uses the CONSTRUCTOR_NO_CLEARING
> > > > > > > flag
> > > > > > > as a
> > > > > > > proxy for whether the non-empty CONSTRUCTOR of UNION_TYPE we're
> > > > > > > assigning to in
> > > > > > > cxx_eval_store_expression is in the process of being initialized,
> > > > > > > which seems to
> > > > > > > work well.
> > > > > > 
> > > > > > If we can't rely on CONSTRUCTOR_NO_CLEARING to be set iff a
> > > > > > CONSTRUCTOR
> > > > > > is in the process of being initialized, then here's an alternative
> > > > > > patch
> > > > > > for consideration, that detects this UB in an indirect way and after
> > > > > > the
> > > > > > fact.
> > > > > 
> > > > > Yeah, I'm not sure if that would work well, especially in C++20 where
> > > > > we
> > > > > sometimes don't clear it:
> > > > > 
> > > > >     /* The result of a constexpr function must be completely
> > > > > initialized.
> > > > > 
> > > > >        However, in C++20, a constexpr constructor doesn't necessarily
> > > > > have
> > > > >        to initialize all the fields, so we don't clear
> > > > > CONSTRUCTOR_NO_CLEARING
> > > > >        in order to detect reading an unitialized object in constexpr
> > > > > instead
> > > > >        of value-initializing it.  (reduced_constant_expression_p is
> > > > > expected to
> > > > >        take care of clearing the flag.)  */
> > > > >     if (TREE_CODE (result) == CONSTRUCTOR
> > > > >         && (cxx_dialect < cxx2a
> > > > >             || !DECL_CONSTRUCTOR_P (fun)))
> > > > >       clear_no_implicit_zero (result);
> > > > > 
> > > > > and rely on reduced_constant_expression_p to clear it.
> > > > 
> > > > I see, thanks.  Here's a reproducer for the issue you pointed out, which
> > > > is a valid testcase but gets rejected with the proposed patch:
> > > > 
> > > >       union U
> > > >       {
> > > >         int x;
> > > >         char y;
> > > >       };
> > > > 
> > > >       constexpr bool
> > > >       baz ()
> > > >       {
> > > >         U u;
> > > >         u.x = 3;
> > > >         u.y = 7;
> > > >         return (u.y == 7);
> > > >       }
> > > > 
> > > >       static_assert (baz ());
> > > > 
> > > > CONSTRUCTOR_NO_CLEARING is set for 'u' and is not cleared after its
> > > > constructor returns, and so the check yields a false positive for the
> > > > assignment to u.y.  That's unfortunate...
> > > 
> > > We should probably clear the flag when we assign to u.x because once we
> > > give a
> > > value to one union member, the union has a value.
> > 
> > That works well.  Further testing revealed a couple of more issues
> > caused by the new hunk
> > 
> > @@ -3751,6 +3751,11 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx,
> > tree t,
> >   	/* 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);
> > +      else if (TREE_CODE (type) == UNION_TYPE)
> > +	/* If we're constructing a union, set the active union member now so
> > +	   that we can later detect if the initializer attempts to activate
> > +	   another member.  */
> > +	CONSTRUCTOR_APPEND_ELT (*p, index, NULL_TREE);
> >         tree elt = cxx_eval_constant_expression (&new_ctx, value,
> >   					       lval,
> >   					       non_constant_p, overflow_p);
> > 
> > where other routines aren't prepared to handle a NULL constructor union
> > element.   They are cxx_eval_component_reference (fixed by emitting a
> > different error message given a NULL constructor elt) and
> > cxx_eval_bare_aggregate itself (fixed by not re-reducing a CONSTRUCTOR
> > returned by lookup_placeholder).
> > 
> > As a drive-by this patch adds a check in reduced_constant_expression_p
> > to correctly return false for an empty CONSTRUCTOR of UNION_TYPE.  This
> > allows us to correctly diagnose the uninitialized use in
> > constexpr-union4.C where we weren't before.
> > 
> > -- >8 --
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	PR c++/94066
> > 	* constexpr.c (reduced_constant_expression_p) [CONSTRUCTOR]: Properly
> > 	handle unions without an initializer.
> > 	(cxx_eval_component_reference): Emit a different diagnostic when the
> > 	constructor element corresponding to a union member is NULL.
> > 	(cxx_eval_bare_aggregate): When constructing a union, always set the
> > 	active union member before evaluating the initializer.  Relax
> > assertion
> > 	that verifies the index of the constructor element we're initializing
> > 	hasn't been changed.
> > 	(cxx_eval_store_expression): Diagnose changing the active union member
> > 	while the union is in the process of being initialized.  After setting
> > 	an active union member, clear CONSTRUCTOR_NO_CLEARING on the
> > underlying
> > 	CONSTRUCTOR.
> > 	(cxx_eval_constant_expression) [PLACEHOLDER_EXPR]: Don't re-reduce a
> > 	CONSTRUCTOR returned by lookup_placeholder.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	PR c++/94066
> > 	* g++.dg/cpp1y/constexpr-union2.C: New test.
> > 	* g++.dg/cpp1y/constexpr-union3.C: New test.
> > 	* g++.dg/cpp1y/constexpr-union4.C: New test.
> > 	* g++.dg/cpp1y/constexpr-union5.C: New test.
> > 	* 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/cpp2a/constexpr-union1.C: New test.
> > ---
> >   gcc/cp/constexpr.c                            | 69 ++++++++++++++++---
> >   gcc/testsuite/g++.dg/cpp1y/constexpr-union2.C |  9 +++
> >   gcc/testsuite/g++.dg/cpp1y/constexpr-union3.C |  9 +++
> >   gcc/testsuite/g++.dg/cpp1y/constexpr-union4.C |  9 +++
> >   gcc/testsuite/g++.dg/cpp1y/constexpr-union5.C | 15 ++++
> >   gcc/testsuite/g++.dg/cpp1y/pr94066-2.C        | 19 +++++
> >   gcc/testsuite/g++.dg/cpp1y/pr94066-3.C        | 16 +++++
> >   gcc/testsuite/g++.dg/cpp1y/pr94066.C          | 18 +++++
> >   gcc/testsuite/g++.dg/cpp2a/constexpr-union1.C | 18 +++++
> >   9 files changed, 173 insertions(+), 9 deletions(-)
> >   create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-union2.C
> >   create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-union3.C
> >   create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-union4.C
> >   create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-union5.C
> >   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.C
> >   create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-union1.C
> > 
> > diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
> > index 192face9a3a..6284ab25b7d 100644
> > --- a/gcc/cp/constexpr.c
> > +++ b/gcc/cp/constexpr.c
> > @@ -2591,10 +2591,17 @@ reduced_constant_expression_p (tree t)
> >   	    return false;
> >   	  else if (cxx_dialect >= cxx2a
> >   		   /* An ARRAY_TYPE doesn't have any TYPE_FIELDS.  */
> > -		   && (TREE_CODE (TREE_TYPE (t)) == ARRAY_TYPE
> > -		       /* A union only initializes one member.  */
> > -		       || TREE_CODE (TREE_TYPE (t)) == UNION_TYPE))
> > +		   && TREE_CODE (TREE_TYPE (t)) == ARRAY_TYPE)
> >   	    field = NULL_TREE;
> > +	  else if (cxx_dialect >= cxx2a
> > +		   && TREE_CODE (TREE_TYPE (t)) == UNION_TYPE)
> > +	    {
> > +	      if (CONSTRUCTOR_NELTS (t) == 0)
> > +		/* An initialized union has a constructor element.  */
> > +		return false;
> > +	      /* And it only initializes one member.  */
> > +	      field = NULL_TREE;
> > +	    }
> >   	  else
> >   	    field = next_initializable_field (TYPE_FIELDS (TREE_TYPE (t)));
> >   	}
> > @@ -3446,8 +3453,14 @@ cxx_eval_component_reference (const constexpr_ctx
> > *ctx, tree t,
> >       {
> >         /* DR 1188 says we don't have to deal with this.  */
> >         if (!ctx->quiet)
> > -	error ("accessing %qD member instead of initialized %qD member in "
> > -	       "constant expression", part, CONSTRUCTOR_ELT (whole,
> > 0)->index);
> > +	{
> > +	  constructor_elt *cep = CONSTRUCTOR_ELT (whole, 0);
> > +	  if (cep->value == NULL_TREE)
> > +	    error ("accessing uninitialized member %qD", part);
> > +	  else
> > +	    error ("accessing %qD member instead of initialized %qD member in
> > "
> > +		   "constant expression", part, cep->index);
> > +	}
> >         *non_constant_p = true;
> >         return t;
> >       }
> > @@ -3751,6 +3764,11 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx,
> > tree t,
> >   	/* 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);
> > +      else if (TREE_CODE (type) == UNION_TYPE)
> > +	/* If we're constructing a union, set the active union member now so
> > +	   that we can later detect if the initializer attempts to activate
> > +	   another member.  */
> > +	CONSTRUCTOR_APPEND_ELT (*p, index, NULL_TREE);
> >         tree elt = cxx_eval_constant_expression (&new_ctx, value,
> >   					       lval,
> >   					       non_constant_p, overflow_p);
> > @@ -3784,7 +3802,13 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx,
> > tree t,
> >   	}
> >         else
> >   	{
> > -	  if (new_ctx.ctor != ctx->ctor)
> > +	  if (TREE_CODE (type) == UNION_TYPE
> > +	      && (*p)->last().index != index)
> > +	    /* The initializer may have 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)
> >   	    {
> >   	      /* We appended this element above; update the value.  */
> >   	      gcc_assert ((*p)->last().index == index);
> > @@ -4567,6 +4591,7 @@ cxx_eval_store_expression (const constexpr_ctx *ctx,
> > tree t,
> >     bool no_zero_init = true;
> >       releasing_vec ctors;
> > +  bool changed_active_union_member_p = false;
> >     while (!refs->is_empty ())
> >       {
> >         if (*valp == NULL_TREE)
> > @@ -4647,6 +4672,19 @@ cxx_eval_store_expression (const constexpr_ctx *ctx,
> > tree t,
> >   			      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.  */
> 
> Hmm, could we also detect this situation by noticing that the current active
> constructor element has NULL_TREE value?

I don't think a looking for a NULL_TREE value here would be sufficient as-is,
only because at that point the active constructor element of an under-construction
union could also be an empty CONSTRUCTOR if the union member we're initializing
is an aggregate and we're coming from cxx_eval_bare_aggregate, which does:

      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);
      else if (TREE_CODE (type) == UNION_TYPE)
        /* If we're constructing a union, set the active union member now so
           that we can later detect if the initializer attempts to activate
           another member.  */
        CONSTRUCTOR_APPEND_ELT (*p, index, NULL_TREE);
      tree elt = cxx_eval_constant_expression (&new_ctx, value,
                                               lval,
                                               non_constant_p, overflow_p);

> 
> > +		  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;
> > @@ -4675,6 +4713,10 @@ cxx_eval_store_expression (const constexpr_ctx *ctx,
> > tree t,
> >     	    vec_safe_insert (CONSTRUCTOR_ELTS (*valp), idx, ce);
> >   	    cep = CONSTRUCTOR_ELT (*valp, idx);
> > +
> > +	    if (code == UNION_TYPE)
> > +	      /* Record that we've changed an active union member.  */
> > +	      changed_active_union_member_p = true;
> >   	  }
> >   	found:;
> >   	}
> > @@ -4805,13 +4847,17 @@ 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)
> > +  if (!c || s || changed_active_union_member_p)
> >       FOR_EACH_VEC_ELT (*ctors, i, elt)
> >         {
> >   	if (!c)
> >   	  TREE_CONSTANT (elt) = false;
> >   	if (s)
> >   	  TREE_SIDE_EFFECTS (elt) = true;
> > +	/* Clear CONSTRUCTOR_NO_CLEARING since we've activated a member of
> > +	   this union.  */
> > +	if (TREE_CODE (TREE_TYPE (elt)) == UNION_TYPE)
> > +	  CONSTRUCTOR_NO_CLEARING (elt) = false;
> >         }
> >       if (*non_constant_p)
> > @@ -6133,8 +6179,13 @@ cxx_eval_constant_expression (const constexpr_ctx
> > *ctx, tree t,
> >       case PLACEHOLDER_EXPR:
> >         /* Use of the value or address of the current object.  */
> >         if (tree ctor = lookup_placeholder (ctx, lval, TREE_TYPE (t)))
> > -	return cxx_eval_constant_expression (ctx, ctor, lval,
> > -					     non_constant_p, overflow_p);
> > +	{
> > +	  if (TREE_CODE (ctor) == CONSTRUCTOR)
> > +	    return ctor;
> > +	  else
> > +	    return cxx_eval_constant_expression (ctx, ctor, lval,
> > +						 non_constant_p, overflow_p);
> > +	}
> >         /* A placeholder without a referent.  We can get here when
> >   	 checking whether NSDMIs are noexcept, or in massage_init_elt;
> >   	 just say it's non-constant for now.  */
> > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-union2.C
> > b/gcc/testsuite/g++.dg/cpp1y/constexpr-union2.C
> > new file mode 100644
> > index 00000000000..7a6a818742b
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-union2.C
> > @@ -0,0 +1,9 @@
> > +// { dg-do compile { target c++14 } }
> > +
> > +union U
> > +{
> > +  char *x = &y;
> > +  char y;
> > +};
> > +
> > +constexpr U u = {};
> > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-union3.C
> > b/gcc/testsuite/g++.dg/cpp1y/constexpr-union3.C
> > new file mode 100644
> > index 00000000000..5cf62e46cb5
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-union3.C
> > @@ -0,0 +1,9 @@
> > +// { dg-do compile { target c++14 } }
> > +
> > +union U
> > +{
> > +  int x = (x = x + 1);
> > +  char y;
> > +};
> > +
> > +constexpr U u = {}; // { dg-error "accessing uninitialized member" }
> > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-union4.C
> > b/gcc/testsuite/g++.dg/cpp1y/constexpr-union4.C
> > new file mode 100644
> > index 00000000000..3e44a1378f3
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-union4.C
> > @@ -0,0 +1,9 @@
> > +// { dg-do compile { target c++14 } }
> > +
> > +union U
> > +{
> > +  int x = y;
> > +  char y;
> > +};
> > +
> > +constexpr U u = {}; // { dg-error "accessing uninitialized member" }
> > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-union5.C
> > b/gcc/testsuite/g++.dg/cpp1y/constexpr-union5.C
> > new file mode 100644
> > index 00000000000..55fe9fa2f0b
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-union5.C
> > @@ -0,0 +1,15 @@
> > +// { 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->a++;
> > +  return {42};
> > +}
> > +
> > +extern constexpr U u = {}; // { dg-error "accessing uninitialized member" }
> > 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..1c00b650961
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-2.C
> > @@ -0,0 +1,19 @@
> > +// 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'" }
> > +  return {42};
> > +}
> > +
> > +extern constexpr U u = {};
> > 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..175018acf86
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-3.C
> > @@ -0,0 +1,16 @@
> > +// 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; // { dg-error "'U::a' to 'U::y'" }
> > +  return {42};
> > +}
> > +
> > +extern constexpr U u = {};
> > diff --git a/gcc/testsuite/g++.dg/cpp1y/pr94066.C
> > b/gcc/testsuite/g++.dg/cpp1y/pr94066.C
> > new file mode 100644
> > index 00000000000..6725c8c737f
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/cpp1y/pr94066.C
> > @@ -0,0 +1,18 @@
> > +// 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'" }
> > +  return {42};
> > +}
> > +
> > +extern constexpr U u = {};
> > diff --git a/gcc/testsuite/g++.dg/cpp2a/constexpr-union1.C
> > b/gcc/testsuite/g++.dg/cpp2a/constexpr-union1.C
> > new file mode 100644
> > index 00000000000..c38167ad798
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/cpp2a/constexpr-union1.C
> > @@ -0,0 +1,18 @@
> > +// { dg-do compile { target c++2a } }
> > +
> > +union U
> > +{
> > +  int x;
> > +  char y;
> > +};
> > +
> > +constexpr bool
> > +baz ()
> > +{
> > +  U u;
> > +  u.x = 3;
> > +  u.y = 7;
> > +  return (u.y == 7);
> > +}
> > +
> > +static_assert (baz ());
> > 
> 
>

On Fri, 20 Mar 2020, Patrick Palka wrote:

> On Fri, 20 Mar 2020, Jason Merrill wrote:
> 
> > On 3/20/20 9:49 AM, Patrick Palka wrote:
> > > On Thu, 19 Mar 2020, Jason Merrill wrote:
> > > 
> > > > On 3/19/20 2:06 PM, Patrick Palka via Gcc-patches wrote:
> > > > > On Thu, 19 Mar 2020, Marek Polacek wrote:
> > > > > 
> > > > > > On Thu, Mar 19, 2020 at 01:06:35PM -0400, Patrick Palka via
> > > > > > Gcc-patches
> > > > > > wrote:
> > > > > > > On Thu, 19 Mar 2020, Patrick Palka wrote:
> > > > > > > 
> > > > > > > > This patch adds a check to detect changing the active union member
> > > > > > > > during
> > > > > > > > initialization of the union.  It uses the CONSTRUCTOR_NO_CLEARING
> > > > > > > > flag
> > > > > > > > as a
> > > > > > > > proxy for whether the non-empty CONSTRUCTOR of UNION_TYPE we're
> > > > > > > > assigning to in
> > > > > > > > cxx_eval_store_expression is in the process of being initialized,
> > > > > > > > which seems to
> > > > > > > > work well.
> > > > > > > 
> > > > > > > If we can't rely on CONSTRUCTOR_NO_CLEARING to be set iff a
> > > > > > > CONSTRUCTOR
> > > > > > > is in the process of being initialized, then here's an alternative
> > > > > > > patch
> > > > > > > for consideration, that detects this UB in an indirect way and after
> > > > > > > the
> > > > > > > fact.
> > > > > > 
> > > > > > Yeah, I'm not sure if that would work well, especially in C++20 where
> > > > > > we
> > > > > > sometimes don't clear it:
> > > > > > 
> > > > > >     /* The result of a constexpr function must be completely
> > > > > > initialized.
> > > > > > 
> > > > > >        However, in C++20, a constexpr constructor doesn't necessarily
> > > > > > have
> > > > > >        to initialize all the fields, so we don't clear
> > > > > > CONSTRUCTOR_NO_CLEARING
> > > > > >        in order to detect reading an unitialized object in constexpr
> > > > > > instead
> > > > > >        of value-initializing it.  (reduced_constant_expression_p is
> > > > > > expected to
> > > > > >        take care of clearing the flag.)  */
> > > > > >     if (TREE_CODE (result) == CONSTRUCTOR
> > > > > >         && (cxx_dialect < cxx2a
> > > > > >             || !DECL_CONSTRUCTOR_P (fun)))
> > > > > >       clear_no_implicit_zero (result);
> > > > > > 
> > > > > > and rely on reduced_constant_expression_p to clear it.
> > > > > 
> > > > > I see, thanks.  Here's a reproducer for the issue you pointed out, which
> > > > > is a valid testcase but gets rejected with the proposed patch:
> > > > > 
> > > > >       union U
> > > > >       {
> > > > >         int x;
> > > > >         char y;
> > > > >       };
> > > > > 
> > > > >       constexpr bool
> > > > >       baz ()
> > > > >       {
> > > > >         U u;
> > > > >         u.x = 3;
> > > > >         u.y = 7;
> > > > >         return (u.y == 7);
> > > > >       }
> > > > > 
> > > > >       static_assert (baz ());
> > > > > 
> > > > > CONSTRUCTOR_NO_CLEARING is set for 'u' and is not cleared after its
> > > > > constructor returns, and so the check yields a false positive for the
> > > > > assignment to u.y.  That's unfortunate...
> > > > 
> > > > We should probably clear the flag when we assign to u.x because once we
> > > > give a
> > > > value to one union member, the union has a value.
> > > 
> > > That works well.  Further testing revealed a couple of more issues
> > > caused by the new hunk
> > > 
> > > @@ -3751,6 +3751,11 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx,
> > > tree t,
> > >   	/* 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);
> > > +      else if (TREE_CODE (type) == UNION_TYPE)
> > > +	/* If we're constructing a union, set the active union member now so
> > > +	   that we can later detect if the initializer attempts to activate
> > > +	   another member.  */
> > > +	CONSTRUCTOR_APPEND_ELT (*p, index, NULL_TREE);
> > >         tree elt = cxx_eval_constant_expression (&new_ctx, value,
> > >   					       lval,
> > >   					       non_constant_p, overflow_p);
> > > 
> > > where other routines aren't prepared to handle a NULL constructor union
> > > element.   They are cxx_eval_component_reference (fixed by emitting a
> > > different error message given a NULL constructor elt) and
> > > cxx_eval_bare_aggregate itself (fixed by not re-reducing a CONSTRUCTOR
> > > returned by lookup_placeholder).
> > > 
> > > As a drive-by this patch adds a check in reduced_constant_expression_p
> > > to correctly return false for an empty CONSTRUCTOR of UNION_TYPE.  This
> > > allows us to correctly diagnose the uninitialized use in
> > > constexpr-union4.C where we weren't before.
> > > 
> > > -- >8 --
> > > 
> > > gcc/cp/ChangeLog:
> > > 
> > > 	PR c++/94066
> > > 	* constexpr.c (reduced_constant_expression_p) [CONSTRUCTOR]: Properly
> > > 	handle unions without an initializer.
> > > 	(cxx_eval_component_reference): Emit a different diagnostic when the
> > > 	constructor element corresponding to a union member is NULL.
> > > 	(cxx_eval_bare_aggregate): When constructing a union, always set the
> > > 	active union member before evaluating the initializer.  Relax
> > > assertion
> > > 	that verifies the index of the constructor element we're initializing
> > > 	hasn't been changed.
> > > 	(cxx_eval_store_expression): Diagnose changing the active union member
> > > 	while the union is in the process of being initialized.  After setting
> > > 	an active union member, clear CONSTRUCTOR_NO_CLEARING on the
> > > underlying
> > > 	CONSTRUCTOR.
> > > 	(cxx_eval_constant_expression) [PLACEHOLDER_EXPR]: Don't re-reduce a
> > > 	CONSTRUCTOR returned by lookup_placeholder.
> > > 
> > > gcc/testsuite/ChangeLog:
> > > 
> > > 	PR c++/94066
> > > 	* g++.dg/cpp1y/constexpr-union2.C: New test.
> > > 	* g++.dg/cpp1y/constexpr-union3.C: New test.
> > > 	* g++.dg/cpp1y/constexpr-union4.C: New test.
> > > 	* g++.dg/cpp1y/constexpr-union5.C: New test.
> > > 	* 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/cpp2a/constexpr-union1.C: New test.
> > > ---
> > >   gcc/cp/constexpr.c                            | 69 ++++++++++++++++---
> > >   gcc/testsuite/g++.dg/cpp1y/constexpr-union2.C |  9 +++
> > >   gcc/testsuite/g++.dg/cpp1y/constexpr-union3.C |  9 +++
> > >   gcc/testsuite/g++.dg/cpp1y/constexpr-union4.C |  9 +++
> > >   gcc/testsuite/g++.dg/cpp1y/constexpr-union5.C | 15 ++++
> > >   gcc/testsuite/g++.dg/cpp1y/pr94066-2.C        | 19 +++++
> > >   gcc/testsuite/g++.dg/cpp1y/pr94066-3.C        | 16 +++++
> > >   gcc/testsuite/g++.dg/cpp1y/pr94066.C          | 18 +++++
> > >   gcc/testsuite/g++.dg/cpp2a/constexpr-union1.C | 18 +++++
> > >   9 files changed, 173 insertions(+), 9 deletions(-)
> > >   create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-union2.C
> > >   create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-union3.C
> > >   create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-union4.C
> > >   create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-union5.C
> > >   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.C
> > >   create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-union1.C
> > > 
> > > diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
> > > index 192face9a3a..6284ab25b7d 100644
> > > --- a/gcc/cp/constexpr.c
> > > +++ b/gcc/cp/constexpr.c
> > > @@ -2591,10 +2591,17 @@ reduced_constant_expression_p (tree t)
> > >   	    return false;
> > >   	  else if (cxx_dialect >= cxx2a
> > >   		   /* An ARRAY_TYPE doesn't have any TYPE_FIELDS.  */
> > > -		   && (TREE_CODE (TREE_TYPE (t)) == ARRAY_TYPE
> > > -		       /* A union only initializes one member.  */
> > > -		       || TREE_CODE (TREE_TYPE (t)) == UNION_TYPE))
> > > +		   && TREE_CODE (TREE_TYPE (t)) == ARRAY_TYPE)
> > >   	    field = NULL_TREE;
> > > +	  else if (cxx_dialect >= cxx2a
> > > +		   && TREE_CODE (TREE_TYPE (t)) == UNION_TYPE)
> > > +	    {
> > > +	      if (CONSTRUCTOR_NELTS (t) == 0)
> > > +		/* An initialized union has a constructor element.  */
> > > +		return false;
> > > +	      /* And it only initializes one member.  */
> > > +	      field = NULL_TREE;
> > > +	    }
> > >   	  else
> > >   	    field = next_initializable_field (TYPE_FIELDS (TREE_TYPE (t)));
> > >   	}
> > > @@ -3446,8 +3453,14 @@ cxx_eval_component_reference (const constexpr_ctx
> > > *ctx, tree t,
> > >       {
> > >         /* DR 1188 says we don't have to deal with this.  */
> > >         if (!ctx->quiet)
> > > -	error ("accessing %qD member instead of initialized %qD member in "
> > > -	       "constant expression", part, CONSTRUCTOR_ELT (whole,
> > > 0)->index);
> > > +	{
> > > +	  constructor_elt *cep = CONSTRUCTOR_ELT (whole, 0);
> > > +	  if (cep->value == NULL_TREE)
> > > +	    error ("accessing uninitialized member %qD", part);
> > > +	  else
> > > +	    error ("accessing %qD member instead of initialized %qD member in
> > > "
> > > +		   "constant expression", part, cep->index);
> > > +	}
> > >         *non_constant_p = true;
> > >         return t;
> > >       }
> > > @@ -3751,6 +3764,11 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx,
> > > tree t,
> > >   	/* 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);
> > > +      else if (TREE_CODE (type) == UNION_TYPE)
> > > +	/* If we're constructing a union, set the active union member now so
> > > +	   that we can later detect if the initializer attempts to activate
> > > +	   another member.  */
> > > +	CONSTRUCTOR_APPEND_ELT (*p, index, NULL_TREE);
> > >         tree elt = cxx_eval_constant_expression (&new_ctx, value,
> > >   					       lval,
> > >   					       non_constant_p, overflow_p);
> > > @@ -3784,7 +3802,13 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx,
> > > tree t,
> > >   	}
> > >         else
> > >   	{
> > > -	  if (new_ctx.ctor != ctx->ctor)
> > > +	  if (TREE_CODE (type) == UNION_TYPE
> > > +	      && (*p)->last().index != index)
> > > +	    /* The initializer may have 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)
> > >   	    {
> > >   	      /* We appended this element above; update the value.  */
> > >   	      gcc_assert ((*p)->last().index == index);
> > > @@ -4567,6 +4591,7 @@ cxx_eval_store_expression (const constexpr_ctx *ctx,
> > > tree t,
> > >     bool no_zero_init = true;
> > >       releasing_vec ctors;
> > > +  bool changed_active_union_member_p = false;
> > >     while (!refs->is_empty ())
> > >       {
> > >         if (*valp == NULL_TREE)
> > > @@ -4647,6 +4672,19 @@ cxx_eval_store_expression (const constexpr_ctx *ctx,
> > > tree t,
> > >   			      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.  */
> > 
> > Hmm, could we also detect this situation by noticing that the current active
> > constructor element has NULL_TREE value?
> 
> I don't think a looking for a NULL_TREE value here would be sufficient as-is,
> only because at that point the active constructor element of an under-construction
> union could also be an empty CONSTRUCTOR if the union member we're initializing
> is an aggregate and we're coming from cxx_eval_bare_aggregate, which does:
> 
>       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);
>       else if (TREE_CODE (type) == UNION_TYPE)
>         /* If we're constructing a union, set the active union member now so
>            that we can later detect if the initializer attempts to activate
>            another member.  */
>         CONSTRUCTOR_APPEND_ELT (*p, index, NULL_TREE);
>       tree elt = cxx_eval_constant_expression (&new_ctx, value,
>                                                lval,
>                                                non_constant_p, overflow_p);

Checking for a NULL_TREE value instead would also not catch the case
when we're called recursively from cxx_eval_store_expression in the
!preeval case with an INIT_EXPR like

   u.a = foo(&u);

for a similar reason -- cxx_eval_store_expression sets the value of the
current active constructor element to an empty CONSTRUCTOR before
evaluating the RHS.  So we would end up ICEing on pr94066.C and not
rejecting pr94066-2.C apparently.

> 
> > 
> > > +		  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;
> > > @@ -4675,6 +4713,10 @@ cxx_eval_store_expression (const constexpr_ctx *ctx,
> > > tree t,
> > >     	    vec_safe_insert (CONSTRUCTOR_ELTS (*valp), idx, ce);
> > >   	    cep = CONSTRUCTOR_ELT (*valp, idx);
> > > +
> > > +	    if (code == UNION_TYPE)
> > > +	      /* Record that we've changed an active union member.  */
> > > +	      changed_active_union_member_p = true;
> > >   	  }
> > >   	found:;
> > >   	}
> > > @@ -4805,13 +4847,17 @@ 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)
> > > +  if (!c || s || changed_active_union_member_p)
> > >       FOR_EACH_VEC_ELT (*ctors, i, elt)
> > >         {
> > >   	if (!c)
> > >   	  TREE_CONSTANT (elt) = false;
> > >   	if (s)
> > >   	  TREE_SIDE_EFFECTS (elt) = true;
> > > +	/* Clear CONSTRUCTOR_NO_CLEARING since we've activated a member of
> > > +	   this union.  */
> > > +	if (TREE_CODE (TREE_TYPE (elt)) == UNION_TYPE)
> > > +	  CONSTRUCTOR_NO_CLEARING (elt) = false;
> > >         }
> > >       if (*non_constant_p)
> > > @@ -6133,8 +6179,13 @@ cxx_eval_constant_expression (const constexpr_ctx
> > > *ctx, tree t,
> > >       case PLACEHOLDER_EXPR:
> > >         /* Use of the value or address of the current object.  */
> > >         if (tree ctor = lookup_placeholder (ctx, lval, TREE_TYPE (t)))
> > > -	return cxx_eval_constant_expression (ctx, ctor, lval,
> > > -					     non_constant_p, overflow_p);
> > > +	{
> > > +	  if (TREE_CODE (ctor) == CONSTRUCTOR)
> > > +	    return ctor;
> > > +	  else
> > > +	    return cxx_eval_constant_expression (ctx, ctor, lval,
> > > +						 non_constant_p, overflow_p);
> > > +	}
> > >         /* A placeholder without a referent.  We can get here when
> > >   	 checking whether NSDMIs are noexcept, or in massage_init_elt;
> > >   	 just say it's non-constant for now.  */
> > > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-union2.C
> > > b/gcc/testsuite/g++.dg/cpp1y/constexpr-union2.C
> > > new file mode 100644
> > > index 00000000000..7a6a818742b
> > > --- /dev/null
> > > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-union2.C
> > > @@ -0,0 +1,9 @@
> > > +// { dg-do compile { target c++14 } }
> > > +
> > > +union U
> > > +{
> > > +  char *x = &y;
> > > +  char y;
> > > +};
> > > +
> > > +constexpr U u = {};
> > > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-union3.C
> > > b/gcc/testsuite/g++.dg/cpp1y/constexpr-union3.C
> > > new file mode 100644
> > > index 00000000000..5cf62e46cb5
> > > --- /dev/null
> > > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-union3.C
> > > @@ -0,0 +1,9 @@
> > > +// { dg-do compile { target c++14 } }
> > > +
> > > +union U
> > > +{
> > > +  int x = (x = x + 1);
> > > +  char y;
> > > +};
> > > +
> > > +constexpr U u = {}; // { dg-error "accessing uninitialized member" }
> > > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-union4.C
> > > b/gcc/testsuite/g++.dg/cpp1y/constexpr-union4.C
> > > new file mode 100644
> > > index 00000000000..3e44a1378f3
> > > --- /dev/null
> > > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-union4.C
> > > @@ -0,0 +1,9 @@
> > > +// { dg-do compile { target c++14 } }
> > > +
> > > +union U
> > > +{
> > > +  int x = y;
> > > +  char y;
> > > +};
> > > +
> > > +constexpr U u = {}; // { dg-error "accessing uninitialized member" }
> > > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-union5.C
> > > b/gcc/testsuite/g++.dg/cpp1y/constexpr-union5.C
> > > new file mode 100644
> > > index 00000000000..55fe9fa2f0b
> > > --- /dev/null
> > > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-union5.C
> > > @@ -0,0 +1,15 @@
> > > +// { 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->a++;
> > > +  return {42};
> > > +}
> > > +
> > > +extern constexpr U u = {}; // { dg-error "accessing uninitialized member" }
> > > 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..1c00b650961
> > > --- /dev/null
> > > +++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-2.C
> > > @@ -0,0 +1,19 @@
> > > +// 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'" }
> > > +  return {42};
> > > +}
> > > +
> > > +extern constexpr U u = {};
> > > 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..175018acf86
> > > --- /dev/null
> > > +++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-3.C
> > > @@ -0,0 +1,16 @@
> > > +// 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; // { dg-error "'U::a' to 'U::y'" }
> > > +  return {42};
> > > +}
> > > +
> > > +extern constexpr U u = {};
> > > diff --git a/gcc/testsuite/g++.dg/cpp1y/pr94066.C
> > > b/gcc/testsuite/g++.dg/cpp1y/pr94066.C
> > > new file mode 100644
> > > index 00000000000..6725c8c737f
> > > --- /dev/null
> > > +++ b/gcc/testsuite/g++.dg/cpp1y/pr94066.C
> > > @@ -0,0 +1,18 @@
> > > +// 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'" }
> > > +  return {42};
> > > +}
> > > +
> > > +extern constexpr U u = {};
> > > diff --git a/gcc/testsuite/g++.dg/cpp2a/constexpr-union1.C
> > > b/gcc/testsuite/g++.dg/cpp2a/constexpr-union1.C
> > > new file mode 100644
> > > index 00000000000..c38167ad798
> > > --- /dev/null
> > > +++ b/gcc/testsuite/g++.dg/cpp2a/constexpr-union1.C
> > > @@ -0,0 +1,18 @@
> > > +// { dg-do compile { target c++2a } }
> > > +
> > > +union U
> > > +{
> > > +  int x;
> > > +  char y;
> > > +};
> > > +
> > > +constexpr bool
> > > +baz ()
> > > +{
> > > +  U u;
> > > +  u.x = 3;
> > > +  u.y = 7;
> > > +  return (u.y == 7);
> > > +}
> > > +
> > > +static_assert (baz ());
> > > 
> > 
> > 
>

On 3/20/20 2:47 PM, Patrick Palka wrote:
> On Fri, 20 Mar 2020, Patrick Palka wrote:
> 
>> On Fri, 20 Mar 2020, Jason Merrill wrote:
>>
>>> On 3/20/20 9:49 AM, Patrick Palka wrote:
>>>> On Thu, 19 Mar 2020, Jason Merrill wrote:
>>>>
>>>>> On 3/19/20 2:06 PM, Patrick Palka via Gcc-patches wrote:
>>>>>> On Thu, 19 Mar 2020, Marek Polacek wrote:
>>>>>>
>>>>>>> On Thu, Mar 19, 2020 at 01:06:35PM -0400, Patrick Palka via
>>>>>>> Gcc-patches
>>>>>>> wrote:
>>>>>>>> On Thu, 19 Mar 2020, Patrick Palka wrote:
>>>>>>>>
>>>>>>>>> This patch adds a check to detect changing the active union member
>>>>>>>>> during
>>>>>>>>> initialization of the union.  It uses the CONSTRUCTOR_NO_CLEARING
>>>>>>>>> flag
>>>>>>>>> as a
>>>>>>>>> proxy for whether the non-empty CONSTRUCTOR of UNION_TYPE we're
>>>>>>>>> assigning to in
>>>>>>>>> cxx_eval_store_expression is in the process of being initialized,
>>>>>>>>> which seems to
>>>>>>>>> work well.
>>>>>>>>
>>>>>>>> If we can't rely on CONSTRUCTOR_NO_CLEARING to be set iff a
>>>>>>>> CONSTRUCTOR
>>>>>>>> is in the process of being initialized, then here's an alternative
>>>>>>>> patch
>>>>>>>> for consideration, that detects this UB in an indirect way and after
>>>>>>>> the
>>>>>>>> fact.
>>>>>>>
>>>>>>> Yeah, I'm not sure if that would work well, especially in C++20 where
>>>>>>> we
>>>>>>> sometimes don't clear it:
>>>>>>>
>>>>>>>      /* The result of a constexpr function must be completely
>>>>>>> initialized.
>>>>>>>
>>>>>>>         However, in C++20, a constexpr constructor doesn't necessarily
>>>>>>> have
>>>>>>>         to initialize all the fields, so we don't clear
>>>>>>> CONSTRUCTOR_NO_CLEARING
>>>>>>>         in order to detect reading an unitialized object in constexpr
>>>>>>> instead
>>>>>>>         of value-initializing it.  (reduced_constant_expression_p is
>>>>>>> expected to
>>>>>>>         take care of clearing the flag.)  */
>>>>>>>      if (TREE_CODE (result) == CONSTRUCTOR
>>>>>>>          && (cxx_dialect < cxx2a
>>>>>>>              || !DECL_CONSTRUCTOR_P (fun)))
>>>>>>>        clear_no_implicit_zero (result);
>>>>>>>
>>>>>>> and rely on reduced_constant_expression_p to clear it.
>>>>>>
>>>>>> I see, thanks.  Here's a reproducer for the issue you pointed out, which
>>>>>> is a valid testcase but gets rejected with the proposed patch:
>>>>>>
>>>>>>        union U
>>>>>>        {
>>>>>>          int x;
>>>>>>          char y;
>>>>>>        };
>>>>>>
>>>>>>        constexpr bool
>>>>>>        baz ()
>>>>>>        {
>>>>>>          U u;
>>>>>>          u.x = 3;
>>>>>>          u.y = 7;
>>>>>>          return (u.y == 7);
>>>>>>        }
>>>>>>
>>>>>>        static_assert (baz ());
>>>>>>
>>>>>> CONSTRUCTOR_NO_CLEARING is set for 'u' and is not cleared after its
>>>>>> constructor returns, and so the check yields a false positive for the
>>>>>> assignment to u.y.  That's unfortunate...
>>>>>
>>>>> We should probably clear the flag when we assign to u.x because once we
>>>>> give a
>>>>> value to one union member, the union has a value.
>>>>
>>>> That works well.  Further testing revealed a couple of more issues
>>>> caused by the new hunk
>>>>
>>>> @@ -3751,6 +3751,11 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx,
>>>> tree t,
>>>>    	/* 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);
>>>> +      else if (TREE_CODE (type) == UNION_TYPE)
>>>> +	/* If we're constructing a union, set the active union member now so
>>>> +	   that we can later detect if the initializer attempts to activate
>>>> +	   another member.  */
>>>> +	CONSTRUCTOR_APPEND_ELT (*p, index, NULL_TREE);
>>>>          tree elt = cxx_eval_constant_expression (&new_ctx, value,
>>>>    					       lval,
>>>>    					       non_constant_p, overflow_p);
>>>>
>>>> where other routines aren't prepared to handle a NULL constructor union
>>>> element.   They are cxx_eval_component_reference (fixed by emitting a
>>>> different error message given a NULL constructor elt) and
>>>> cxx_eval_bare_aggregate itself (fixed by not re-reducing a CONSTRUCTOR
>>>> returned by lookup_placeholder).
>>>>
>>>> As a drive-by this patch adds a check in reduced_constant_expression_p
>>>> to correctly return false for an empty CONSTRUCTOR of UNION_TYPE.  This
>>>> allows us to correctly diagnose the uninitialized use in
>>>> constexpr-union4.C where we weren't before.
>>>>
>>>> -- >8 --
>>>>
>>>> gcc/cp/ChangeLog:
>>>>
>>>> 	PR c++/94066
>>>> 	* constexpr.c (reduced_constant_expression_p) [CONSTRUCTOR]: Properly
>>>> 	handle unions without an initializer.
>>>> 	(cxx_eval_component_reference): Emit a different diagnostic when the
>>>> 	constructor element corresponding to a union member is NULL.
>>>> 	(cxx_eval_bare_aggregate): When constructing a union, always set the
>>>> 	active union member before evaluating the initializer.  Relax
>>>> assertion
>>>> 	that verifies the index of the constructor element we're initializing
>>>> 	hasn't been changed.
>>>> 	(cxx_eval_store_expression): Diagnose changing the active union member
>>>> 	while the union is in the process of being initialized.  After setting
>>>> 	an active union member, clear CONSTRUCTOR_NO_CLEARING on the
>>>> underlying
>>>> 	CONSTRUCTOR.
>>>> 	(cxx_eval_constant_expression) [PLACEHOLDER_EXPR]: Don't re-reduce a
>>>> 	CONSTRUCTOR returned by lookup_placeholder.
>>>>
>>>> gcc/testsuite/ChangeLog:
>>>>
>>>> 	PR c++/94066
>>>> 	* g++.dg/cpp1y/constexpr-union2.C: New test.
>>>> 	* g++.dg/cpp1y/constexpr-union3.C: New test.
>>>> 	* g++.dg/cpp1y/constexpr-union4.C: New test.
>>>> 	* g++.dg/cpp1y/constexpr-union5.C: New test.
>>>> 	* 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/cpp2a/constexpr-union1.C: New test.
>>>> ---
>>>>    gcc/cp/constexpr.c                            | 69 ++++++++++++++++---
>>>>    gcc/testsuite/g++.dg/cpp1y/constexpr-union2.C |  9 +++
>>>>    gcc/testsuite/g++.dg/cpp1y/constexpr-union3.C |  9 +++
>>>>    gcc/testsuite/g++.dg/cpp1y/constexpr-union4.C |  9 +++
>>>>    gcc/testsuite/g++.dg/cpp1y/constexpr-union5.C | 15 ++++
>>>>    gcc/testsuite/g++.dg/cpp1y/pr94066-2.C        | 19 +++++
>>>>    gcc/testsuite/g++.dg/cpp1y/pr94066-3.C        | 16 +++++
>>>>    gcc/testsuite/g++.dg/cpp1y/pr94066.C          | 18 +++++
>>>>    gcc/testsuite/g++.dg/cpp2a/constexpr-union1.C | 18 +++++
>>>>    9 files changed, 173 insertions(+), 9 deletions(-)
>>>>    create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-union2.C
>>>>    create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-union3.C
>>>>    create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-union4.C
>>>>    create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-union5.C
>>>>    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.C
>>>>    create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-union1.C
>>>>
>>>> diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
>>>> index 192face9a3a..6284ab25b7d 100644
>>>> --- a/gcc/cp/constexpr.c
>>>> +++ b/gcc/cp/constexpr.c
>>>> @@ -2591,10 +2591,17 @@ reduced_constant_expression_p (tree t)
>>>>    	    return false;
>>>>    	  else if (cxx_dialect >= cxx2a
>>>>    		   /* An ARRAY_TYPE doesn't have any TYPE_FIELDS.  */
>>>> -		   && (TREE_CODE (TREE_TYPE (t)) == ARRAY_TYPE
>>>> -		       /* A union only initializes one member.  */
>>>> -		       || TREE_CODE (TREE_TYPE (t)) == UNION_TYPE))
>>>> +		   && TREE_CODE (TREE_TYPE (t)) == ARRAY_TYPE)
>>>>    	    field = NULL_TREE;
>>>> +	  else if (cxx_dialect >= cxx2a
>>>> +		   && TREE_CODE (TREE_TYPE (t)) == UNION_TYPE)
>>>> +	    {
>>>> +	      if (CONSTRUCTOR_NELTS (t) == 0)
>>>> +		/* An initialized union has a constructor element.  */
>>>> +		return false;
>>>> +	      /* And it only initializes one member.  */
>>>> +	      field = NULL_TREE;
>>>> +	    }
>>>>    	  else
>>>>    	    field = next_initializable_field (TYPE_FIELDS (TREE_TYPE (t)));
>>>>    	}
>>>> @@ -3446,8 +3453,14 @@ cxx_eval_component_reference (const constexpr_ctx
>>>> *ctx, tree t,
>>>>        {
>>>>          /* DR 1188 says we don't have to deal with this.  */
>>>>          if (!ctx->quiet)
>>>> -	error ("accessing %qD member instead of initialized %qD member in "
>>>> -	       "constant expression", part, CONSTRUCTOR_ELT (whole,
>>>> 0)->index);
>>>> +	{
>>>> +	  constructor_elt *cep = CONSTRUCTOR_ELT (whole, 0);
>>>> +	  if (cep->value == NULL_TREE)
>>>> +	    error ("accessing uninitialized member %qD", part);
>>>> +	  else
>>>> +	    error ("accessing %qD member instead of initialized %qD member in
>>>> "
>>>> +		   "constant expression", part, cep->index);
>>>> +	}
>>>>          *non_constant_p = true;
>>>>          return t;
>>>>        }
>>>> @@ -3751,6 +3764,11 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx,
>>>> tree t,
>>>>    	/* 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);
>>>> +      else if (TREE_CODE (type) == UNION_TYPE)
>>>> +	/* If we're constructing a union, set the active union member now so
>>>> +	   that we can later detect if the initializer attempts to activate
>>>> +	   another member.  */
>>>> +	CONSTRUCTOR_APPEND_ELT (*p, index, NULL_TREE);
>>>>          tree elt = cxx_eval_constant_expression (&new_ctx, value,
>>>>    					       lval,
>>>>    					       non_constant_p, overflow_p);
>>>> @@ -3784,7 +3802,13 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx,
>>>> tree t,
>>>>    	}
>>>>          else
>>>>    	{
>>>> -	  if (new_ctx.ctor != ctx->ctor)
>>>> +	  if (TREE_CODE (type) == UNION_TYPE
>>>> +	      && (*p)->last().index != index)
>>>> +	    /* The initializer may have 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)
>>>>    	    {
>>>>    	      /* We appended this element above; update the value.  */
>>>>    	      gcc_assert ((*p)->last().index == index);
>>>> @@ -4567,6 +4591,7 @@ cxx_eval_store_expression (const constexpr_ctx *ctx,
>>>> tree t,
>>>>      bool no_zero_init = true;
>>>>        releasing_vec ctors;
>>>> +  bool changed_active_union_member_p = false;
>>>>      while (!refs->is_empty ())
>>>>        {
>>>>          if (*valp == NULL_TREE)
>>>> @@ -4647,6 +4672,19 @@ cxx_eval_store_expression (const constexpr_ctx *ctx,
>>>> tree t,
>>>>    			      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.  */
>>>
>>> Hmm, could we also detect this situation by noticing that the current active
>>> constructor element has NULL_TREE value?
>>
>> I don't think a looking for a NULL_TREE value here would be sufficient as-is,
>> only because at that point the active constructor element of an under-construction
>> union could also be an empty CONSTRUCTOR if the union member we're initializing
>> is an aggregate and we're coming from cxx_eval_bare_aggregate, which does:
>>
>>        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);
>>        else if (TREE_CODE (type) == UNION_TYPE)
>>          /* If we're constructing a union, set the active union member now so
>>             that we can later detect if the initializer attempts to activate
>>             another member.  */
>>          CONSTRUCTOR_APPEND_ELT (*p, index, NULL_TREE);
>>        tree elt = cxx_eval_constant_expression (&new_ctx, value,
>>                                                 lval,
>>                                                 non_constant_p, overflow_p);
> 
> Checking for a NULL_TREE value instead would also not catch the case
> when we're called recursively from cxx_eval_store_expression in the
> !preeval case with an INIT_EXPR like
> 
>     u.a = foo(&u);
> 
> for a similar reason -- cxx_eval_store_expression sets the value of the
> current active constructor element to an empty CONSTRUCTOR before
> evaluating the RHS.  So we would end up ICEing on pr94066.C and not
> rejecting pr94066-2.C apparently.

Thanks.  Then the patch is OK.

Jason