Re: [PATCH v3 2/2] c++: Fix instantiation of imported temploid friends [PR114275]

[Jason Merrill <jason@redhat.com>]

On 4/19/24 09:29, Nathaniel Shead wrote:
> On Fri, Apr 19, 2024 at 12:14:06PM +1000, Nathaniel Shead wrote:
>> On Wed, Apr 17, 2024 at 02:02:21PM -0400, Patrick Palka wrote:
>>> On Mon, 15 Apr 2024, Nathaniel Shead wrote:
>>>
>>>> I'm not a huge fan of always streaming 'imported_temploid_friends' for
>>>> all decls, but I don't think it adds much performance cost over adding a
>>>> new flag to categorise decls that might be marked as such.
>>>
>>> IIUC this value is going to be almost always null which is encoded as a
>>> single 0 byte, which should be fast to stream.  But I wonder how much
>>> larger <bits/stdc++.h> gets?  Can we get away with streaming this value
>>> only for TEMPLATE_DECLs?
>>
>> Yes, it should either just be a 0 byte or an additional backref
>> somewhere, which will likely also be small. On my system it increases
>> the size by 0.26%, from 31186800 bytes to 31268672.
>>
>> But I've just found that this patch has a bug anyway, in that it doesn't
>> correctly dedup if the friend types are instantiated in two separate
>> modules that are then both imported.  I'll see what I need to do to fix
>> this which may influence what we need to stream here.
>>
> 
> Here's an updated version of the patch that fixes this. Also changed to
> only stream when 'inner' is either TYPE_DECL or FUNCTION_DECL, which
> cuts the size of <bits/stdc++.h> down a bit to 31246992 (0.19% growth).
> 
> Another alternative would be to add another boolean flag at the top of
> 'decl_value' and branch on that; that would make use of the bitpacking
> logic and probably cut down on the size further.  (I haven't measured
> this yet though.)
> 
> Bootstrapped and regtested (so far just dg.exp and modules.exp) on
> x86_64-pc-linux-gnu, OK for trunk if full regtest succeeds?
> 
> -- >8 --
> 
> This patch fixes a number of issues with the handling of temploid friend
> declarations.
> 
> The primary issue is that instantiations of friend declarations should
> attach the declaration to the same module as the befriending class, by
> [module.unit] p7.1 and [temp.friend] p2; this could be a different
> module from the current TU, and so needs special handling.

This is only an issue for DECL_UNINSTANTIATED_TEMPLATE_FRIEND_P, right?

Hmm, CWG2588 should probably touch [module.unit]/7.1 as well as 
[basic.link].

> The other main issue here is that we can't assume that just because name
> lookup didn't find a definition for a hidden template class, it doesn't
> mean that it doesn't exist: it could be a non-exported entity that we've
> nevertheless streamed in from an imported module.  We need to ensure
> that when instantiating friend classes that we return the same TYPE_DECL
> that we got from our imports, otherwise we will get later issues with
> 'duplicate_decls' (rightfully) complaining that they're different.

Tricksy.

> This doesn't appear necessary for functions due to the existing name
> lookup handling already finding these hidden declarations.
> 
> 	PR c++/105320
> 	PR c++/114275
> 
> gcc/cp/ChangeLog:
> 
> 	* cp-tree.h (propagate_defining_module): Declare.
> 	(lookup_imported_hidden_friend): Declare.
> 	* decl.cc (duplicate_decls): Also check if hidden declarations
> 	can be redeclared in this module.
> 	* module.cc (imported_temploid_friends): New map.
> 	(init_modules): Initialize it.
> 	(trees_out::decl_value): Write it; don't consider imported
> 	temploid friends as attached to this module.
> 	(trees_in::decl_value): Read it.
> 	(depset::hash::add_specializations): Don't treat instantiations
> 	of a friend type as a specialisation.
> 	(get_originating_module_decl): Follow the owning decl for an
> 	imported temploid friend.
> 	(propagate_defining_module): New function.
> 	* name-lookup.cc (lookup_imported_hidden_friend): New function.
> 	* pt.cc (tsubst_friend_function): Propagate defining module for
> 	new friend functions.
> 	(tsubst_friend_class): Lookup imported hidden friends. Check
> 	for valid redeclaration. Propagate defining module for new
> 	friend classes.
> 
> diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
> index aa66da4829d..56752cf6872 100644
> --- a/gcc/cp/decl.cc
> +++ b/gcc/cp/decl.cc
> @@ -2276,30 +2276,34 @@ duplicate_decls (tree newdecl, tree olddecl, bool hiding, bool was_hidden)
>   
>     if (modules_p ()
>         && TREE_CODE (CP_DECL_CONTEXT (olddecl)) == NAMESPACE_DECL
> -      && TREE_CODE (olddecl) != NAMESPACE_DECL
> -      && !hiding)
> +      && TREE_CODE (olddecl) != NAMESPACE_DECL)
>       {
>         if (!module_may_redeclare (olddecl, newdecl))
>   	return error_mark_node;
>   
> -      tree not_tmpl = STRIP_TEMPLATE (olddecl);
> -      if (DECL_LANG_SPECIFIC (not_tmpl)
> -	  && DECL_MODULE_ATTACH_P (not_tmpl)
> -	  /* Typedefs are not entities and so are OK to be redeclared
> -	     as exported: see [module.interface]/p6.  */
> -	  && TREE_CODE (olddecl) != TYPE_DECL)
> +      if (!hiding)
>   	{
> -	  if (DECL_MODULE_EXPORT_P (STRIP_TEMPLATE (newdecl))
> -	      && !DECL_MODULE_EXPORT_P (not_tmpl))
> +	  /* Hidden friend declarations just use exportingness of the
> +	     old declaration; see CWG2588.  */

I'm not sure what this comment is trying to say, wouldn't hiding be true 
for a hidden friend?

> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index 36e035544f4..bd3ab686543 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -2727,6 +2727,11 @@ vec<tree, va_heap, vl_embed> *post_load_decls;
>   typedef hash_map<tree, auto_vec<tree>> keyed_map_t;
>   static keyed_map_t *keyed_table;
>   
> +/* Instantiations of temploid friends imported from another module
> +   need to be owned by the same module as their instantiating template.
> +   This maps these to the template that instantiated them.  */
> +static hash_map<tree, tree> *imported_temploid_friends;

So IIUC the way to express the attachment to an imported module is by 
association with an actually imported decl.

And the problem is that there's no current way to get from the result of 
tsubst_friend_* to the original temploid friend; as 
tsubst_class_template says, "The new TMPL is not an instantiation of 
anything, so we forget its origins."  And this map provides that lookup?

Rather than "their instantiating template" and "the template that 
instantiated them" I'd just say "the temploid", maybe "the temploid they 
are instantiated from".

I'd also change "owned" to "attached".

> @@ -13370,10 +13393,17 @@ depset::hash::add_specializations (bool decl_p)
>         int use_tpl = 0;
>         bool is_friend = false;
>   
> -      if (decl_p && DECL_UNINSTANTIATED_TEMPLATE_FRIEND_P (entry->tmpl))
> -	/* A friend of a template.  This is keyed to the
> -	   instantiation.  */
> -	is_friend = true;
> +      if (DECL_UNINSTANTIATED_TEMPLATE_FRIEND_P (entry->tmpl))
> +	{
> +	  if (decl_p)
> +	    /* A friend of a template.  This is keyed to the
> +	       instantiation.  */
> +	    is_friend = true;
> +	  else
> +	    /* An instantiated friend struct.  Don't count this as
> +	       a specialization, it'll be picked up later.  */

You mean, in propagate_defining_module?

Would it make sense to call propagate_defining_module here, rather than 
in tsubst_friend_class?

> @@ -18930,6 +18960,12 @@ get_originating_module_decl (tree decl)
>   	  && DECL_UNINSTANTIATED_TEMPLATE_FRIEND_P (decl))
>   	decl = TYPE_NAME (DECL_CHAIN (decl));
>   
> +      /* An imported temploid friend is attached to the same module the
> +	 befriending class was.  */
> +      if (imported_temploid_friends)
> +	if (tree *slot = imported_temploid_friends->get (decl))
> +	  decl = *slot;
> +
>         int use;
>         if (tree ti = node_template_info (decl, use))
>   	{

I note that just below this is

>           decl = TI_TEMPLATE (ti);
>           if (TREE_CODE (decl) != TEMPLATE_DECL)
>             {
>               /* A friend template specialization.  */
>               gcc_checking_assert (OVL_P (decl));
>               return global_namespace;
>             }

which seems to want to say "global module" for a friend template 
specialization, rather than the correct module.  Is this code obsolete 
with this patch?

> diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
> index 7af7f00e34c..dd6e7b6eaea 100644
> --- a/gcc/cp/name-lookup.cc
> +++ b/gcc/cp/name-lookup.cc
> @@ -4453,6 +4453,48 @@ push_local_binding (tree id, tree decl, bool is_using)
>     add_decl_to_level (b, decl);
>   }
>   
> +/* Lookup the FRIEND_TMPL within all module imports.  Used to dedup
> +   instantiations of temploid hidden friends from imported modules.  */
> +
> +tree
> +lookup_imported_hidden_friend (tree friend_tmpl)
> +{
> +  tree inner = DECL_TEMPLATE_RESULT (friend_tmpl);
> +  if (!DECL_LANG_SPECIFIC (inner)
> +      || !DECL_MODULE_IMPORT_P (inner))
> +    return NULL_TREE;
> +
> +  tree name = DECL_NAME (inner);
> +  tree *slot = find_namespace_slot (current_namespace, name);

Maybe checking_assert CP_DECL_CONTEXT (friend_tmpl) == current_namespace?

> +  if (!slot || !*slot || TREE_CODE (*slot) != BINDING_VECTOR)
> +    return NULL_TREE;
> +
> +  /* Look in the appropriate slot, as with check_module_override.  */
> +  binding_slot mslot;
> +  if (named_module_p ())
> +    mslot = BINDING_VECTOR_CLUSTER (*slot, BINDING_SLOT_PARTITION
> +				    / BINDING_VECTOR_SLOTS_PER_CLUSTER)
> +      .slots[BINDING_SLOT_PARTITION % BINDING_VECTOR_SLOTS_PER_CLUSTER];
> +  else
> +    mslot = BINDING_VECTOR_CLUSTER (*slot, 0).slots[BINDING_SLOT_GLOBAL];
> +  gcc_assert (!mslot.is_lazy ());
> +
> +  tree ovl = mslot;
> +  if (!ovl)
> +    return NULL_TREE;
> +
> +  /* We're only interested in declarations coming from the same module
> +     of the friend class we're attempting to instantiate.  */
> +  int m = get_originating_module (friend_tmpl);
> +  gcc_assert (m != 0);
> +
> +  for (ovl_iterator iter (ovl); iter; ++iter)
> +    if (get_originating_module (*iter) == m)
> +      return *iter;
> +
> +  return NULL_TREE;
> +}

Maybe factor much of this out into a get_namespace_binding_in_module 
function?
    \f
> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> index 3b2106dd3f6..e7e7f2fbc3b 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -11723,6 +11733,11 @@ tsubst_friend_class (tree friend_tmpl, tree args)
>   	 of course.  We only need the innermost template parameters
>   	 because that is all that redeclare_class_template will look
>   	 at.  */
> +
> +      if (modules_p ())
> +	/* Check that we can redeclare TMPL in the current context.  */
> +	module_may_redeclare (tmpl, friend_tmpl);

Isn't this redundant with the duplicate_decls change?

Jason