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