diff mbox series

c++: cxx_eval_vec_init after zero initialization [PR96282]

Message ID 20200803124356.385746-1-ppalka@redhat.com
State New
Headers show
Series c++: cxx_eval_vec_init after zero initialization [PR96282] | expand

Commit Message

Patrick Palka Aug. 3, 2020, 12:43 p.m. UTC
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 side benefit of the approach taken by this patch is that constexpr
evaluation of a simple VEC_INIT_EXPR for a high-dimensional array no
longer scales exponentially with the number of dimensions.  This is
because after the RANGE_EXPR optimization, the CONSTRUCTOR for each
array dimension now consists of a single constructor_elt (with index
0...max-1) instead of two constructor_elts (with indexes 0 and 1...max-1
respectively).  This is verified by the second testcase below.

Bootstrapped and regtested on x86_64-pc-linux-gnu.  Does this look OK
for trunk and perhaps for backports?

gcc/cp/ChangeLog:

	PR c++/96282
	* constexpr.c (cxx_eval_vec_init_1): Move the i == 0 test to the
	if statement that guards the RANGE_EXPR optimization. Consider the
	RANGE_EXPR optimization before we append the first element.
	Truncate ctx->ctor when performing the RANGE_EXPR optimization.
	Make the built RANGE_EXPR start at index 0 instead of 1.  Don't
	call unshare_constructor.

gcc/testsuite/ChangeLog:

	PR c++/96282
	* g++.dg/cpp0x/constexpr-array26.C: New test.
	* g++.dg/cpp0x/constexpr-array27.C: New test.
---
 gcc/cp/constexpr.c                            | 36 ++++++++++---------
 .../g++.dg/cpp0x/constexpr-array26.C          | 13 +++++++
 .../g++.dg/cpp0x/constexpr-array27.C          | 18 ++++++++++
 3 files changed, 51 insertions(+), 16 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-array26.C
 create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-array27.C

Comments

Patrick Palka Aug. 3, 2020, 12:53 p.m. UTC | #1
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?

Another complete fix would be to replace the uses of CONSTRUCTOR_APPEND_ELT
with get_or_insert_ctor_field, which does the right thing when a
matching constructor_elt already exists.  But get_or_insert_ctor_field
was introduced only in GCC 10, .

I'm not sure which fix we should go with in light of this being an
8/9/10/11 regression..

> 
> A side benefit of the approach taken by this patch is that constexpr
> evaluation of a simple VEC_INIT_EXPR for a high-dimensional array no
> longer scales exponentially with the number of dimensions.  This is
> because after the RANGE_EXPR optimization, the CONSTRUCTOR for each
> array dimension now consists of a single constructor_elt (with index
> 0...max-1) instead of two constructor_elts (with indexes 0 and 1...max-1
> respectively).  This is verified by the second testcase below.
> 
> Bootstrapped and regtested on x86_64-pc-linux-gnu.  Does this look OK
> for trunk and perhaps for backports?
> 
> gcc/cp/ChangeLog:
> 
> 	PR c++/96282
> 	* constexpr.c (cxx_eval_vec_init_1): Move the i == 0 test to the
> 	if statement that guards the RANGE_EXPR optimization. Consider the
> 	RANGE_EXPR optimization before we append the first element.
> 	Truncate ctx->ctor when performing the RANGE_EXPR optimization.
> 	Make the built RANGE_EXPR start at index 0 instead of 1.  Don't
> 	call unshare_constructor.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR c++/96282
> 	* g++.dg/cpp0x/constexpr-array26.C: New test.
> 	* g++.dg/cpp0x/constexpr-array27.C: New test.
> ---
>  gcc/cp/constexpr.c                            | 36 ++++++++++---------
>  .../g++.dg/cpp0x/constexpr-array26.C          | 13 +++++++
>  .../g++.dg/cpp0x/constexpr-array27.C          | 18 ++++++++++
>  3 files changed, 51 insertions(+), 16 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-array26.C
>  create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-array27.C
> 
> diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
> index b1c1d249c6e..3808a0713ba 100644
> --- a/gcc/cp/constexpr.c
> +++ b/gcc/cp/constexpr.c
> @@ -4189,7 +4189,7 @@ cxx_eval_vec_init_1 (const constexpr_ctx *ctx, tree atype, tree init,
>  	  if (value_init || init == NULL_TREE)
>  	    {
>  	      eltinit = NULL_TREE;
> -	      reuse = i == 0;
> +	      reuse = true;
>  	    }
>  	  else
>  	    eltinit = cp_build_array_ref (input_location, init, idx, complain);
> @@ -4206,7 +4206,7 @@ cxx_eval_vec_init_1 (const constexpr_ctx *ctx, tree atype, tree init,
>  	    return ctx->ctor;
>  	  eltinit = cxx_eval_constant_expression (&new_ctx, init, lval,
>  						  non_constant_p, overflow_p);
> -	  reuse = i == 0;
> +	  reuse = true;
>  	}
>        else
>  	{
> @@ -4222,33 +4222,37 @@ cxx_eval_vec_init_1 (const constexpr_ctx *ctx, tree atype, tree init,
>  	}
>        if (*non_constant_p && !ctx->quiet)
>  	break;
> -      if (new_ctx.ctor != ctx->ctor)
> -	{
> -	  /* We appended this element above; update the value.  */
> -	  gcc_assert ((*p)->last().index == idx);
> -	  (*p)->last().value = eltinit;
> -	}
> -      else
> -	CONSTRUCTOR_APPEND_ELT (*p, idx, eltinit);
> +
>        /* Reuse the result of cxx_eval_constant_expression call
>  	 from the first iteration to all others if it is a constant
>  	 initializer that doesn't require relocations.  */
> -      if (reuse
> -	  && max > 1
> +      if (i == 0
> +	  && reuse
>  	  && (eltinit == NULL_TREE
>  	      || (initializer_constant_valid_p (eltinit, TREE_TYPE (eltinit))
>  		  == null_pointer_node)))
>  	{
>  	  if (new_ctx.ctor != ctx->ctor)
>  	    eltinit = new_ctx.ctor;
> -	  tree range = build2 (RANGE_EXPR, size_type_node,
> -			       build_int_cst (size_type_node, 1),
> -			       build_int_cst (size_type_node, max - 1));
> -	  CONSTRUCTOR_APPEND_ELT (*p, range, unshare_constructor (eltinit));
> +	  vec_safe_truncate (*p, 0);
> +	  if (max > 1)
> +	    idx = build2 (RANGE_EXPR, size_type_node,
> +			  build_int_cst (size_type_node, 0),
> +			  build_int_cst (size_type_node, max - 1));
> +	  CONSTRUCTOR_APPEND_ELT (*p, idx, eltinit);
>  	  break;
>  	}
>        else if (i == 0)
>  	vec_safe_reserve (*p, max);
> +
> +      if (new_ctx.ctor != ctx->ctor)
> +	{
> +	  /* We appended this element above; update the value.  */
> +	  gcc_assert ((*p)->last().index == idx);
> +	  (*p)->last().value = eltinit;
> +	}
> +      else
> +	CONSTRUCTOR_APPEND_ELT (*p, idx, eltinit);
>      }
>  
>    if (!*non_constant_p)
> 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..a5ce3f7be08
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-array27.C
> @@ -0,0 +1,18 @@
> +// Verify that default initialization an array of aggregates within an aggregate
> +// does not scale exponentially with the number of dimensions.
> +
> +// { dg-do compile { target c++11 } }
> +// Pass -fsyntax-only to stress only the performance of the frontend.
> +// { dg-additional-options "-fsyntax-only" }
> +
> +struct A
> +{
> +  int a = 42;
> +};
> +
> +struct B
> +{
> +  A b[2][2][2][2][2][2][2][2][2][2][2][2][2][2][2][2][2][2][2][2][2][2][2][2];
> +};
> +
> +constexpr B c;
> -- 
> 2.28.0.89.g85b4e0a6dc
> 
>
Jason Merrill Aug. 3, 2020, 3:35 p.m. UTC | #2
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?

We could probably still do the truncation, but clear the 
CONSTRUCTOR_NO_CLEARING flag on the element initializer.

> Another complete fix would be to replace the uses of CONSTRUCTOR_APPEND_ELT
> with get_or_insert_ctor_field, which does the right thing when a
> matching constructor_elt already exists.  But get_or_insert_ctor_field
> was introduced only in GCC 10, .
> 
> I'm not sure which fix we should go with in light of this being an
> 8/9/10/11 regression..
> 
>>
>> A side benefit of the approach taken by this patch is that constexpr
>> evaluation of a simple VEC_INIT_EXPR for a high-dimensional array no
>> longer scales exponentially with the number of dimensions.  This is
>> because after the RANGE_EXPR optimization, the CONSTRUCTOR for each
>> array dimension now consists of a single constructor_elt (with index
>> 0...max-1) instead of two constructor_elts (with indexes 0 and 1...max-1
>> respectively).  This is verified by the second testcase below.
>>
>> Bootstrapped and regtested on x86_64-pc-linux-gnu.  Does this look OK
>> for trunk and perhaps for backports?
>>
>> gcc/cp/ChangeLog:
>>
>> 	PR c++/96282
>> 	* constexpr.c (cxx_eval_vec_init_1): Move the i == 0 test to the
>> 	if statement that guards the RANGE_EXPR optimization. Consider the
>> 	RANGE_EXPR optimization before we append the first element.
>> 	Truncate ctx->ctor when performing the RANGE_EXPR optimization.
>> 	Make the built RANGE_EXPR start at index 0 instead of 1.  Don't
>> 	call unshare_constructor.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	PR c++/96282
>> 	* g++.dg/cpp0x/constexpr-array26.C: New test.
>> 	* g++.dg/cpp0x/constexpr-array27.C: New test.
>> ---
>>   gcc/cp/constexpr.c                            | 36 ++++++++++---------
>>   .../g++.dg/cpp0x/constexpr-array26.C          | 13 +++++++
>>   .../g++.dg/cpp0x/constexpr-array27.C          | 18 ++++++++++
>>   3 files changed, 51 insertions(+), 16 deletions(-)
>>   create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-array26.C
>>   create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-array27.C
>>
>> diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
>> index b1c1d249c6e..3808a0713ba 100644
>> --- a/gcc/cp/constexpr.c
>> +++ b/gcc/cp/constexpr.c
>> @@ -4189,7 +4189,7 @@ cxx_eval_vec_init_1 (const constexpr_ctx *ctx, tree atype, tree init,
>>   	  if (value_init || init == NULL_TREE)
>>   	    {
>>   	      eltinit = NULL_TREE;
>> -	      reuse = i == 0;
>> +	      reuse = true;
>>   	    }
>>   	  else
>>   	    eltinit = cp_build_array_ref (input_location, init, idx, complain);
>> @@ -4206,7 +4206,7 @@ cxx_eval_vec_init_1 (const constexpr_ctx *ctx, tree atype, tree init,
>>   	    return ctx->ctor;
>>   	  eltinit = cxx_eval_constant_expression (&new_ctx, init, lval,
>>   						  non_constant_p, overflow_p);
>> -	  reuse = i == 0;
>> +	  reuse = true;
>>   	}
>>         else
>>   	{
>> @@ -4222,33 +4222,37 @@ cxx_eval_vec_init_1 (const constexpr_ctx *ctx, tree atype, tree init,
>>   	}
>>         if (*non_constant_p && !ctx->quiet)
>>   	break;
>> -      if (new_ctx.ctor != ctx->ctor)
>> -	{
>> -	  /* We appended this element above; update the value.  */
>> -	  gcc_assert ((*p)->last().index == idx);
>> -	  (*p)->last().value = eltinit;
>> -	}
>> -      else
>> -	CONSTRUCTOR_APPEND_ELT (*p, idx, eltinit);
>> +
>>         /* Reuse the result of cxx_eval_constant_expression call
>>   	 from the first iteration to all others if it is a constant
>>   	 initializer that doesn't require relocations.  */
>> -      if (reuse
>> -	  && max > 1
>> +      if (i == 0
>> +	  && reuse
>>   	  && (eltinit == NULL_TREE
>>   	      || (initializer_constant_valid_p (eltinit, TREE_TYPE (eltinit))
>>   		  == null_pointer_node)))
>>   	{
>>   	  if (new_ctx.ctor != ctx->ctor)
>>   	    eltinit = new_ctx.ctor;
>> -	  tree range = build2 (RANGE_EXPR, size_type_node,
>> -			       build_int_cst (size_type_node, 1),
>> -			       build_int_cst (size_type_node, max - 1));
>> -	  CONSTRUCTOR_APPEND_ELT (*p, range, unshare_constructor (eltinit));
>> +	  vec_safe_truncate (*p, 0);
>> +	  if (max > 1)
>> +	    idx = build2 (RANGE_EXPR, size_type_node,
>> +			  build_int_cst (size_type_node, 0),
>> +			  build_int_cst (size_type_node, max - 1));
>> +	  CONSTRUCTOR_APPEND_ELT (*p, idx, eltinit);
>>   	  break;
>>   	}
>>         else if (i == 0)
>>   	vec_safe_reserve (*p, max);
>> +
>> +      if (new_ctx.ctor != ctx->ctor)
>> +	{
>> +	  /* We appended this element above; update the value.  */
>> +	  gcc_assert ((*p)->last().index == idx);
>> +	  (*p)->last().value = eltinit;
>> +	}
>> +      else
>> +	CONSTRUCTOR_APPEND_ELT (*p, idx, eltinit);
>>       }
>>   
>>     if (!*non_constant_p)
>> 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..a5ce3f7be08
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-array27.C
>> @@ -0,0 +1,18 @@
>> +// Verify that default initialization an array of aggregates within an aggregate
>> +// does not scale exponentially with the number of dimensions.
>> +
>> +// { dg-do compile { target c++11 } }
>> +// Pass -fsyntax-only to stress only the performance of the frontend.
>> +// { dg-additional-options "-fsyntax-only" }
>> +
>> +struct A
>> +{
>> +  int a = 42;
>> +};
>> +
>> +struct B
>> +{
>> +  A b[2][2][2][2][2][2][2][2][2][2][2][2][2][2][2][2][2][2][2][2][2][2][2][2];
>> +};
>> +
>> +constexpr B c;
>> -- 
>> 2.28.0.89.g85b4e0a6dc
>>
>>
>
Patrick Palka Aug. 3, 2020, 6:45 p.m. UTC | #3
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))
+    {
+      /* We're default-initializing an array object that had been
+	 zero-initialized earlier.  We'll preserve this prior state
+	 by clearing CONSTRUCTOR_NO_CLEARING on each of its element
+	 initializers.  */
+      zero_initialized_p = 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 +4193,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 (zero_initialized_p)
+	    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..bd5dd58d1ab
--- /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]; };
+
+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..47c15edfc73
--- /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]; };
+
+template<int N>
+struct t : b<N> { constexpr t() : b<N>() {} };
+
+constexpr t<1> h1;
+static_assert(h1.m[0].w == false);
+
+constexpr t<42> h2;
+static_assert(h2.m[17].w == false);
Jason Merrill Aug. 3, 2020, 7:48 p.m. UTC | #4
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?

This all seems parallel to the no_zero_init code in 
cxx_eval_store_expression.

> +    {
> +      /* We're default-initializing an array object that had been
> +	 zero-initialized earlier.  We'll preserve this prior state
> +	 by clearing CONSTRUCTOR_NO_CLEARING on each of its element
> +	 initializers.  */
> +      zero_initialized_p = 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 +4193,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 (zero_initialized_p)
> +	    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..bd5dd58d1ab
> --- /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]; };
> +
> +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..47c15edfc73
> --- /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]; };
> +
> +template<int N>
> +struct t : b<N> { constexpr t() : b<N>() {} };
> +
> +constexpr t<1> h1;
> +static_assert(h1.m[0].w == false);
> +
> +constexpr t<42> h2;
> +static_assert(h2.m[17].w == false);
>
Patrick Palka Aug. 3, 2020, 9:06 p.m. UTC | #5
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.

> 
> 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);
Patrick Palka Aug. 4, 2020, 1:45 p.m. UTC | #6
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.

> 
> > 
> > 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
> 
>
Jason Merrill Aug. 4, 2020, 5:28 p.m. UTC | #7
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
>>
>>
>
Patrick Palka Aug. 4, 2020, 6:31 p.m. UTC | #8
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));
+      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);
Jason Merrill Aug. 5, 2020, 6:08 p.m. UTC | #9
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);
>
diff mbox series

Patch

diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
index b1c1d249c6e..3808a0713ba 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -4189,7 +4189,7 @@  cxx_eval_vec_init_1 (const constexpr_ctx *ctx, tree atype, tree init,
 	  if (value_init || init == NULL_TREE)
 	    {
 	      eltinit = NULL_TREE;
-	      reuse = i == 0;
+	      reuse = true;
 	    }
 	  else
 	    eltinit = cp_build_array_ref (input_location, init, idx, complain);
@@ -4206,7 +4206,7 @@  cxx_eval_vec_init_1 (const constexpr_ctx *ctx, tree atype, tree init,
 	    return ctx->ctor;
 	  eltinit = cxx_eval_constant_expression (&new_ctx, init, lval,
 						  non_constant_p, overflow_p);
-	  reuse = i == 0;
+	  reuse = true;
 	}
       else
 	{
@@ -4222,33 +4222,37 @@  cxx_eval_vec_init_1 (const constexpr_ctx *ctx, tree atype, tree init,
 	}
       if (*non_constant_p && !ctx->quiet)
 	break;
-      if (new_ctx.ctor != ctx->ctor)
-	{
-	  /* We appended this element above; update the value.  */
-	  gcc_assert ((*p)->last().index == idx);
-	  (*p)->last().value = eltinit;
-	}
-      else
-	CONSTRUCTOR_APPEND_ELT (*p, idx, eltinit);
+
       /* Reuse the result of cxx_eval_constant_expression call
 	 from the first iteration to all others if it is a constant
 	 initializer that doesn't require relocations.  */
-      if (reuse
-	  && max > 1
+      if (i == 0
+	  && reuse
 	  && (eltinit == NULL_TREE
 	      || (initializer_constant_valid_p (eltinit, TREE_TYPE (eltinit))
 		  == null_pointer_node)))
 	{
 	  if (new_ctx.ctor != ctx->ctor)
 	    eltinit = new_ctx.ctor;
-	  tree range = build2 (RANGE_EXPR, size_type_node,
-			       build_int_cst (size_type_node, 1),
-			       build_int_cst (size_type_node, max - 1));
-	  CONSTRUCTOR_APPEND_ELT (*p, range, unshare_constructor (eltinit));
+	  vec_safe_truncate (*p, 0);
+	  if (max > 1)
+	    idx = build2 (RANGE_EXPR, size_type_node,
+			  build_int_cst (size_type_node, 0),
+			  build_int_cst (size_type_node, max - 1));
+	  CONSTRUCTOR_APPEND_ELT (*p, idx, eltinit);
 	  break;
 	}
       else if (i == 0)
 	vec_safe_reserve (*p, max);
+
+      if (new_ctx.ctor != ctx->ctor)
+	{
+	  /* We appended this element above; update the value.  */
+	  gcc_assert ((*p)->last().index == idx);
+	  (*p)->last().value = eltinit;
+	}
+      else
+	CONSTRUCTOR_APPEND_ELT (*p, idx, eltinit);
     }
 
   if (!*non_constant_p)
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..a5ce3f7be08
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-array27.C
@@ -0,0 +1,18 @@ 
+// Verify that default initialization an array of aggregates within an aggregate
+// does not scale exponentially with the number of dimensions.
+
+// { dg-do compile { target c++11 } }
+// Pass -fsyntax-only to stress only the performance of the frontend.
+// { dg-additional-options "-fsyntax-only" }
+
+struct A
+{
+  int a = 42;
+};
+
+struct B
+{
+  A b[2][2][2][2][2][2][2][2][2][2][2][2][2][2][2][2][2][2][2][2][2][2][2][2];
+};
+
+constexpr B c;