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 CF7323858D37 for ; Mon, 26 Sep 2022 19:05:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org CF7323858D37 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=1664219107; 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=TY33BnyyOhEki1PeQC85u6gwyjmC6yKleWZgoQVUEqw=; b=eH0QXTC+FBWIAkywWpZOpP0wzofsb7Hwb90X8S6J6V2HC8qv6/ARzthZjYidm7hPqYfZhW xkYZ+XM8LW/5yvU6aTWJPQxigTRBvzK7bTCMaCRzOeSWI89TwaT4lh/XuhESGy0MaCvlZL HWW+uPM9NEgAvKlSCUPeIhWfJknXUrg= 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-572-CFArXzqvMryeRbVTIxZjiw-1; Mon, 26 Sep 2022 15:05:03 -0400 X-MC-Unique: CFArXzqvMryeRbVTIxZjiw-1 Received: by mail-qt1-f199.google.com with SMTP id g21-20020ac87d15000000b0035bb6f08778so5315256qtb.2 for ; Mon, 26 Sep 2022 12:05:03 -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; bh=UZZe23XvEoDhkOuu/ivsT+lQDa7Vamw3WF9GmXfn+tU=; b=45CqmzOoUNjW4LlB0jV3a6Mm4FA1wqblERb1+ru0ydgprbF1f4YSSRvsgo0uLobNZR 057h5uHrUTMAOMN/f2q4YNgef1zitRh8X2kVLpqbP4oBYHXRd+bWSBLxLBeQz0NyIZjP PsmA378ri+vwkNiF/fQw8lh2cym2t2CrX9sSi/NOK42h8FM6VHZp+0OXtm3Z4/z4C40z m1Bp2NNW3J13IyaXg9YmTpRCxctCAZe5B6REUaRhNxbFjkIwHNP6Kig6BDBcFnc8WLiC +OPlBc4lPYSmYRy5usxynRS9OFW+9l8LuE3qPuWY/U7s2VS3ErdaD4cU/yjeSfuWluDo ozyw== X-Gm-Message-State: ACrzQf14sWk6lMKG34AP5aP62Mz1L6sTfYO9456sRQ0UxCy5k9bJcmKH VNQXPGeiR8e04FvVbuhyjHp/24qhf3uXdWIZefDQChEIhhie7rs2KS6DlT30v75s/rCLnXe252i P2uyRoEtMmuENMLN78A== X-Received: by 2002:a05:622a:c1:b0:35d:43de:9b8a with SMTP id p1-20020a05622a00c100b0035d43de9b8amr3310648qtw.428.1664219103239; Mon, 26 Sep 2022 12:05:03 -0700 (PDT) X-Google-Smtp-Source: AMsMyM58dDnriEsWGzzqFxzg79gPCxmCPR+hxAFXKdi1OHa2EVu35GslB6LlKsC2O6V3Ma43pcl1dw== X-Received: by 2002:a05:622a:c1:b0:35d:43de:9b8a with SMTP id p1-20020a05622a00c100b0035d43de9b8amr3310607qtw.428.1664219102829; Mon, 26 Sep 2022 12:05:02 -0700 (PDT) Received: from [192.168.1.130] (ool-457670bb.dyn.optonline.net. [69.118.112.187]) by smtp.gmail.com with ESMTPSA id k7-20020a05620a138700b006b9c6d590fasm11747638qki.61.2022.09.26.12.05.01 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 26 Sep 2022 12:05:02 -0700 (PDT) From: Patrick Palka X-Google-Original-From: Patrick Palka Date: Mon, 26 Sep 2022 15:05:01 -0400 (EDT) To: Patrick Palka cc: Nathan Sidwell , gcc-patches@gcc.gnu.org, jason@redhat.com Subject: Re: [PATCH] c++ modules: ICE with class NTTP argument [PR100616] In-Reply-To: <8728ddf0-ba58-60b3-9083-781901e350f9@idea> Message-ID: <9c72067f-c3ff-50d7-cd07-ca1eec819f82@idea> References: <20220922182502.3218391-1-ppalka@redhat.com> <91a5a188-c4f2-39ff-fff4-8a667f515a19@acm.org> <0745ddf7-8e48-909b-d8e2-a9a1e4064cef@idea> <8728ddf0-ba58-60b3-9083-781901e350f9@idea> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: multipart/mixed; boundary="8323329-2117147436-1664219102=:2760339" X-Spam-Status: No, score=-24.4 required=5.0 tests=BAYES_00,DKIM_INVALID,DKIM_SIGNED,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,RCVD_IN_DNSWL_NONE,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: This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --8323329-2117147436-1664219102=:2760339 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT On Mon, 26 Sep 2022, Patrick Palka wrote: > On Mon, 26 Sep 2022, Nathan Sidwell wrote: > > > On 9/26/22 10:08, Nathan Sidwell wrote: > > > On 9/23/22 09:32, Patrick Palka wrote: > > > > > > > Judging by the two commits that introduced/modified this part of > > > > maybe_register_incomplete_var, r196852 and r214333, ISTM the code > > > > is really only concerned with constexpr static data members (whose > > > > initializer may contain a pointer-to-member for a currently open class). > > > > So maybe we ought to restrict the branch like so, which effectively > > > > disables this part of maybe_register_incomplete_var during stream-in, and > > > > guarantees that outermost_open_class doesn't return NULL if the branch is > > > > taken? > > > > > > I think the problem is that we're streaming these VAR_DECLs as regular > > > VAR_DECLS, when we should be handling them as a new kind of object fished > > > out from the template they're instantiating. (I'm guessing that'll just be a > > > new tag, a type and an initializer?) > > > > > > Then on stream-in we can handle them in the same way as a non-modules > > > compilation handles such redeclarations.  I.e. how does: > > > > > > template struct C { }; > > > struct A { }; > > > C c1; // #1 > > > C c2; // #2 > > > > > > work.  Presumably at some point #2's A{} gets unified such that we find the > > > instantation that occurred at #1? > > This works because the lookup in get_template_parm_object for #2's A{} > finds and reuses the VAR_DECL created for #1's A{}. > > But IIUC this lookup (performed via get_global_binding) isn't > import-aware, which I suppose explains why we don't find the VAR_DECL > from another TU. > > > > > > > I notice the template arg for C is a var decl mangled as _ZTAXtl1AEE, > > > which is a 'template paramete object for A{}'.  I see that's a special > > > mangler 'mangle_template_parm_object', called from > > > get_template_parm_object.  Perhaps these VAR_DECLs need an additional > > > in-tree flag that the streamer can check for? > > > > I wonder if we're setting the module attachment for these variables sanely? > > They should be attached to the global module. My guess is the > > pushdecl_top_level_and_finish call in get_templatE_parm_object is not doing > > what is needed (as well as the other issues). > > This is a bit of a shot in the dark, but the following seems to work: > when pushing the VAR_DECL, we need to call set_originating_module to > attach it to the global module, and when looking it up, we need to do so > in an import-aware way. Hopefully something like this is sufficient > to properly handle these VAR_DECLs and we don't need to stream them > specially? Err, rather than changing the behavior of get_namespace_binding (which has many unrelated callers), I guess we could just use the already import-aware lookup_qualified_name instead where appropriate. WDYT of the following? (testing in progress) -- >8 -- Subject: [PATCH] c++ modules: ICE with class NTTP argument [PR100616] PR c++/100616 PR c++/102576 gcc/cp/ChangeLog: * pt.cc (get_template_parm_object): Use lookup_qualified_name instead of get_global_binding. Call set_originating_module before pushing the VAR_DECL. (listify): Use lookup_qualified_name instead of get_global_binding. gcc/testsuite/ChangeLog: * g++.dg/modules/pr100616_a.C: New test. * g++.dg/modules/pr100616_b.C: New test. * g++.dg/modules/pr102576_a.H: New test. * g++.dg/modules/pr102576_b.C: New test. --- gcc/cp/pt.cc | 10 ++++++---- gcc/testsuite/g++.dg/modules/pr100616_a.C | 8 ++++++++ gcc/testsuite/g++.dg/modules/pr100616_b.C | 8 ++++++++ gcc/testsuite/g++.dg/modules/pr102576_a.H | 5 +++++ gcc/testsuite/g++.dg/modules/pr102576_b.C | 9 +++++++++ 5 files changed, 36 insertions(+), 4 deletions(-) create mode 100644 gcc/testsuite/g++.dg/modules/pr100616_a.C create mode 100644 gcc/testsuite/g++.dg/modules/pr100616_b.C create mode 100644 gcc/testsuite/g++.dg/modules/pr102576_a.H create mode 100644 gcc/testsuite/g++.dg/modules/pr102576_b.C diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc index 1f088fe281e..e030d9db2f6 100644 --- a/gcc/cp/pt.cc +++ b/gcc/cp/pt.cc @@ -7284,8 +7284,8 @@ get_template_parm_object (tree expr, tsubst_flags_t complain) gcc_assert (!TREE_HAS_CONSTRUCTOR (expr)); tree name = mangle_template_parm_object (expr); - tree decl = get_global_binding (name); - if (decl) + tree decl = lookup_qualified_name (global_namespace, name); + if (decl != error_mark_node) return decl; tree type = cp_build_qualified_type (TREE_TYPE (expr), TYPE_QUAL_CONST); @@ -7307,6 +7307,7 @@ get_template_parm_object (tree expr, tsubst_flags_t complain) hash_map_safe_put (tparm_obj_values, decl, copy); } + set_originating_module (decl); pushdecl_top_level_and_finish (decl, expr); return decl; @@ -29150,9 +29151,10 @@ finish_concept_definition (cp_expr id, tree init) static tree listify (tree arg) { - tree std_init_list = get_namespace_binding (std_node, init_list_identifier); + tree std_init_list = lookup_qualified_name (std_node, init_list_identifier); - if (!std_init_list || !DECL_CLASS_TEMPLATE_P (std_init_list)) + if (std_init_list == error_mark_node + || !DECL_CLASS_TEMPLATE_P (std_init_list)) { gcc_rich_location richloc (input_location); maybe_add_include_fixit (&richloc, "", false); diff --git a/gcc/testsuite/g++.dg/modules/pr100616_a.C b/gcc/testsuite/g++.dg/modules/pr100616_a.C new file mode 100644 index 00000000000..788af2eb533 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr100616_a.C @@ -0,0 +1,8 @@ +// PR c++/100616 +// { dg-additional-options "-std=c++20 -fmodules-ts" } +// { dg-module-cmi pr100616 } +export module pr100616; + +template struct C { }; +struct A { }; +C c1; diff --git a/gcc/testsuite/g++.dg/modules/pr100616_b.C b/gcc/testsuite/g++.dg/modules/pr100616_b.C new file mode 100644 index 00000000000..fc89cd08ac5 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr100616_b.C @@ -0,0 +1,8 @@ +// PR c++/100616 +// { dg-additional-options "-std=c++20 -fmodules-ts" } +module pr100616; + +C c2; + +using type = decltype(c1); +using type = decltype(c2); diff --git a/gcc/testsuite/g++.dg/modules/pr102576_a.H b/gcc/testsuite/g++.dg/modules/pr102576_a.H new file mode 100644 index 00000000000..87ba9b52031 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr102576_a.H @@ -0,0 +1,5 @@ +// PR c++/102576 +// { dg-additional-options -fmodule-header } +// { dg-module-cmi {} } + +#include diff --git a/gcc/testsuite/g++.dg/modules/pr102576_b.C b/gcc/testsuite/g++.dg/modules/pr102576_b.C new file mode 100644 index 00000000000..10251ed5304 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr102576_b.C @@ -0,0 +1,9 @@ +// PR c++/102576 +// { dg-additional-options -fmodules-ts } + +import "pr102576_a.H"; + +int main() { + for (int i : {1, 2, 3}) + ; +} -- 2.38.0.rc1.2.g4b79ee4b0c > > -- >8 -- > > Subject: [PATCH] c++ modules: ICE with class NTTP argument [PR100616] > > The function get_template_parm_object returns an artifical mangled > VAR_DECL that's unique to the given class NTTP argument. To enforce > this uniqueness, we first look up the mangled name of the VAR_DECL from > the global scope via get_global_binding, and only create/push a VAR_DECL > if this lookup fails. > > But with modules, we need to do more to enforce uniqueness: the VAR_DECL > needs to be attached to the global module, and the lookup needs to be > import-aware, which get_global_binding currently isn't. > > So this patch makes us call set_originating_module from > get_template_parm_object before pushing, and makes get_namespace_binding > use name_lookup::search_qualified which does look into imports. > > It turns out this change to get_namespace_binding also fixes PR102576 > where we were failing to find an imported std::initializer_list due to > the function not being import-aware. > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for > trunk? > > PR c++/100616 > PR c++/102576 > > gcc/cp/ChangeLog: > > * name-lookup.cc (get_namespace_binding): Rewrite in terms of > name_lookup::search_unqualified. > * pt.cc (get_template_parm_object): Call set_originating_module > before pushing the VAR_DECL. > > gcc/testsuite/ChangeLog: > > * g++.dg/modules/pr100616_a.C: New test. > * g++.dg/modules/pr100616_b.C: New test. > * g++.dg/modules/pr102576_a.H: New test. > * g++.dg/modules/pr102576_b.C: New test. > --- > gcc/cp/name-lookup.cc | 21 ++++++--------------- > gcc/cp/pt.cc | 1 + > gcc/testsuite/g++.dg/modules/pr100616_a.C | 8 ++++++++ > gcc/testsuite/g++.dg/modules/pr100616_b.C | 8 ++++++++ > gcc/testsuite/g++.dg/modules/pr102576_a.H | 5 +++++ > gcc/testsuite/g++.dg/modules/pr102576_b.C | 9 +++++++++ > 6 files changed, 37 insertions(+), 15 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/modules/pr100616_a.C > create mode 100644 gcc/testsuite/g++.dg/modules/pr100616_b.C > create mode 100644 gcc/testsuite/g++.dg/modules/pr102576_a.H > create mode 100644 gcc/testsuite/g++.dg/modules/pr102576_b.C > > diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc > index 69d555ddf1f..6cd73276cf5 100644 > --- a/gcc/cp/name-lookup.cc > +++ b/gcc/cp/name-lookup.cc > @@ -5772,9 +5772,7 @@ do_class_using_decl (tree scope, tree name) > > > /* Return the binding for NAME in NS in the current TU. If NS is > - NULL, look in global_namespace. We will not find declarations > - from imports. Users of this who, having found nothing, push a new > - decl must be prepared for that pushing to match an existing decl. */ > + NULL, look in global_namespace. .*/ > > tree > get_namespace_binding (tree ns, tree name) > @@ -5783,19 +5781,12 @@ get_namespace_binding (tree ns, tree name) > if (!ns) > ns = global_namespace; > gcc_checking_assert (!DECL_NAMESPACE_ALIAS (ns)); > - tree ret = NULL_TREE; > > - if (tree *b = find_namespace_slot (ns, name)) > - { > - ret = *b; > - > - if (TREE_CODE (ret) == BINDING_VECTOR) > - ret = BINDING_VECTOR_CLUSTER (ret, 0).slots[0]; > - if (ret) > - ret = MAYBE_STAT_DECL (ret); > - } > - > - return ret; > + name_lookup lookup (name); > + if (lookup.search_qualified (ns)) > + return lookup.value; > + else > + return NULL_TREE; > } > > /* Push internal DECL into the global namespace. Does not do the > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc > index 1f088fe281e..95cfc5b70a3 100644 > --- a/gcc/cp/pt.cc > +++ b/gcc/cp/pt.cc > @@ -7307,6 +7307,7 @@ get_template_parm_object (tree expr, tsubst_flags_t complain) > hash_map_safe_put (tparm_obj_values, decl, copy); > } > > + set_originating_module (decl); > pushdecl_top_level_and_finish (decl, expr); > > return decl; > diff --git a/gcc/testsuite/g++.dg/modules/pr100616_a.C b/gcc/testsuite/g++.dg/modules/pr100616_a.C > new file mode 100644 > index 00000000000..788af2eb533 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/pr100616_a.C > @@ -0,0 +1,8 @@ > +// PR c++/100616 > +// { dg-additional-options "-std=c++20 -fmodules-ts" } > +// { dg-module-cmi pr100616 } > +export module pr100616; > + > +template struct C { }; > +struct A { }; > +C c1; > diff --git a/gcc/testsuite/g++.dg/modules/pr100616_b.C b/gcc/testsuite/g++.dg/modules/pr100616_b.C > new file mode 100644 > index 00000000000..fc89cd08ac5 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/pr100616_b.C > @@ -0,0 +1,8 @@ > +// PR c++/100616 > +// { dg-additional-options "-std=c++20 -fmodules-ts" } > +module pr100616; > + > +C c2; > + > +using type = decltype(c1); > +using type = decltype(c2); > diff --git a/gcc/testsuite/g++.dg/modules/pr102576_a.H b/gcc/testsuite/g++.dg/modules/pr102576_a.H > new file mode 100644 > index 00000000000..87ba9b52031 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/pr102576_a.H > @@ -0,0 +1,5 @@ > +// PR c++/102576 > +// { dg-additional-options -fmodule-header } > +// { dg-module-cmi {} } > + > +#include > diff --git a/gcc/testsuite/g++.dg/modules/pr102576_b.C b/gcc/testsuite/g++.dg/modules/pr102576_b.C > new file mode 100644 > index 00000000000..10251ed5304 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/pr102576_b.C > @@ -0,0 +1,9 @@ > +// PR c++/102576 > +// { dg-additional-options -fmodules-ts } > + > +import "pr102576_a.H"; > + > +int main() { > + for (int i : {1, 2, 3}) > + ; > +} > -- > 2.38.0.rc1.2.g4b79ee4b0c > --8323329-2117147436-1664219102=:2760339--