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 ());
-- 
2.26.0.rc1.11.g30e9940356

Reply via email to