On 8/4/20 9:45 AM, Patrick Palka wrote:
On Mon, 3 Aug 2020, Patrick Palka wrote:On Mon, 3 Aug 2020, Jason Merrill wrote:On 8/3/20 2:45 PM, Patrick Palka wrote:On Mon, 3 Aug 2020, Jason Merrill wrote:On 8/3/20 8:53 AM, Patrick Palka wrote:On Mon, 3 Aug 2020, Patrick Palka wrote:In the first testcase below, expand_aggr_init_1 sets up t's default constructor such that the ctor first zero-initializes the entire base b, followed by calling b's default constructor, the latter of which just default-initializes the array member b::m via a VEC_INIT_EXPR. So upon constexpr evaluation of this latter VEC_INIT_EXPR, ctx->ctor is nonempty due to the prior zero-initialization, and we proceed in cxx_eval_vec_init to append new constructor_elts to the end of ctx->ctor without first checking if a matching constructor_elt already exists. This leads to ctx->ctor having two matching constructor_elts for each index. This patch partially fixes this issue by making the RANGE_EXPR optimization in cxx_eval_vec_init truncate ctx->ctor before adding the single RANGE_EXPR constructor_elt. This isn't a complete fix because the RANGE_EXPR optimization applies only when the constant initializer is relocatable, so whenever it's not relocatable we can still build up an invalid CONSTRUCTOR, e.g. if in the first testcase we add an NSDMI such as 'e *p = this;' to struct e, then the ICE still occurs even with this patch.A complete but more risky one-line fix would be to always truncate ctx->ctor beforehand, not just when the RANGE_EXPR optimization applies. If it's true that the initializer of a VEC_INIT_EXPR can't observe the previous elements of the target array, then it should be safe to always truncate I think?What if default-initialization of the array element type doesn't fully initialize the elements, e.g. if 'e' had another member without a default initializer? Does truncation first mean we lose the zero-initialization of such a member?Hmm, it looks like we would lose the zero-initialization of such a member with or without truncation first (so with any one of the three proposed fixes). I think it's because the evaluation loop in cxx_eval_vec_init disregards each element's prior (zero-initialized) state.We could probably still do the truncation, but clear the CONSTRUCTOR_NO_CLEARING flag on the element initializer.Ah, this seems to work well. Like this? -- >8 -- Subject: [PATCH] c++: cxx_eval_vec_init after zero initialization [PR96282] In the first testcase below, expand_aggr_init_1 sets up t's default constructor such that the ctor first zero-initializes the entire base b, followed by calling b's default constructor, the latter of which just default-initializes the array member b::m via a VEC_INIT_EXPR. So upon constexpr evaluation of this latter VEC_INIT_EXPR, ctx->ctor is nonempty due to the prior zero-initialization, and we proceed in cxx_eval_vec_init to append new constructor_elts to the end of ctx->ctor without first checking if a matching constructor_elt already exists. This leads to ctx->ctor having two matching constructor_elts for each index. This patch fixes this issue by truncating a zero-initialized array object in cxx_eval_vec_init_1 before we begin appending default-initialized array elements to it. Since default-initialization may leave parts of the element type unitialized, we also preserve the array's prior zero-initialized state by clearing CONSTRUCTOR_NO_CLEARING on each appended element initializers. gcc/cp/ChangeLog: PR c++/96282 * constexpr.c (cxx_eval_vec_init_1): Truncate ctx->ctor and then clear CONSTRUCTOR_NO_CLEARING on each appended element initializer if we're default-initializing a previously zero-initialized array object. gcc/testsuite/ChangeLog: PR c++/96282 * g++.dg/cpp0x/constexpr-array26.C: New test. * g++.dg/cpp0x/constexpr-array27.C: New test. * g++.dg/cpp2a/constexpr-init18.C: New test. --- gcc/cp/constexpr.c | 17 ++++++++++++++++- gcc/testsuite/g++.dg/cpp0x/constexpr-array26.C | 13 +++++++++++++ gcc/testsuite/g++.dg/cpp0x/constexpr-array27.C | 13 +++++++++++++ gcc/testsuite/g++.dg/cpp2a/constexpr-init18.C | 16 ++++++++++++++++ 4 files changed, 58 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-array26.C create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-array27.C create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-init18.C diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c index b1c1d249c6e..706bef323b2 100644 --- a/gcc/cp/constexpr.c +++ b/gcc/cp/constexpr.c @@ -4171,6 +4171,17 @@ cxx_eval_vec_init_1 (const constexpr_ctx *ctx, tree atype, tree init, pre_init = true; } + bool zero_initialized_p = false; + if ((pre_init || value_init || !init) && initializer_zerop (ctx->ctor))Does initializer_zerop capture the difference between a default-initialized or value-initialized containing object? I would expect it to be true for both. Maybe check CONSTRUCTOR_NO_CLEARING (ctx->ctor) instead?D'oh, indeed it looks like initializer_zerop is not what we want here.Hmm, might we need to check both? !CONSTRUCTOR_NO_CLEARING tells us about the omitted initializers, but it seems we'd still need to test if the explicit initializers are zero.
We might check initializer_zerop as an assert; at this point the only previous initialization should be zero-initialization.
What are you going for with (pre_init || value_init || !init)? All cases other than array copy? We shouldn't ever see zero-initialization before copy, so this test seems unnecessary.
This all seems parallel to the no_zero_init code in cxx_eval_store_expression.I am testing the following, which inspects CONSTRUCTOR_NO_CLEARING and also beefs up the tests to verify that we handle the multidimensional array case correctly. -- >8 -- Subject: [PATCH] c++: cxx_eval_vec_init after zero initialization [PR96282] In the first testcase below, expand_aggr_init_1 sets up t's default constructor such that the ctor first zero-initializes the entire base b, followed by calling b's default constructor, the latter of which just default-initializes the array member b::m via a VEC_INIT_EXPR. So upon constexpr evaluation of this latter VEC_INIT_EXPR, ctx->ctor is nonempty due to the prior zero-initialization, and we proceed in cxx_eval_vec_init to append new constructor_elts to the end of ctx->ctor without first checking if a matching constructor_elt already exists. This leads to ctx->ctor having two matching constructor_elts for each index. This patch fixes this issue by truncating a zeroed out array CONSTRUCTOR in cxx_eval_vec_init_1 before we begin appending array elements to it. We preserve its zeroed out state during evaluation by clearing CONSTRUCTOR_NO_CLEARING on each new appended element. gcc/cp/ChangeLog: PR c++/96282 * constexpr.c (cxx_eval_vec_init_1): Truncate ctx->ctor and then clear CONSTRUCTOR_NO_CLEARING on each appended element initializer if we're initializing a previously zeroed out array object. gcc/testsuite/ChangeLog: PR c++/96282 * g++.dg/cpp0x/constexpr-array26.C: New test. * g++.dg/cpp0x/constexpr-array27.C: New test. * g++.dg/cpp2a/constexpr-init18.C: New test. --- gcc/cp/constexpr.c | 18 +++++++++++++++++- gcc/testsuite/g++.dg/cpp0x/constexpr-array26.C | 13 +++++++++++++ gcc/testsuite/g++.dg/cpp0x/constexpr-array27.C | 13 +++++++++++++ gcc/testsuite/g++.dg/cpp2a/constexpr-init18.C | 16 ++++++++++++++++ 4 files changed, 59 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-array26.C create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-array27.C create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-init18.C diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c index b1c1d249c6e..364630f6333 100644 --- a/gcc/cp/constexpr.c +++ b/gcc/cp/constexpr.c @@ -4171,6 +4171,18 @@ cxx_eval_vec_init_1 (const constexpr_ctx *ctx, tree atype, tree init, pre_init = true; }+ bool zeroed_out = false;+ if ((pre_init || value_init || !init) + && !CONSTRUCTOR_NO_CLEARING (ctx->ctor)) + { + /* We're initializing an array object that had been zeroed out + earlier. Truncate ctx->ctor and preserve its prior state by + propagating CONSTRUCTOR_NO_CLEARING to each of the element + initializers we append to it. */ + zeroed_out = true; + vec_safe_truncate (*p, 0); + } + tree nelts = get_array_or_vector_nelts (ctx, atype, non_constant_p, overflow_p); unsigned HOST_WIDE_INT max = tree_to_uhwi (nelts); @@ -4182,7 +4194,11 @@ cxx_eval_vec_init_1 (const constexpr_ctx *ctx, tree atype, tree init, constexpr_ctx new_ctx; init_subob_ctx (ctx, new_ctx, idx, pre_init ? init : elttype); if (new_ctx.ctor != ctx->ctor) - CONSTRUCTOR_APPEND_ELT (*p, idx, new_ctx.ctor); + { + if (zeroed_out) + CONSTRUCTOR_NO_CLEARING (new_ctx.ctor) = false; + CONSTRUCTOR_APPEND_ELT (*p, idx, new_ctx.ctor); + } if (TREE_CODE (elttype) == ARRAY_TYPE) { /* A multidimensional array; recurse. */ diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-array26.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-array26.C new file mode 100644 index 00000000000..274f55a88bf --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-array26.C @@ -0,0 +1,13 @@ +// PR c++/96282 +// { dg-do compile { target c++11 } } + +struct e { bool v = true; }; + +template<int N> +struct b { e m[N]; }; + +template<int N> +struct t : b<N> { constexpr t() : b<N>() {} }; + +constexpr t<1> h1; +constexpr t<42> h2; diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-array27.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-array27.C new file mode 100644 index 00000000000..1234caef31d --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-array27.C @@ -0,0 +1,13 @@ +// PR c++/96282 +// { dg-do compile { target c++11 } } + +struct e { bool v = true; e *p = this; }; + +template<int N> +struct b { e m[N][N]; }; + +template<int N> +struct t : b<N> { constexpr t() : b<N>() {} }; + +constexpr t<1> h1; +constexpr t<42> h2; diff --git a/gcc/testsuite/g++.dg/cpp2a/constexpr-init18.C b/gcc/testsuite/g++.dg/cpp2a/constexpr-init18.C new file mode 100644 index 00000000000..47ad11f2290 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp2a/constexpr-init18.C @@ -0,0 +1,16 @@ +// PR c++/96282 +// { dg-do compile { target c++20 } } + +struct e { bool v = true; bool w; }; + +template<int N> +struct b { e m[N][N]; }; + +template<int N> +struct t : b<N> { constexpr t() : b<N>() {} }; + +constexpr t<1> h1; +static_assert(h1.m[0][0].w == false); + +constexpr t<42> h2; +static_assert(h2.m[17][17].w == false); -- 2.28.0.89.g85b4e0a6dc
