Re: [PATCH] c++: constexpr array reference and value-initialization [PR101371]
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, t
Re: [PATCH] c++: constexpr array reference and value-initialization [PR101371]
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/constex
Re: [PATCH] c++: constexpr array reference and value-initialization [PR101371]
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 000..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() {} + constexp
[PATCH] c++: constexpr array reference and value-initialization [PR101371]
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 000..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 000..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 { + i