[PATCH] c++ modules: ICE with templated friend and std namespace [PR100134]

[PATCH] c++ modules: ICE with templated friend and std namespace [PR100134]

[Patrick Palka]

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

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;
-- 
2.38.0.15.gbbe21b64a0

Re: [PATCH] c++ modules: ICE with templated friend and std namespace [PR100134]

[Nathan Sidwell]

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;

-- 
Nathan Sidwell

Re: [PATCH] c++ modules: ICE with templated friend and std namespace [PR100134]

[Jason Merrill]

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;
> 

Re: [PATCH] c++ modules: ICE with templated friend and std namespace [PR100134]

[Nathan Sidwell]

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<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.

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<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;
>>
> 

-- 
Nathan Sidwell