On 10/31/24 5:13 AM, Nathaniel Shead wrote:
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?
OK.
-- >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;
+}
