From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 54689 invoked by alias); 22 Feb 2020 00:42:01 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 54666 invoked by uid 89); 22 Feb 2020 00:42:00 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-20.0 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=Attached, sk:host_wi, sk:HOST_WI, tree_to_uhwi X-HELO: mail-qt1-f193.google.com Received: from mail-qt1-f193.google.com (HELO mail-qt1-f193.google.com) (209.85.160.193) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 22 Feb 2020 00:41:57 +0000 Received: by mail-qt1-f193.google.com with SMTP id i23so2647945qtr.5 for ; Fri, 21 Feb 2020 16:41:56 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:subject:to:references:message-id:date:user-agent:mime-version :in-reply-to:content-language; bh=cYBbStjo2lyXKW5aZdbXLmypeQF26RuGQ9W4P6s9P0I=; b=EHEnPLe+oMpfpdEDhM071jxgQ5IGkAbZ/vYl+2Zej7CtTfUZXGMpn3KASU0bnoQJWG wPrlnTl8C/inPjxKTzH5pSZJPNKNKM/Q/pifQizV/U35P0VpHWNezYmtxQh+1dXmbHAV 5Sm8HrKH90Wa+pb5/YZeeOG2uvwQt4bI6VXDc6qIGMUr/uy4sQY2964t1/Xt7R6bOP0B /U9Xgs14JCPQTGT1cXxL84hDMB0qA2PFcFyoAtzKiEfDxFIlp3WZk/4uq+uVOulYOWDo xI+hQn1rF58CTkc2yNnCjkoZYyNHWx9Un/ri9mOFBDi2UgdZO6OQmfvs5jUbWHieTxjz GlnQ== Return-Path: Received: from [192.168.0.41] (174-16-112-158.hlrn.qwest.net. [174.16.112.158]) by smtp.gmail.com with ESMTPSA id q25sm2372945qkc.60.2020.02.21.16.41.53 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 21 Feb 2020 16:41:54 -0800 (PST) From: Martin Sebor Subject: Re: [PATCH] avoid user-constructible types in reshape_init_array (PR 90938) To: Jason Merrill , gcc-patches References: <32339bf2-7d63-66ec-7ffa-77086e84dfa5@gmail.com> Message-ID: <86a6c502-6218-7d3c-46ed-bf867527d8f2@gmail.com> Date: Sat, 22 Feb 2020 00:42:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/mixed; boundary="------------909A812E5B5B225A4A3958C8" X-IsSubscribed: yes X-SW-Source: 2020-02/txt/msg01242.txt.bz2 This is a multi-part message in MIME format. --------------909A812E5B5B225A4A3958C8 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-length: 4658 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. --------------909A812E5B5B225A4A3958C8 Content-Type: text/x-patch; name="gcc-90938.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="gcc-90938.diff" Content-length: 13359 PR c++/90938 - Initializing array with {1} works but not {0} gcc/cp/ChangeLog: PR c++/90938 * decl.c (reshape_init_array_1): Avoid stripping redundant trailing zero initializers here... * typeck2.c (process_init_constructor_array): ...and instead strip them here. Extend the range of same trailing implicit initializers to also include preceding explicit initializers. gcc/testsuite/ChangeLog: PR c++/90938 * g++.dg/init/array55.C: New test. * g++.dg/init/array56.C: New test. * g++.dg/cpp2a/nontype-class33.C: New test. diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c index 31a556a0a1f..b2259fc6f20 100644 --- a/gcc/cp/decl.c +++ b/gcc/cp/decl.c @@ -6010,9 +6010,6 @@ reshape_init_array_1 (tree elt_type, tree max_index, reshape_iter *d, max_index_cst = tree_to_uhwi (fold_convert (size_type_node, max_index)); } - /* Set to the index of the last element with a non-zero initializer. - Zero initializers for elements past this one can be dropped. */ - unsigned HOST_WIDE_INT last_nonzero = -1; /* Loop until there are no more initializers. */ for (index = 0; d->cur != d->end && (!sized_array_p || index <= max_index_cst); @@ -6039,32 +6036,11 @@ reshape_init_array_1 (tree elt_type, tree max_index, reshape_iter *d, if (!TREE_CONSTANT (elt_init)) TREE_CONSTANT (new_init) = false; - /* Pointers initialized to strings must be treated as non-zero - even if the string is empty. */ - tree init_type = TREE_TYPE (elt_init); - if (POINTER_TYPE_P (elt_type) != POINTER_TYPE_P (init_type) - || !type_initializer_zero_p (elt_type, elt_init)) - last_nonzero = index; - /* This can happen with an invalid initializer (c++/54501). */ if (d->cur == old_cur && !sized_array_p) break; } - if (sized_array_p && trivial_type_p (elt_type)) - { - /* Strip trailing zero-initializers from an array of a trivial - type of known size. They are redundant and get in the way - of telling them apart from those with implicit zero value. */ - unsigned HOST_WIDE_INT nelts = CONSTRUCTOR_NELTS (new_init); - if (last_nonzero > nelts) - nelts = 0; - else if (last_nonzero < nelts - 1) - nelts = last_nonzero + 1; - - vec_safe_truncate (CONSTRUCTOR_ELTS (new_init), nelts); - } - return new_init; } diff --git a/gcc/cp/typeck2.c b/gcc/cp/typeck2.c index 48920894b3b..cafcb6a7ee7 100644 --- a/gcc/cp/typeck2.c +++ b/gcc/cp/typeck2.c @@ -1473,6 +1473,39 @@ process_init_constructor_array (tree type, tree init, int nested, int flags, return PICFLAG_ERRONEOUS; } + /* Process any explicit initializers, stripping any redundant trailing + initializers, and, if appropriate, appending initializers for any + trailing elements that aren't initialized explicitly. Series of + same trailing initializers are appeneded as just one, using a range + of indices. For example: + int a[7] = { 1, 2, 0, 0 }; + is transformed into a CONSTRUCTOR with just two elements: + { 1, 2 } + while + struct A { int i; }; typedef int A::* P; + P[7] = { &A::i, &A::i, 0, 0 }; + into a CONSTRUCTOR with the three elements: + { 0, 0, [2..6] = -1 } + (because the "address" or offset of A::i is zero and because a null + data member pointer is represented as all ones). This is done so + that equivalent non-type template arguments that literals of such + types are treated as identical regardless of the form their + (equivalent) initializers take. */ + const tree eltype = TREE_TYPE (type); + /* Set if ELTYPE requires a ctor call. */ + const bool build_ctor = type_build_ctor_call (eltype); + /* Set if default-initializing ELTYPE zeroes out its bits. */ + const bool zero_init = zero_init_p (eltype); + /* Set if trailing zeros should be truncated. */ + const bool trunc_zero = !unbounded && !build_ctor && zero_init; + + /* Set to the index of the last element with a non-zero initializer. + Zero initializers for elements past this one can be dropped. */ + unsigned HOST_WIDE_INT last_nonzero = HOST_WIDE_INT_M1U; + /* Set to the index of the last element with a distinct initializer + value. Subsequent elements (if any) have the same value. */ + unsigned HOST_WIDE_INT last_distinct = 0; + FOR_EACH_VEC_SAFE_ELT (v, i, ce) { if (!ce->index) @@ -1491,57 +1524,107 @@ process_init_constructor_array (tree type, tree init, int nested, int flags, strip_array_types (TREE_TYPE (ce->value))))); picflags |= picflag_from_initializer (ce->value); + + if (!trunc_zero || !type_initializer_zero_p (eltype, ce->value)) + last_nonzero = i; + + if (ce->value != (*v)[last_distinct].value) + last_distinct = i; + } + + if (trunc_zero) + { + /* Strip redundant explicit zero initializers. */ + if (last_nonzero > i) + last_nonzero = 0; + if (last_nonzero < i - 1) + { + vec_safe_truncate (v, last_nonzero + 1); + len = i = vec_safe_length (v); + } } - /* No more initializers. If the array is unbounded, we are done. Otherwise, - we must add initializers ourselves. */ - if (!unbounded) - for (; i < len; ++i) - { - tree next; + /* No more initializers. If the array is unbounded or if trailing + zero-initialized elements can be elided, we are done. */ + if (unbounded || trunc_zero) + { + CONSTRUCTOR_ELTS (init) = v; + return picflags; + } - if (type_build_ctor_call (TREE_TYPE (type))) - { - /* If this type needs constructors run for default-initialization, - we can't rely on the back end to do it for us, so make the - initialization explicit by list-initializing from T{}. */ - next = build_constructor (init_list_type_node, NULL); - next = massage_init_elt (TREE_TYPE (type), next, nested, flags, - complain); - if (initializer_zerop (next)) - /* The default zero-initialization is fine for us; don't - add anything to the CONSTRUCTOR. */ - next = NULL_TREE; - } - else if (!zero_init_p (TREE_TYPE (type))) - next = build_zero_init (TREE_TYPE (type), - /*nelts=*/NULL_TREE, - /*static_storage_p=*/false); - else - /* The default zero-initialization is fine for us; don't - add anything to the CONSTRUCTOR. */ - next = NULL_TREE; + tree next = NULL_TREE; - if (next) - { - picflags |= picflag_from_initializer (next); - if (len > i+1 - && (initializer_constant_valid_p (next, TREE_TYPE (next)) - == null_pointer_node)) - { - tree range = build2 (RANGE_EXPR, size_type_node, - build_int_cst (size_type_node, i), - build_int_cst (size_type_node, len - 1)); - CONSTRUCTOR_APPEND_ELT (v, range, next); - break; - } - else - CONSTRUCTOR_APPEND_ELT (v, size_int (i), next); - } - else - /* Don't bother checking all the other elements. */ + for (; i < len; ++i) + { + if (build_ctor) + { + /* If this type needs constructors run for default-initialization, + we can't rely on the back end to do it for us, so make the + initialization explicit by list-initializing from T{}. */ + next = build_constructor (init_list_type_node, NULL); + next = massage_init_elt (eltype, next, nested, flags, complain); + if (initializer_zerop (next)) + /* The default zero-initialization is fine for us; don't + add anything to the CONSTRUCTOR. */ + next = NULL_TREE; + } + else if (!zero_init) + next = build_zero_init (eltype, + /*nelts=*/NULL_TREE, + /*static_storage_p=*/false); + else + /* The default zero-initialization is fine for us; don't + add anything to the CONSTRUCTOR. */ + next = NULL_TREE; + + if (next) + { + picflags |= picflag_from_initializer (next); + if (initializer_constant_valid_p (next, TREE_TYPE (next)) + == null_pointer_node) + { + /* If the last distinct explicit initializer value is + the same as the implicit initializer NEXT built above, + include the former in the range built below. */ + if (i && next == (*v)[last_distinct].value) + i = last_distinct; + + break; + } + + CONSTRUCTOR_APPEND_ELT (v, size_int (i), next); + last_distinct = i; + } + else + { + /* Don't bother checking all the other elements and avoid + appending to the initializer list below. */ + last_distinct = i++; break; - } + } + } + + if (last_distinct < i - 1) + { + /* Insert a range [LAST_DISTINCT, LEN) initializing trailing + elements to the same constant value built above and stored + in NEXT. */ + tree range = build2 (RANGE_EXPR, size_type_node, + build_int_cst (size_type_node, last_distinct), + build_int_cst (size_type_node, len - 1)); + unsigned HOST_WIDE_INT nelts = last_distinct + 1; + if (vec_safe_length (v) < nelts) + /* Append the range if there is no LAST_DISTINCT initializer. */ + CONSTRUCTOR_APPEND_ELT (v, range, next); + else + { + /* Replace the LAST_DISTINCT initializer with NEXT. */ + vec_safe_truncate (v, nelts); + (*v)[last_distinct].index = range; + } + } + else if (i < len && next) + CONSTRUCTOR_APPEND_ELT (v, size_int (i), next); CONSTRUCTOR_ELTS (init) = v; return picflags; diff --git a/gcc/testsuite/g++.dg/cpp2a/nontype-class33.C b/gcc/testsuite/g++.dg/cpp2a/nontype-class33.C new file mode 100644 index 00000000000..048e748a316 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp2a/nontype-class33.C @@ -0,0 +1,36 @@ +/* PR c++/90938 - Initializing array with {1} works, but not {0} + { dg-do compile } + { dg-options "-Wall -std=c++2a" } */ + +struct A { int i; }; +struct B { A a[2]; }; + +static const constexpr A a0 = { 0 }; +static const constexpr A a_ = { }; + +template struct X { }; + +typedef X XB; +typedef X XB; +typedef X XB; +typedef X XB; +typedef X XB; +typedef X XB; +typedef X XB; +typedef X XB; +typedef X XB; + + +struct C { constexpr C () = default; }; +struct D { C c[2]; }; + +static const constexpr C c_ = { }; + +template struct Y { }; + +typedef Y YD; +typedef Y YD; +typedef Y YD; +typedef Y YD; +typedef Y YD; +typedef Y YD; diff --git a/gcc/testsuite/g++.dg/init/array55.C b/gcc/testsuite/g++.dg/init/array55.C new file mode 100644 index 00000000000..70fb183b897 --- /dev/null +++ b/gcc/testsuite/g++.dg/init/array55.C @@ -0,0 +1,27 @@ +/* PR c++/90938 - Initializing array with {1} works, but not {0} + { dg-do compile { target c++11 } } */ + +struct A +{ + A () = delete; + A (int) = delete; +}; + +A a_[] = { 0 }; // { dg-error "use of deleted function 'A::A\\\(int\\\)'" } + +A a1[1] = { 0 }; // { dg-error "use of deleted function 'A::A\\\(int\\\)'" } + + +struct B +{ + B () = delete; + B (int) = delete; + B (long); +}; + +B b_[] = { 0 }; // { dg-error "use of deleted function 'B::B\\\(int\\\)'" } + +B b1[1] = { 0 }; // { dg-error "use of deleted function 'B::B\\\(int\\\)'" } + +B b2[] = { 0L }; +B b3[1] = { 0L }; diff --git a/gcc/testsuite/g++.dg/init/array56.C b/gcc/testsuite/g++.dg/init/array56.C new file mode 100644 index 00000000000..63e16663ec1 --- /dev/null +++ b/gcc/testsuite/g++.dg/init/array56.C @@ -0,0 +1,107 @@ +/* PR c++/90938 - Initializing array with {1} works, but not {0} + { dg-do compile { target c++11 } } + { dg-options "-O -Wall -fdump-tree-optimized" } */ + +#define assert(e) \ + ((e) ? (void)0 \ + : (__builtin_printf ("assertion failed on line %i: %s\n", \ + __LINE__, #e), \ + __builtin_abort ())) + +namespace A { + +struct X +{ + X () = default; + X (int n) : n (n + 1) { } + int n; +}; + +static_assert (__is_trivial (X), "X is trivial"); + +static void test () +{ + { + X x[] { 0 }; + assert (1 == x->n); + } + + { + X x[1] { 0 }; + assert (1 == x->n); // fails + } + + { + X x[2] { 0 }; + assert (1 == x[0].n && 0 == x[1].n); // fails + } + + { + X x[] { 1, 0 }; + assert (2 == x[0].n && 1 == x[1].n); // passes + } + + { + X x[2] { 1, 0 }; + assert (2 == x[0].n && 1 == x[1].n); // fails + } +} + +} + +namespace B { + +struct X +{ + X () = default; + X (int *p) : p (p ? p : new int (1)) { } + int *p; +}; + +static_assert (__is_trivial (X), "X is trivial"); + +static void test () +{ + X x[1] { nullptr }; + assert (*x->p == 1); // fails + + X y[1] { 0 }; + assert (*y->p == 1); // fails +} + +} + +namespace C { + +static const char *vector_swizzle (int vecsize, int index) +{ + static const char *swizzle[4][4] = + { + { ".x", ".y", ".z", ".w" }, + { ".xy", ".yz", ".zw", nullptr }, + { ".xyz", ".yzw", nullptr, nullptr }, + { "", nullptr, nullptr, nullptr }, + }; + + assert (vecsize >= 1 && vecsize <= 4); + assert (index >= 0 && index < 4); + assert (swizzle[vecsize - 1][index]); + + return swizzle[vecsize - 1][index]; +} + +static void test () +{ + assert (!*vector_swizzle(4, 0)); +} + +} + +int main () +{ + A::test (); + B::test (); + C::test (); +} + +// { dg-final { scan-tree-dump-not "abort" "optimized" } } --------------909A812E5B5B225A4A3958C8--