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 165FA3858C66 for ; Tue, 25 Oct 2022 17:46:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 165FA3858C66 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1666719968; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=UEFg9BdA9x+mKQ6G/VxYVOvXoSkMSyG/K35qYorNdF0=; b=Gtgec7msfHRaqpDxHD+NrIurCOtOtOCR0Zvfhu4np8ymqUl+fDOUtM4IKZRApHQNR/Plxm iWeWf56L8A83kQZVX/xEMZmh4ciEksKrdQraZAKtCelRGSa4YU3CGUZYdPYH5pbhOAI+7p zTFXoS1sVQtq1vAFrji8tJveJb8xe84= Received: from mail-qt1-f199.google.com (mail-qt1-f199.google.com [209.85.160.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-75-p5twnwNQOGyUqEz1jXzVcw-1; Tue, 25 Oct 2022 13:46:07 -0400 X-MC-Unique: p5twnwNQOGyUqEz1jXzVcw-1 Received: by mail-qt1-f199.google.com with SMTP id d25-20020ac847d9000000b0039d0dd90182so9480796qtr.3 for ; Tue, 25 Oct 2022 10:46:06 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:references:message-id:in-reply-to:subject:cc:to:date :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=UEFg9BdA9x+mKQ6G/VxYVOvXoSkMSyG/K35qYorNdF0=; b=7KT5+ebSRgyW8OhdyyXXG78XfAvNXxHpFQiIkiRAGNufNy8AqHdZqQt410kJhodRSF j12HGKJEQyhfhXhXvnXCWHz/k/BAnrZO0ygYiyLb8Kv2dAhlt58Oe5T5vlBBpGOl/KGt eBbwxevoNv4VNgkHXJValgDinZrHen015d/5/wgQhkY0Nx3YppTJ32uViHBUXv4IA+dc aYj7GySNZZFdIczXd0j0RiugHpo+JHsUPsZBt9tCQUmL7k/3dyb4Lg4F24gNgWU6rh5L YYjo9D4Om64IZTdKEdrmtayZluwERe3jhkLRAnwaoF+UkJQZYsY5HWpW/Sub9ryKmOkR OUfQ== X-Gm-Message-State: ACrzQf3tavA2W8l92cL59TS17oQPaQV239z/4qCjW4pXoaauaQDvkbox rvp6mM3gD18lDbdw31OCHxJXFfYaZACFAt/XrlljnXga+asoqgkHC8IofCALjnITkMcFe2i1zLR b6X9h3b+SdeyTsLWQtQ== X-Received: by 2002:a05:622a:1654:b0:39c:ded0:4780 with SMTP id y20-20020a05622a165400b0039cded04780mr33088217qtj.688.1666719966282; Tue, 25 Oct 2022 10:46:06 -0700 (PDT) X-Google-Smtp-Source: AMsMyM5ZAi0pa1vhzujLwcPGgv3lu9R1esktMP0aUg+ZOJ8cjAhAnk+RaCPH/Br7XJ6LROtOfQCbWQ== X-Received: by 2002:a05:622a:1654:b0:39c:ded0:4780 with SMTP id y20-20020a05622a165400b0039cded04780mr33088200qtj.688.1666719965975; Tue, 25 Oct 2022 10:46:05 -0700 (PDT) Received: from [192.168.1.130] (ool-457670bb.dyn.optonline.net. [69.118.112.187]) by smtp.gmail.com with ESMTPSA id s16-20020a05620a255000b006cbe3be300esm2441527qko.12.2022.10.25.10.46.04 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 25 Oct 2022 10:46:04 -0700 (PDT) From: Patrick Palka X-Google-Original-From: Patrick Palka Date: Tue, 25 Oct 2022 13:46:03 -0400 (EDT) To: Nathan Sidwell cc: Patrick Palka , Richard Biener , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] c++ modules: verify_type failure with typedef enum [PR106848] In-Reply-To: Message-ID: <056b89f0-1795-0cd3-fda5-ab7cc8fb68ac@idea> References: <20221013153921.3795800-1-ppalka@redhat.com> <5cf07598-68ce-efda-05c3-625a1466195f@idea> <5601279b-939f-4492-c14b-c495d7a2a3b2@idea> <2072c1af-9f73-3c9d-8c52-f0edf82e289f@acm.org> <617594a2-c890-948f-9215-8d58c6c8f473@idea> 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.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Mon, 24 Oct 2022, Nathan Sidwell wrote: > On 10/21/22 09:11, Patrick Palka wrote: > > On Fri, 21 Oct 2022, Nathan Sidwell wrote: > > > > > > > Thanks for the explanation, it's a situation I didn;t anticipate and your > > > fix > > > is good. Could you add a comment about why you need to propagate the > > > values > > > though? > > > > Thanks a lot, will do. Just to make sure since there are multiple > > solutions proposed, do you prefer to go with > > https://gcc.gnu.org/pipermail/gcc-patches/2022-October/603487.html > > or > > https://gcc.gnu.org/pipermail/gcc-patches/2022-October/603831.html ? > > > > Both solutions fix the PR106848 issue (empty TYPE_MIN/MAX_VALUE on an > > enum type variant), but the latter also fixes the related PR102600 > > (empty TYPE_MIN/MAX_VALUE on the main variant of an enum with no > > enumerators). (We could maybe even combine the two solutions: stream > > TYPE_MIN/MAX_VALUE as part of ENUMERAL_TYPE, and also update TYPE_VALUES > > of each variant during trees_in::read_enum_def) > > > Oh, let's go with the latter: > > * module.cc (trees_out::core_vals): Stream TYPE_MAX_VALUE and > TYPE_MIN_VALUE of ENUMERAL_TYPE. > (trees_in::core_vals): Likewise. > (trees_out::write_enum_def): Don't stream them here. > (trees_in::read_enum_def): Likewise. > > but, again, some comments -- at the new streaming point, and in the defn > streamer were we no longer stream them. > > thanks. Thanks a lot, here's what I ended up committing: -- >8 -- Subject: [PATCH] c++ modules: enum TYPE_MIN/MAX_VALUE streaming [PR106848] In the frontend, the TYPE_MIN/MAX_VALUE of ENUMERAL_TYPE is the same as that of the enum's underlying type (see start_enum). And the underlying type of an enum is always known, even for an opaque enum that lacks a definition. But currently, we stream TYPE_MIN/MAX_VALUE of an enum only as part of its definition. So if the enum is declared but never defined, the ENUMERAL_TYPE we stream in will have empty TYPE_MIN/MAX_VALUE fields despite these fields being non-empty on stream out. And even if the enum is defined, read_enum_def updates these fields only on the main variant of the enum type, so for other variants (that we may have streamed in earlier) these fields remain empty. That these fields are unexpectedly empty for some ENUMERAL_TYPEs is ultimately the cause of the below two PRs. This patch fixes this by making us stream TYPE_MIN/MAX_VALUE directly for each ENUMERAL_TYPE rather than as part of the enum's definition, so that we naturally also stream these fields for opaque enums (and each enum type variant). PR c++/106848 PR c++/102600 gcc/cp/ChangeLog: * module.cc (trees_out::core_vals): Stream TYPE_MAX_VALUE and TYPE_MIN_VALUE of ENUMERAL_TYPE. (trees_in::core_vals): Likewise. (trees_out::write_enum_def): Don't stream them here. (trees_in::read_enum_def): Likewise. gcc/testsuite/ChangeLog: * g++.dg/modules/enum-9_a.H: New test. * g++.dg/modules/enum-9_b.C: New test. * g++.dg/modules/enum-10_a.H: New test. * g++.dg/modules/enum-10_b.C: New test. * g++.dg/modules/enum-11_a.H: New test. * g++.dg/modules/enum-11_b.C: New test. --- gcc/cp/module.cc | 39 +++++++++++++++--------- gcc/testsuite/g++.dg/modules/enum-10_a.H | 5 +++ gcc/testsuite/g++.dg/modules/enum-10_b.C | 6 ++++ gcc/testsuite/g++.dg/modules/enum-11_a.H | 5 +++ gcc/testsuite/g++.dg/modules/enum-11_b.C | 8 +++++ gcc/testsuite/g++.dg/modules/enum-9_a.H | 13 ++++++++ gcc/testsuite/g++.dg/modules/enum-9_b.C | 6 ++++ 7 files changed, 67 insertions(+), 15 deletions(-) create mode 100644 gcc/testsuite/g++.dg/modules/enum-10_a.H create mode 100644 gcc/testsuite/g++.dg/modules/enum-10_b.C create mode 100644 gcc/testsuite/g++.dg/modules/enum-11_a.H create mode 100644 gcc/testsuite/g++.dg/modules/enum-11_b.C create mode 100644 gcc/testsuite/g++.dg/modules/enum-9_a.H create mode 100644 gcc/testsuite/g++.dg/modules/enum-9_b.C diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc index 73971e7ff47..9957df510e6 100644 --- a/gcc/cp/module.cc +++ b/gcc/cp/module.cc @@ -6017,9 +6017,17 @@ trees_out::core_vals (tree t) if (CODE_CONTAINS_STRUCT (code, TS_TYPE_NON_COMMON)) { + if (code == ENUMERAL_TYPE) + { + /* These fields get set even for opaque enums that lack a + definition, so we stream them directly for each ENUMERAL_TYPE. + We stream TYPE_VALUES as part of the definition. */ + WT (t->type_non_common.maxval); + WT (t->type_non_common.minval); + } /* Records and unions hold FIELDS, VFIELD & BINFO on these things. */ - if (!RECORD_OR_UNION_CODE_P (code) && code != ENUMERAL_TYPE) + else if (!RECORD_OR_UNION_CODE_P (code)) { // FIXME: These are from tpl_parm_value's 'type' writing. // Perhaps it should just be doing them directly? @@ -6530,9 +6538,17 @@ trees_in::core_vals (tree t) if (CODE_CONTAINS_STRUCT (code, TS_TYPE_NON_COMMON)) { + if (code == ENUMERAL_TYPE) + { + /* These fields get set even for opaque enums that lack a + definition, so we stream them directly for each ENUMERAL_TYPE. + We stream TYPE_VALUES as part of the definition. */ + RT (t->type_non_common.maxval); + RT (t->type_non_common.minval); + } /* Records and unions hold FIELDS, VFIELD & BINFO on these things. */ - if (!RECORD_OR_UNION_CODE_P (code) && code != ENUMERAL_TYPE) + else if (!RECORD_OR_UNION_CODE_P (code)) { /* This is not clobbering TYPE_CACHED_VALUES, because this is a type that doesn't have any. */ @@ -12217,8 +12233,8 @@ trees_out::write_enum_def (tree decl) tree type = TREE_TYPE (decl); tree_node (TYPE_VALUES (type)); - tree_node (TYPE_MIN_VALUE (type)); - tree_node (TYPE_MAX_VALUE (type)); + /* Note that we stream TYPE_MIN/MAX_VALUE directly as part of the + ENUMERAL_TYPE. */ } void @@ -12242,8 +12258,6 @@ trees_in::read_enum_def (tree defn, tree maybe_template) { tree type = TREE_TYPE (defn); tree values = tree_node (); - tree min = tree_node (); - tree max = tree_node (); if (get_overrun ()) return false; @@ -12254,8 +12268,8 @@ trees_in::read_enum_def (tree defn, tree maybe_template) if (installing) { TYPE_VALUES (type) = values; - TYPE_MIN_VALUE (type) = min; - TYPE_MAX_VALUE (type) = max; + /* Note that we stream TYPE_MIN/MAX_VALUE directly as part of the + ENUMERAL_TYPE. */ rest_of_type_compilation (type, DECL_NAMESPACE_SCOPE_P (defn)); } @@ -12269,22 +12283,17 @@ trees_in::read_enum_def (tree defn, tree maybe_template) tree new_decl = TREE_VALUE (values); if (DECL_NAME (known_decl) != DECL_NAME (new_decl)) - goto bad; + break; new_decl = maybe_duplicate (new_decl); if (!cp_tree_equal (DECL_INITIAL (known_decl), DECL_INITIAL (new_decl))) - goto bad; + break; } if (known || values) - goto bad; - - if (!cp_tree_equal (TYPE_MIN_VALUE (type), min) - || !cp_tree_equal (TYPE_MAX_VALUE (type), max)) { - bad:; error_at (DECL_SOURCE_LOCATION (maybe_dup), "definition of %qD does not match", maybe_dup); inform (DECL_SOURCE_LOCATION (defn), diff --git a/gcc/testsuite/g++.dg/modules/enum-10_a.H b/gcc/testsuite/g++.dg/modules/enum-10_a.H new file mode 100644 index 00000000000..fb7d10ad3b6 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/enum-10_a.H @@ -0,0 +1,5 @@ +// PR c++/106848 +// { dg-additional-options -fmodule-header } +// { dg-module-cmi {} } + +typedef enum memory_order { memory_order_seq_cst } memory_order; diff --git a/gcc/testsuite/g++.dg/modules/enum-10_b.C b/gcc/testsuite/g++.dg/modules/enum-10_b.C new file mode 100644 index 00000000000..76dc3152963 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/enum-10_b.C @@ -0,0 +1,6 @@ +// PR c++/106848 +// { dg-additional-options "-fmodules-ts -g" } + +import "enum-10_a.H"; + +memory_order x = memory_order_seq_cst; diff --git a/gcc/testsuite/g++.dg/modules/enum-11_a.H b/gcc/testsuite/g++.dg/modules/enum-11_a.H new file mode 100644 index 00000000000..1aecabfd0bd --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/enum-11_a.H @@ -0,0 +1,5 @@ +// PR c++/102600 +// { dg-additional-options -fmodule-header } +// { dg-module-cmi {} } + +enum class byte : unsigned char { }; diff --git a/gcc/testsuite/g++.dg/modules/enum-11_b.C b/gcc/testsuite/g++.dg/modules/enum-11_b.C new file mode 100644 index 00000000000..4d77cab8953 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/enum-11_b.C @@ -0,0 +1,8 @@ +// PR c++/102600 +// { dg-additional-options -fmodules-ts } + +import "enum-11_a.H"; + +void push(byte) {} +void write(char v) { push(static_cast(v)); } +int main() { write(char{}); } diff --git a/gcc/testsuite/g++.dg/modules/enum-9_a.H b/gcc/testsuite/g++.dg/modules/enum-9_a.H new file mode 100644 index 00000000000..0dd4a0f2fb1 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/enum-9_a.H @@ -0,0 +1,13 @@ +// PR c++/106848 +// { dg-additional-options -fmodule-header } +// { dg-module-cmi {} } + +template +struct pair { + using type = void(*)(const _T1&); +}; + +struct _ScannerBase { + enum _TokenT { _S_token_anychar }; + pair<_TokenT> _M_token_tbl; +}; diff --git a/gcc/testsuite/g++.dg/modules/enum-9_b.C b/gcc/testsuite/g++.dg/modules/enum-9_b.C new file mode 100644 index 00000000000..95e2812b81e --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/enum-9_b.C @@ -0,0 +1,6 @@ +// PR c++/106848 +// { dg-additional-options "-fmodules-ts -g" } + +import "enum-9_a.H"; + +_ScannerBase s; -- 2.38.1.143.g1fc3c0ad40