avoid user-constructible types in reshape_init_array (PR 90938)

r270155, committed in GCC 9, introduced a transformation that strips
redundant trailing zero initializers from array initializer lists in
order to support string literals as template arguments.

The transformation neglected to consider the case of array elements
of trivial class types with user-defined conversion ctors and either
defaulted or deleted default ctors.  (It didn't occur to me that
those qualify as trivial types despite the user-defined ctors.)  As
a result, some valid initialization expressions are rejected when
the explicit zero-initializers are dropped in favor of the (deleted)
default ctor, and others are eliminated in favor of the defaulted
ctor instead of invoking a user-defined conversion ctor, leading to
wrong code.

The attached patch fixes that but avoiding this transformation for
such types.

Tested on x86_64-linux.  I'd like to commit the patch to both trunk
and to GCC 9 (with testsuite adjustments if necessary).

Martin

On Tue, Feb 11, 2020 at 01:00:05PM -0700, Martin Sebor wrote:
> r270155, committed in GCC 9, introduced a transformation that strips
> redundant trailing zero initializers from array initializer lists in
> order to support string literals as template arguments.
> 
> The transformation neglected to consider the case of array elements
> of trivial class types with user-defined conversion ctors and either
> defaulted or deleted default ctors.  (It didn't occur to me that
> those qualify as trivial types despite the user-defined ctors.)  As
> a result, some valid initialization expressions are rejected when
> the explicit zero-initializers are dropped in favor of the (deleted)
> default ctor, and others are eliminated in favor of the defaulted
> ctor instead of invoking a user-defined conversion ctor, leading to
> wrong code.
> 
> The attached patch fixes that but avoiding this transformation for
> such types.
> 
> Tested on x86_64-linux.  I'd like to commit the patch to both trunk
> and to GCC 9 (with testsuite adjustments if necessary).
> 
> Martin

> PR c++/90938 - Initializing array with {1} works but not {0}
> 
> gcc/cp/ChangeLog:
> 
> 	PR c++/90938
> 	* decl.c (reshape_init_array_1): Avoid types with non-trivial
> 	user-defined ctors.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR c++/90938
> 	* g++.dg/init/array55.C: New test.
> 	* g++.dg/init/array56.C: New test.
> 
> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
> index 31a556a0a1f..60731cb3f9d 100644
> --- a/gcc/cp/decl.c
> +++ b/gcc/cp/decl.c
> @@ -6051,11 +6051,14 @@ reshape_init_array_1 (tree elt_type, tree max_index, reshape_iter *d,
>  	break;
>      }
>  
> -  if (sized_array_p && trivial_type_p (elt_type))
> +  if (sized_array_p
> +      && trivial_type_p (elt_type)
> +      && !TYPE_NEEDS_CONSTRUCTING (elt_type))

Looks like this will still do the wrong thing for

struct X
{
  X () = delete;
  X (int) = delete;
};

X x1[1] { 0 }; // use of deleted function

which should be rejected since we use a deleted function, but
TYPE_NEEDS_CONSTRUCTING will be 0 fox X, so we'd do the truncation
and the initialization would succeed.

Marek

On 2/11/20 9:00 PM, Martin Sebor wrote:
> r270155, committed in GCC 9, introduced a transformation that strips
> redundant trailing zero initializers from array initializer lists in
> order to support string literals as template arguments.
> 
> The transformation neglected to consider the case of array elements
> of trivial class types with user-defined conversion ctors and either
> defaulted or deleted default ctors.  (It didn't occur to me that
> those qualify as trivial types despite the user-defined ctors.)  As
> a result, some valid initialization expressions are rejected when
> the explicit zero-initializers are dropped in favor of the (deleted)
> default ctor,

Hmm, a type with only a deleted default constructor is not trivial, that 
should have been OK already.

> and others are eliminated in favor of the defaulted
> ctor instead of invoking a user-defined conversion ctor, leading to
> wrong code.

This seems like a bug in type_initializer_zero_p; it shouldn't treat 0 
as a zero initializer for any class.

Jason

On 2/11/20 5:01 PM, Marek Polacek wrote:
> On Tue, Feb 11, 2020 at 01:00:05PM -0700, Martin Sebor wrote:
>> r270155, committed in GCC 9, introduced a transformation that strips
>> redundant trailing zero initializers from array initializer lists in
>> order to support string literals as template arguments.
>>
>> The transformation neglected to consider the case of array elements
>> of trivial class types with user-defined conversion ctors and either
>> defaulted or deleted default ctors.  (It didn't occur to me that
>> those qualify as trivial types despite the user-defined ctors.)  As
>> a result, some valid initialization expressions are rejected when
>> the explicit zero-initializers are dropped in favor of the (deleted)
>> default ctor, and others are eliminated in favor of the defaulted
>> ctor instead of invoking a user-defined conversion ctor, leading to
>> wrong code.
>>
>> The attached patch fixes that but avoiding this transformation for
>> such types.
>>
>> Tested on x86_64-linux.  I'd like to commit the patch to both trunk
>> and to GCC 9 (with testsuite adjustments if necessary).
>>
>> Martin
> 
>> PR c++/90938 - Initializing array with {1} works but not {0}
>>
>> gcc/cp/ChangeLog:
>>
>> 	PR c++/90938
>> 	* decl.c (reshape_init_array_1): Avoid types with non-trivial
>> 	user-defined ctors.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	PR c++/90938
>> 	* g++.dg/init/array55.C: New test.
>> 	* g++.dg/init/array56.C: New test.
>>
>> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
>> index 31a556a0a1f..60731cb3f9d 100644
>> --- a/gcc/cp/decl.c
>> +++ b/gcc/cp/decl.c
>> @@ -6051,11 +6051,14 @@ reshape_init_array_1 (tree elt_type, tree max_index, reshape_iter *d,
>>   	break;
>>       }
>>   
>> -  if (sized_array_p && trivial_type_p (elt_type))
>> +  if (sized_array_p
>> +      && trivial_type_p (elt_type)
>> +      && !TYPE_NEEDS_CONSTRUCTING (elt_type))
> 
> Looks like this will still do the wrong thing for
> 
> struct X
> {
>    X () = delete;
>    X (int) = delete;
> };
> 
> X x1[1] { 0 }; // use of deleted function
> 
> which should be rejected since we use a deleted function, but
> TYPE_NEEDS_CONSTRUCTING will be 0 fox X, so we'd do the truncation
> and the initialization would succeed.

And conversely, this should be accepted but isn't, with or without
the patch:

   struct X
   {
     X () = delete;
     X (int) = delete;
     X (long) { }
   };

   static_assert (__is_trivial (X));

   X x1[1] { 0L }; // okay

but this rejected:

   X x2[1] { 0 };  // error: use of deleted X::X(int)

It seems like each initializer in the list needs to be matched against
the right ctor one by one and the transformation only done if none of
them is deleted or non-trivial.

Martin

On 2/11/20 5:28 PM, Jason Merrill wrote:
> On 2/11/20 9:00 PM, Martin Sebor wrote:
>> r270155, committed in GCC 9, introduced a transformation that strips
>> redundant trailing zero initializers from array initializer lists in
>> order to support string literals as template arguments.
>>
>> The transformation neglected to consider the case of array elements
>> of trivial class types with user-defined conversion ctors and either
>> defaulted or deleted default ctors.  (It didn't occur to me that
>> those qualify as trivial types despite the user-defined ctors.)  As
>> a result, some valid initialization expressions are rejected when
>> the explicit zero-initializers are dropped in favor of the (deleted)
>> default ctor,
> 
> Hmm, a type with only a deleted default constructor is not trivial, that 
> should have been OK already.

For Marek's test case:
   struct A { A () == delete; A (int) = delete; };

trivial_type_p() returns true (as does __is_trivial (A) in both GCC
and Clang).

[class.prop] says that

   A trivial class is a class that is trivially copyable and has one
   or more default constructors (10.3.4.1), all of which are either
   trivial or deleted and at least one of which is not deleted.

That sounds like A above is not trivial because it doesn't have
at least one default ctor that's not deleted, but both GCC and
Clang say it is.  What am I missing?  Is there some other default
constructor hiding in there that I don't know about?

>> and others are eliminated in favor of the defaulted
>> ctor instead of invoking a user-defined conversion ctor, leading to
>> wrong code.
> 
> This seems like a bug in type_initializer_zero_p; it shouldn't treat 0 
> as a zero initializer for any class.

That does fix it, and it seems like the right solution to me as well.
Thanks for the suggestion.  I'm a little unsure about the condition
I put in place though.

Attached is an updated patch rested on x86_64-linux.

Martin

On Wed, Feb 12, 2020 at 01:21:58PM -0700, Martin Sebor wrote:
> On 2/11/20 5:28 PM, Jason Merrill wrote:
> > On 2/11/20 9:00 PM, Martin Sebor wrote:
> > > r270155, committed in GCC 9, introduced a transformation that strips
> > > redundant trailing zero initializers from array initializer lists in
> > > order to support string literals as template arguments.
> > > 
> > > The transformation neglected to consider the case of array elements
> > > of trivial class types with user-defined conversion ctors and either
> > > defaulted or deleted default ctors.  (It didn't occur to me that
> > > those qualify as trivial types despite the user-defined ctors.)  As
> > > a result, some valid initialization expressions are rejected when
> > > the explicit zero-initializers are dropped in favor of the (deleted)
> > > default ctor,
> > 
> > Hmm, a type with only a deleted default constructor is not trivial, that
> > should have been OK already.
> 
> For Marek's test case:
>   struct A { A () == delete; A (int) = delete; };
> 
> trivial_type_p() returns true (as does __is_trivial (A) in both GCC
> and Clang).
> 
> [class.prop] says that
> 
>   A trivial class is a class that is trivially copyable and has one
>   or more default constructors (10.3.4.1), all of which are either
>   trivial or deleted and at least one of which is not deleted.
> 
> That sounds like A above is not trivial because it doesn't have
> at least one default ctor that's not deleted, but both GCC and
> Clang say it is.  What am I missing?  Is there some other default
> constructor hiding in there that I don't know about?

Note that [class.prop]/2 now says something other than that:

"A trivial class is a class that is trivially copyable and has one or
more eligible default constructors ([class.default.ctor]), all of which
are trivial."
I think this changed in P0848R3 (Conditionally Trivial Special Member
Functions).

Here A has a default constructor, but it's not eligible: [special]/6 says
"An eligible special member function is a special member function for which:
 -- the function is not deleted,
[...]"

So it seems that A should not be trivial.  But the wording you quoted
also means that: it was changed in
CWG1496 Triviality with deleted and missing default constructors
but we don't implement that (https://gcc.gnu.org/PR85723).

So going further down memory lane, [class.prop] used to say
"A trivial class is a class that has a default constructor (12.1), has no
non-trivial default constructors, and is trivially copyable."
which A is (because it has an implicit copy ctor).

So I think it all comes down to the fact that neither g++ not clang++
implement CWG 1496.  And that's probably GCC 11 work.

Marek

On 2/12/20 9:21 PM, Martin Sebor wrote:
> On 2/11/20 5:28 PM, Jason Merrill wrote:
>> On 2/11/20 9:00 PM, Martin Sebor wrote:
>>> r270155, committed in GCC 9, introduced a transformation that strips
>>> redundant trailing zero initializers from array initializer lists in
>>> order to support string literals as template arguments.
>>>
>>> The transformation neglected to consider the case of array elements
>>> of trivial class types with user-defined conversion ctors and either
>>> defaulted or deleted default ctors.  (It didn't occur to me that
>>> those qualify as trivial types despite the user-defined ctors.)  As
>>> a result, some valid initialization expressions are rejected when
>>> the explicit zero-initializers are dropped in favor of the (deleted)
>>> default ctor,
>>
>> Hmm, a type with only a deleted default constructor is not trivial, 
>> that should have been OK already.
> 
> For Marek's test case:
>    struct A { A () == delete; A (int) = delete; };
> 
> trivial_type_p() returns true (as does __is_trivial (A) in both GCC
> and Clang).
> 
> [class.prop] says that
> 
>    A trivial class is a class that is trivially copyable and has one
>    or more default constructors (10.3.4.1), all of which are either
>    trivial or deleted and at least one of which is not deleted.
> 
> That sounds like A above is not trivial because it doesn't have
> at least one default ctor that's not deleted, but both GCC and
> Clang say it is.  What am I missing?  Is there some other default
> constructor hiding in there that I don't know about?
> 
>>> and others are eliminated in favor of the defaulted
>>> ctor instead of invoking a user-defined conversion ctor, leading to
>>> wrong code.
>>
>> This seems like a bug in type_initializer_zero_p; it shouldn't treat 0 
>> as a zero initializer for any class.
> 
> That does fix it, and it seems like the right solution to me as well.
> Thanks for the suggestion.  I'm a little unsure about the condition
> I put in place though.
> 
> Attached is an updated patch rested on x86_64-linux.

> -  if (sized_array_p && trivial_type_p (elt_type))
> +  if (sized_array_p
> +      && trivial_type_p (elt_type)
> +      && !TYPE_NEEDS_CONSTRUCTING (elt_type))

Do we still need this change?  If so, please add a comment about the 
trivial_type_p bug.

>    if (TREE_CODE (init) != CONSTRUCTOR
I might change this to

  if (!CP_AGGREGATE_TYPE_P (type))
    return initializer_zerop (init);
  else if (TREE_CODE (init) != CONSTRUCTOR)
    return false;

and then remove the

>   if (TYPE_NON_AGGREGATE_CLASS (type))
>     return false;

later in the function.

More generally, this function could recognize when the initializer is 
equivalent to {}-initialization and return true in that case, but that 
sounds probably too tricky for stage 4.

Jason

On 2/13/20 3:59 PM, Jason Merrill wrote:
> On 2/12/20 9:21 PM, Martin Sebor wrote:
>> On 2/11/20 5:28 PM, Jason Merrill wrote:
>>> On 2/11/20 9:00 PM, Martin Sebor wrote:
>>>> r270155, committed in GCC 9, introduced a transformation that strips
>>>> redundant trailing zero initializers from array initializer lists in
>>>> order to support string literals as template arguments.
>>>>
>>>> The transformation neglected to consider the case of array elements
>>>> of trivial class types with user-defined conversion ctors and either
>>>> defaulted or deleted default ctors.  (It didn't occur to me that
>>>> those qualify as trivial types despite the user-defined ctors.)  As
>>>> a result, some valid initialization expressions are rejected when
>>>> the explicit zero-initializers are dropped in favor of the (deleted)
>>>> default ctor,
>>>
>>> Hmm, a type with only a deleted default constructor is not trivial, 
>>> that should have been OK already.
>>
>> For Marek's test case:
>>    struct A { A () == delete; A (int) = delete; };
>>
>> trivial_type_p() returns true (as does __is_trivial (A) in both GCC
>> and Clang).
>>
>> [class.prop] says that
>>
>>    A trivial class is a class that is trivially copyable and has one
>>    or more default constructors (10.3.4.1), all of which are either
>>    trivial or deleted and at least one of which is not deleted.
>>
>> That sounds like A above is not trivial because it doesn't have
>> at least one default ctor that's not deleted, but both GCC and
>> Clang say it is.  What am I missing?  Is there some other default
>> constructor hiding in there that I don't know about?
>>
>>>> and others are eliminated in favor of the defaulted
>>>> ctor instead of invoking a user-defined conversion ctor, leading to
>>>> wrong code.
>>>
>>> This seems like a bug in type_initializer_zero_p; it shouldn't treat 
>>> 0 as a zero initializer for any class.
>>
>> That does fix it, and it seems like the right solution to me as well.
>> Thanks for the suggestion.  I'm a little unsure about the condition
>> I put in place though.
>>
>> Attached is an updated patch rested on x86_64-linux.
> 
>> -  if (sized_array_p && trivial_type_p (elt_type))
>> +  if (sized_array_p
>> +      && trivial_type_p (elt_type)
>> +      && !TYPE_NEEDS_CONSTRUCTING (elt_type))
> 
> Do we still need this change?  If so, please add a comment about the 
> trivial_type_p bug.

The change isn't needed with my patch as it was, but it would still
be needed with the changes you suggested (even then it doesn't help
with the problem I describe below).

> 
>>    if (TREE_CODE (init) != CONSTRUCTOR
> I might change this to
> 
>   if (!CP_AGGREGATE_TYPE_P (type))
>     return initializer_zerop (init);

This behaves differently in C++ 2a mode (the whole condition evaluates
to true for class A below) than in earlier modes and causes a failure
in the new array55.C test:

   struct A
   {
     A () = delete;
     A (int) = delete;
   };

   A a1[1] = { 0 };  // { dg-error "use of deleted function 
'A::A\\\(int\\\)'" }

because GCC ends up printing:

   error: use of deleted function 'A::A()'

This is because A is considered trivial and TYPE_NEEDS_CONSTRUCTING
is false.  IIUC what Marek pointed to (CWG1496), A should not be
considered trivial but GCC doesn't implement that change in
the standard, hence the failure.

>   else if (TREE_CODE (init) != CONSTRUCTOR)
>     return false;
> 
> and then remove the
> 
>>   if (TYPE_NON_AGGREGATE_CLASS (type))
>>     return false;
> 
> later in the function.
> 
> More generally, this function could recognize when the initializer is 
> equivalent to {}-initialization and return true in that case, but that 
> sounds probably too tricky for stage 4.

My preference is to leave the fix as it is (i.e., leave the test
for RECORD_OR_UNION_TYPE_P in place), just with
the TYPE_NEEDS_CONSTRUCTING test removed, and defer improvements
until PR 85723 is fixed.  Let me know how you want me to proceed.

Martin

On 2/14/20 9:06 PM, Martin Sebor wrote:
> On 2/13/20 3:59 PM, Jason Merrill wrote:
>> On 2/12/20 9:21 PM, Martin Sebor wrote:
>>> On 2/11/20 5:28 PM, Jason Merrill wrote:
>>>> On 2/11/20 9:00 PM, Martin Sebor wrote:
>>>>> r270155, committed in GCC 9, introduced a transformation that strips
>>>>> redundant trailing zero initializers from array initializer lists in
>>>>> order to support string literals as template arguments.
>>>>>
>>>>> The transformation neglected to consider the case of array elements
>>>>> of trivial class types with user-defined conversion ctors and either
>>>>> defaulted or deleted default ctors.  (It didn't occur to me that
>>>>> those qualify as trivial types despite the user-defined ctors.)  As
>>>>> a result, some valid initialization expressions are rejected when
>>>>> the explicit zero-initializers are dropped in favor of the (deleted)
>>>>> default ctor,
>>>>
>>>> Hmm, a type with only a deleted default constructor is not trivial, 
>>>> that should have been OK already.
>>>
>>> For Marek's test case:
>>>    struct A { A () == delete; A (int) = delete; };
>>>
>>> trivial_type_p() returns true (as does __is_trivial (A) in both GCC
>>> and Clang).
>>>
>>> [class.prop] says that
>>>
>>>    A trivial class is a class that is trivially copyable and has one
>>>    or more default constructors (10.3.4.1), all of which are either
>>>    trivial or deleted and at least one of which is not deleted.
>>>
>>> That sounds like A above is not trivial because it doesn't have
>>> at least one default ctor that's not deleted, but both GCC and
>>> Clang say it is.  What am I missing?  Is there some other default
>>> constructor hiding in there that I don't know about?
>>>
>>>>> and others are eliminated in favor of the defaulted
>>>>> ctor instead of invoking a user-defined conversion ctor, leading to
>>>>> wrong code.
>>>>
>>>> This seems like a bug in type_initializer_zero_p; it shouldn't treat 
>>>> 0 as a zero initializer for any class.
>>>
>>> That does fix it, and it seems like the right solution to me as well.
>>> Thanks for the suggestion.  I'm a little unsure about the condition
>>> I put in place though.
>>>
>>> Attached is an updated patch rested on x86_64-linux.
>>
>>> -  if (sized_array_p && trivial_type_p (elt_type))
>>> +  if (sized_array_p
>>> +      && trivial_type_p (elt_type)
>>> +      && !TYPE_NEEDS_CONSTRUCTING (elt_type))
>>
>> Do we still need this change?  If so, please add a comment about the 
>> trivial_type_p bug.
> 
> The change isn't needed with my patch as it was, but it would still
> be needed with the changes you suggested (even then it doesn't help
> with the problem I describe below).
> 
>>
>>>    if (TREE_CODE (init) != CONSTRUCTOR
>> I might change this to
>>
>>   if (!CP_AGGREGATE_TYPE_P (type))
>>     return initializer_zerop (init);
> 
> This behaves differently in C++ 2a mode (the whole condition evaluates
> to true for class A below) than in earlier modes and causes a failure
> in the new array55.C test:

True, my suggestion above does the wrong thing for non-aggregate classes.

> +      /* A class can only be initialized by a non-class type if it has
> +	 a ctor that converts from that type.  Such classes are excluded
> +	 since their semantics are unknown.  */
> +      if (RECORD_OR_UNION_TYPE_P (type)
> +	  && !RECORD_OR_UNION_TYPE_P (TREE_TYPE (init)))
> +	return false;

How about if (!SCALAR_TYPE_P (type)) here?

More broadly, it seems like doing this optimization here at all is 
questionable, since we don't yet know whether there's a valid conversion 
from the zero-valued initializer to the element type.  It would seem 
better to do it in process_init_constructor_array after the call to 
massage_init_elt, when we know the actual value of the element.

Jason

On 2/17/20 10:54 AM, Jason Merrill wrote:
> On 2/14/20 9:06 PM, Martin Sebor wrote:
>> On 2/13/20 3:59 PM, Jason Merrill wrote:
>>> On 2/12/20 9:21 PM, Martin Sebor wrote:
>>>> On 2/11/20 5:28 PM, Jason Merrill wrote:
>>>>> On 2/11/20 9:00 PM, Martin Sebor wrote:
>>>>>> r270155, committed in GCC 9, introduced a transformation that strips
>>>>>> redundant trailing zero initializers from array initializer lists in
>>>>>> order to support string literals as template arguments.
>>>>>>
>>>>>> The transformation neglected to consider the case of array elements
>>>>>> of trivial class types with user-defined conversion ctors and either
>>>>>> defaulted or deleted default ctors.  (It didn't occur to me that
>>>>>> those qualify as trivial types despite the user-defined ctors.)  As
>>>>>> a result, some valid initialization expressions are rejected when
>>>>>> the explicit zero-initializers are dropped in favor of the (deleted)
>>>>>> default ctor,
>>>>>
>>>>> Hmm, a type with only a deleted default constructor is not trivial, 
>>>>> that should have been OK already.
>>>>
>>>> For Marek's test case:
>>>>    struct A { A () == delete; A (int) = delete; };
>>>>
>>>> trivial_type_p() returns true (as does __is_trivial (A) in both GCC
>>>> and Clang).
>>>>
>>>> [class.prop] says that
>>>>
>>>>    A trivial class is a class that is trivially copyable and has one
>>>>    or more default constructors (10.3.4.1), all of which are either
>>>>    trivial or deleted and at least one of which is not deleted.
>>>>
>>>> That sounds like A above is not trivial because it doesn't have
>>>> at least one default ctor that's not deleted, but both GCC and
>>>> Clang say it is.  What am I missing?  Is there some other default
>>>> constructor hiding in there that I don't know about?
>>>>
>>>>>> and others are eliminated in favor of the defaulted
>>>>>> ctor instead of invoking a user-defined conversion ctor, leading to
>>>>>> wrong code.
>>>>>
>>>>> This seems like a bug in type_initializer_zero_p; it shouldn't 
>>>>> treat 0 as a zero initializer for any class.
>>>>
>>>> That does fix it, and it seems like the right solution to me as well.
>>>> Thanks for the suggestion.  I'm a little unsure about the condition
>>>> I put in place though.
>>>>
>>>> Attached is an updated patch rested on x86_64-linux.
>>>
>>>> -  if (sized_array_p && trivial_type_p (elt_type))
>>>> +  if (sized_array_p
>>>> +      && trivial_type_p (elt_type)
>>>> +      && !TYPE_NEEDS_CONSTRUCTING (elt_type))
>>>
>>> Do we still need this change?  If so, please add a comment about the 
>>> trivial_type_p bug.
>>
>> The change isn't needed with my patch as it was, but it would still
>> be needed with the changes you suggested (even then it doesn't help
>> with the problem I describe below).
>>
>>>
>>>>    if (TREE_CODE (init) != CONSTRUCTOR
>>> I might change this to
>>>
>>>   if (!CP_AGGREGATE_TYPE_P (type))
>>>     return initializer_zerop (init);
>>
>> This behaves differently in C++ 2a mode (the whole condition evaluates
>> to true for class A below) than in earlier modes and causes a failure
>> in the new array55.C test:
> 
> True, my suggestion above does the wrong thing for non-aggregate classes.
> 
>> +      /* A class can only be initialized by a non-class type if it has
>> +     a ctor that converts from that type.  Such classes are excluded
>> +     since their semantics are unknown.  */
>> +      if (RECORD_OR_UNION_TYPE_P (type)
>> +      && !RECORD_OR_UNION_TYPE_P (TREE_TYPE (init)))
>> +    return false;
> 
> How about if (!SCALAR_TYPE_P (type)) here?
> 
> More broadly, it seems like doing this optimization here at all is 
> questionable, since we don't yet know whether there's a valid conversion 
> from the zero-valued initializer to the element type.  It would seem 
> better to do it in process_init_constructor_array after the call to 
> massage_init_elt, when we know the actual value of the element.

Yes, it seems that it might be better to do it there.  Attached
is a revised patch that implements your suggestion.  It's probably
more intrusive than you envisioned.  The stripping of the redundant
trailing initializers was straightforward.  Most of the rest of
the changes are only necessary to strip redundant initializers
for pointers to data members.

Martin

PS I'm uneasy about this patch this late in the cycle.  The bug I'm
fixing was introduced at the end of the last release, as a result
of a last minute patch not unlike this one.  It caused at least two
codegen regressions in all language modes.  I'd hate for this change
to have similar repercussions.

On 2/21/20 7:41 PM, Martin Sebor wrote:
> On 2/17/20 10:54 AM, Jason Merrill wrote:
>> On 2/14/20 9:06 PM, Martin Sebor wrote:
>>> On 2/13/20 3:59 PM, Jason Merrill wrote:
>>>> On 2/12/20 9:21 PM, Martin Sebor wrote:
>>>>> On 2/11/20 5:28 PM, Jason Merrill wrote:
>>>>>> On 2/11/20 9:00 PM, Martin Sebor wrote:
>>>>>>> r270155, committed in GCC 9, introduced a transformation that strips
>>>>>>> redundant trailing zero initializers from array initializer lists in
>>>>>>> order to support string literals as template arguments.
>>>>>>>
>>>>>>> The transformation neglected to consider the case of array elements
>>>>>>> of trivial class types with user-defined conversion ctors and either
>>>>>>> defaulted or deleted default ctors.  (It didn't occur to me that
>>>>>>> those qualify as trivial types despite the user-defined ctors.)  As
>>>>>>> a result, some valid initialization expressions are rejected when
>>>>>>> the explicit zero-initializers are dropped in favor of the (deleted)
>>>>>>> default ctor,
>>>>>>
>>>>>> Hmm, a type with only a deleted default constructor is not 
>>>>>> trivial, that should have been OK already.
>>>>>
>>>>> For Marek's test case:
>>>>>    struct A { A () == delete; A (int) = delete; };
>>>>>
>>>>> trivial_type_p() returns true (as does __is_trivial (A) in both GCC
>>>>> and Clang).
>>>>>
>>>>> [class.prop] says that
>>>>>
>>>>>    A trivial class is a class that is trivially copyable and has one
>>>>>    or more default constructors (10.3.4.1), all of which are either
>>>>>    trivial or deleted and at least one of which is not deleted.
>>>>>
>>>>> That sounds like A above is not trivial because it doesn't have
>>>>> at least one default ctor that's not deleted, but both GCC and
>>>>> Clang say it is.  What am I missing?  Is there some other default
>>>>> constructor hiding in there that I don't know about?
>>>>>
>>>>>>> and others are eliminated in favor of the defaulted
>>>>>>> ctor instead of invoking a user-defined conversion ctor, leading to
>>>>>>> wrong code.
>>>>>>
>>>>>> This seems like a bug in type_initializer_zero_p; it shouldn't 
>>>>>> treat 0 as a zero initializer for any class.
>>>>>
>>>>> That does fix it, and it seems like the right solution to me as well.
>>>>> Thanks for the suggestion.  I'm a little unsure about the condition
>>>>> I put in place though.
>>>>>
>>>>> Attached is an updated patch rested on x86_64-linux.
>>>>
>>>>> -  if (sized_array_p && trivial_type_p (elt_type))
>>>>> +  if (sized_array_p
>>>>> +      && trivial_type_p (elt_type)
>>>>> +      && !TYPE_NEEDS_CONSTRUCTING (elt_type))
>>>>
>>>> Do we still need this change?  If so, please add a comment about the 
>>>> trivial_type_p bug.
>>>
>>> The change isn't needed with my patch as it was, but it would still
>>> be needed with the changes you suggested (even then it doesn't help
>>> with the problem I describe below).
>>>
>>>>
>>>>>    if (TREE_CODE (init) != CONSTRUCTOR
>>>> I might change this to
>>>>
>>>>   if (!CP_AGGREGATE_TYPE_P (type))
>>>>     return initializer_zerop (init);
>>>
>>> This behaves differently in C++ 2a mode (the whole condition evaluates
>>> to true for class A below) than in earlier modes and causes a failure
>>> in the new array55.C test:
>>
>> True, my suggestion above does the wrong thing for non-aggregate classes.
>>
>>> +      /* A class can only be initialized by a non-class type if it has
>>> +     a ctor that converts from that type.  Such classes are excluded
>>> +     since their semantics are unknown.  */
>>> +      if (RECORD_OR_UNION_TYPE_P (type)
>>> +      && !RECORD_OR_UNION_TYPE_P (TREE_TYPE (init)))
>>> +    return false;
>>
>> How about if (!SCALAR_TYPE_P (type)) here?
>>
>> More broadly, it seems like doing this optimization here at all is 
>> questionable, since we don't yet know whether there's a valid 
>> conversion from the zero-valued initializer to the element type.  It 
>> would seem better to do it in process_init_constructor_array after the 
>> call to massage_init_elt, when we know the actual value of the element.
> 
> Yes, it seems that it might be better to do it there.  Attached
> is a revised patch that implements your suggestion.  It's probably
> more intrusive than you envisioned.  The stripping of the redundant
> trailing initializers was straightforward.  Most of the rest of
> the changes are only necessary to strip redundant initializers
> for pointers to data members.
> 
> Martin
> 
> PS I'm uneasy about this patch this late in the cycle.  The bug I'm
> fixing was introduced at the end of the last release, as a result
> of a last minute patch not unlike this one.  It caused at least two
> codegen regressions in all language modes.  I'd hate for this change
> to have similar repercussions.

Hmm, yes, the last_distinct logic is a bit opaque.  Please rework it so 
you aren't changing i within the loop.

Jason

On 2/14/20 3:06 PM, Martin Sebor wrote:
> On 2/13/20 3:59 PM, Jason Merrill wrote:
>> On 2/12/20 9:21 PM, Martin Sebor wrote:
>>> On 2/11/20 5:28 PM, Jason Merrill wrote:
>>>> On 2/11/20 9:00 PM, Martin Sebor wrote:
>>>>> r270155, committed in GCC 9, introduced a transformation that strips
>>>>> redundant trailing zero initializers from array initializer lists in
>>>>> order to support string literals as template arguments.
>>>>>
>>>>> The transformation neglected to consider the case of array elements
>>>>> of trivial class types with user-defined conversion ctors and either
>>>>> defaulted or deleted default ctors.  (It didn't occur to me that
>>>>> those qualify as trivial types despite the user-defined ctors.)  As
>>>>> a result, some valid initialization expressions are rejected when
>>>>> the explicit zero-initializers are dropped in favor of the (deleted)
>>>>> default ctor,
>>>>
>>>> Hmm, a type with only a deleted default constructor is not trivial, 
>>>> that should have been OK already.
>>>
>>> For Marek's test case:
>>>    struct A { A () == delete; A (int) = delete; };
>>>
>>> trivial_type_p() returns true (as does __is_trivial (A) in both GCC
>>> and Clang).
>>>
>>> [class.prop] says that
>>>
>>>    A trivial class is a class that is trivially copyable and has one
>>>    or more default constructors (10.3.4.1), all of which are either
>>>    trivial or deleted and at least one of which is not deleted.
>>>
>>> That sounds like A above is not trivial because it doesn't have
>>> at least one default ctor that's not deleted, but both GCC and
>>> Clang say it is.  What am I missing?  Is there some other default
>>> constructor hiding in there that I don't know about?
>>>
>>>>> and others are eliminated in favor of the defaulted
>>>>> ctor instead of invoking a user-defined conversion ctor, leading to
>>>>> wrong code.
>>>>
>>>> This seems like a bug in type_initializer_zero_p; it shouldn't treat 
>>>> 0 as a zero initializer for any class.
>>>
>>> That does fix it, and it seems like the right solution to me as well.
>>> Thanks for the suggestion.  I'm a little unsure about the condition
>>> I put in place though.
>>>
>>> Attached is an updated patch rested on x86_64-linux.
>>
>>> -  if (sized_array_p && trivial_type_p (elt_type))
>>> +  if (sized_array_p
>>> +      && trivial_type_p (elt_type)
>>> +      && !TYPE_NEEDS_CONSTRUCTING (elt_type))
>>
>> Do we still need this change?  If so, please add a comment about the 
>> trivial_type_p bug.
> 
> The change isn't needed with my patch as it was

Let's go ahead and commit the type_initializer_zero_p hunk on both trunk 
and 9 now to fix this bug for 9.3, and pursue the 
process_init_constructor_array patch for later.

Jason

On 3/4/20 10:41 AM, Jason Merrill wrote:
> On 2/14/20 3:06 PM, Martin Sebor wrote:
>> On 2/13/20 3:59 PM, Jason Merrill wrote:
>>> On 2/12/20 9:21 PM, Martin Sebor wrote:
>>>> On 2/11/20 5:28 PM, Jason Merrill wrote:
>>>>> On 2/11/20 9:00 PM, Martin Sebor wrote:
>>>>>> r270155, committed in GCC 9, introduced a transformation that strips
>>>>>> redundant trailing zero initializers from array initializer lists in
>>>>>> order to support string literals as template arguments.
>>>>>>
>>>>>> The transformation neglected to consider the case of array elements
>>>>>> of trivial class types with user-defined conversion ctors and either
>>>>>> defaulted or deleted default ctors.  (It didn't occur to me that
>>>>>> those qualify as trivial types despite the user-defined ctors.)  As
>>>>>> a result, some valid initialization expressions are rejected when
>>>>>> the explicit zero-initializers are dropped in favor of the (deleted)
>>>>>> default ctor,
>>>>>
>>>>> Hmm, a type with only a deleted default constructor is not trivial, 
>>>>> that should have been OK already.
>>>>
>>>> For Marek's test case:
>>>>    struct A { A () == delete; A (int) = delete; };
>>>>
>>>> trivial_type_p() returns true (as does __is_trivial (A) in both GCC
>>>> and Clang).
>>>>
>>>> [class.prop] says that
>>>>
>>>>    A trivial class is a class that is trivially copyable and has one
>>>>    or more default constructors (10.3.4.1), all of which are either
>>>>    trivial or deleted and at least one of which is not deleted.
>>>>
>>>> That sounds like A above is not trivial because it doesn't have
>>>> at least one default ctor that's not deleted, but both GCC and
>>>> Clang say it is.  What am I missing?  Is there some other default
>>>> constructor hiding in there that I don't know about?
>>>>
>>>>>> and others are eliminated in favor of the defaulted
>>>>>> ctor instead of invoking a user-defined conversion ctor, leading to
>>>>>> wrong code.
>>>>>
>>>>> This seems like a bug in type_initializer_zero_p; it shouldn't 
>>>>> treat 0 as a zero initializer for any class.
>>>>
>>>> That does fix it, and it seems like the right solution to me as well.
>>>> Thanks for the suggestion.  I'm a little unsure about the condition
>>>> I put in place though.
>>>>
>>>> Attached is an updated patch rested on x86_64-linux.
>>>
>>>> -  if (sized_array_p && trivial_type_p (elt_type))
>>>> +  if (sized_array_p
>>>> +      && trivial_type_p (elt_type)
>>>> +      && !TYPE_NEEDS_CONSTRUCTING (elt_type))
>>>
>>> Do we still need this change?  If so, please add a comment about the 
>>> trivial_type_p bug.
>>
>> The change isn't needed with my patch as it was
> 
> Let's go ahead and commit the type_initializer_zero_p hunk on both trunk 
> and 9 now to fix this bug for 9.3,

Just to be sure: You mean the patch at this link, correct?

   https://gcc.gnu.org/ml/gcc-patches/2020-02/msg00742.html

(Without the change above that's not needed.)

> and pursue the 
> process_init_constructor_array patch for later.

Ack.

Martin

On 3/4/20 4:14 PM, Martin Sebor wrote:
> On 3/4/20 10:41 AM, Jason Merrill wrote:
>> On 2/14/20 3:06 PM, Martin Sebor wrote:
>>> On 2/13/20 3:59 PM, Jason Merrill wrote:
>>>> On 2/12/20 9:21 PM, Martin Sebor wrote:
>>>>> On 2/11/20 5:28 PM, Jason Merrill wrote:
>>>>>> On 2/11/20 9:00 PM, Martin Sebor wrote:
>>>>>>> r270155, committed in GCC 9, introduced a transformation that strips
>>>>>>> redundant trailing zero initializers from array initializer lists in
>>>>>>> order to support string literals as template arguments.
>>>>>>>
>>>>>>> The transformation neglected to consider the case of array elements
>>>>>>> of trivial class types with user-defined conversion ctors and either
>>>>>>> defaulted or deleted default ctors.  (It didn't occur to me that
>>>>>>> those qualify as trivial types despite the user-defined ctors.)  As
>>>>>>> a result, some valid initialization expressions are rejected when
>>>>>>> the explicit zero-initializers are dropped in favor of the (deleted)
>>>>>>> default ctor,
>>>>>>
>>>>>> Hmm, a type with only a deleted default constructor is not 
>>>>>> trivial, that should have been OK already.
>>>>>
>>>>> For Marek's test case:
>>>>>    struct A { A () == delete; A (int) = delete; };
>>>>>
>>>>> trivial_type_p() returns true (as does __is_trivial (A) in both GCC
>>>>> and Clang).
>>>>>
>>>>> [class.prop] says that
>>>>>
>>>>>    A trivial class is a class that is trivially copyable and has one
>>>>>    or more default constructors (10.3.4.1), all of which are either
>>>>>    trivial or deleted and at least one of which is not deleted.
>>>>>
>>>>> That sounds like A above is not trivial because it doesn't have
>>>>> at least one default ctor that's not deleted, but both GCC and
>>>>> Clang say it is.  What am I missing?  Is there some other default
>>>>> constructor hiding in there that I don't know about?
>>>>>
>>>>>>> and others are eliminated in favor of the defaulted
>>>>>>> ctor instead of invoking a user-defined conversion ctor, leading to
>>>>>>> wrong code.
>>>>>>
>>>>>> This seems like a bug in type_initializer_zero_p; it shouldn't 
>>>>>> treat 0 as a zero initializer for any class.
>>>>>
>>>>> That does fix it, and it seems like the right solution to me as well.
>>>>> Thanks for the suggestion.  I'm a little unsure about the condition
>>>>> I put in place though.
>>>>>
>>>>> Attached is an updated patch rested on x86_64-linux.
>>>>
>>>>> -  if (sized_array_p && trivial_type_p (elt_type))
>>>>> +  if (sized_array_p
>>>>> +      && trivial_type_p (elt_type)
>>>>> +      && !TYPE_NEEDS_CONSTRUCTING (elt_type))
>>>>
>>>> Do we still need this change?  If so, please add a comment about the 
>>>> trivial_type_p bug.
>>>
>>> The change isn't needed with my patch as it was
>>
>> Let's go ahead and commit the type_initializer_zero_p hunk on both 
>> trunk and 9 now to fix this bug for 9.3,
> 
> Just to be sure: You mean the patch at this link, correct?
> 
>    https://gcc.gnu.org/ml/gcc-patches/2020-02/msg00742.html
> 
> (Without the change above that's not needed.)

Correct.

>> and pursue the process_init_constructor_array patch for later.
> 
> Ack.
> 
> Martin
>