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

[Nathan Sidwell <nathan@acm.org>]

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