Message ID | 20200319163550.2301246-1-ppalka@redhat.com |
---|---|
State | New |
Headers | show |
Series | c++: Reject changing active member of union during initialization [PR94066] | expand |
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
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 = {};