Message ID | 660e9cbd.620a0220.e9163.84e7@mx.google.com |
---|---|
State | New |
Headers | show |
Series | [v2] c++/modules: Track declarations imported from partitions [PR99377] | expand |
On 4/4/24 08:27, Nathaniel Shead wrote: > On Wed, Apr 03, 2024 at 02:16:25PM -0400, Jason Merrill wrote: >> On 3/28/24 08:22, Nathaniel Shead wrote: >>> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? >>> >>> -- >8 -- >>> >>> The testcase in comment 15 of the linked PR is caused because the >>> following assumption in depset::hash::make_dependency doesn't hold: >>> >>> if (DECL_LANG_SPECIFIC (not_tmpl) >>> && DECL_MODULE_IMPORT_P (not_tmpl)) >>> { >>> /* Store the module number and index in cluster/section, >>> so we don't have to look them up again. */ >>> unsigned index = import_entity_index (decl); >>> module_state *from = import_entity_module (index); >>> /* Remap will be zero for imports from partitions, which >>> we want to treat as-if declared in this TU. */ >>> if (from->remap) >>> { >>> dep->cluster = index - from->entity_lwm; >>> dep->section = from->remap; >>> dep->set_flag_bit<DB_IMPORTED_BIT> (); >>> } >>> } >>> >>> This is because at least for template specialisations, we first see the >>> declaration in the header unit imported from the partition, and then the >>> instantiation provided by the partition itself. This means that the >>> 'import_entity_index' lookup doesn't report that the specialisation was >>> declared in the partition and thus should be considered as-if it was >>> part of the TU, and get exported. >> >> I think "exported" is the wrong term here; IIUC template specializations are >> not themselves exported, just the template itself. > > Yes, sorry; I meant "emitted" here, in terms of whether the definition > is in the CMI (regardless of whether or not that means that importers > can name it). > >> But if the declaration or point of instantiation of the specialization is >> within a module instantiation unit, it is reachable to any importers, >> including the primary module interface unit importing the partition >> interface unit. >> >> Does this work differently if "check" is a separate module rather than a >> partition? > > Yes. When in a non-partition module (say, Bar), then the instantiation > is emitted into Bar's CMI. When a caller imports Foo, it transitively > streams in Bar as well when looking for the entity and imports its > definition from there. > > However, partitions work differently. In the testcase the instantiation > is emitted into Foo:check's CMI, but partition CMIs are only used within > that module: importers don't know that partitions exist, and only read > Foo's CMI. And because make_dependency doesn't realise that the > instantiation came from a partition it hasn't emitted it into Foo's CMI > which means that importers don't see a definition for it at all. > >>> To fix this, this patch allows, as a special case for installing an >>> entity from a partition, to overwrite the entity_map entry with the >>> (later) index into the partition so that this assumption holds again. >> >> Rather than special-casing partitions, would it make sense to override a >> declaration with a definition? > > And so in this case I think that special-casing partitions is exactly > what needs to happen, because the special case is that it came from a > partition (rather than just it was a definition). > > That said, on further reflection I don't think I like the way I did > this, since it means that for this instantiation, errors will refer to > it as belonging to Foo:check instead of pr99377-3_a.H, which seems > wrong (or at least inconsistent with existing behaviour). Hmm, I don't think it's wrong; that's where the point of instantiation is, and this problem is about that same distinction. So I think I prefer the first patch, just correcting the use of "exported" as discussed above. OK with that change. Jason
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 52d53589e51..802ae002ee2 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -1772,6 +1772,11 @@ check_constraint_info (tree t) #define DECL_MODULE_ENTITY_P(NODE) \ (DECL_LANG_SPECIFIC (DECL_MODULE_CHECK (NODE))->u.base.module_entity_p) +/* True if this decl was provided from a module partition, and should be + treated as-if it was declared in this TU. */ +#define DECL_MODULE_PARTITION_P(NODE) \ + (DECL_LANG_SPECIFIC (DECL_MODULE_CHECK (NODE))->u.base.module_partition_p) + /* DECL that has attached decls for ODR-relatedness. */ #define DECL_MODULE_KEYED_DECLS_P(NODE) \ (DECL_LANG_SPECIFIC (DECL_MODULE_CHECK (NODE))->u.base.module_keyed_decls_p) @@ -2890,16 +2895,17 @@ struct GTY(()) lang_decl_base { unsigned var_declared_inline_p : 1; /* var */ unsigned dependent_init_p : 1; /* var */ - /* The following four apply to VAR, FUNCTION, TYPE, CONCEPT, & NAMESPACE + /* The following five apply to VAR, FUNCTION, TYPE, CONCEPT, & NAMESPACE decls. */ unsigned module_purview_p : 1; /* in named-module purview */ unsigned module_attach_p : 1; /* attached to named module */ unsigned module_import_p : 1; /* from an import */ unsigned module_entity_p : 1; /* is in the entitity ary & hash */ + unsigned module_partition_p : 1; /* imported from a partition */ unsigned module_keyed_decls_p : 1; /* has keys, applies to all decls */ - /* 11 spare bits. */ + /* 10 spare bits. */ }; /* True for DECL codes which have template info and access. */ diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc index 8aab9ea0bae..29e2930485a 100644 --- a/gcc/cp/module.cc +++ b/gcc/cp/module.cc @@ -7650,6 +7650,11 @@ trees_in::install_entity (tree decl) slot = ident; } + if (state->is_partition ()) + /* Remember that this came from a partition, even if we've seen it from a + non-partition before (e.g. for template instantiations). */ + DECL_MODULE_PARTITION_P (not_tmpl) = true; + return true; } @@ -12717,20 +12722,22 @@ depset::hash::make_dependency (tree decl, entity_kind ek) tree not_tmpl = STRIP_TEMPLATE (decl); if (DECL_LANG_SPECIFIC (not_tmpl) - && DECL_MODULE_IMPORT_P (not_tmpl)) + && DECL_MODULE_IMPORT_P (not_tmpl) + /* Treat entities imported from partitions as-if declared in + this TU, unless we ourselves are also a partition. */ + && (!DECL_MODULE_PARTITION_P (not_tmpl) + || module_partition_p ())) { /* Store the module number and index in cluster/section, so we don't have to look them up again. */ unsigned index = import_entity_index (decl); module_state *from = import_entity_module (index); /* Remap will be zero for imports from partitions, which - we want to treat as-if declared in this TU. */ - if (from->remap) - { - dep->cluster = index - from->entity_lwm; - dep->section = from->remap; - dep->set_flag_bit<DB_IMPORTED_BIT> (); - } + should have been covered by the check above. */ + gcc_assert (from->remap); + dep->cluster = index - from->entity_lwm; + dep->section = from->remap; + dep->set_flag_bit<DB_IMPORTED_BIT> (); } if (ek == EK_DECL diff --git a/gcc/testsuite/g++.dg/modules/pr99377-3_a.H b/gcc/testsuite/g++.dg/modules/pr99377-3_a.H new file mode 100644 index 00000000000..580a7631ae1 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr99377-3_a.H @@ -0,0 +1,17 @@ +// { dg-additional-options "-fmodule-header" } +// { dg-module-cmi {} } + +template<typename> +struct Widget +{ + Widget (int) { } + + bool First() const { return true; } + + bool Second () const { return true;} +}; + +inline void Frob (const Widget<int>& w) noexcept +{ + w.First (); +} diff --git a/gcc/testsuite/g++.dg/modules/pr99377-3_b.C b/gcc/testsuite/g++.dg/modules/pr99377-3_b.C new file mode 100644 index 00000000000..5cbce7b3544 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr99377-3_b.C @@ -0,0 +1,10 @@ +// { dg-additional-options "-fmodules-ts" } +// { dg-module-cmi Foo:check } + +export module Foo:check; +import "pr99377-3_a.H"; + +export inline bool Check (const Widget<int>& w) +{ + return w.Second (); +} diff --git a/gcc/testsuite/g++.dg/modules/pr99377-3_c.C b/gcc/testsuite/g++.dg/modules/pr99377-3_c.C new file mode 100644 index 00000000000..fa7c24203bd --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr99377-3_c.C @@ -0,0 +1,5 @@ +// { dg-additional-options "-fmodules-ts" } +// { dg-module-cmi Foo } + +export module Foo; +export import :check; diff --git a/gcc/testsuite/g++.dg/modules/pr99377-3_d.C b/gcc/testsuite/g++.dg/modules/pr99377-3_d.C new file mode 100644 index 00000000000..cb1f28321b1 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr99377-3_d.C @@ -0,0 +1,8 @@ +// { dg-module-do link } +// { dg-additional-options "-fmodules-ts" } + +import Foo; + +int main() { + return Check(0) ? 0 : 1; +}