diff mbox series

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

Message ID 660e9cbd.620a0220.e9163.84e7@mx.google.com
State New
Headers show
Series [v2] c++/modules: Track declarations imported from partitions [PR99377] | expand

Commit Message

Nathaniel Shead April 4, 2024, 12:27 p.m. UTC 
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?
> 
> Jason
> 

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).

So maybe something like this would make more sense?  (Or using a new
hash_set rather than a new flag on decls, perhaps?)

Bootstrapped and regtested on x86_64-pc-linux-gnu (so far just
modules.exp), OK for trunk if full regtest passes?

-- >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 emitted into the CMI.

We always need to emit definitions from module partitions into the
primary module interface's CMI, as unlike with other kinds of transitive
imports the built CMIs for module partitions are not visible to
importers.

To fix this, rather than relying on the owning module for a declaration
being the partition it was defined in, this patch adds a new flag to
determine whether the declaration we read came from a partition or not,
and then use that flag to determine whether we should treat the
declaration as-if it was provided in this TU.

	PR c++/99377

gcc/cp/ChangeLog:

	* cp-tree.h (DECL_MODULE_PARTITION_P): New.
	(struct lang_decl_base): Add module_partition_p.
	* module.cc (trees_in::install_entity): Set
	DECL_MODULE_PARTITION_P when installing from partition.
	(depset::hash::make_dependency): Use DECL_MODULE_PARTITION_P to
	determine if we treat a decl as-if it came from this TU.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/pr99377-3_a.H: New test.
	* g++.dg/modules/pr99377-3_b.C: New test.
	* g++.dg/modules/pr99377-3_c.C: New test.
	* g++.dg/modules/pr99377-3_d.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 gcc/cp/cp-tree.h                           | 10 ++++++++--
 gcc/cp/module.cc                           | 23 ++++++++++++++--------
 gcc/testsuite/g++.dg/modules/pr99377-3_a.H | 17 ++++++++++++++++
 gcc/testsuite/g++.dg/modules/pr99377-3_b.C | 10 ++++++++++
 gcc/testsuite/g++.dg/modules/pr99377-3_c.C |  5 +++++
 gcc/testsuite/g++.dg/modules/pr99377-3_d.C |  8 ++++++++
 6 files changed, 63 insertions(+), 10 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/pr99377-3_a.H
 create mode 100644 gcc/testsuite/g++.dg/modules/pr99377-3_b.C
 create mode 100644 gcc/testsuite/g++.dg/modules/pr99377-3_c.C
 create mode 100644 gcc/testsuite/g++.dg/modules/pr99377-3_d.C

Comments

Jason Merrill April 9, 2024, 2:37 a.m. UTC | #1 
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 mbox series

Patch

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;
+}