Re: [PATCH] c++/modules: Ensure all partial specialisations are tracked [PR114947]

[Nathaniel Shead <nathanieloshead@gmail.com>]

On Sun, May 12, 2024 at 11:29:39PM +1000, Nathaniel Shead wrote:
> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> 
> -- >8 --
> 
> Constrained partial specialisations aren't all necessarily tracked on
> the instantiation table.  The modules code uses a separate
> 'partial_specializations' table to track them instead to ensure that
> they get walked and emitted when emitting a module, but currently this
> does not always happen.
> 
> The attached testcase fails in two ways.  First, because the partial
> specialisation is just a declaration (and not a definition),
> 'set_defining_module' never ends up getting called on it and so it never
> gets added to the partial specialisation table.  We fix this by ensuring
> that when partial specializations are created they always get added, and
> so we never miss one. To prevent adding partial specialisations multiple
> times we split this out as a new function.
> 
> The second way it fails is that when exporting the primary interface for
> a module with partitions, we also re-walk the specializations of all
> imported partitions to merge them into a single BMI.  So this patch
> ensures that after calling 'match_mergeable_specialization' we also
> ensure that if the name came from a partition it gets added to the
> specialization table so that a dependency is correctly created for it.
> 
> 	PR c++/114947
> 
> gcc/cp/ChangeLog:
> 
> 	* cp-tree.h (set_defining_module_for_partial_spec): Declare.
> 	* module.cc (trees_in::decl_value): Track partial specs coming
> 	from partitions.
> 	(set_defining_module): Don't track partial specialisations here
> 	anymore.
> 	(set_defining_module_for_partial_spec): New function.
> 	* pt.cc (process_partial_specialization): Call it.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/partial-4_a.C: New test.
> 	* g++.dg/modules/partial-4_b.C: New test.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>  gcc/cp/cp-tree.h                           |  1 +
>  gcc/cp/module.cc                           | 22 ++++++++++++++++++----
>  gcc/cp/pt.cc                               |  2 ++
>  gcc/testsuite/g++.dg/modules/partial-4_a.C |  8 ++++++++
>  gcc/testsuite/g++.dg/modules/partial-4_b.C |  5 +++++
>  5 files changed, 34 insertions(+), 4 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/modules/partial-4_a.C
>  create mode 100644 gcc/testsuite/g++.dg/modules/partial-4_b.C
> 
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index db098c32f2d..2580bf05fb2 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -7418,6 +7418,7 @@ extern unsigned get_importing_module (tree, bool = false) ATTRIBUTE_PURE;
>  /* Where current instance of the decl got declared/defined/instantiated.  */
>  extern void set_instantiating_module (tree);
>  extern void set_defining_module (tree);
> +extern void set_defining_module_for_partial_spec (tree);
>  extern void maybe_key_decl (tree ctx, tree decl);
>  extern void propagate_defining_module (tree decl, tree orig);
>  extern void remove_defining_module (tree decl);
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index 520dd710549..3ca963cb3e9 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -8416,6 +8416,11 @@ trees_in::decl_value ()
>  	  add_mergeable_specialization (!is_type, &spec, decl, spec_flags);
>  	}
>  
> +      /* When making a CMI from a partition we're going to need to walk partial
> +	 specializations again, so make sure they're tracked.  */
> +      if (state->is_partition () && (spec_flags & 2))
> +	set_defining_module_for_partial_spec (inner);
> +
>        if (NAMESPACE_SCOPE_P (decl)
>  	  && (mk == MK_named || mk == MK_unique
>  	      || mk == MK_enum || mk == MK_friend_spec)
> @@ -19246,13 +19251,22 @@ set_defining_module (tree decl)
>  	      vec_safe_push (class_members, decl);
>  	    }
>  	}
> -      else if (DECL_IMPLICIT_TYPEDEF_P (decl)
> -	       && CLASSTYPE_TEMPLATE_SPECIALIZATION (TREE_TYPE (decl)))
> -	/* This is a partial or explicit specialization.  */
> -	vec_safe_push (partial_specializations, decl);
>      }
>  }
>  
> +/* Also remember DECL if it's a newly declared class template partial
> +   specialization, because these are not necessarily added to the
> +   instantiation tables.  */
> +
> +void
> +set_defining_module_for_partial_spec (tree decl)
> +{
> +  if (module_p ()

...but this can be 'module_maybe_has_cmi_p' if my change to
'module_has_cmi_p' to include header units is merged first.

> +      && DECL_IMPLICIT_TYPEDEF_P (decl)
> +      && CLASSTYPE_TEMPLATE_SPECIALIZATION (TREE_TYPE (decl)))
> +    vec_safe_push (partial_specializations, decl);
> +}
> +
>  void
>  set_originating_module (tree decl, bool friend_p ATTRIBUTE_UNUSED)
>  {
> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> index 1816bfd1f40..6d33bac90b0 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -5456,6 +5456,8 @@ process_partial_specialization (tree decl)
>    gcc_checking_assert (!TI_PARTIAL_INFO (tinfo));
>    TI_PARTIAL_INFO (tinfo) = build_template_info (tmpl, NULL_TREE);
>  
> +  set_defining_module_for_partial_spec (decl);
> +
>    for (inst = DECL_TEMPLATE_INSTANTIATIONS (maintmpl); inst;
>         inst = TREE_CHAIN (inst))
>      {
> diff --git a/gcc/testsuite/g++.dg/modules/partial-4_a.C b/gcc/testsuite/g++.dg/modules/partial-4_a.C
> new file mode 100644
> index 00000000000..e74bb0ad866
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/partial-4_a.C
> @@ -0,0 +1,8 @@
> +// PR c++/114947
> +// { dg-additional-options "-fmodules-ts -std=c++20" }
> +// { dg-module-cmi M:part }
> +module M:part;
> +
> +template <typename> struct R {};
> +template <typename T> requires false struct R<T> {};
> +template <typename T> requires true struct R<T>;
> diff --git a/gcc/testsuite/g++.dg/modules/partial-4_b.C b/gcc/testsuite/g++.dg/modules/partial-4_b.C
> new file mode 100644
> index 00000000000..1c645671a55
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/partial-4_b.C
> @@ -0,0 +1,5 @@
> +// PR c++/114947
> +// { dg-additional-options "-fmodules-ts -std=c++20" }
> +// { dg-module-cmi M }
> +export module M;
> +import :part;
> -- 
> 2.43.2
>