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 ());
> > >
> >
> >
>
