[PATCH] c++: cxx_eval_vec_init after zero initialization [PR96282]

Jason Merrill jason@redhat.com
Wed Aug 5 18:08:01 GMT 2020


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);
> 



More information about the Gcc-patches mailing list