From: Jason Merrill <jason@redhat.com>
To: Nathaniel Shead <nathanieloshead@gmail.com>, gcc-patches@gcc.gnu.org
Cc: Nathan Sidwell <nathan@acm.org>
Subject: Re: [PATCH] c++/modules: Ensure all partial specialisations are tracked [PR114947]
Date: Thu, 23 May 2024 15:56:59 -0400 [thread overview]
Message-ID: <5164fd27-5a68-4b07-954c-e57e519b8ebd@redhat.com> (raw)
In-Reply-To: <6640c449.170a0220.9790b.fc84@mx.google.com>
On 5/12/24 09:29, 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.
Hmm, I wonder if it would make sense to move
DECL_TEMPLATE_SPECIALIZATIONS from the template's DECL_SIZE into a hash
table that the modules code could walk instead of managing its own
separate table?
The patch is OK.
> 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 ()
> + && 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;
prev parent reply other threads:[~2024-05-23 19:57 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-12 13:29 Nathaniel Shead
2024-05-23 13:33 ` Nathaniel Shead
2024-05-23 19:56 ` Jason Merrill [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5164fd27-5a68-4b07-954c-e57e519b8ebd@redhat.com \
--to=jason@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=nathan@acm.org \
--cc=nathanieloshead@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).