Message ID | 20221011153507.784631-1-ppalka@redhat.com |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id CE4263858401 for <patchwork@sourceware.org>; Tue, 11 Oct 2022 15:35:44 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org CE4263858401 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1665502544; bh=Kkh8R2DAx4bDikwBTF+SaKix5RSTtshHn/voynFMo88=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=HQX0MQ9+YcFpnjHxSpUJJLQzywY2tmY2JTV4ToBD0E8owDJ/sCtOTi8Si/RFKrJcu pqgIwUQexUv+7m7BSgdKIFRp+Hz7R9VnTVIt44FRFhlXnfbhA8X4f5KcLd6TB9PXGe HhXWuDY66+GhkZbS+I6mR4aapbPA5VrCAVOXFyQs= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org 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 242B53858C2D for <gcc-patches@gcc.gnu.org>; Tue, 11 Oct 2022 15:35:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 242B53858C2D Received: from mail-qk1-f197.google.com (mail-qk1-f197.google.com [209.85.222.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-92-Ga2M2OwcMmubLa2j9t_ydw-1; Tue, 11 Oct 2022 11:35:10 -0400 X-MC-Unique: Ga2M2OwcMmubLa2j9t_ydw-1 Received: by mail-qk1-f197.google.com with SMTP id j13-20020a05620a410d00b006e08208eb31so11871506qko.3 for <gcc-patches@gcc.gnu.org>; Tue, 11 Oct 2022 08:35:10 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=Kkh8R2DAx4bDikwBTF+SaKix5RSTtshHn/voynFMo88=; b=YmOiBT1s9XF3NklTnDLxIhwgM2G2LTypEldXBnhmNrGrxKspfg9k4+UjD7MBRGd3YT lmnUuhYKwWO6POgbKvTZMgspWt3WbGrXo6SPchrEfQz0dlCJqVFKQqLeZ2TYryS+dd/8 ALTwIziT0nkmT4dQ85JEi1oSoo1ej6SOZHDPh4u4hvegoCF3W9AU1j7nnP8BZ4AF1zSG a+SA5uhh2vniB/BG9kJEu3YqLddOJCai3XnzjqGZFG162RZW/gEEf7TSsZYKj4+NlcMg dG8RDSTPiiXeg/l5s7EM5hNZgcUatUswNqcsOqpd3TJ+zk5xKUlgVCNdItCOj+5m5j/9 ZE/Q== X-Gm-Message-State: ACrzQf21EDgZL/zLLl4TKAEgEZhgBu4ypzWXxNUgAcSbdG5sK7Wxb6Sh URUm2iold3TlvFImk3GCgVO4AGOaJ54En9iAlc9XmtFpYYl7MZ/BKGsmVaRZ0bKjKY8ECFiV0xy 4mioMVTJU4DRdgq8VBwkJK8X3c+NEtRsgIeXBhs3HkuDVoiUJ3qSqw+OwLQbpfs76+Iw= X-Received: by 2002:a05:6214:d0d:b0:4b1:996d:c200 with SMTP id 13-20020a0562140d0d00b004b1996dc200mr19148814qvh.9.1665502509655; Tue, 11 Oct 2022 08:35:09 -0700 (PDT) X-Google-Smtp-Source: AMsMyM4jzcVqbzub81VzDU4oMBxbEYAcVCeyOtX1DGoXOcFu3V1CPKahuefJEzVtgC/exciDSkX96Q== X-Received: by 2002:a05:6214:d0d:b0:4b1:996d:c200 with SMTP id 13-20020a0562140d0d00b004b1996dc200mr19148782qvh.9.1665502509281; Tue, 11 Oct 2022 08:35:09 -0700 (PDT) Received: from localhost.localdomain (ool-457670bb.dyn.optonline.net. [69.118.112.187]) by smtp.gmail.com with ESMTPSA id cb16-20020a05622a1f9000b0039cb5c9dbacsm831829qtb.22.2022.10.11.08.35.08 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 11 Oct 2022 08:35:08 -0700 (PDT) To: gcc-patches@gcc.gnu.org Subject: [PATCH] c++ modules: ICE with templated friend and std namespace [PR100134] Date: Tue, 11 Oct 2022 11:35:07 -0400 Message-Id: <20221011153507.784631-1-ppalka@redhat.com> X-Mailer: git-send-email 2.38.0.15.gbbe21b64a0 MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="US-ASCII"; x-default=true X-Spam-Status: No, score=-14.0 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_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 X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Patrick Palka via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Patrick Palka <ppalka@redhat.com> Cc: nathan@acm.org Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
c++ modules: ICE with templated friend and std namespace [PR100134]
|
|
Commit Message
Patrick Palka
Oct. 11, 2022, 3:35 p.m. UTC
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<int>, 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. 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
Comments
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<int>, 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. These are probably rare. Thinking about the right solution though ... nathan > > 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<class T> 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<int> a;
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<int>, 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. > > These are probably rare.? Thinking about the right solution though ... This seems closely connected to https://cplusplus.github.io/CWG/issues/2588.html Jason > nathan > > >> >> 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<class T> 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<int> a; >
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<class T> 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<int> a;