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.129.124]) by sourceware.org (Postfix) with ESMTPS id 65055385DC16 for ; Tue, 31 May 2022 16:41:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 65055385DC16 Received: from mail-qv1-f70.google.com (mail-qv1-f70.google.com [209.85.219.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-633-2rfqF9xBO8q_b26av0hMDQ-1; Tue, 31 May 2022 12:41:57 -0400 X-MC-Unique: 2rfqF9xBO8q_b26av0hMDQ-1 Received: by mail-qv1-f70.google.com with SMTP id bv14-20020ad4488e000000b004646673e80bso820947qvb.23 for ; Tue, 31 May 2022 09:41:57 -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=YGME3BCOX0L/Bg2nflVz3ZkKw5pUql2Qssa9cEMeEPQ=; b=hVCi718K/phzwAhxJqEnDJKy0mXXJ8pi7w6fYCtzy5/6lUFZm75M3e9XYOszurHIrn dLmwIoXRP44CpbGX/4gEyeEPF/vRFNsnKVpJLbVqWANDhLXGzIc156t4qVZWprd/1Eco AKwFL/UrPi3Tmk8zDo4nKABYEW4qdjkPTxCRHIhk7OZ7jIknOHm3SewC7rsAPwdwZsgh hFh2aPJnncbKkOSwueRLLHfwzZV8INwOTatMrjqCA1HB33hb2WM4r7AwuN/R2SkG40b0 u0V+6dmFGVEkVOiuzqkNe9fTpdPkYg/Qx7+wbhkIyRct6Rdnp3H5udOXVSy+wJw6/XEv C/Qg== X-Gm-Message-State: AOAM531H1zpLGDGAsRlrfqal1XkSQc2XVK1YkJrE6Gey2eQ1RhayRCGl VQo29k9akIY1zzgaC3WbwXqPCJ/qfl2xD+hsM9qlQm7nrlU4PwUcmVo0wJWDlXeHQqbJYqtShB9 LfNnLNqg15tpR6ePpXA== X-Received: by 2002:ac8:7d89:0:b0:2fc:1be0:6fc0 with SMTP id c9-20020ac87d89000000b002fc1be06fc0mr23540847qtd.482.1654015316951; Tue, 31 May 2022 09:41:56 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyEzy5mPnTQZv2rO/+YRifz5o4wv33XT/OF3NO16kr9hBZQiDdSoNCKhuJ/74KeqNaDe+c98g== X-Received: by 2002:ac8:7d89:0:b0:2fc:1be0:6fc0 with SMTP id c9-20020ac87d89000000b002fc1be06fc0mr23540816qtd.482.1654015316550; Tue, 31 May 2022 09:41:56 -0700 (PDT) Received: from [192.168.1.130] (ool-457670bb.dyn.optonline.net. [69.118.112.187]) by smtp.gmail.com with ESMTPSA id w9-20020a05620a148900b006a36dedb53bsm9530926qkj.45.2022.05.31.09.41.55 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 31 May 2022 09:41:55 -0700 (PDT) From: Patrick Palka X-Google-Original-From: Patrick Palka Date: Tue, 31 May 2022 12:41:55 -0400 (EDT) To: Jason Merrill cc: Patrick Palka , GCC Patches Subject: Re: [PATCH] c++: constexpr init of union sub-aggr w/ base [PR105491] In-Reply-To: <89968899-201b-9dc6-5bcf-eca32b9d47ae@redhat.com> Message-ID: <1c566819-8c54-ad63-83e0-4e4cbbd0bd17@idea> 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> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-14.4 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_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 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT X-Content-Filtered-By: Mailman/MimeDel 2.1.29 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 16:42:01 -0000 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? -- >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_; -- 2.36.1.203.g1bcf4f6271