Re: [PATCH v2] c++/modules: Track declarations imported from partitions [PR99377]

[Jason Merrill <jason@redhat.com>]

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