https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114630
--- Comment #11 from Nathaniel Shead <nshead at gcc dot gnu.org> ---
I've taken another quick look at this. First, here's a slightly more minimal
testcase that still errors after my r15-101 change:
// header.h
template <typename _CharT>
void _M_do_parse() {
struct A {};
struct B {};
}
template <typename> struct formatter;
template <> struct formatter<int> {
void parse() { _M_do_parse<int>(); }
};
// pr114630_a.C
module;
#include "header.h"
export module X;
formatter<int> a;
// pr114630_b.C
module;
#include "header.h"
export module Y;
import X;
formatter<int> b;
// pr114630_c.C
#include "header.h"
import Y;
Compile with g++ -fmodules-ts pr114630_*.C to error as before.
I believe the root cause here is actually in pr114630_b.C: it is exporting a
new (duplicate) definition of `_M_do_parse<int>`, but using backreferences to
imported decls of `_M_do_parse<int>::A@X` and `_M_do_parse<int>::B@X`, which
seems wrong to me. I don't think fixing this from the reader side is the
correct way to solve this in general.
I think there are two possible approaches to solve this particular ICE, both of
which are probably desirable for different reasons:
1. As discussed in comment 3, instantiate all pending templates at the end of
the GMF. This will ensure that rather than having a mixed-importedness
`_M_do_parse<int>` instantiation, we instead dedup all the decls coming from
module A after the template is already instantiated and have a consistent view
of the world. We'll still read the function twice from pr114630_c.C but rather
than seeing backreferences we'll see duplicates that we will handle correctly
already.
I've been very slowly working on a patch in this direction, since it's required
for private module fragments and helps with the issues of non-discarded GMF
entities, but I've ran into a lot of issues so far so it'll probably take some
time.
2. The other approach here is to acknowledge that we know the definition of
`_M_do_parse<int>` in module Y must match that in module X; we don't yet check
this but it's IF not to be. As such, there's really no reason that Y should
have to emit the definition at all: it should be perfectly fine to just rely on
users importing transitively from X.
One solution here that *almost* works is to mark a declaration as imported in
odr_duplicate if we know the existing decl already has a definition, like:
diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index fd9b1d3bf2e..679025ab3fc 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -11901,6 +11901,16 @@ trees_in::odr_duplicate (tree maybe_existing, bool
has_defn)
assert_definition (maybe_existing, res && !has_defn);
+ /* Remember that we're deduping this definition with an import,
+ so we don't need to stream our definition to importers. */
+ tree not_tmpl = STRIP_TEMPLATE (maybe_existing);
+ if (res && has_defn
+ && !(VAR_P (not_tmpl) && DECL_VTABLE_OR_VTT_P (not_tmpl)))
+ {
+ gcc_checking_assert (DECL_MODULE_ENTITY_P (not_tmpl));
+ DECL_MODULE_IMPORT_P (not_tmpl) = true;
+ }
+
// FIXME: We probably need to return the template, so that the
// template header can be checked?
return res ? STRIP_TEMPLATE (res) : NULL_TREE;
I say this *almost* works because this unfortunately will cause us to delegate
importing the whole declaration, not just the definition. The reason that we
don't want to do this (and, in general, we want to emit any duplicated
declarations in our own purview) is because our declaration could add
(mergeable) attributes not in the imported declaration, such as default
function arguments.
I think this general approach is still worth exploring however, because apart
from just being a comparatively simple way to solve this ICE, this way we can
also cut down on the number of function definitions we need to emit
transitively from module units using headers or in module partitions, which
should be a small win. We would just need some new flag or map to say that we
should delegate specifically the definition of an entity to an imported module.
I haven't had a chance to explore a 'fixed' version of this 2nd approach yet
though, and likely won't for some time as I'm working on some other issues, but
thought I'd dump my thoughts here in case anyone has any suggestions :)
