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
