On Mon, Jan 13, 2025 at 07:00:18PM -0500, Jason Merrill wrote:
> On 1/12/25 7:03 AM, Nathaniel Shead wrote:
> > 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.
>
> OK.
>
> Do we still need any_reachable in check_module_override?
>
Thanks. And yes, otherwise the check for an ambiguating declaration of
Attached(int) -> Attached<double>;
in dguide-4_c.C doesn't error; we still need to find all reachable
imports for the error 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;
> > +}
>
