From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 10D58395B419 for ; Tue, 31 May 2022 19:06:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 10D58395B419 Received: from mail-qt1-f198.google.com (mail-qt1-f198.google.com [209.85.160.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-80-WIwNnW_lNIypFaPbaecA5Q-1; Tue, 31 May 2022 15:06:08 -0400 X-MC-Unique: WIwNnW_lNIypFaPbaecA5Q-1 Received: by mail-qt1-f198.google.com with SMTP id f22-20020ac859d6000000b00304bf4dba7fso1338988qtf.3 for ; Tue, 31 May 2022 12:06:08 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=5r2vhYyK1UvytATJHNsqAjK1Kr4M2Q3iMBF2W5TZfwA=; b=MUdq6ZW/0yOFruSFy5n5E3rqk8FKyL3PNZ5r/0/L6gS9a4a9Jd2oo/sxqYroydKFrN NUL2PZtWxeoaxyFed9tZmSMPyCEMTGlnc5iylT1OyMz6tvLpE7LclGKRA4kOv6oFNrW9 FCDRXFdDufGv+hNLwZvRXoIUC2Td852xk/ZxIHzxh3nXsqmcA0zyCuOfLPykQmgGIIrE mkWCswdmAz5/8yE4nOPpvd2ganaRxtk18VPKpnO87BF6Lkj2zuvgxFeFvRO7eY3e7KfC PIsM93hyAumF55E5B4C1zt1+s9cuNIqVby1+8pCB4k9yh5j54Xmm/Pz4vTCsCbqWIZLd NleQ== X-Gm-Message-State: AOAM533xe33ECnuMx8pXqESpsJqlO8zxKXKOSMIJyZEHOEYdQilQ8MId cQxn/bo+gRAVy9NjsjQxyiJjkDUpnQQ2fMhSh05Z/NBB3o2xPH1tzPdsca8K4ZVHz22mLLeOr6d 29Q7LC050GwQNSmpjnw== X-Received: by 2002:a05:620a:3198:b0:6a3:a7e1:786e with SMTP id bi24-20020a05620a319800b006a3a7e1786emr28833130qkb.292.1654023966685; Tue, 31 May 2022 12:06:06 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwQa7xhmy9zTCX7agnIuLPChCHdK3X1MkSXqH9QpU3NTdgYR6vYcjLDpUjfRRHaSMp7jJrlBQ== X-Received: by 2002:a05:620a:3198:b0:6a3:a7e1:786e with SMTP id bi24-20020a05620a319800b006a3a7e1786emr28833094qkb.292.1654023966170; Tue, 31 May 2022 12:06:06 -0700 (PDT) Received: from [192.168.1.100] (130-44-159-43.s15913.c3-0.arl-cbr1.sbo-arl.ma.cable.rcncustomer.com. [130.44.159.43]) by smtp.gmail.com with ESMTPSA id m15-20020a05620a214f00b006a650276ecesm1770567qkm.14.2022.05.31.12.06.04 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 31 May 2022 12:06:05 -0700 (PDT) Message-ID: <1281bab0-c4ff-a784-9d3c-90b2e1d719f2@redhat.com> Date: Tue, 31 May 2022 15:06:04 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.1 Subject: Re: [PATCH] c++: constexpr init of union sub-aggr w/ base [PR105491] To: Patrick Palka Cc: GCC Patches References: <20220506152232.2843007-1-ppalka@redhat.com> <659fd0c4-5fa7-147d-9b97-bebf06ae7c23@redhat.com> <7ab4b646-bade-97b0-8083-c392cf37ea49@idea> <48baf04b-9cb9-8668-cbb6-047eba1033d7@idea> <660ef433-cb10-e137-e734-5fbf3223a39f@idea> <89968899-201b-9dc6-5bcf-eca32b9d47ae@redhat.com> <1c566819-8c54-ad63-83e0-4e4cbbd0bd17@idea> From: Jason Merrill In-Reply-To: <1c566819-8c54-ad63-83e0-4e4cbbd0bd17@idea> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-14.0 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 31 May 2022 19:06:13 -0000 On 5/31/22 12:41, Patrick Palka wrote: > On Wed, 18 May 2022, Jason Merrill wrote: > >> On 5/17/22 12:34, Patrick Palka wrote: >>> On Sat, May 7, 2022 at 5:18 PM Jason Merrill wrote: >>>> >>>> On 5/6/22 16:46, Patrick Palka wrote: >>>>> On Fri, 6 May 2022, Jason Merrill wrote: >>>>> >>>>>> On 5/6/22 16:10, Patrick Palka wrote: >>>>>>> On Fri, 6 May 2022, Patrick Palka wrote: >>>>>>> >>>>>>>> On Fri, 6 May 2022, Jason Merrill wrote: >>>>>>>> >>>>>>>>> On 5/6/22 14:00, Patrick Palka wrote: >>>>>>>>>> On Fri, 6 May 2022, Patrick Palka wrote: >>>>>>>>>> >>>>>>>>>>> On Fri, 6 May 2022, Jason Merrill wrote: >>>>>>>>>>> >>>>>>>>>>>> On 5/6/22 11:22, Patrick Palka wrote: >>>>>>>>>>>>> Here ever since r10-7313-gb599bf9d6d1e18, >>>>>>>>>>>>> reduced_constant_expression_p >>>>>>>>>>>>> in C++11/14 is rejecting the marked sub-aggregate >>>>>>>>>>>>> initializer >>>>>>>>>>>>> (of type >>>>>>>>>>>>> S) >>>>>>>>>>>>> >>>>>>>>>>>>> W w = {.D.2445={.s={.D.2387={.m=0}, .b=0}}} >>>>>>>>>>>>> ^ >>>>>>>>>>>>> >>>>>>>>>>>>> ultimately because said initializer has >>>>>>>>>>>>> CONSTRUCTOR_NO_CLEARING >>>>>>>>>>>>> set, >>>>>>>>>>>>> and >>>>>>>>>>>>> so the function proceeds to verify that all fields of S >>>>>>>>>>>>> are >>>>>>>>>>>>> initialized. >>>>>>>>>>>>> And before C++17 we don't expect to see base class >>>>>>>>>>>>> fields (since >>>>>>>>>>>>> next_initializable_field skips over the), so the base >>>>>>>>>>>>> class >>>>>>>>>>>>> initializer >>>>>>>>>>>>> causes r_c_e_p to return false. >>>>>>>>>>>> >>>>>>>>>>>> That seems like the primary bug. I guess r_c_e_p >>>>>>>>>>>> shouldn't be >>>>>>>>>>>> using >>>>>>>>>>>> next_initializable_field. Really that function should >>>>>>>>>>>> only be >>>>>>>>>>>> used for >>>>>>>>>>>> aggregates. >>>>>>>>>>> >>>>>>>>>>> I see, I'll try replacing it in r_c_e_p. Would that be in >>>>>>>>>>> addition >>>>>>>>>>> to >>>>>>>>>>> or instead of the clear_no_implicit_zero approach? >>>>>>>>>> >>>>>>>>>> I'm testing the following, which uses a custom predicate >>>>>>>>>> instead of >>>>>>>>>> next_initializable_field in r_c_e_p. >>>>>>>>> >>>>>>>>> Let's make it a public predicate, not internal to r_c_e_p. >>>>>>>>> Maybe it >>>>>>>>> could be >>>>>>>>> next_subobject_field, and the current next_initializable_field >>>>>>>>> change to >>>>>>>>> next_aggregate_field? >>>>>>>> >>>>>>>> Will do. >>>>>>>> >>>>>>>>> >>>>>>>>>> Looks like the inner initializer {.D.2387={.m=0}, .b=0} is >>>>>>>>>> formed >>>>>>>>>> during >>>>>>>>>> the subobject constructor call: >>>>>>>>>> >>>>>>>>>> V::V (&((struct S *) this)->D.2120); >>>>>>>>>> >>>>>>>>>> after the evaluation of which, 'result' in >>>>>>>>>> cxx_eval_call_expression is >>>>>>>>>> NULL >>>>>>>>>> (presumably because it's a CALL_EXPR, not AGGR_INIT_EXPR?): >>>>>>>>>> >>>>>>>>>> /* This can be null for a subobject constructor call, in >>>>>>>>>> which case what we care about is the initialization >>>>>>>>>> side-effects rather than the value. We could get at >>>>>>>>>> the >>>>>>>>>> value by evaluating *this, but we don't bother; >>>>>>>>>> there's >>>>>>>>>> no need to put such a call in the hash table. */ >>>>>>>>>> result = lval ? ctx->object : ctx->ctor; >>>>>>>>>> >>>>>>>>>> so we end up not calling clear_no_implicit_zero for the inner >>>>>>>>>> initializer >>>>>>>>>> directly. We only call clear_no_implicit_zero after >>>>>>>>>> evaluating the >>>>>>>>>> AGGR_INIT_EXPR for outermost initializer (of type W). >>>>>>>>> >>>>>>>>> Maybe for constructors we could call it on ctx->ctor instead of >>>>>>>>> result, >>>>>>>>> or >>>>>>>>> call r_c_e_p in C++20+? >>>>>>>> >>>>>>>> But both ctx->ctor and ->object are NULL during a subobject >>>>>>>> constructor >>>>>>>> call (since we apparently clear these fields when entering a >>>>>>>> STATEMENT_LIST): >>>>>>>> >>>>>>>> So I tried instead obtaining the constructor by evaluating new_obj >>>>>>>> via >>>>>>>> >>>>>>>> --- a/gcc/cp/constexpr.cc >>>>>>>> +++ b/gcc/cp/constexpr.cc >>>>>>>> @@ -2993,6 +2988,9 @@ cxx_eval_call_expression (const >>>>>>>> constexpr_ctx *ctx, >>>>>>>> tree t, >>>>>>>> in order to detect reading an unitialized object in >>>>>>>> constexpr >>>>>>>> instead >>>>>>>> of value-initializing it. (reduced_constant_expression_p >>>>>>>> is >>>>>>>> expected to >>>>>>>> take care of clearing the flag.) */ >>>>>>>> + if (new_obj && DECL_CONSTRUCTOR_P (fun)) >>>>>>>> + result = cxx_eval_constant_expression (ctx, new_obj, >>>>>>>> /*lval=*/false, >>>>>>>> + non_constant_p, >>>>>>>> overflow_p); >>>>>>>> if (TREE_CODE (result) == CONSTRUCTOR >>>>>>>> && (cxx_dialect < cxx20 >>>>>>>> || !DECL_CONSTRUCTOR_P (fun))) >>>>>>>> >>>>>>>> but that seems to break e.g. g++.dg/cpp2a/constexpr-init12.C >>>>>>>> because >>>>>>>> after the subobject constructor call >>>>>>>> >>>>>>>> S::S (&((struct W *) this)->s, NON_LVALUE_EXPR <8>); >>>>>>>> >>>>>>>> the constructor for the subobject a.s in new_obj is still >>>>>>>> completely >>>>>>>> missing (I suppose because S::S doesn't initialize any of its >>>>>>>> members) >>>>>>>> so trying to obtain it causes us to complain too soon from >>>>>>>> cxx_eval_component_reference: >>>>>>>> >>>>>>>> constexpr-init12.C:16:24: in ‘constexpr’ expansion of ‘W(42)’ >>>>>>>> constexpr-init12.C:10:22: in ‘constexpr’ expansion of >>>>>>>> ‘((W*)this)->W::s.S::S(8)’ >>>>>>>> constexpr-init12.C:16:24: error: accessing uninitialized member >>>>>>>> ‘W::s’ >>>>>>>> 16 | constexpr auto a = W(42); // { dg-error "not a constant >>>>>>>> expression" } >>>>>>>> | ^ >>>>>>>> >>>>>>>>> >>>>>>>>> It does seem dubious that we would clear the flag on an outer >>>>>>>>> ctor when >>>>>>>>> it's >>>>>>>>> still set on an inner ctor, should probably add an assert >>>>>>>>> somewhere. >>>>>>>> >>>>>>>> Makes sense, not sure where the best place would be.. >>>>>>> >>>>>>> On second thought, if I'm understanding your suggestion correctly, I >>>>>>> don't think we can generally enforce such a property for >>>>>>> CONSTRUCTOR_NO_CLEARING, given how cxx_eval_store_expression uses it >>>>>>> for >>>>>>> unions: >>>>>>> >>>>>>> union U { >>>>>>> struct { int x, y; } a; >>>>>>> } u; >>>>>>> u.a.x = 0; >>>>>>> >>>>>>> Here after evaluating the assignment, the outer ctor for the union >>>>>>> will >>>>>>> have CONSTRUCTOR_NO_CLEARING cleared to indicate we finished >>>>>>> activating >>>>>>> the union member, but the inner ctor is certainly not fully >>>>>>> initialized >>>>>>> so it'll have CONSTRUCTOR_NO_CLEARING set still. >>>>>> >>>>>> Why clear the flag on the union before the inner ctor is fully >>>>>> initialized, if >>>>>> the intent is to prevent changing the active member during >>>>>> initialization? >>>>>> >>>>>> In the loop over ctors in cxx_eval_store_expression, I'd think if we >>>>>> encounter >>>>>> a CONSTRUCTOR_NO_CLEARING ctor along the way, we shouldn't clear the >>>>>> flag on >>>>>> an outer ctor. >>>>> >>>>> Wouldn't that prevent us from later activating a different member, which >>>>> is >>>>> allowed in C++20? It would cause us to reject e.g. >>>>> >>>>> constexpr auto f() { >>>>> union U { >>>>> struct { int x, y; } a; >>>>> int b; >>>>> } u; >>>>> u.a.x = 0; >>>>> u.b = 0; >>>>> return u; >>>>> } >>>>> >>>>> static_assert(f().b == 0); >>>>> >>>>> even in C++20 with >>>>> >>>>> :7:7: error: change of the active member of a union from >>>>> ‘f()::U::a’ to ‘f()::U::b’ during initialization >>>>> 7 | u.b = 0; >>>>> | ~~~~^~~ >>>>> >>>>> because when evaluating the second assignment we'd see that the >>>>> CONSTRUCTOR_NO_CLEARING flag is still set on the union while it already >>>>> has an activated member. >>>> >>>> Ah, makes sense. >>>> >>>>> On a related note, here's the patch that mechanically renames >>>>> next_initializable_field to next_aggregate_field and makes it skip over >>>>> vptr >>>>> fields, and defines next_subobject_field alongside it for use by >>>>> r_c_e_p. >>>>> Bootstrap and regtesting in progress. >>>> >>>> OK if successful. >>> >>> Thanks, committed as r13-211-g0c7bce0ac184c0. Would this patch be >>> suitable for backporting to 12? It seems safe enough. Alternatively >>> I guess we could backport the original workaround that tweaks >>> clear_no_implicit_zero. >> >> Maybe for 12 just add next_subobject_field without touching any other users of >> next_initializable_field? > > Ah makes sense, like so? OK. > -- >8 -- > > Subject: [PATCH] c++: constexpr init of union sub-aggr w/ base [PR105491] > > ... > > NB: This minimal backport of r13-211-g0c7bce0ac184c0 for 12.2 just adds > next_subobject_field and makes reduced_constant_expression_p use it. > > PR c++/105491 > > gcc/cp/ChangeLog: > > * constexpr.cc (reduced_constant_expression_p): Use > next_subobject_field instead. > * cp-tree.h (next_subobject_field): Declare. > * decl.cc (next_subobject_field): Define. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp0x/constexpr-union7.C: New test. > * g++.dg/cpp0x/constexpr-union7a.C: New test. > * g++.dg/cpp2a/constinit17.C: New test. > > (cherry picked from commit 0c7bce0ac184c057bacad9c8e615ce82923835fd) > --- > gcc/cp/constexpr.cc | 8 +++---- > gcc/cp/cp-tree.h | 1 + > gcc/cp/decl.cc | 19 +++++++++++++++ > gcc/testsuite/g++.dg/cpp0x/constexpr-union7.C | 17 +++++++++++++ > .../g++.dg/cpp0x/constexpr-union7a.C | 15 ++++++++++++ > gcc/testsuite/g++.dg/cpp2a/constinit17.C | 24 +++++++++++++++++++ > 6 files changed, 80 insertions(+), 4 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-union7.C > create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-union7a.C > create mode 100644 gcc/testsuite/g++.dg/cpp2a/constinit17.C > > diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc > index 47d5113ace2..1e3342adbb9 100644 > --- a/gcc/cp/constexpr.cc > +++ b/gcc/cp/constexpr.cc > @@ -3053,7 +3053,7 @@ reduced_constant_expression_p (tree t) > field = NULL_TREE; > } > else > - field = next_initializable_field (TYPE_FIELDS (TREE_TYPE (t))); > + field = next_subobject_field (TYPE_FIELDS (TREE_TYPE (t))); > } > else > field = NULL_TREE; > @@ -3065,15 +3065,15 @@ reduced_constant_expression_p (tree t) > return false; > /* Empty class field may or may not have an initializer. */ > for (; field && e.index != field; > - field = next_initializable_field (DECL_CHAIN (field))) > + field = next_subobject_field (DECL_CHAIN (field))) > if (!is_really_empty_class (TREE_TYPE (field), > /*ignore_vptr*/false)) > return false; > if (field) > - field = next_initializable_field (DECL_CHAIN (field)); > + field = next_subobject_field (DECL_CHAIN (field)); > } > /* There could be a non-empty field at the end. */ > - for (; field; field = next_initializable_field (DECL_CHAIN (field))) > + for (; field; field = next_subobject_field (DECL_CHAIN (field))) > if (!is_really_empty_class (TREE_TYPE (field), /*ignore_vptr*/false)) > return false; > ok: > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > index e9a3d09ac4c..63437415296 100644 > --- a/gcc/cp/cp-tree.h > +++ b/gcc/cp/cp-tree.h > @@ -6884,6 +6884,7 @@ extern void initialize_artificial_var (tree, vec *); > extern tree check_var_type (tree, tree, location_t); > extern tree reshape_init (tree, tree, tsubst_flags_t); > extern tree next_initializable_field (tree); > +extern tree next_subobject_field (tree); > extern tree first_field (const_tree); > extern tree fndecl_declared_return_type (tree); > extern bool undeduced_auto_decl (tree); > diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc > index 2852093d624..af5eca71ba1 100644 > --- a/gcc/cp/decl.cc > +++ b/gcc/cp/decl.cc > @@ -6409,6 +6409,25 @@ next_initializable_field (tree field) > return field; > } > > +/* FIELD is an element of TYPE_FIELDS or NULL. In the former case, the value > + returned is the next FIELD_DECL (possibly FIELD itself) that corresponds > + to a subobject. If there are no more such fields, the return value will be > + NULL. */ > + > +tree > +next_subobject_field (tree field) > +{ > + while (field > + && (TREE_CODE (field) != FIELD_DECL > + || DECL_UNNAMED_BIT_FIELD (field) > + || (DECL_ARTIFICIAL (field) > + && !DECL_FIELD_IS_BASE (field) > + && !DECL_VIRTUAL_P (field)))) > + field = DECL_CHAIN (field); > + > + return field; > +} > + > /* Return true for [dcl.init.list] direct-list-initialization from > single element of enumeration with a fixed underlying type. */ > > diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-union7.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-union7.C > new file mode 100644 > index 00000000000..b3147d9db50 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-union7.C > @@ -0,0 +1,17 @@ > +// PR c++/105491 > +// { dg-do compile { target c++11 } } > + > +struct V { > + int m = 0; > +}; > +struct S : V { > + constexpr S(int) : b() { } > + bool b; > +}; > +struct W { > + constexpr W() : s(0) { } > + union { > + S s; > + }; > +}; > +constexpr W w; > diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-union7a.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-union7a.C > new file mode 100644 > index 00000000000..b676e7d1748 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-union7a.C > @@ -0,0 +1,15 @@ > +// PR c++/105491 > +// { dg-do compile { target c++11 } } > + > +struct V { > + int m = 0; > +}; > +struct S : V { > + constexpr S(int) : b() { } > + bool b; > +}; > +union W { > + constexpr W() : s(0) { } > + S s; > +}; > +constexpr W w; > diff --git a/gcc/testsuite/g++.dg/cpp2a/constinit17.C b/gcc/testsuite/g++.dg/cpp2a/constinit17.C > new file mode 100644 > index 00000000000..6431654ac85 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp2a/constinit17.C > @@ -0,0 +1,24 @@ > +// PR c++/105491 > +// { dg-do compile { target c++11 } } > + > +class Message { > + virtual int GetMetadata(); > +}; > +class ProtobufCFileOptions : Message { > +public: > + constexpr ProtobufCFileOptions(int); > + bool no_generate_; > + bool const_strings_; > + bool use_oneof_field_name_; > + bool gen_pack_helpers_; > + bool gen_init_helpers_; > +}; > +constexpr ProtobufCFileOptions::ProtobufCFileOptions(int) > + : no_generate_(), const_strings_(), use_oneof_field_name_(), > + gen_pack_helpers_(), gen_init_helpers_() {} > +struct ProtobufCFileOptionsDefaultTypeInternal { > + constexpr ProtobufCFileOptionsDefaultTypeInternal() : _instance({}) {} > + union { > + ProtobufCFileOptions _instance; > + }; > +} __constinit _ProtobufCFileOptions_default_instance_;