* [PATCH] c++: constexpr array reference and value-initialization [PR101371]
@ 2021-07-14 0:15 Marek Polacek
2021-07-14 4:15 ` Jason Merrill
0 siblings, 1 reply; 4+ messages in thread
From: Marek Polacek @ 2021-07-14 0:15 UTC (permalink / raw)
To: Jason Merrill, GCC Patches
This PR gave me a hard time: I saw multiple issues starting with
different revisions. But ultimately the root cause seems to be
the following, and the attached patch fixes all issues I've found
here.
In cxx_eval_array_reference we create a new constexpr context for the
CP_AGGREGATE_TYPE_P case, but we also have to create it for the
non-aggregate case. In this test, we are evaluating
((B *)this)->a = rhs->a
which means that we set ctx.object to ((B *)this)->a. Then we proceed
to evaluate the initializer, rhs->a. For *rhs, we eval rhs, a PARM_DECL,
for which we have (const B &) &c.arr[0] in the hash table. Then
cxx_fold_indirect_ref gives us c.arr[0]. c is evaluated to {.arr={}} so
c.arr is {}. Now we want c.arr[0], so we end up in cxx_eval_array_reference
and since we're initializing from {}, we call build_value_init which
gives us an AGGR_INIT_EXPR that calls 'constexpr B::B()'. Then we
evaluate this AGGR_INIT_EXPR and since its first argument is dummy,
we take ctx.object instead. But that is the wrong object, we're not
initializing ((B *)this)->a here. And so we wound up with an
initializer for A, and then crash in cxx_eval_component_reference:
gcc_assert (DECL_CONTEXT (part) == TYPE_MAIN_VARIANT (TREE_TYPE (whole)));
where DECL_CONTEXT (part) is B (as it should be) but the type of whole
was A.
With that in mind, the fix is straightforward, except that when the
value-init produced an AGGR_INIT_EXPR, we shouldn't set ctx.object so
that
2508 if (DECL_CONSTRUCTOR_P (fun) && !ctx->object
2509 && TREE_CODE (t) == AGGR_INIT_EXPR)
2510 {
2511 /* We want to have an initialization target for an AGGR_INIT_EXPR.
2512 If we don't already have one in CTX, use the AGGR_INIT_EXPR_SLOT. */
2513 new_ctx.object = AGGR_INIT_EXPR_SLOT (t);
comes into play.
Bootstrapped/regtested on {x86_64,ppc64le,aarch64}-pc-linux-gnu, ok for trunk?
PR c++/101371
gcc/cp/ChangeLog:
* constexpr.c (cxx_eval_array_reference): Create a new .object
and .ctor for the non-aggregate case too when value-initializing.
gcc/testsuite/ChangeLog:
* g++.dg/cpp1y/constexpr-101371-2.C: New test.
* g++.dg/cpp1y/constexpr-101371.C: New test.
---
gcc/cp/constexpr.c | 15 ++++++----
.../g++.dg/cpp1y/constexpr-101371-2.C | 23 +++++++++++++++
gcc/testsuite/g++.dg/cpp1y/constexpr-101371.C | 29 +++++++++++++++++++
3 files changed, 61 insertions(+), 6 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-101371-2.C
create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-101371.C
diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
index 39787f3f5d5..584ef55703c 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -3844,23 +3844,26 @@ cxx_eval_array_reference (const constexpr_ctx *ctx, tree t,
initializer, it's initialized from {}. But use build_value_init
directly for non-aggregates to avoid creating a garbage CONSTRUCTOR. */
tree val;
- constexpr_ctx new_ctx;
if (is_really_empty_class (elem_type, /*ignore_vptr*/false))
return build_constructor (elem_type, NULL);
else if (CP_AGGREGATE_TYPE_P (elem_type))
{
tree empty_ctor = build_constructor (init_list_type_node, NULL);
val = digest_init (elem_type, empty_ctor, tf_warning_or_error);
- new_ctx = *ctx;
- new_ctx.object = t;
- new_ctx.ctor = build_constructor (elem_type, NULL);
- ctx = &new_ctx;
}
else
val = build_value_init (elem_type, tf_warning_or_error);
+
+ constexpr_ctx new_ctx = *ctx;
+ /* If we are using an AGGR_INIT_EXPR, clear OBJECT for now so that
+ cxx_eval_call_expression can make use of AGGR_INIT_EXPR_SLOT. */
+ new_ctx.object = (TREE_CODE (val) == AGGR_INIT_EXPR
+ ? NULL_TREE : t);
+ new_ctx.ctor = build_constructor (elem_type, NULL);
+ ctx = &new_ctx;
t = cxx_eval_constant_expression (ctx, val, lval, non_constant_p,
overflow_p);
- if (CP_AGGREGATE_TYPE_P (elem_type) && t != ctx->ctor)
+ if (t != ctx->ctor)
free_constructor (ctx->ctor);
return t;
}
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-101371-2.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-101371-2.C
new file mode 100644
index 00000000000..fb67b67c265
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-101371-2.C
@@ -0,0 +1,23 @@
+// PR c++/101371
+// { dg-do compile { target c++14 } }
+
+struct A {
+ int i;
+};
+struct B {
+ A a{};
+ constexpr B() : a() {}
+ constexpr B(const B &rhs) : a(rhs.a) {}
+};
+struct C {
+ B arr[1];
+};
+
+constexpr C
+fn ()
+{
+ C c{};
+ return c;
+}
+
+constexpr C c = fn();
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-101371.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-101371.C
new file mode 100644
index 00000000000..b6351b806b9
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-101371.C
@@ -0,0 +1,29 @@
+// PR c++/101371
+// { dg-do compile { target c++14 } }
+
+struct A {
+ int i;
+};
+struct B {
+ A a{};
+ constexpr B() : a() {}
+ constexpr B(const B &rhs) : a(rhs.a) {}
+};
+struct C {
+ B arr[1];
+};
+
+struct X {
+ constexpr C fn () const
+ {
+ C c{};
+ return c;
+ }
+};
+
+void
+g ()
+{
+ X x;
+ constexpr auto z = x.fn();
+}
base-commit: 8695bf78dad1a42636775843ca832a2f4dba4da3
--
2.31.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] c++: constexpr array reference and value-initialization [PR101371]
2021-07-14 0:15 [PATCH] c++: constexpr array reference and value-initialization [PR101371] Marek Polacek
@ 2021-07-14 4:15 ` Jason Merrill
2021-07-14 13:56 ` Marek Polacek
0 siblings, 1 reply; 4+ messages in thread
From: Jason Merrill @ 2021-07-14 4:15 UTC (permalink / raw)
To: Marek Polacek, GCC Patches
On 7/13/21 8:15 PM, Marek Polacek wrote:
> This PR gave me a hard time: I saw multiple issues starting with
> different revisions. But ultimately the root cause seems to be
> the following, and the attached patch fixes all issues I've found
> here.
>
> In cxx_eval_array_reference we create a new constexpr context for the
> CP_AGGREGATE_TYPE_P case, but we also have to create it for the
> non-aggregate case.
But not for the scalar case, surely? Other similar places check
AGGREGATE_TYPE_P || VECTOR_TYPE_P, or !SCALAR_TYPE_P.
> In this test, we are evaluating
>
> ((B *)this)->a = rhs->a
>
> which means that we set ctx.object to ((B *)this)->a. Then we proceed
> to evaluate the initializer, rhs->a. For *rhs, we eval rhs, a PARM_DECL,
> for which we have (const B &) &c.arr[0] in the hash table. Then
> cxx_fold_indirect_ref gives us c.arr[0]. c is evaluated to {.arr={}} so
> c.arr is {}. Now we want c.arr[0], so we end up in cxx_eval_array_reference
> and since we're initializing from {}, we call build_value_init which
> gives us an AGGR_INIT_EXPR that calls 'constexpr B::B()'. Then we
> evaluate this AGGR_INIT_EXPR and since its first argument is dummy,
> we take ctx.object instead. But that is the wrong object, we're not
> initializing ((B *)this)->a here. And so we wound up with an
> initializer for A, and then crash in cxx_eval_component_reference:
>
> gcc_assert (DECL_CONTEXT (part) == TYPE_MAIN_VARIANT (TREE_TYPE (whole)));
>
> where DECL_CONTEXT (part) is B (as it should be) but the type of whole
> was A.
>
> With that in mind, the fix is straightforward, except that when the
> value-init produced an AGGR_INIT_EXPR, we shouldn't set ctx.object so
> that
>
> 2508 if (DECL_CONSTRUCTOR_P (fun) && !ctx->object
> 2509 && TREE_CODE (t) == AGGR_INIT_EXPR)
> 2510 {
> 2511 /* We want to have an initialization target for an AGGR_INIT_EXPR.
> 2512 If we don't already have one in CTX, use the AGGR_INIT_EXPR_SLOT. */
> 2513 new_ctx.object = AGGR_INIT_EXPR_SLOT (t);
>
> comes into play.
Hmm, setting new_ctx.object to t here looks like it should be the
correct c.arr[0], not ((B*)this)->a. It was wrong in the current code
because we weren't setting up new_ctx at all, but once that's fixed I
don't think you need special AGGR_INIT_EXPR handling.
> Bootstrapped/regtested on {x86_64,ppc64le,aarch64}-pc-linux-gnu, ok for trunk?
>
> PR c++/101371
>
> gcc/cp/ChangeLog:
>
> * constexpr.c (cxx_eval_array_reference): Create a new .object
> and .ctor for the non-aggregate case too when value-initializing.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/cpp1y/constexpr-101371-2.C: New test.
> * g++.dg/cpp1y/constexpr-101371.C: New test.
> ---
> gcc/cp/constexpr.c | 15 ++++++----
> .../g++.dg/cpp1y/constexpr-101371-2.C | 23 +++++++++++++++
> gcc/testsuite/g++.dg/cpp1y/constexpr-101371.C | 29 +++++++++++++++++++
> 3 files changed, 61 insertions(+), 6 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-101371-2.C
> create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-101371.C
>
> diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
> index 39787f3f5d5..584ef55703c 100644
> --- a/gcc/cp/constexpr.c
> +++ b/gcc/cp/constexpr.c
> @@ -3844,23 +3844,26 @@ cxx_eval_array_reference (const constexpr_ctx *ctx, tree t,
> initializer, it's initialized from {}. But use build_value_init
> directly for non-aggregates to avoid creating a garbage CONSTRUCTOR. */
> tree val;
> - constexpr_ctx new_ctx;
> if (is_really_empty_class (elem_type, /*ignore_vptr*/false))
> return build_constructor (elem_type, NULL);
> else if (CP_AGGREGATE_TYPE_P (elem_type))
> {
> tree empty_ctor = build_constructor (init_list_type_node, NULL);
> val = digest_init (elem_type, empty_ctor, tf_warning_or_error);
> - new_ctx = *ctx;
> - new_ctx.object = t;
> - new_ctx.ctor = build_constructor (elem_type, NULL);
> - ctx = &new_ctx;
> }
> else
> val = build_value_init (elem_type, tf_warning_or_error);
> +
> + constexpr_ctx new_ctx = *ctx;
> + /* If we are using an AGGR_INIT_EXPR, clear OBJECT for now so that
> + cxx_eval_call_expression can make use of AGGR_INIT_EXPR_SLOT. */
> + new_ctx.object = (TREE_CODE (val) == AGGR_INIT_EXPR
> + ? NULL_TREE : t);
> + new_ctx.ctor = build_constructor (elem_type, NULL);
> + ctx = &new_ctx;
> t = cxx_eval_constant_expression (ctx, val, lval, non_constant_p,
> overflow_p);
> - if (CP_AGGREGATE_TYPE_P (elem_type) && t != ctx->ctor)
> + if (t != ctx->ctor)
> free_constructor (ctx->ctor);
> return t;
> }
> diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-101371-2.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-101371-2.C
> new file mode 100644
> index 00000000000..fb67b67c265
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-101371-2.C
> @@ -0,0 +1,23 @@
> +// PR c++/101371
> +// { dg-do compile { target c++14 } }
> +
> +struct A {
> + int i;
> +};
> +struct B {
> + A a{};
> + constexpr B() : a() {}
> + constexpr B(const B &rhs) : a(rhs.a) {}
> +};
> +struct C {
> + B arr[1];
> +};
> +
> +constexpr C
> +fn ()
> +{
> + C c{};
> + return c;
> +}
> +
> +constexpr C c = fn();
> diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-101371.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-101371.C
> new file mode 100644
> index 00000000000..b6351b806b9
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-101371.C
> @@ -0,0 +1,29 @@
> +// PR c++/101371
> +// { dg-do compile { target c++14 } }
> +
> +struct A {
> + int i;
> +};
> +struct B {
> + A a{};
> + constexpr B() : a() {}
> + constexpr B(const B &rhs) : a(rhs.a) {}
> +};
> +struct C {
> + B arr[1];
> +};
> +
> +struct X {
> + constexpr C fn () const
> + {
> + C c{};
> + return c;
> + }
> +};
> +
> +void
> +g ()
> +{
> + X x;
> + constexpr auto z = x.fn();
> +}
>
> base-commit: 8695bf78dad1a42636775843ca832a2f4dba4da3
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] c++: constexpr array reference and value-initialization [PR101371]
2021-07-14 4:15 ` Jason Merrill
@ 2021-07-14 13:56 ` Marek Polacek
2021-07-14 15:14 ` Jason Merrill
0 siblings, 1 reply; 4+ messages in thread
From: Marek Polacek @ 2021-07-14 13:56 UTC (permalink / raw)
To: Jason Merrill; +Cc: GCC Patches
On Wed, Jul 14, 2021 at 12:15:48AM -0400, Jason Merrill wrote:
> On 7/13/21 8:15 PM, Marek Polacek wrote:
> > This PR gave me a hard time: I saw multiple issues starting with
> > different revisions. But ultimately the root cause seems to be
> > the following, and the attached patch fixes all issues I've found
> > here.
> >
> > In cxx_eval_array_reference we create a new constexpr context for the
> > CP_AGGREGATE_TYPE_P case, but we also have to create it for the
> > non-aggregate case.
>
> But not for the scalar case, surely? Other similar places check
> AGGREGATE_TYPE_P || VECTOR_TYPE_P, or !SCALAR_TYPE_P.
Yea, I suppose I should avoid doing any extra work for scalars.
> > In this test, we are evaluating
> >
> > ((B *)this)->a = rhs->a
> >
> > which means that we set ctx.object to ((B *)this)->a. Then we proceed
> > to evaluate the initializer, rhs->a. For *rhs, we eval rhs, a PARM_DECL,
> > for which we have (const B &) &c.arr[0] in the hash table. Then
> > cxx_fold_indirect_ref gives us c.arr[0]. c is evaluated to {.arr={}} so
> > c.arr is {}. Now we want c.arr[0], so we end up in cxx_eval_array_reference
> > and since we're initializing from {}, we call build_value_init which
> > gives us an AGGR_INIT_EXPR that calls 'constexpr B::B()'. Then we
> > evaluate this AGGR_INIT_EXPR and since its first argument is dummy,
> > we take ctx.object instead. But that is the wrong object, we're not
> > initializing ((B *)this)->a here. And so we wound up with an
> > initializer for A, and then crash in cxx_eval_component_reference:
> >
> > gcc_assert (DECL_CONTEXT (part) == TYPE_MAIN_VARIANT (TREE_TYPE (whole)));
> >
> > where DECL_CONTEXT (part) is B (as it should be) but the type of whole
> > was A.
> >
> > With that in mind, the fix is straightforward, except that when the
> > value-init produced an AGGR_INIT_EXPR, we shouldn't set ctx.object so
> > that
> >
> > 2508 if (DECL_CONSTRUCTOR_P (fun) && !ctx->object
> > 2509 && TREE_CODE (t) == AGGR_INIT_EXPR)
> > 2510 {
> > 2511 /* We want to have an initialization target for an AGGR_INIT_EXPR.
> > 2512 If we don't already have one in CTX, use the AGGR_INIT_EXPR_SLOT. */
> > 2513 new_ctx.object = AGGR_INIT_EXPR_SLOT (t);
> >
> > comes into play.
>
> Hmm, setting new_ctx.object to t here looks like it should be the correct
> c.arr[0], not ((B*)this)->a. It was wrong in the current code because we
> weren't setting up new_ctx at all, but once that's fixed I don't think you
> need special AGGR_INIT_EXPR handling.
If you don't want the special AGGR_INIT_EXPR handling, we could do something
like the following. That any better?
Full testing in progress.
-- >8 --
This PR gave me a hard time: I saw multiple issues starting with
different revisions. But ultimately the root cause seems to be
the following, and the attached patch fixes all issues I've found
here.
In cxx_eval_array_reference we create a new constexpr context for the
CP_AGGREGATE_TYPE_P case, but we also have to create it for the
non-aggregate case. In this test, we are evaluating
((B *)this)->a = rhs->a
which means that we set ctx.object to ((B *)this)->a. Then we proceed
to evaluate the initializer, rhs->a. For *rhs, we eval rhs, a PARM_DECL,
for which we have (const B &) &c.arr[0] in the hash table. Then
cxx_fold_indirect_ref gives us c.arr[0]. c is evaluated to {.arr={}} so
c.arr is {}. Now we want c.arr[0], so we end up in cxx_eval_array_reference
and since we're initializing from {}, we call build_value_init which
gives us an AGGR_INIT_EXPR that calls 'constexpr B::B()'. Then we
evaluate this AGGR_INIT_EXPR and since its first argument is dummy,
we take ctx.object instead. But that is the wrong object, we're not
initializing ((B *)this)->a here. And so we wound up with an
initializer for A, and then crash in cxx_eval_component_reference:
gcc_assert (DECL_CONTEXT (part) == TYPE_MAIN_VARIANT (TREE_TYPE (whole)));
where DECL_CONTEXT (part) is B (as it should be) but the type of whole
was A.
So create a new object, if there already was one, and the element type
is not a scalar.
PR c++/101371
gcc/cp/ChangeLog:
* constexpr.c (cxx_eval_array_reference): Create a new .object
and .ctor for the non-aggregate non-scalar case too when
value-initializing.
gcc/testsuite/ChangeLog:
* g++.dg/cpp1y/constexpr-101371-2.C: New test.
* g++.dg/cpp1y/constexpr-101371.C: New test.
---
gcc/cp/constexpr.c | 15 +++++++---
.../g++.dg/cpp1y/constexpr-101371-2.C | 23 +++++++++++++++
gcc/testsuite/g++.dg/cpp1y/constexpr-101371.C | 29 +++++++++++++++++++
3 files changed, 63 insertions(+), 4 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-101371-2.C
create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-101371.C
diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
index 39787f3f5d5..31fa5b66865 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -3851,16 +3851,23 @@ cxx_eval_array_reference (const constexpr_ctx *ctx, tree t,
{
tree empty_ctor = build_constructor (init_list_type_node, NULL);
val = digest_init (elem_type, empty_ctor, tf_warning_or_error);
+ }
+ else
+ val = build_value_init (elem_type, tf_warning_or_error);
+
+ if (!SCALAR_TYPE_P (elem_type))
+ {
new_ctx = *ctx;
- new_ctx.object = t;
+ if (ctx->object)
+ /* If there was no object, don't add one: it could confuse us
+ into thinking we're modifying a const object. */
+ new_ctx.object = t;
new_ctx.ctor = build_constructor (elem_type, NULL);
ctx = &new_ctx;
}
- else
- val = build_value_init (elem_type, tf_warning_or_error);
t = cxx_eval_constant_expression (ctx, val, lval, non_constant_p,
overflow_p);
- if (CP_AGGREGATE_TYPE_P (elem_type) && t != ctx->ctor)
+ if (!SCALAR_TYPE_P (elem_type) && t != ctx->ctor)
free_constructor (ctx->ctor);
return t;
}
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-101371-2.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-101371-2.C
new file mode 100644
index 00000000000..fb67b67c265
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-101371-2.C
@@ -0,0 +1,23 @@
+// PR c++/101371
+// { dg-do compile { target c++14 } }
+
+struct A {
+ int i;
+};
+struct B {
+ A a{};
+ constexpr B() : a() {}
+ constexpr B(const B &rhs) : a(rhs.a) {}
+};
+struct C {
+ B arr[1];
+};
+
+constexpr C
+fn ()
+{
+ C c{};
+ return c;
+}
+
+constexpr C c = fn();
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-101371.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-101371.C
new file mode 100644
index 00000000000..b6351b806b9
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-101371.C
@@ -0,0 +1,29 @@
+// PR c++/101371
+// { dg-do compile { target c++14 } }
+
+struct A {
+ int i;
+};
+struct B {
+ A a{};
+ constexpr B() : a() {}
+ constexpr B(const B &rhs) : a(rhs.a) {}
+};
+struct C {
+ B arr[1];
+};
+
+struct X {
+ constexpr C fn () const
+ {
+ C c{};
+ return c;
+ }
+};
+
+void
+g ()
+{
+ X x;
+ constexpr auto z = x.fn();
+}
base-commit: f9c2ce1dae270d8d5dc261a57a21f96a1da5ea2d
--
2.31.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] c++: constexpr array reference and value-initialization [PR101371]
2021-07-14 13:56 ` Marek Polacek
@ 2021-07-14 15:14 ` Jason Merrill
0 siblings, 0 replies; 4+ messages in thread
From: Jason Merrill @ 2021-07-14 15:14 UTC (permalink / raw)
To: Marek Polacek; +Cc: GCC Patches
On 7/14/21 9:56 AM, Marek Polacek wrote:
> On Wed, Jul 14, 2021 at 12:15:48AM -0400, Jason Merrill wrote:
>> On 7/13/21 8:15 PM, Marek Polacek wrote:
>>> This PR gave me a hard time: I saw multiple issues starting with
>>> different revisions. But ultimately the root cause seems to be
>>> the following, and the attached patch fixes all issues I've found
>>> here.
>>>
>>> In cxx_eval_array_reference we create a new constexpr context for the
>>> CP_AGGREGATE_TYPE_P case, but we also have to create it for the
>>> non-aggregate case.
>>
>> But not for the scalar case, surely? Other similar places check
>> AGGREGATE_TYPE_P || VECTOR_TYPE_P, or !SCALAR_TYPE_P.
>
> Yea, I suppose I should avoid doing any extra work for scalars.
>
>>> In this test, we are evaluating
>>>
>>> ((B *)this)->a = rhs->a
>>>
>>> which means that we set ctx.object to ((B *)this)->a. Then we proceed
>>> to evaluate the initializer, rhs->a. For *rhs, we eval rhs, a PARM_DECL,
>>> for which we have (const B &) &c.arr[0] in the hash table. Then
>>> cxx_fold_indirect_ref gives us c.arr[0]. c is evaluated to {.arr={}} so
>>> c.arr is {}. Now we want c.arr[0], so we end up in cxx_eval_array_reference
>>> and since we're initializing from {}, we call build_value_init which
>>> gives us an AGGR_INIT_EXPR that calls 'constexpr B::B()'. Then we
>>> evaluate this AGGR_INIT_EXPR and since its first argument is dummy,
>>> we take ctx.object instead. But that is the wrong object, we're not
>>> initializing ((B *)this)->a here. And so we wound up with an
>>> initializer for A, and then crash in cxx_eval_component_reference:
>>>
>>> gcc_assert (DECL_CONTEXT (part) == TYPE_MAIN_VARIANT (TREE_TYPE (whole)));
>>>
>>> where DECL_CONTEXT (part) is B (as it should be) but the type of whole
>>> was A.
>>>
>>> With that in mind, the fix is straightforward, except that when the
>>> value-init produced an AGGR_INIT_EXPR, we shouldn't set ctx.object so
>>> that
>>>
>>> 2508 if (DECL_CONSTRUCTOR_P (fun) && !ctx->object
>>> 2509 && TREE_CODE (t) == AGGR_INIT_EXPR)
>>> 2510 {
>>> 2511 /* We want to have an initialization target for an AGGR_INIT_EXPR.
>>> 2512 If we don't already have one in CTX, use the AGGR_INIT_EXPR_SLOT. */
>>> 2513 new_ctx.object = AGGR_INIT_EXPR_SLOT (t);
>>>
>>> comes into play.
>>
>> Hmm, setting new_ctx.object to t here looks like it should be the correct
>> c.arr[0], not ((B*)this)->a. It was wrong in the current code because we
>> weren't setting up new_ctx at all, but once that's fixed I don't think you
>> need special AGGR_INIT_EXPR handling.
>
> If you don't want the special AGGR_INIT_EXPR handling, we could do something
> like the following. That any better?
>
> Full testing in progress.
OK if testing succeeds.
> -- >8 --
> This PR gave me a hard time: I saw multiple issues starting with
> different revisions. But ultimately the root cause seems to be
> the following, and the attached patch fixes all issues I've found
> here.
>
> In cxx_eval_array_reference we create a new constexpr context for the
> CP_AGGREGATE_TYPE_P case, but we also have to create it for the
> non-aggregate case. In this test, we are evaluating
>
> ((B *)this)->a = rhs->a
>
> which means that we set ctx.object to ((B *)this)->a. Then we proceed
> to evaluate the initializer, rhs->a. For *rhs, we eval rhs, a PARM_DECL,
> for which we have (const B &) &c.arr[0] in the hash table. Then
> cxx_fold_indirect_ref gives us c.arr[0]. c is evaluated to {.arr={}} so
> c.arr is {}. Now we want c.arr[0], so we end up in cxx_eval_array_reference
> and since we're initializing from {}, we call build_value_init which
> gives us an AGGR_INIT_EXPR that calls 'constexpr B::B()'. Then we
> evaluate this AGGR_INIT_EXPR and since its first argument is dummy,
> we take ctx.object instead. But that is the wrong object, we're not
> initializing ((B *)this)->a here. And so we wound up with an
> initializer for A, and then crash in cxx_eval_component_reference:
>
> gcc_assert (DECL_CONTEXT (part) == TYPE_MAIN_VARIANT (TREE_TYPE (whole)));
>
> where DECL_CONTEXT (part) is B (as it should be) but the type of whole
> was A.
>
> So create a new object, if there already was one, and the element type
> is not a scalar.
>
> PR c++/101371
>
> gcc/cp/ChangeLog:
>
> * constexpr.c (cxx_eval_array_reference): Create a new .object
> and .ctor for the non-aggregate non-scalar case too when
> value-initializing.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/cpp1y/constexpr-101371-2.C: New test.
> * g++.dg/cpp1y/constexpr-101371.C: New test.
> ---
> gcc/cp/constexpr.c | 15 +++++++---
> .../g++.dg/cpp1y/constexpr-101371-2.C | 23 +++++++++++++++
> gcc/testsuite/g++.dg/cpp1y/constexpr-101371.C | 29 +++++++++++++++++++
> 3 files changed, 63 insertions(+), 4 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-101371-2.C
> create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-101371.C
>
> diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
> index 39787f3f5d5..31fa5b66865 100644
> --- a/gcc/cp/constexpr.c
> +++ b/gcc/cp/constexpr.c
> @@ -3851,16 +3851,23 @@ cxx_eval_array_reference (const constexpr_ctx *ctx, tree t,
> {
> tree empty_ctor = build_constructor (init_list_type_node, NULL);
> val = digest_init (elem_type, empty_ctor, tf_warning_or_error);
> + }
> + else
> + val = build_value_init (elem_type, tf_warning_or_error);
> +
> + if (!SCALAR_TYPE_P (elem_type))
> + {
> new_ctx = *ctx;
> - new_ctx.object = t;
> + if (ctx->object)
> + /* If there was no object, don't add one: it could confuse us
> + into thinking we're modifying a const object. */
> + new_ctx.object = t;
> new_ctx.ctor = build_constructor (elem_type, NULL);
> ctx = &new_ctx;
> }
> - else
> - val = build_value_init (elem_type, tf_warning_or_error);
> t = cxx_eval_constant_expression (ctx, val, lval, non_constant_p,
> overflow_p);
> - if (CP_AGGREGATE_TYPE_P (elem_type) && t != ctx->ctor)
> + if (!SCALAR_TYPE_P (elem_type) && t != ctx->ctor)
> free_constructor (ctx->ctor);
> return t;
> }
> diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-101371-2.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-101371-2.C
> new file mode 100644
> index 00000000000..fb67b67c265
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-101371-2.C
> @@ -0,0 +1,23 @@
> +// PR c++/101371
> +// { dg-do compile { target c++14 } }
> +
> +struct A {
> + int i;
> +};
> +struct B {
> + A a{};
> + constexpr B() : a() {}
> + constexpr B(const B &rhs) : a(rhs.a) {}
> +};
> +struct C {
> + B arr[1];
> +};
> +
> +constexpr C
> +fn ()
> +{
> + C c{};
> + return c;
> +}
> +
> +constexpr C c = fn();
> diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-101371.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-101371.C
> new file mode 100644
> index 00000000000..b6351b806b9
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-101371.C
> @@ -0,0 +1,29 @@
> +// PR c++/101371
> +// { dg-do compile { target c++14 } }
> +
> +struct A {
> + int i;
> +};
> +struct B {
> + A a{};
> + constexpr B() : a() {}
> + constexpr B(const B &rhs) : a(rhs.a) {}
> +};
> +struct C {
> + B arr[1];
> +};
> +
> +struct X {
> + constexpr C fn () const
> + {
> + C c{};
> + return c;
> + }
> +};
> +
> +void
> +g ()
> +{
> + X x;
> + constexpr auto z = x.fn();
> +}
>
> base-commit: f9c2ce1dae270d8d5dc261a57a21f96a1da5ea2d
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-07-14 15:14 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-14 0:15 [PATCH] c++: constexpr array reference and value-initialization [PR101371] Marek Polacek
2021-07-14 4:15 ` Jason Merrill
2021-07-14 13:56 ` Marek Polacek
2021-07-14 15:14 ` Jason Merrill
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).