On Fri, Oct 25, 2024 at 11:25:15AM -0400, Jason Merrill wrote:
> On 10/24/24 5:10 PM, Nathaniel Shead wrote:
> > On Thu, Oct 24, 2024 at 12:05:18PM -0400, Jason Merrill wrote:
> > > On 10/24/24 3:25 AM, Nathaniel Shead wrote:
> > > > I wasn't sure whether I should include the ambiguity checking logic from
> > > > process_partial_specialization; we don't do this anywhere else in the
> > > > modules handling code that I could see so I left it out for now.
> > >
> > > The relevant bit in the standard seems to be
> > > https://eel.is/c++draft/temp#spec.partial.general-1
> > > "A partial specialization shall be reachable from any use of a template
> > > specialization that would make use of the partial specialization as the
> > > result of an implicit or explicit instantiation; no diagnostic is
> > > required."
> > >
> > > So we aren't required to diagnose an imported partial spec after an
> > > instantiation. It's not entirely clear whether the ambiguity checking in
> > > https://eel.is/c++draft/temp#spec.partial.match-1.2 qualifies as "make
> > > use",
> > > but that seems a natural interpretation.
> > >
> > > So checking these on import seems optional, but could also be helpful in
> > > finding subtle bugs.
> > >
> >
> > Makes sense. I might add this as a follow-up patch then, since if we're
> > doing this here it would make sense to do in add_mergeable_specialization
> > as well now that I think about it.
> >
> > > > I could also rework process_partial_specialization to call the new
> > > > create_mergeable_partial_spec function if you prefer, to reduce code
> > > > duplication, though the early exit for VAR_DECL and the call to
> > > > associate_classtype_constraints that relies on global state complicated
> > > > it enough I didn't bother for this patch.
> > >
> > > That sounds desirable, if we need to create the mergeable partial spec at
> > > all.
> > >
> > > > Finally, I'm not entirely sure whether the choice of DECL_TEMPLATE_PARMS
> > > > that I'm providing is correct, especially considering this hunk:
> > > >
> > > > /* Give template template parms a DECL_CONTEXT of the template
> > > > for which they are a parameter. */
> > > > for (i = 0; i < ntparms; ++i)
> > > > {
> > > > tree parm = TREE_VALUE (TREE_VEC_ELT (inner_parms, i));
> > > > if (TREE_CODE (parm) == TEMPLATE_DECL)
> > > > DECL_CONTEXT (parm) = tmpl;
> > > > }
> > > >
> > > > I haven't been able to produce any testcase that relies on this context
> > > > setting to cause a failure, though, and there doesn't seem to be one in
> > > > the testsuite for the matching hunk in process_partial_specialization.
> > > > Any hints on where this might be needed?
> > >
> > > I believe that's for the benefit of coerce_template_args_for_ttp.
> > >
> >
> > OK thanks, I'll do some exploring to see if I can cause a failure here
> > then.
> >
> > > > In some cases, when we go to import a partial specialisation there might
> > > > already be an incomplete implicit instantiation in the specialisation
> > > > table. This causes ICEs described in the linked PR as we now have two
> > > > separate matching specialisations for this same arguments.
> > > >
> > > > This issue doesn't appear to happen for variable templates as it doesn't
> > > > appear that we can create incomplete specialisations in the same way.
> > > >
> > > > This patch attempts to solve the issue by retrofitting the existing
> > > > implicit instantiation as a partial specialisation (similarly to how
> > > > maybe_process_partial_specialization operates) and then relying on the
> > > > existing merging logic to fill in the definition of the type. This
> > > > works because for types almost all relevant details are streamed and
> > > > merged in 'read_class_def'.
> > >
> > > I find it surprising that we would need to build a partial specialization
> > > to
> > > merge with, when in a single TU we have no trouble merging a partial
> > > specialization with an existing implicit instantiation. Why can't we do
> > > that in this case as well?
> > >
> >
> > As far as I can tell, this is the normal behaviour for declarations;
> > when we do
> >
> > template <typename T> struct S;
> >
> > template <typename T> struct S<T*>
> > ^~~~~
> >
> > This registers an implicit instantiation in the specialisation table
> > already; the parser then calls 'maybe_process_partial_specialization'
> > which overwrites the implicit instantiation as a partial specialization
> > instead, see:
> >
> > /* This is for ordinary explicit specialization and partial
> > specialization of a template class such as:
> >
> > template <> class C<int>;
> >
> > or:
> >
> > template <class T> class C<T*>;
> >
> > Make sure that `C<int>' and `C<T*>' are implicit instantiations. */
> >
> > if (maybe_new_partial_specialization (type))
> > {
> > if (!check_specialization_namespace (CLASSTYPE_TI_TEMPLATE (type))
> > && !at_namespace_scope_p ())
> > return error_mark_node;
> > SET_CLASSTYPE_TEMPLATE_SPECIALIZATION (type);
> > DECL_SOURCE_LOCATION (TYPE_MAIN_DECL (type)) = input_location;
> > if (processing_template_decl)
> > {
> > tree decl = push_template_decl (TYPE_MAIN_DECL (type));
> > if (decl == error_mark_node)
> > return error_mark_node;
> > return TREE_TYPE (decl);
> > }
> > }
> >
> > The difference with modules is that rather than being able to overwrite
> > the existing instantiation with a definition in-place, we instead
> > currently get given a new tree with the entire template/decl/type of a
> > partial specialisation, but there is an existing template/decl/type of
> > an implicit instantiation in the specialisation table (and pointed to by
> > other by decls, in the case of the PR via the return type of a function).
>
> Yes.
>
> > So we need to somehow update in-place the existing type to be the type
> > of the partial specialisation so that we don't break any decls that
> > refer to it already. My approach here is to attempt to copy the
> > relevant parts of what maybe_process_partial_specialization is doing so
> > we can pretend that we're merging an existing partial specialisation
> > (and use the rest of the existing logic) rather than doing part of the
> > in-place update of the implicit instantiation from within
> > add_mergeable_specialization, and then again once we get to
> > read_class_def.
> >
> > Happy to try another route if you think there's something I've missed
> > here though.
>
> Basically I was unclear why we don't automatically merge the types already.
>
> But actually, it seems just as well that the types aren't merged; the
> primary template pattern and its equivalent specialization are separate
> types even though they compare identical, and with concepts we can have
> multiple partial specializations of the same template/args with different
> constraints; it seems like it might be better to consistently distinguish
> the partial specialization pattern from the matching instance of the
> template, even when there's only a single partial specialization of that
> type.
>
> That way, I think we only need to make sure that the A<T*> in the partial
> specialization and the A<T*> in the declaration of f have the same
> TYPE_CANONICAL. The attached seems to be enough to make the testcase
> compile, anyway.
>
> Jason
Interesting; I'm not sure I fully understand how TYPE_CANONICAL
interacts with templates, but I think that makes sense; it also seems to
line up with this comment in 'maybe_new_partial_specialization':
/* We only need a separate type node for storing the definition of this
partial specialization; uses of S<T*> are unconstrained, so all are
equivalent. So keep TYPE_CANONICAL the same. */
TYPE_CANONICAL (t) = TYPE_CANONICAL (type);
I bootstrapped and regtested on x86_64-pc-linux-gnu a variant of your
patch which does indeed seem to work properly. OK for trunk?
-- >8 --
Subject: [PATCH] c++/modules: Propagate TYPE_CANONICAL for partial
specialisations [PR113814]
In some cases, when we go to import a partial specialisation there might
already be an incomplete implicit instantiation in the specialisation
table. This causes ICEs described in the linked PR as we now have two
separate matching specialisations for this same arguments with different
TYPE_CANONICAL.
We already support multiple specialisations with the same args however,
as they may be differently constrained. So we can solve this by simply
ensuring that the TYPE_CANONICAL of the new partial specialisation
matches the existing specialisation.
PR c++/113814
gcc/cp/ChangeLog:
* pt.cc (add_mergeable_specialization): Propagate
TYPE_CANONICAL.
gcc/testsuite/ChangeLog:
* g++.dg/modules/partial-6.h: New test.
* g++.dg/modules/partial-6_a.H: New test.
* g++.dg/modules/partial-6_b.H: New test.
* g++.dg/modules/partial-6_c.C: New test.
Signed-off-by: Nathaniel Shead <nathanielosh...@gmail.com>
Co-authored-by: Jason Merrill <ja...@redhat.com>
---
gcc/cp/pt.cc | 14 +++++++++-----
gcc/testsuite/g++.dg/modules/partial-6.h | 7 +++++++
gcc/testsuite/g++.dg/modules/partial-6_a.H | 11 +++++++++++
gcc/testsuite/g++.dg/modules/partial-6_b.H | 21 +++++++++++++++++++++
gcc/testsuite/g++.dg/modules/partial-6_c.C | 12 ++++++++++++
5 files changed, 60 insertions(+), 5 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/modules/partial-6.h
create mode 100644 gcc/testsuite/g++.dg/modules/partial-6_a.H
create mode 100644 gcc/testsuite/g++.dg/modules/partial-6_b.H
create mode 100644 gcc/testsuite/g++.dg/modules/partial-6_c.C
diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index e20038a81ee..fcff282db37 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -31728,12 +31728,16 @@ add_mergeable_specialization (bool decl_p, spec_entry
*elt, tree decl,
auto *slot = type_specializations->find_slot (elt, INSERT);
/* We don't distinguish different constrained partial type
- specializations, so there could be duplicates. Everything else
- must be new. */
- if (!(flags & 2 && *slot))
+ specializations, so there could be duplicates. In that case we
+ must propagate TYPE_CANONICAL so that they are treated as the
+ same type. Everything else must be new. */
+ if (*slot)
+ {
+ gcc_checking_assert (flags & 2);
+ TYPE_CANONICAL (elt->spec) = TYPE_CANONICAL ((*slot)->spec);
+ }
+ else
{
- gcc_checking_assert (!*slot);
-
auto entry = ggc_alloc<spec_entry> ();
*entry = *elt;
*slot = entry;
diff --git a/gcc/testsuite/g++.dg/modules/partial-6.h
b/gcc/testsuite/g++.dg/modules/partial-6.h
new file mode 100644
index 00000000000..702c9a1b7ec
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/partial-6.h
@@ -0,0 +1,7 @@
+// PR c++/113814
+
+template <typename> struct A {};
+template <typename T> A<T*> f();
+
+template <template <typename> typename, typename> struct B;
+template <template <typename> typename TT> B<TT, int> g();
diff --git a/gcc/testsuite/g++.dg/modules/partial-6_a.H
b/gcc/testsuite/g++.dg/modules/partial-6_a.H
new file mode 100644
index 00000000000..6e0d5ddcaf0
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/partial-6_a.H
@@ -0,0 +1,11 @@
+// PR c++/113814
+// { dg-additional-options "-fmodule-header" }
+// { dg-module-cmi {} }
+
+#include "partial-6.h"
+
+template <typename T>
+struct A<T*> { int a; };
+
+template <template <typename> typename TT>
+struct B<TT, int> { int b; };
diff --git a/gcc/testsuite/g++.dg/modules/partial-6_b.H
b/gcc/testsuite/g++.dg/modules/partial-6_b.H
new file mode 100644
index 00000000000..569959b4f03
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/partial-6_b.H
@@ -0,0 +1,21 @@
+// PR c++/113814
+// { dg-additional-options "-fmodules-ts -fdump-lang-module-alias" }
+// { dg-module-cmi {} }
+
+#include "partial-6.h"
+import "partial-6_a.H";
+
+template <typename, typename = void>
+struct TestTTP;
+
+inline void test() {
+ int a = f<int>().a;
+ int b = g<TestTTP>().b;
+}
+
+// { dg-final { scan-lang-dump {Read:-[0-9]*'s partial merge key \(new\)
template_decl:'::template A'} module } }
+// { dg-final { scan-lang-dump {Read:-[0-9]*'s partial merge key \(new\)
template_decl:'::template B'} module } }
+
+// Don't need to write the partial specialisations
+// { dg-final { scan-lang-dump-not {Wrote declaration entity:[0-9]*
template_decl:'::template A<#null#>'} module } }
+// { dg-final { scan-lang-dump-not {Wrote declaration entity:[0-9]*
template_decl:'::template B<template TT,int>'} module } }
diff --git a/gcc/testsuite/g++.dg/modules/partial-6_c.C
b/gcc/testsuite/g++.dg/modules/partial-6_c.C
new file mode 100644
index 00000000000..2a34457f5af
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/partial-6_c.C
@@ -0,0 +1,12 @@
+// PR c++/113814
+// { dg-additional-options "-fmodules-ts" }
+
+import "partial-6_b.H";
+
+template <typename>
+struct TestTTP2;
+
+int main() {
+ int a = f<double>().a;
+ int b = g<TestTTP2>().b;
+}
--
2.47.0
