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 C74B93858016 for ; Wed, 28 Sep 2022 14:42:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C74B93858016 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=1664376130; 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=XT8ykg/ol1qjc40DiZB+u1Y1AJodolDm4RsJ6/IRMiU=; b=f0wlfqNlCQuc3d8QhiIlOmBB3k9rAnUeaDBUTmLnm91T/YT/beoBZE/6gAZB8RVJNyEqKW JWqpsdP8vMkhbu48tVBKIdVzw22ZcTGMy/78949PPVDCPqGamyeqkAle5QnauyefsgotOb cwuyrs8h9y9CbABTZrKEuBji4JMFgvk= Received: from mail-qv1-f69.google.com (mail-qv1-f69.google.com [209.85.219.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-347-4XtP8E28Mbe8C3YBzx-uDA-1; Wed, 28 Sep 2022 10:42:07 -0400 X-MC-Unique: 4XtP8E28Mbe8C3YBzx-uDA-1 Received: by mail-qv1-f69.google.com with SMTP id ly5-20020a0562145c0500b004af8e9c4d94so3054075qvb.0 for ; Wed, 28 Sep 2022 07:42: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; bh=GuqNtJOL0F0ZJC5NPbX3HUOzV1Mb0FY0Z6ycYvTrJII=; b=v+vD+NC4RAsryj720L/yFMCK3FwKun0U1g1k4HUE/+LPJgSXxFMBXA63Ac5/DOl/vh NWpK/sS3PH3RCtAU3pijMbCSniET2vvtACNmacWb8Esnv5C+VOUK/x1McIdWAG9IxxS1 ddaAbrALGnl+51tyuGJJQLAT+/qjEPSIG3F9Y4dRYZZYKVr1FfuYsp7IZdmI4g8hjh5r lPdlvd2UTPA7b1AYhXH7RAfR8m+k5Yb8GQ5irTAcAjyp8yacIKW0Lq+38dFsdOYPUzWP RQneY+p7ErvxQ/Bett8WH06gbUXB5oFrO2j+lh0p/ydvSwEQXdd2HWl7ygVwGW9x1i9u Bfyg== X-Gm-Message-State: ACrzQf2YL1OcS+RWhc+fSwtyLJJ/Y19wOw7yi4FyYrZfX/vaIichCTYT TcKEqB5t6AhWcaPfA8gnUvr3hH9PqR1KrafXw5h9daZ4nhYZo5ckbUz8ZkWXOdKv/9gSL4BrfWs QT2bpiJewf7nneZbY6g== X-Received: by 2002:a05:622a:4c11:b0:35c:bbb2:d177 with SMTP id ey17-20020a05622a4c1100b0035cbbb2d177mr27465565qtb.314.1664376126441; Wed, 28 Sep 2022 07:42:06 -0700 (PDT) X-Google-Smtp-Source: AMsMyM4K+X2BQEi6rhVPFP6TgQRAODpHwyn7BxEqJ73Cx4EtIO4IL/C/wthQpXnRa1H5FC9FaYG3DQ== X-Received: by 2002:a05:622a:4c11:b0:35c:bbb2:d177 with SMTP id ey17-20020a05622a4c1100b0035cbbb2d177mr27465533qtb.314.1664376126131; Wed, 28 Sep 2022 07:42:06 -0700 (PDT) Received: from [192.168.1.130] (ool-457670bb.dyn.optonline.net. [69.118.112.187]) by smtp.gmail.com with ESMTPSA id u35-20020a05622a19a300b0035bbb6268e2sm3179716qtc.67.2022.09.28.07.42.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 28 Sep 2022 07:42:05 -0700 (PDT) From: Patrick Palka X-Google-Original-From: Patrick Palka Date: Wed, 28 Sep 2022 10:42:04 -0400 (EDT) To: Nathan Sidwell cc: Patrick Palka , gcc-patches@gcc.gnu.org, jason@redhat.com Subject: Re: [PATCH] c++ modules: ICE with class NTTP argument [PR100616] In-Reply-To: Message-ID: <5943173d-4a56-c759-d1c2-80f389723ff1@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> <9c72067f-c3ff-50d7-cd07-ca1eec819f82@idea> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: multipart/mixed; boundary="8323329-1603249459-1664376125=:1716" X-Spam-Status: No, score=-14.6 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 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-1603249459-1664376125=:1716 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT On Tue, 27 Sep 2022, Nathan Sidwell wrote: > On 9/26/22 15:05, Patrick Palka wrote: > > 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) > > I'm going to have to think further. Morally these VAR_DECLs are like the > typeinfo objects -- which we have to handle specially. Reminding myself, I > see rtti.cc does the pushdecl_top_level stuff creating them -- so they go into > the slot for the current TU. But the streaming code writes > tt_tinfo_var/tt_tinfo_typedef records, and recreates the typeinfo on stream > in, using the same above pushdec_top_level path. So even though the tinfo > decls might seem attached to the current module, that doesn;t confuse the > streaming, nor create collisions on read back. Nor do we stream out tinfo > decls that are not reachable through the streamed AST (if we treated then as > normal decls, we'd stream them cos they're inthe current TU in the symbol > table. I have the same fear for these NTTPs.) > > It looks like TREE_LANG_FLAG_5 can be used to note these VAR_DECLs are NTTPs, > and then the streaming can deal with them. Let me look further. I see, thanks very much for the enlightening explanation. > > > @@ -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); > > this is wrong. You're attaching the decl to the current module. which will > mean conflicts when reading in such VAR_DECLs for the same NTTP from different > modules. Your test case might be hiding that as you have an interface and > implementation unit from the same module (but you should be getting some kind > of multiple definition error anyway?) Makes sense. Indeed this approach falls apart for the following testcase which uses the same NTTP argument in two different modules, for the reasons you mentioned: $ cat 100616_a.H template struct C { }; struct A { }; $ cat 100616_b.C export module pr100616_b; import "100616_a.H"; export C c1; $ cat 100616_c.C export module pr100616_c; import "100616_a.H"; export C c2; $ cat 100616_d.C import pr100616_b; import pr100616_c; using type = decltype(c1); using type = decltype(c2); // bogus error: types of c1 and c2 don't match > > > > 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); What do you think about this independent change to use lookup_qualified_name instead of get_namespace_binding in listify so that the lookup for std::initializer_list is import-aware, which seems to fix PR102576? > > 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}) > > + ; > > +} > > -- > Nathan Sidwell > > --8323329-1603249459-1664376125=:1716--