From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ej1-x634.google.com (mail-ej1-x634.google.com [IPv6:2a00:1450:4864:20::634]) by sourceware.org (Postfix) with ESMTPS id 6E8D73858D39 for ; Wed, 19 Oct 2022 07:33:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 6E8D73858D39 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-ej1-x634.google.com with SMTP id w18so37850900ejq.11 for ; Wed, 19 Oct 2022 00:33:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=kjnCXeMx+WJcHuhesMDMj5RUHAB9uqmTrgZaKUFukCQ=; b=fvCDCsnDjpzoGecaDcPLr0GVUPdR/F3kLafg9ibN8+eZMjjUj+azH9aOLRa19lv0ZR EymTSrGrSzZ8qRdGEsawPVUH/2zbCdv4vB5nxkMhzoai8xPEmn3yZVy92+LkQExXHh9e YD4AdfmvtBsY9kNvTPw5zgijBVjCIUsyeVosPJgdthwPOVCxL4h3PrVeX38aTkAhOwho sG839kN94NpqEhZQRia62m6sZ0L4B4sp2EHj4KHyUH52VFsaS85KscMu+O4dtWF7eQ2t ur/EpMCr5t0PHYvOO9XOmIpLSb34ZAt5LBGOyhvrmAFdjowkY1nWbnAclGZvOWxHSLRL fckA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=kjnCXeMx+WJcHuhesMDMj5RUHAB9uqmTrgZaKUFukCQ=; b=kX6HjppmDdPtG6LftPv7JM4QWYc9/ou8NoXdFAPpYJUjeaH8ZUVA9QKFHLKATOprzT 2so8Jc8kHx74uYiQMv1Hbo8fUYzFHM2Pav4HoEAG8eIbC60Nkhk36kIMwlZo4QDSMxDy paZkSAAL84wgkVGjlwoh9KzeLceVdNyqaFo988ZoJXnBWZ2TbeK+SRRNG4K9LZD1Vc5W l1Xv1NOdmK1hmn7xIZ1yMJOJBPcwH3Op43eLCCT3djh1VuhKwjxvmlPeub1Ie4ziaxUb zGYPJGSY3F+efa00+fa/RjdkziEaiESGgzsC5lefrg7U+1lhL9G+bWkKCA7s1YmO6uF8 43bQ== X-Gm-Message-State: ACrzQf0v7aJsK38OsM1l22tZ4DGE+AI44bjPFn0uOdwT5GFm6Rh2OIgk AIUHEJq5hbFEc5lyXpy6+ZRiWhPs+8mMkBjWVvw3VlkGhE8= X-Google-Smtp-Source: AMsMyM7LpeVbktdNH78/LdslPUoMqSGCmO+sn4cDIhLjRu5QcynU5gRKLMtflCmF86nobdWDJU7ZATu7Q2nqHL3ruKY= X-Received: by 2002:a17:907:6d08:b0:78e:e87:5c06 with SMTP id sa8-20020a1709076d0800b0078e0e875c06mr5390600ejc.511.1666164838161; Wed, 19 Oct 2022 00:33:58 -0700 (PDT) MIME-Version: 1.0 References: <20221013153921.3795800-1-ppalka@redhat.com> <5cf07598-68ce-efda-05c3-625a1466195f@idea> In-Reply-To: <5cf07598-68ce-efda-05c3-625a1466195f@idea> From: Richard Biener Date: Wed, 19 Oct 2022 09:33:46 +0200 Message-ID: Subject: Re: [PATCH] c++ modules: verify_type failure with typedef enum [PR106848] To: Patrick Palka Cc: gcc-patches@gcc.gnu.org, nathan@acm.org Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-8.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,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 Tue, Oct 18, 2022 at 8:26 PM Patrick Palka wrote: > > On Fri, 14 Oct 2022, Richard Biener wrote: > > > On Thu, Oct 13, 2022 at 5:40 PM Patrick Palka via Gcc-patches > > wrote: > > > > > > Here during stream in we end up having created a type variant for the enum > > > before we read the enum's definition, and thus the variant inherited stale > > > TYPE_VALUES and TYPE_MIN/MAX_VALUES, which leads to an ICE (with -g). The > > > stale variant got created from set_underlying_type during earlier stream in > > > of the (redundant) typedef for the enum. > > > > > > This patch works around this by setting TYPE_VALUES and TYPE_MIN/MAX_VALUES > > > for all variants when reading in an enum definition. Does this look like > > > the right approach? Or perhaps we need to arrange that we read the enum > > > definition before reading in the typedef decl? Note that seems to be an > > > issue only when the typedef name and enum names are the same (thus the > > > typedef is redundant), otherwise we seem to read the enum definition first > > > as desired. > > > > > > PR c++/106848 > > > > > > gcc/cp/ChangeLog: > > > > > > * module.cc (trees_in::read_enum_def): Set the TYPE_VALUES, > > > TYPE_MIN_VALUE and TYPE_MAX_VALUE of all type variants. > > > > > > gcc/testsuite/ChangeLog: > > > > > > * g++.dg/modules/enum-9_a.H: New test. > > > * g++.dg/modules/enum-9_b.C: New test. > > > --- > > > gcc/cp/module.cc | 9 ++++++--- > > > gcc/testsuite/g++.dg/modules/enum-9_a.H | 5 +++++ > > > gcc/testsuite/g++.dg/modules/enum-9_b.C | 6 ++++++ > > > 3 files changed, 17 insertions(+), 3 deletions(-) > > > 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 7ffeefa7c1f..97fb80bcd44 100644 > > > --- a/gcc/cp/module.cc > > > +++ b/gcc/cp/module.cc > > > @@ -12303,9 +12303,12 @@ 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; > > > + for (tree t = type; t; t = TYPE_NEXT_VARIANT (t)) > > > + { > > > + TYPE_VALUES (t) = values; > > > + TYPE_MIN_VALUE (t) = min; > > > + TYPE_MAX_VALUE (t) = max; > > > + } > > > > it's definitely somewhat ugly but at least type_hash_canon doesn't hash > > these for ENUMERAL_TYPE (but it does compare them! which in principle > > means it could as well hash them ...) > > > > I think that if you read both from the same module that you should arrange > > to read what you refer to first? But maybe that's not the actual issue here. > > *nod* reading in the enum before reading in the typedef seems like > the most direct solution, though not sure how to accomplish that :/ For LTO streaming we DFS walk tree edges from all entries into the tree graph we want to stream, collecting and streaming SCCs. Not sure if doing similar for module streaming would help this case though. > A somewhat orthogonal issue (that incidentally fixes this testcase) is > that we stream TYPE_MIN/MAX_VALUE only for enums with a definition, but > the frontend sets these fields even for opaque enums. If we make sure > to stream these fields for all ENUMERAL_TYPEs, then we won't have to > worry about these fields being stale for variants that may have been > created before reading in the enum definition (their TYPE_VALUES field > will still be stale I guess, but verify_type doesn't worry about that > it seems, so we avoid the ICE). > > patch to that effect is at > https://gcc.gnu.org/pipermail/gcc-patches/2022-October/603831.html > > > > > Richard. > > > > > > > > rest_of_type_compilation (type, DECL_NAMESPACE_SCOPE_P (defn)); > > > } > > > 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..fb7d10ad3b6 > > > --- /dev/null > > > +++ b/gcc/testsuite/g++.dg/modules/enum-9_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-9_b.C b/gcc/testsuite/g++.dg/modules/enum-9_b.C > > > new file mode 100644 > > > index 00000000000..63e81675d0a > > > --- /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"; > > > + > > > +memory_order x = memory_order_seq_cst; > > > -- > > > 2.38.0.68.ge85701b4af > > > > > > > >