Re: [PATCH] c++/modules: Prefer partition indexes when installing imported entities [PR99377]

2024-04-03 Thread Jason Merrill

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 ();
 }
 }

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.


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?



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



[PATCH] c++/modules: Prefer partition indexes when installing imported entities [PR99377]

2024-03-28 Thread Nathaniel Shead
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 ();
}
}

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.

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.

We only do this for the first time we override with a partition, so that
entities are at least still reported as originating from the first
imported partition that declares them (rather than the last); existing
tests check for this and this seems to be a friendlier approach to go
for, albeit slightly more expensive.

PR c++/99377

gcc/cp/ChangeLog:

* module.cc (trees_in::install_entity): Overwrite entity map
index if installing from a partition.

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 
---
 gcc/cp/module.cc   | 13 +
 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 
 5 files changed, 53 insertions(+)
 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

diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index 8aab9ea0bae..55ca17a88da 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -7649,6 +7649,19 @@ trees_in::install_entity (tree decl)
   gcc_checking_assert (!existed);
   slot = ident;
 }
+  else if (state->is_partition ())
+{
+  /* The decl is already in the entity map but we see it again now from a
+partition: we want to overwrite if the original decl wasn't also from
+a (possibly different) partition.  Otherwise, for things like template
+instantiations, make_dependency might not realise that this is also
+provided from a partition and should be considered part of this module
+(and thus always exported).  */
+  unsigned *slot = entity_map->get (DECL_UID (decl));
+  module_state *imp = import_entity_module (*slot);
+  if (!imp->is_partition ())
+   *slot = ident;
+}
 
   return true;
 }
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 000..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
+struct Widget
+{
+  Widget (int) { }
+
+  bool First() const { return true; }
+
+  bool Second () const { return true;}
+};
+
+inline void Frob (const Widget& 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 000..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& 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 000..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