From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk1-x72f.google.com (mail-qk1-x72f.google.com [IPv6:2607:f8b0:4864:20::72f]) by sourceware.org (Postfix) with ESMTPS id 22CD938582A0 for ; Fri, 14 Oct 2022 12:04:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 22CD938582A0 Authentication-Results: sourceware.org; dmarc=fail (p=none dis=none) header.from=acm.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-qk1-x72f.google.com with SMTP id s17so1071769qkj.12 for ; Fri, 14 Oct 2022 05:04:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :sender:from:to:cc:subject:date:message-id:reply-to; bh=TeMi/FKZYVU1d5M4TpMQ91Y3Jyt1ACQ2hiVlTPEzuZs=; b=FyPz+JUJE1cXZrmx88VjpFBxS6L8MJW13e27wZATbWtWIbsLk7WqMWHNUCyYbJfYO8 FiCpW0/pQCK1DovI2gqQXTX7iIxXPZrOASQyxB2yDzx4kagK/txKhoVvkhIJCAyTKIjz kDOPVjrB7ph+Fd+DFQBuC348t/g98ydXP5aIj58W/COhSG+Ol3lSk41Hu/HBS2sO6LaW k2mj8HGp8A7E42AQZysZcKYUGIlTOsAdElvP2THTyIJGoje0ajEEK0WvxYMl3WhYG9OG PAKaZdOvxFOxD8kiS0YuATAHcAVMJtEPxpGKo9jnQPspS0el+BZq/cyriMesBKjVkYwN FWrg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :sender:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=TeMi/FKZYVU1d5M4TpMQ91Y3Jyt1ACQ2hiVlTPEzuZs=; b=WBJBZsXXbyNl8FdbxoGyqSGKzmCXgDL/fJpLX+sQJsswWO+9va8YIVrYLByjd9wG4G JIYx580NDilyqJheNtjm0xhAOtNThpg6JaN2rv6Q8e/1mpzNOaC/Awps/XdQngfN7HcH 6iIscXiDYXUVUfTmKNhNHpo51lPvfiBxYhtRldas4zDipvUiuvUDhjVlRnzXKAUX/CBN 83nuvbbUd8ixx8VNh/qWI2L0TUDn42PUZzPKvq4eFB47ZX+7J/sxwsIJ4IOkHi3S0zo5 HE28lDHvO+HLvfockJF4p6SykLb6P+Uhum+XXjrX+3MWEyptE8rY7EBpNy+JoRNYVkcd reVw== X-Gm-Message-State: ACrzQf2jBUhTbg5TlZcNyeBR4fBOQD5+glcKL+fsNb0+I4SIa7O3zLge WHqMaOoKWfUK2tXis7nKHGA= X-Google-Smtp-Source: AMsMyM4P1M7Efxb23EYtH+IKKtu5RNRsgqvpJ24K91HMcTATrDQ+ae2ETMHIuSoHdC2Zd4t5vBfJrQ== X-Received: by 2002:a37:9147:0:b0:6cf:8490:ef53 with SMTP id t68-20020a379147000000b006cf8490ef53mr3335478qkd.640.1665749074267; Fri, 14 Oct 2022 05:04:34 -0700 (PDT) Received: from ?IPV6:2620:10d:c0a8:11d9::105c? ([2620:10d:c091:480::a170]) by smtp.googlemail.com with ESMTPSA id i9-20020a05620a404900b006bc192d277csm2381100qko.10.2022.10.14.05.04.33 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 14 Oct 2022 05:04:33 -0700 (PDT) Sender: Nathan Sidwell Message-ID: Date: Fri, 14 Oct 2022 08:04:33 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.3.1 Subject: Re: [PATCH] c++ modules: ICE with templated friend and std namespace [PR100134] Content-Language: en-US To: Jason Merrill , Patrick Palka , gcc-patches@gcc.gnu.org References: <20221011153507.784631-1-ppalka@redhat.com> <4a5f7f70-9626-f067-0ba9-9bf993da8a34@acm.org> <072f5a5d-2e17-5f29-7707-3d43472053e6@redhat.com> From: Nathan Sidwell In-Reply-To: <072f5a5d-2e17-5f29-7707-3d43472053e6@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-3039.0 required=5.0 tests=BAYES_00,BODY_8BITS,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_EF,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,GIT_PATCH_0,HEADER_FROM_DIFFERENT_DOMAINS,NICE_REPLY_A,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 10/13/22 11:27, Jason Merrill wrote: > On 10/11/22 13:40, Nathan Sidwell wrote: >> On 10/11/22 11:35, Patrick Palka wrote: >>> IIUC the function depset::hash::add_binding_entity has an assert >>> verifying that if a namespace contains an exported entity, then >>> the namespace must have been opened in the module purview: >>> >>>    if (data->hash->add_namespace_entities (decl, data->partitions)) >>>      { >>>        /* It contains an exported thing, so it is exported.  */ >>>        gcc_checking_assert (DECL_MODULE_PURVIEW_P (decl)); >>>        DECL_MODULE_EXPORT_P (decl) = true; >>>      } >>> >>> We're tripping over this assert in the below testcase because by >>> instantiating and exporting std::A, we end up in turn defining >>> and exporting the hidden friend std::f without ever having opening >>> the enclosing namespace std within the module purview and thus >>> DECL_MODULE_PURVIEW_P (std_node) is false. >>> >>> Note that it's important that the enclosing namespace is std here: if we >>> use a different namespace then the ICE disappears.  This probably has >>> something to do with the fact that we predefine std via push_namespace >>> from cxx_init_decl_processing (which makes it look like we've opened the >>> namespace in the TU), whereas with another namespace we would instead >>> lazily obtain the NAMESPACE_DECL from add_imported_namespace. >>> >>> Since templated frined functions are special in that they give us a way >>> to declare a new namespace-scope function without having to explicitly >>> open the namespace, this patch proposes to fix this issue by propagating >>> DECL_MODULE_PURVIEW_P from a friend function to the enclosing namespace >>> when instantiating the friend. >> >> ouch.  This is ok, but I think we have a bug -- what is the module >> ownership of the friend introduced by the instantiation? >> >> Haha, there's a note on 13.7.5/3 -- the attachment is to the same >> module as the befriending class. >> >> That means we end up creating and writing out entities that exist in >> the symbol table (albeit hidden) whose module ownership is neither the >> global module or the tu's module.  That's not something the module >> machinery anticipates. We'll get the mangling wrong for starters. Hmm. I suspect we're writing out the friend definition, regardless of whether the class instantiation is actually referenced from a written-out entity. That's wrong, but it may simply be an inefficiency, rather than an error. >> >> These are probably rare.  Thinking about the right solution though ... > > This seems closely connected to > >  https://cplusplus.github.io/CWG/issues/2588.html > ah, I had gotten myself confused, I don't think there's an ODR[*] problem in the std, this is 'just' a horrible surprise. One of the reasons I disliked the (original) TS's cross-module extern declaration. Not sure of the spelling, but something like: extern export other.module int entity (); was that it had the same requirements as this friend instantiation needs. (this got killed when ATOM ws merged in, and thus never needed to be implemented) [*] The instantiated friend definition is just as temploidy as a non-inline member function of the templated class. And therefore must be just as well-formed to have multiple definitions reachable. Hm, I thought that such in-template-class-defined friends could have no other declaration, but I cannot find the wording for that. >>> Tested on x86_64-pc-linux-gnu, does this look like the right fix?  Other >>> solutions that seem to work are to set DECL_MODULE_PURVIEW_P on std_node >>> after the fact from declare_module, or simply to suppress the assert for >>> std_node. >>> >>>     PR c++/100134 >>> >>> gcc/cp/ChangeLog: >>> >>>     * pt.cc (tsubst_friend_function): Propagate DECL_MODULE_PURVIEW_P >>>     from the new declaration to the enclosing namespace scope. >>> >>> gcc/testsuite/ChangeLog: >>> >>>     * g++.dg/modules/tpl-friend-8_a.H: New test. >>>     * g++.dg/modules/tpl-friend-8_b.C: New test. >>> --- >>>   gcc/cp/pt.cc                                  | 7 +++++++ >>>   gcc/testsuite/g++.dg/modules/tpl-friend-8_a.H | 9 +++++++++ >>>   gcc/testsuite/g++.dg/modules/tpl-friend-8_b.C | 8 ++++++++ >>>   3 files changed, 24 insertions(+) >>>   create mode 100644 gcc/testsuite/g++.dg/modules/tpl-friend-8_a.H >>>   create mode 100644 gcc/testsuite/g++.dg/modules/tpl-friend-8_b.C >>> >>> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc >>> index 5b9fc588a21..9e3085f3fa6 100644 >>> --- a/gcc/cp/pt.cc >>> +++ b/gcc/cp/pt.cc >>> @@ -11448,6 +11448,13 @@ tsubst_friend_function (tree decl, tree args) >>>            by duplicate_decls.  */ >>>         new_friend = old_decl; >>>       } >>> + >>> +      /* We've just added a new namespace-scope entity to the >>> purview without >>> +     necessarily having opened the enclosing namespace, so make sure >>> the >>> +     enclosing namespace is in the purview now too.  */ >>> +      if (TREE_CODE (DECL_CONTEXT (new_friend)) == NAMESPACE_DECL) >>> +    DECL_MODULE_PURVIEW_P (DECL_CONTEXT (new_friend)) >>> +      |= DECL_MODULE_PURVIEW_P (STRIP_TEMPLATE (new_friend)); >>>       } >>>     else >>>       { >>> diff --git a/gcc/testsuite/g++.dg/modules/tpl-friend-8_a.H >>> b/gcc/testsuite/g++.dg/modules/tpl-friend-8_a.H >>> new file mode 100644 >>> index 00000000000..bd2290460b5 >>> --- /dev/null >>> +++ b/gcc/testsuite/g++.dg/modules/tpl-friend-8_a.H >>> @@ -0,0 +1,9 @@ >>> +// PR c++/100134 >>> +// { dg-additional-options -fmodule-header } >>> +// { dg-module-cmi {} } >>> + >>> +namespace std { >>> +  template struct A { >>> +    friend void f(A) { } >>> +  }; >>> +} >>> diff --git a/gcc/testsuite/g++.dg/modules/tpl-friend-8_b.C >>> b/gcc/testsuite/g++.dg/modules/tpl-friend-8_b.C >>> new file mode 100644 >>> index 00000000000..76d7447c2eb >>> --- /dev/null >>> +++ b/gcc/testsuite/g++.dg/modules/tpl-friend-8_b.C >>> @@ -0,0 +1,8 @@ >>> +// PR c++/100134 >>> +// { dg-additional-options -fmodules-ts } >>> +// { dg-module-cmi pr100134 } >>> +export module pr100134; >>> + >>> +import "tpl-friend-8_a.H"; >>> + >>> +export std::A a; >> > -- Nathan Sidwell