On 8/4/20 2:31 PM, Patrick Palka wrote:
On Tue, 4 Aug 2020, Jason Merrill wrote:
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.
Sounds good. I wasn't quite sure what we may or may not assume about
the previous initializer.
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.
Yeah, the intent was to make sure to include the recursive
initialization case while excluding the array copy case. I suppose we
can add this test as an assert too, but it doesn't seem that worthwhile.
In the below I removed the pre_init || value_init || !init test and
added the initializer_zerop test as an assert. Does it look OK to
commit after bootstrap/regtest succeeds?
-- >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
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 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 | 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..bec888646ea 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 (!CONSTRUCTOR_NO_CLEARING (ctx->ctor))
+ {
+ /* We're initializing an array object that had been zero-initialized
+ earlier. Truncate ctx->ctor and preserve its zeroed state by
+ clearing CONSTRUCTOR_NO_CLEARING on each of the element
+ initializers we append to it. */
+ gcc_assert (initializer_zerop (ctx->ctor));
Let's make this gcc_checking_assert. OK with that change.
+ 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);
