On Sun, Jan 12, 2025 at 04:14:41AM +1100, Nathaniel Shead wrote:
> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
>
> -- >8 --
>
> The ICE in the linked PR is caused because name lookup finds duplicate
> copies of the deduction guides, causing a checking assert to fail.
>
> This is ultimately because we're exporting an imported guide; when name
> lookup processes 'dguide-5_b.H' it goes via the 'tt_entity' path and
> just returns the entity from 'dguide-5_a.H'. Because this doesn't ever
> go through 'key_mergeable' we never set 'BINDING_VECTOR_GLOBAL_DUPS_P'
> and so deduping is not engaged, allowing duplicate results.
>
> Currently I believe this to be a perculiarity of the ANY_REACHABLE
> handling for deduction guides; in no other case that I can find do we
> emit bindings purely to imported entities. As such, this patch fixes
> this problem from that end, by ensuring that we simply do not emit any
> imported deduction guides. This avoids the ICE because no duplicates
> need deduping to start with, and should otherwise have no functional
> change because lookup of deduction guides will look at all reachable
> modules (exported or not) regardless.
>
> Since we're now deliberately not emitting imported deduction guides we
> can use LOOK_want::NORMAL instead of LOOK_want::ANY_REACHABLE, since the
> extra work to find as-yet undiscovered deduction guides in transitive
> importers is not necessary here.
>
> PR c++/117397
>
> gcc/cp/ChangeLog:
>
> * module.cc (depset::hash::add_deduction_guides): Don't emit
> imported deduction guides.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/modules/dguide-5_a.H: New test.
> * g++.dg/modules/dguide-5_b.H: New test.
> * g++.dg/modules/dguide-5_c.H: New test.
>
> Signed-off-by: Nathaniel Shead <nathanielosh...@gmail.com>
> ---
> gcc/cp/module.cc | 30 +++++++++++++++--------
> gcc/testsuite/g++.dg/modules/dguide-5_a.H | 6 +++++
> gcc/testsuite/g++.dg/modules/dguide-5_b.H | 6 +++++
> gcc/testsuite/g++.dg/modules/dguide-5_c.H | 7 ++++++
> 4 files changed, 39 insertions(+), 10 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/modules/dguide-5_a.H
> create mode 100644 gcc/testsuite/g++.dg/modules/dguide-5_b.H
> create mode 100644 gcc/testsuite/g++.dg/modules/dguide-5_c.H
>
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index 51990f36284..c4df18b9ca9 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -14341,22 +14341,32 @@ depset::hash::add_deduction_guides (tree decl)
> if (find_binding (ns, name))
> return;
>
> - tree guides = lookup_qualified_name (ns, name, LOOK_want::ANY_REACHABLE,
> + tree guides = lookup_qualified_name (ns, name, LOOK_want::NORMAL,
> /*complain=*/false);
> if (guides == error_mark_node)
> return;
>
> - /* We have bindings to add. */
> - depset *binding = make_binding (ns, name);
> - add_namespace_context (binding, ns);
> + depset *binding = nullptr;
> + for (tree t : lkp_range (guides))
> + {
> + /* We don't want to export imported deduction guides, since searches
> will
> + look there anyway. */
> + if (DECL_LANG_SPECIFIC (STRIP_TEMPLATE (t))
> + && DECL_MODULE_IMPORT_P (STRIP_TEMPLATE (t)))
> + continue;
Actually on further thought, this is not correct; this will prevent us
from exporting declarations inherited from a partition. Here's an
updated version of the patch that will handle this properly (using
depset::is_import which handles this case correctly) and with a bonus
assertion re: bindings of imported decls to help ensure we don't run
into similar errors in the future.
Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
-- >8 --
The ICE in the linked PR is caused because name lookup finds duplicate
copies of the deduction guides, causing a checking assert to fail.
This is ultimately because we're exporting an imported guide; when name
lookup processes 'dguide-5_b.H' it goes via the 'tt_entity' path and
just returns the entity from 'dguide-5_a.H'. Because this doesn't ever
go through 'key_mergeable' we never set 'BINDING_VECTOR_GLOBAL_DUPS_P'
and so deduping is not engaged, allowing duplicate results.
Currently I believe this to be a perculiarity of the ANY_REACHABLE
handling for deduction guides; in no other case that I can find do we
emit bindings purely to imported entities. As such, this patch fixes
this problem from that end, by ensuring that we simply do not emit any
imported deduction guides. This avoids the ICE because no duplicates
need deduping to start with, and should otherwise have no functional
change because lookup of deduction guides will look at all reachable
modules (exported or not) regardless.
Since we're now deliberately not emitting imported deduction guides we
can use LOOK_want::NORMAL instead of LOOK_want::ANY_REACHABLE, since the
extra work to find as-yet undiscovered deduction guides in transitive
importers is not necessary here.
PR c++/117397
gcc/cp/ChangeLog:
* module.cc (depset::hash::add_deduction_guides): Don't emit
imported deduction guides.
(depset::hash::finalize_dependencies): Add check for any
bindings referring to imported entities.
gcc/testsuite/ChangeLog:
* g++.dg/modules/dguide-5_a.H: New test.
* g++.dg/modules/dguide-5_b.H: New test.
* g++.dg/modules/dguide-5_c.H: New test.
* g++.dg/modules/dguide-6.h: New test.
* g++.dg/modules/dguide-6_a.C: New test.
* g++.dg/modules/dguide-6_b.C: New test.
* g++.dg/modules/dguide-6_c.C: New test.
Signed-off-by: Nathaniel Shead <nathanielosh...@gmail.com>
---
gcc/cp/module.cc | 35 ++++++++++++++++-------
gcc/testsuite/g++.dg/modules/dguide-5_a.H | 6 ++++
gcc/testsuite/g++.dg/modules/dguide-5_b.H | 6 ++++
gcc/testsuite/g++.dg/modules/dguide-5_c.H | 7 +++++
gcc/testsuite/g++.dg/modules/dguide-6.h | 4 +++
gcc/testsuite/g++.dg/modules/dguide-6_a.C | 7 +++++
gcc/testsuite/g++.dg/modules/dguide-6_b.C | 8 ++++++
gcc/testsuite/g++.dg/modules/dguide-6_c.C | 12 ++++++++
8 files changed, 75 insertions(+), 10 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/modules/dguide-5_a.H
create mode 100644 gcc/testsuite/g++.dg/modules/dguide-5_b.H
create mode 100644 gcc/testsuite/g++.dg/modules/dguide-5_c.H
create mode 100644 gcc/testsuite/g++.dg/modules/dguide-6.h
create mode 100644 gcc/testsuite/g++.dg/modules/dguide-6_a.C
create mode 100644 gcc/testsuite/g++.dg/modules/dguide-6_b.C
create mode 100644 gcc/testsuite/g++.dg/modules/dguide-6_c.C
diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index 51990f36284..61116fe7669 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -14341,22 +14341,32 @@ depset::hash::add_deduction_guides (tree decl)
if (find_binding (ns, name))
return;
- tree guides = lookup_qualified_name (ns, name, LOOK_want::ANY_REACHABLE,
+ tree guides = lookup_qualified_name (ns, name, LOOK_want::NORMAL,
/*complain=*/false);
if (guides == error_mark_node)
return;
- /* We have bindings to add. */
- depset *binding = make_binding (ns, name);
- add_namespace_context (binding, ns);
+ depset *binding = nullptr;
+ for (tree t : lkp_range (guides))
+ {
+ gcc_checking_assert (!TREE_VISITED (t));
+ depset *dep = make_dependency (t, EK_FOR_BINDING);
- depset **slot = binding_slot (ns, name, /*insert=*/true);
- *slot = binding;
+ /* We don't want to create bindings for imported deduction guides, as
+ this would potentially cause name lookup to return duplicates. */
+ if (dep->is_import ())
+ continue;
+
+ if (!binding)
+ {
+ /* We have bindings to add. */
+ binding = make_binding (ns, name);
+ add_namespace_context (binding, ns);
+
+ depset **slot = binding_slot (ns, name, /*insert=*/true);
+ *slot = binding;
+ }
- for (lkp_iterator it (guides); it; ++it)
- {
- gcc_checking_assert (!TREE_VISITED (*it));
- depset *dep = make_dependency (*it, EK_FOR_BINDING);
binding->deps.safe_push (dep);
dep->deps.safe_push (binding);
}
@@ -14592,6 +14602,11 @@ depset::hash::finalize_dependencies ()
if (dep->deps.length () > 2)
gcc_qsort (&dep->deps[1], dep->deps.length () - 1,
sizeof (dep->deps[1]), binding_cmp);
+
+ /* Bindings shouldn't refer to imported entities. */
+ if (CHECKING_P)
+ for (depset *entity : dep->deps)
+ gcc_checking_assert (!entity->is_import ());
}
else if (dep->is_exposure () && !dep->is_tu_local ())
{
diff --git a/gcc/testsuite/g++.dg/modules/dguide-5_a.H
b/gcc/testsuite/g++.dg/modules/dguide-5_a.H
new file mode 100644
index 00000000000..42421059c7f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/dguide-5_a.H
@@ -0,0 +1,6 @@
+// PR c++/117397
+// { dg-additional-options "-fmodule-header" }
+// { dg-module-cmi {} }
+
+template <typename T> struct S;
+template <typename T> S(T) -> S<T>;
diff --git a/gcc/testsuite/g++.dg/modules/dguide-5_b.H
b/gcc/testsuite/g++.dg/modules/dguide-5_b.H
new file mode 100644
index 00000000000..d31f24d54de
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/dguide-5_b.H
@@ -0,0 +1,6 @@
+// PR c++/117397
+// { dg-additional-options "-fmodule-header" }
+// { dg-module-cmi {} }
+
+import "dguide-5_a.H";
+template <typename T> struct S;
diff --git a/gcc/testsuite/g++.dg/modules/dguide-5_c.H
b/gcc/testsuite/g++.dg/modules/dguide-5_c.H
new file mode 100644
index 00000000000..a9bf2dd0583
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/dguide-5_c.H
@@ -0,0 +1,7 @@
+// PR c++/117397
+// { dg-additional-options "-fmodule-header" }
+// { dg-module-cmi {} }
+
+template <typename T> struct S;
+import "dguide-5_b.H";
+S<int> foo();
diff --git a/gcc/testsuite/g++.dg/modules/dguide-6.h
b/gcc/testsuite/g++.dg/modules/dguide-6.h
new file mode 100644
index 00000000000..f204af49e8c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/dguide-6.h
@@ -0,0 +1,4 @@
+template <typename T> struct S {
+ S(int);
+ S(int, int);
+};
diff --git a/gcc/testsuite/g++.dg/modules/dguide-6_a.C
b/gcc/testsuite/g++.dg/modules/dguide-6_a.C
new file mode 100644
index 00000000000..727336edd90
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/dguide-6_a.C
@@ -0,0 +1,7 @@
+// { dg-additional-options "-fmodules" }
+// { dg-module-cmi M:a }
+
+module;
+#include "dguide-6.h"
+export module M:a;
+S(int) -> S<int>;
diff --git a/gcc/testsuite/g++.dg/modules/dguide-6_b.C
b/gcc/testsuite/g++.dg/modules/dguide-6_b.C
new file mode 100644
index 00000000000..b101d0e1661
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/dguide-6_b.C
@@ -0,0 +1,8 @@
+// { dg-additional-options "-fmodules" }
+// { dg-module-cmi M }
+
+module;
+#include "dguide-6.h"
+export module M;
+export import :a;
+S(int, int) -> S<double>;
diff --git a/gcc/testsuite/g++.dg/modules/dguide-6_c.C
b/gcc/testsuite/g++.dg/modules/dguide-6_c.C
new file mode 100644
index 00000000000..5da934fbade
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/dguide-6_c.C
@@ -0,0 +1,12 @@
+// { dg-additional-options "-fmodules" }
+
+#include "dguide-6.h"
+import M;
+
+int main() {
+ S a(1);
+ S<int> a_copy = a;
+
+ S b(2, 3);
+ S<double> b_copy = b;
+}
--
2.47.0
