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 1AA173856DF5 for ; Fri, 6 May 2022 16:41:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1AA173856DF5 Received: from mail-qv1-f71.google.com (mail-qv1-f71.google.com [209.85.219.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-270-JMohtGq-OiOqDO9Sa8Yffw-1; Fri, 06 May 2022 12:40:59 -0400 X-MC-Unique: JMohtGq-OiOqDO9Sa8Yffw-1 Received: by mail-qv1-f71.google.com with SMTP id t15-20020ad45bcf000000b0045a8cfef66bso6352473qvt.11 for ; Fri, 06 May 2022 09:40:59 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:date:to:cc:subject:in-reply-to:message-id :references:mime-version; bh=+4Uh0XhEaSUygn9UztHhZRLgRgcJZ+hJ2qWJNDWNwzA=; b=F7loX9f2EQD4Jc3zIU1ZwJNf/SWww+Bn8OD33osXvCs0YLbXDwgu/TMub6HFhsfcDf bU2kwKxgI4RM1DJwWHF3kiESNaxXiY0cqorxuxyEG8NGu2I3L+7Wy8DRdz9y8B0aR6AF RTukSqAtBRCBFvlflsb+0fYENSbBvyazOXdA2nw2Gff16KR92iZFzQsn+FFTzUJZWCl/ treAPLs/zBrnx9W/yh54fNzxlDKHw2jAKfC1tdlmq4MZO4nWJuDm9dhpzAc4HdPG8vB3 j+Ui/dhEhVdx6yUxj1Q/6xsoMNUZX0gvd0LrNTYPfxCO/+pXguR94IgeiDI5F3ccnP5m QGqw== X-Gm-Message-State: AOAM533Xc0hhXa7z9PxpM0/zco90Xd+MixtgzBjW3O7nDhvmSGdPo5w+ uFuvMT+Ch5+jd4fbxNdnKOqxc1m8Ocy8GFAnc/QFHzyFPt97kSkbAAI+YJPfqkeRtfM4yDqvs++ ehk5iP3ErYznXDMSQGg== X-Received: by 2002:a05:620a:1292:b0:69f:b0ef:e50a with SMTP id w18-20020a05620a129200b0069fb0efe50amr2892097qki.733.1651855258962; Fri, 06 May 2022 09:40:58 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwJK/tCGXHDvRb7bzUKaTf6qn+YoJjXS97NqPMvPuI49CD3PUq7DWgWiW9bIDsDCRU/6+W7VQ== X-Received: by 2002:a05:620a:1292:b0:69f:b0ef:e50a with SMTP id w18-20020a05620a129200b0069fb0efe50amr2892081qki.733.1651855258647; Fri, 06 May 2022 09:40:58 -0700 (PDT) Received: from [192.168.1.130] (ool-457670bb.dyn.optonline.net. [69.118.112.187]) by smtp.gmail.com with ESMTPSA id s65-20020ae9de44000000b006a007f40477sm2519774qkf.118.2022.05.06.09.40.57 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 06 May 2022 09:40:58 -0700 (PDT) From: Patrick Palka X-Google-Original-From: Patrick Palka Date: Fri, 6 May 2022 12:40:57 -0400 (EDT) To: Jason Merrill cc: Patrick Palka , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] c++: constexpr init of union sub-aggr w/ base [PR105491] In-Reply-To: <659fd0c4-5fa7-147d-9b97-bebf06ae7c23@redhat.com> Message-ID: <7ab4b646-bade-97b0-8083-c392cf37ea49@idea> References: <20220506152232.2843007-1-ppalka@redhat.com> <659fd0c4-5fa7-147d-9b97-bebf06ae7c23@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-14.9 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Fri, 06 May 2022 16:41:02 -0000 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? > > > The base class initializer comes from > > the constructor call S::S(int). > > > > The reason this is caused by r10-7313-gb599bf9d6d1e18 is because in that > > commit we began using CONSTRUCTOR_NO_CLEARING to also track whether we're > > in middle of activating a union member. This overloaded use of the flag > > affects clear_no_implicit_zero, which recurses into sub-aggregate > > initializers only if the outer initializer has CONSTRUCTOR_NO_CLEARING > > set. > > Is that really overloaded? In both union and non-union cases, it indicates > that the object is not completely initialized. Ah yes, makes sense. More accurately the immediate clearing of CONSTRUCTOR_NO_CLEARING after activating a union member at the end of cxx_eval_store_expression is what affects clear_no_implicit_zero later. > > > After that commit, the outer union initializer above no longer has > > the flag set at this point and so clear_no_implicit_zero no longer clears > > CONSTRUCTOR_NO_CLEARING for the marked inner initializer. > > Why wasn't it cleared for the inner initializer as part of that evaluation? 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). > > > This patch fixes this by restoring the recursive behavior of > > clear_no_implicit_zero for union initializers. Arguably we should > > we could improve reduced_constant_expression_p to accept the marked > > initializer in C++11/14 even if it has CONSTRUCTOR_NO_CLEARING set, but > > adjusting clear_no_implicit_zero seems safer to backport. > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for > > trunk/12/11/10? > > > > PR c++/105491 > > > > gcc/cp/ChangeLog: > > > > * constexpr.cc (clear_no_implicit_zero): Recurse into a union > > initializer even if CONSTRUCTOR_NO_CLEARING is already cleared. > > > > 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. > > --- > > gcc/cp/constexpr.cc | 7 +++++- > > gcc/testsuite/g++.dg/cpp0x/constexpr-union7.C | 17 +++++++++++++ > > .../g++.dg/cpp0x/constexpr-union7a.C | 15 ++++++++++++ > > gcc/testsuite/g++.dg/cpp2a/constinit17.C | 24 +++++++++++++++++++ > > 4 files changed, 62 insertions(+), 1 deletion(-) > > 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 9b1e71857fc..75fecbcbcb7 100644 > > --- a/gcc/cp/constexpr.cc > > +++ b/gcc/cp/constexpr.cc > > @@ -1886,7 +1886,12 @@ cxx_eval_internal_function (const constexpr_ctx *ctx, > > tree t, > > static void > > clear_no_implicit_zero (tree ctor) > > { > > - if (CONSTRUCTOR_NO_CLEARING (ctor)) > > + if (CONSTRUCTOR_NO_CLEARING (ctor) > > + /* For a union initializer, the flag could already be cleared but not > > + necessarily yet for its sub-aggregates, since for unions the flag is > > + also used by cxx_eval_store_expression to track whether we're in the > > + middle of activating one of its members. */ > > + || TREE_CODE (TREE_TYPE (ctor)) == UNION_TYPE) > > { > > CONSTRUCTOR_NO_CLEARING (ctor) = false; > > for (auto &e: CONSTRUCTOR_ELTS (ctor)) > > 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_; > >