diff mbox series

c++: modules ICE with typename friend declaration

Message ID	20220915201627.2942314-1-ppalka@redhat.com
State	Accepted, archived
Headers	Received-SPF: pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 8.43.85.97 as permitted sender) client-ip=8.43.85.97; DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 340B5384BC2C To: gcc-patches@gcc.gnu.org Subject: [PATCH] c++: modules ICE with typename friend declaration Date: Thu, 15 Sep 2022 16:16:27 -0400 Message-Id: <20220915201627.2942314-1-ppalka@redhat.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="US-ASCII"; x-default=true Precedence: list 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+ouuuleilei=gmail.com@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org> X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?=
Series	c++: modules ICE with typename friend declaration \| c++: modules ICE with typename friend declaration

Checks

Context	Check	Description
snail/gcc-patches-check	success	Github commit url

Commit Message

Patrick Palka Sept. 15, 2022, 8:16 p.m. UTC

  A couple of xtreme-header-* modules tests began ICEing in C++23 mode
ever since r13-2650-g5d84a4418aa962 introduced into <ranges> the
dependently scoped friend declaration

  friend /* typename */ _OuterIter::value_type;

ultimately because the streaming code assumes a TYPE_P friend must
be a class type, but here it's a TYPENAME_TYPE, which doesn't have
a TEMPLATE_INFO or CLASSTYPE_BEFRIENDING_CLASSES.  This patch tries
to correct this in a minimal way.

Tested on x86_64-pc-linux-gnu, does this look OK for trunk?

gcc/cp/ChangeLog:

	* module.cc (friend_from_decl_list): Don't consider
	CLASSTYPE_TEMPLATE_INFO for a TYPENAME_TYPE friend.
	(trees_in::read_class_def): Don't add to
	CLASSTYPE_BEFRIENDING_CLASSES for a TYPENAME_TYPE friend.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/typename-friend.C: New test.
---
 gcc/cp/module.cc                               | 5 +++--
 gcc/testsuite/g++.dg/modules/typename-friend.C | 9 +++++++++
 2 files changed, 12 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/typename-friend.C

Comments

Nathan Sidwell Sept. 16, 2022, 7:45 a.m. UTC | #1

Thanks, this looks right. Sigh templates can mess up ones mental invariants!

The test case should really be a foo_[ab].C kind, to test both sides of the
streaming. Bonus points for using the template after importing.  And you
need the dg-module-cmi annotation to check /and then delete/ the gcm file
produced.

nathan

On Thu, Sep 15, 2022, 22:16 Patrick Palka <ppalka@redhat.com> wrote:

> A couple of xtreme-header-* modules tests began ICEing in C++23 mode
> ever since r13-2650-g5d84a4418aa962 introduced into <ranges> the
> dependently scoped friend declaration
>
>   friend /* typename */ _OuterIter::value_type;
>
> ultimately because the streaming code assumes a TYPE_P friend must
> be a class type, but here it's a TYPENAME_TYPE, which doesn't have
> a TEMPLATE_INFO or CLASSTYPE_BEFRIENDING_CLASSES.  This patch tries
> to correct this in a minimal way.
>
> Tested on x86_64-pc-linux-gnu, does this look OK for trunk?
>
> gcc/cp/ChangeLog:
>
>         * module.cc (friend_from_decl_list): Don't consider
>         CLASSTYPE_TEMPLATE_INFO for a TYPENAME_TYPE friend.
>         (trees_in::read_class_def): Don't add to
>         CLASSTYPE_BEFRIENDING_CLASSES for a TYPENAME_TYPE friend.
>
> gcc/testsuite/ChangeLog:
>
>         * g++.dg/modules/typename-friend.C: New test.
> ---
>  gcc/cp/module.cc                               | 5 +++--
>  gcc/testsuite/g++.dg/modules/typename-friend.C | 9 +++++++++
>  2 files changed, 12 insertions(+), 2 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/modules/typename-friend.C
>
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index f27f4d091e5..1a1ff5be574 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -4734,7 +4734,8 @@ friend_from_decl_list (tree frnd)
>        if (TYPE_P (frnd))
>         {
>           res = TYPE_NAME (frnd);
> -         if (CLASSTYPE_TEMPLATE_INFO (frnd))
> +         if (CLASS_TYPE_P (frnd)
> +             && CLASSTYPE_TEMPLATE_INFO (frnd))
>             tmpl = CLASSTYPE_TI_TEMPLATE (frnd);
>         }
>        else if (DECL_TEMPLATE_INFO (frnd))
> @@ -12121,7 +12122,7 @@ trees_in::read_class_def (tree defn, tree
> maybe_template)
>             {
>               tree f = TREE_VALUE (friend_classes);
>
> -             if (TYPE_P (f))
> +             if (CLASS_TYPE_P (f))
>                 {
>                   CLASSTYPE_BEFRIENDING_CLASSES (f)
>                     = tree_cons (NULL_TREE, type,
> diff --git a/gcc/testsuite/g++.dg/modules/typename-friend.C
> b/gcc/testsuite/g++.dg/modules/typename-friend.C
> new file mode 100644
> index 00000000000..d8faf7955c3
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/typename-friend.C
> @@ -0,0 +1,9 @@
> +// { dg-additional-options "-fmodules-ts" }
> +
> +export module x;
> +
> +template<class T>
> +struct A {
> +  friend typename T::type;
> +  friend void f(A) { }
> +};
> --
> 2.37.3.662.g36f8e7ed7d
>
>

Patrick Palka Sept. 16, 2022, 3:54 p.m. UTC | #2

On Fri, 16 Sep 2022, Nathan Sidwell wrote:

> Thanks, this looks right. Sigh templates can mess up ones mental invariants!
> The test case should really be a foo_[ab].C kind, to test both sides of the streaming. Bonus points for using the template after importing.  And you need the dg-module-cmi annotation to check /and then
> delete/ the gcm file produced.

Aha, thanks very much for the pointers, I redid the testcase using
lang-3_[abc].C as an example.  How does the following look?

-- >8 --

gcc/cp/ChangeLog:

	* module.cc (friend_from_decl_list): Don't consider
	CLASSTYPE_TEMPLATE_INFO for a TYPENAME_TYPE friend.
	(trees_in::read_class_def): Don't add to
	CLASSTYPE_BEFRIENDING_CLASSES for a TYPENAME_TYPE friend.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/typename-friend_a.C: New test.
	* g++.dg/modules/typename-friend_b.C: New test.
---
 gcc/cp/module.cc                                 |  5 +++--
 gcc/testsuite/g++.dg/modules/typename-friend_a.C | 11 +++++++++++
 gcc/testsuite/g++.dg/modules/typename-friend_b.C |  6 ++++++
 3 files changed, 20 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/typename-friend_a.C
 create mode 100644 gcc/testsuite/g++.dg/modules/typename-friend_b.C

diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index f27f4d091e5..1a1ff5be574 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -4734,7 +4734,8 @@ friend_from_decl_list (tree frnd)
       if (TYPE_P (frnd))
 	{
 	  res = TYPE_NAME (frnd);
-	  if (CLASSTYPE_TEMPLATE_INFO (frnd))
+	  if (CLASS_TYPE_P (frnd)
+	      && CLASSTYPE_TEMPLATE_INFO (frnd))
 	    tmpl = CLASSTYPE_TI_TEMPLATE (frnd);
 	}
       else if (DECL_TEMPLATE_INFO (frnd))
@@ -12121,7 +12122,7 @@ trees_in::read_class_def (tree defn, tree maybe_template)
 	    {
 	      tree f = TREE_VALUE (friend_classes);
 
-	      if (TYPE_P (f))
+	      if (CLASS_TYPE_P (f))
 		{
 		  CLASSTYPE_BEFRIENDING_CLASSES (f)
 		    = tree_cons (NULL_TREE, type,
diff --git a/gcc/testsuite/g++.dg/modules/typename-friend_a.C b/gcc/testsuite/g++.dg/modules/typename-friend_a.C
new file mode 100644
index 00000000000..aa426fe6cf0
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/typename-friend_a.C
@@ -0,0 +1,11 @@
+// { dg-additional-options "-fmodules-ts" }
+export module foo;
+// { dg-module-cmi foo }
+
+template<class T>
+struct A {
+  friend typename T::type;
+  friend void f(A) { }
+private:
+  static constexpr int value = 42;
+};
diff --git a/gcc/testsuite/g++.dg/modules/typename-friend_b.C b/gcc/testsuite/g++.dg/modules/typename-friend_b.C
new file mode 100644
index 00000000000..97da9d82e11
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/typename-friend_b.C
@@ -0,0 +1,6 @@
+// { dg-additional-options "-fmodules-ts" }
+module foo;
+
+struct C;
+struct B { using type = C; };
+struct C { static_assert(A<B>::value == 42); };

Bernhard Reutner-Fischer Sept. 16, 2022, 6:39 p.m. UTC | #3

On 16 September 2022 17:54:32 CEST, Patrick Palka via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:

>+++ b/gcc/testsuite/g++.dg/modules/typename-friend_a.C
>@@ -0,0 +1,11 @@
>+// { dg-additional-options "-fmodules-ts" }
>+export module foo;
>+// { dg-module-cmi foo }
>+

If that's a constant, repeating pain, you could instrument the test suite so that upon seeing -fmodules-ts (or maybe a later std) it greps for export module and calls dg-module-cmi on those names automatically on its own.
We do the same for stuff like PCH or fortran module .mod or many other cleanup stuff because such manual annotations are IMHO a waste of developer resources..

Just a thought..
cheers,

Nathan Sidwell Sept. 17, 2022, 6:17 a.m. UTC | #4

On 9/16/22 11:54, Patrick Palka wrote:
> On Fri, 16 Sep 2022, Nathan Sidwell wrote:
> 
>> Thanks, this looks right. Sigh templates can mess up ones mental invariants!
>> The test case should really be a foo_[ab].C kind, to test both sides of the streaming. Bonus points for using the template after importing.  And you need the dg-module-cmi annotation to check /and then
>> delete/ the gcm file produced.
> 
> Aha, thanks very much for the pointers, I redid the testcase using
> lang-3_[abc].C as an example.  How does the following look?
> 

yes, that's right, thanks!

nathan

diff mbox series

Patch

diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index f27f4d091e5..1a1ff5be574 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -4734,7 +4734,8 @@  friend_from_decl_list (tree frnd)
       if (TYPE_P (frnd))
 	{
 	  res = TYPE_NAME (frnd);
-	  if (CLASSTYPE_TEMPLATE_INFO (frnd))
+	  if (CLASS_TYPE_P (frnd)
+	      && CLASSTYPE_TEMPLATE_INFO (frnd))
 	    tmpl = CLASSTYPE_TI_TEMPLATE (frnd);
 	}
       else if (DECL_TEMPLATE_INFO (frnd))
@@ -12121,7 +12122,7 @@  trees_in::read_class_def (tree defn, tree maybe_template)
 	    {
 	      tree f = TREE_VALUE (friend_classes);
 
-	      if (TYPE_P (f))
+	      if (CLASS_TYPE_P (f))
 		{
 		  CLASSTYPE_BEFRIENDING_CLASSES (f)
 		    = tree_cons (NULL_TREE, type,
diff --git a/gcc/testsuite/g++.dg/modules/typename-friend.C b/gcc/testsuite/g++.dg/modules/typename-friend.C
new file mode 100644
index 00000000000..d8faf7955c3
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/typename-friend.C
@@ -0,0 +1,9 @@ 
+// { dg-additional-options "-fmodules-ts" }
+
+export module x;
+
+template<class T>
+struct A {
+  friend typename T::type;
+  friend void f(A) { }
+};