| Message ID | 66b210fc.630a0220.268b03.48d0@mx.google.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] c++/modules: Ensure deduction guides are always reachable [PR115231] | expand |
On 8/6/24 8:03 AM, Nathaniel Shead wrote: > On Fri, Jul 26, 2024 at 01:17:57PM -0400, Jason Merrill wrote: >> On 7/26/24 12:52 AM, Nathaniel Shead wrote: >>> On Tue, Jul 23, 2024 at 04:17:22PM -0400, Jason Merrill wrote: >>>> On 6/15/24 10:29 PM, Nathaniel Shead wrote: >>>>> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? >>>>> >>>>> This probably isn't the most efficient approach, since we need to do >>>>> name lookup to find deduction guides for a type which will also >>>>> potentially do a bunch of pointless lazy loading from imported modules, >>>>> but I wasn't able to work out a better approach without completely >>>>> reworking how deduction guides are stored and represented. >>>> >>>> Indeed. We likely want to find them more directly from the template; it's >>>> not clear to me that DECL_INITIAL is used for TEMPLATE_DECL, or we could put >>>> them in an internal attribute or a separate hash table. >>>> > > I had a go at exploring some other representations of deduction guides > but I didn't really land anywhere; most things I tried would require > reimplementing a lot of the merging logic handled by name lookup > currently. Potentially just having an additional hash table for > deduction guides maintained by pushdecl would work but that felt like > unnecessary duplication, I'm not sure that the benefits outweight the > cost of the name lookup. (But I haven't measured this.) > >>>>> -- >8 -- >>>>> >>>>> Deduction guides are represented as 'normal' functions currently, and >>>>> have no special handling in modules. However, this causes some issues; >>>>> by [temp.deduct.guide] a deduction guide is not found by normal name >>>>> lookup and instead all reachable deduction guides for a class template >>>>> should be considered, but this does not happen currently. >>>>> >>>>> To solve this, this patch ensures that all deduction guides are >>>>> considered exported to ensure that they are always visible to importers >>>>> if they are reachable. Another alternative here would be to add a new >>>>> kind of "all reachable" flag to name lookup, but that is complicated by >>>>> some difficulties in handling GM entities; this may be a better way to >>>>> go if more kinds of entities end up needing this handling, however. >>>>> >>>>> Another issue here is that because deduction guides are "unrelated" >>>>> functions, they will usually get discarded from the GMF, so this patch >>>>> ensures that when finding dependencies, GMF deduction guides will also >>>>> have bindings created. We do this in find_dependencies so that we don't >>>>> unnecessarily create bindings for GMF deduction guides that are never >>>>> reached; for consistency we do this for *all* deduction guides, not just >>>>> GM ones. >>>> >>>> If you fixed the dependency calculation, why do they also need to be >>>> exported? >>> >>> Deduction guides aren't found using normal name lookup, but any >>> reachable deduction guide must be considered. This means that even if >>> the module interface exports no declarations whatsoever, a deduction >>> guide declared in the module purview must still be considered by >>> importers. >> >> Ah, I was missing the name lookup issue. >> >>> The other option I've considered is adding a new "ANY_REACHABLE" flag to >>> name lookup which would also consider non-exported reachable decls. On >>> further consideration I might actually go this way; I've been thinking >>> about how to resolve some issues adjacent to supporting textual >>> redefinitions that I believe this will be necessary for anyway, and we >>> can probably use this in tsubst_friend_class as well rather than the >>> current relatively ad-hoc solution. >> >> There's also my hack in lookup_elaborated_type for ABI namespace types. >> >> I'm not sure that it should be necessary to do this for redefinitions, >> though; what's the advantage over merging in check_module_override (apart >> from that needing to be fixed)? >> >>> That said, I've realised that this patch isn't completely sufficient >>> anyway; consider: >>> >>> // m.cpp >>> module; >>> template <typename T> struct S; >>> export module M; >>> S(int) -> S<double>; >>> >>> // x.cpp >>> template <typename T> struct S { S(int); }; >>> import M; >>> int main() { >>> S s(10); // should be S<double> s; >>> } >>> >>> This patch doesn't correctly handle this case yet, we need to also >>> consider cases where only the deduction guide is in purview. >> >> Indeed. >> >> Jason >> > > Here's a small update to the patch which fixes the above bug by ensuring > that dependencies are created for purview deduction guides rather than > just being skipped entirely, and includes that testcase with dguide-3. > > This patch is still using force-exported bindings rather than attempting > to do anything more clever with name lookup just yet. > > Bootstrapped and regtested (so far just modules.exp) on > x86_64-pc-linux-gnu, OK for trunk if full regtest succeeds? > > -- >8 -- > > Deduction guides are represented as 'normal' functions currently, and > have no special handling in modules. However, this causes some issues; > by [temp.deduct.guide] a deduction guide is not found by normal name > lookup and instead all reachable deduction guides for a class template > should be considered, but this does not happen currently. > > To solve this, this patch ensures that all deduction guides are > considered exported to ensure that they are always visible to importers > if they are reachable. Another alternative here would be to add a new > kind of "all reachable" flag to name lookup, but that is complicated by > some difficulties in handling GM entities; this may be a better way to > go if more kinds of entities end up needing this handling, however. > > Another issue here is that because deduction guides are "unrelated" > functions, they will usually get discarded from the GMF, so this patch > ensures that when finding dependencies, GMF deduction guides will also > have bindings created. We do this in find_dependencies so that we don't > unnecessarily create bindings for GMF deduction guides that are never > reached; for consistency we do this for *all* deduction guides, not just > GM ones. We also make sure that the opposite (a deduction guide being > the only purview reference to a GMF template) correctly marks it as > reachable. > > Finally, when merging deduction guides from multiple modules, the name > lookup code may now return two-dimensional overload sets, so update > callers to match. > > As a small drive-by improvement this patch also updates the error pretty > printing code to add a space before the '->' when printing a deduction > guide, so we get 'S(int) -> S<int>' instead of 'S(int)-> S<int>'. > > @@ -15158,6 +15218,10 @@ module_state::write_cluster (elf_out *to, depset *scc[], unsigned size, > flags |= cbf_hidden; > else if (DECL_MODULE_EXPORT_P (STRIP_TEMPLATE (bound))) > flags |= cbf_export; > + else if (deduction_guide_p (bound)) > + /* Deduction guides are always exported so that they are > + reachable whenever their class template is. */ I'd adjust "reachable" to "visible to name lookup"; they don't need to be exported to be reachable. OK with that change. Jason
diff --git a/gcc/cp/error.cc b/gcc/cp/error.cc index d80bac822ba..601e5672174 100644 --- a/gcc/cp/error.cc +++ b/gcc/cp/error.cc @@ -1871,6 +1871,7 @@ dump_function_decl (cxx_pretty_printer *pp, tree t, int flags) dump_type_suffix (pp, ret, flags); else if (deduction_guide_p (t)) { + pp->set_padding (pp_before); pp_cxx_ws_string (pp, "->"); dump_type (pp, TREE_TYPE (TREE_TYPE (t)), flags); } diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc index d1607a06757..aef010e91d1 100644 --- a/gcc/cp/module.cc +++ b/gcc/cp/module.cc @@ -2589,6 +2589,9 @@ public: void add_partial_entities (vec<tree, va_gc> *); void add_class_entities (vec<tree, va_gc> *); + private: + void add_deduction_guides (tree decl); + public: void find_dependencies (module_state *); bool finalize_dependencies (); @@ -13172,6 +13175,15 @@ depset::hash::add_binding_entity (tree decl, WMB_Flags flags, void *data_) /* Ignore NTTP objects. */ return false; + if (deduction_guide_p (decl)) + { + /* Ignore deduction guides, bindings for them will be created within + find_dependencies for their class template. But still build a dep + for them so that we don't discard them. */ + data->hash->make_dependency (decl, EK_FOR_BINDING); + return false; + } + if (!(flags & WMB_Using) && CP_DECL_CONTEXT (decl) != data->ns) { /* An unscoped enum constant implicitly brought into the containing @@ -13600,6 +13612,50 @@ find_pending_key (tree decl, tree *decl_p = nullptr) return ns; } +/* Creates bindings and dependencies for all deduction guides of + the given class template DECL as needed. */ + +void +depset::hash::add_deduction_guides (tree decl) +{ + /* Alias templates never have deduction guides. */ + if (DECL_ALIAS_TEMPLATE_P (decl)) + return; + + /* We don't need to do anything for class-scope deduction guides, + as they will be added as members anyway. */ + if (!DECL_NAMESPACE_SCOPE_P (decl)) + return; + + tree ns = CP_DECL_CONTEXT (decl); + tree name = dguide_name (decl); + + /* We always add all deduction guides with a given name at once, + so if there's already a binding there's nothing to do. */ + if (find_binding (ns, name)) + return; + + 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 **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); + } +} + /* Iteratively find dependencies. During the walk we may find more entries on the same binding that need walking. */ @@ -13659,6 +13715,10 @@ depset::hash::find_dependencies (module_state *module) } walker.end (); + if (!walker.is_key_order () + && DECL_CLASS_TEMPLATE_P (decl)) + add_deduction_guides (decl); + if (!walker.is_key_order () && TREE_CODE (decl) == TEMPLATE_DECL && !DECL_UNINSTANTIATED_TEMPLATE_FRIEND_P (decl)) @@ -15158,6 +15218,10 @@ module_state::write_cluster (elf_out *to, depset *scc[], unsigned size, flags |= cbf_hidden; else if (DECL_MODULE_EXPORT_P (STRIP_TEMPLATE (bound))) flags |= cbf_export; + else if (deduction_guide_p (bound)) + /* Deduction guides are always exported so that they are + reachable whenever their class template is. */ + flags |= cbf_export; } gcc_checking_assert (DECL_P (bound)); diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc index 77fa5907c3d..53917cb6dc6 100644 --- a/gcc/cp/pt.cc +++ b/gcc/cp/pt.cc @@ -30739,7 +30739,7 @@ deduction_guides_for (tree tmpl, bool &any_dguides_p, tsubst_flags_t complain) else { cands = ctor_deduction_guides_for (tmpl, complain); - for (ovl_iterator it (guides); it; ++it) + for (lkp_iterator it (guides); it; ++it) cands = lookup_add (*it, cands); } diff --git a/gcc/testsuite/g++.dg/modules/dguide-1_a.C b/gcc/testsuite/g++.dg/modules/dguide-1_a.C new file mode 100644 index 00000000000..834e033eae3 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/dguide-1_a.C @@ -0,0 +1,44 @@ +// PR c++/115231 +// { dg-additional-options "-fmodules-ts -Wno-global-module" } +// { dg-module-cmi M } + +module; + +template <typename T> +struct A { + template <typename U> A(U); +}; + +template <typename T> A(T) -> A<T>; + +export module M; + +// Exporting a GMF entity should make the deduction guides reachable. +export using ::A; + + +export template <typename T> +struct B { + template <typename U> B(U); +}; + +// Not exported, but should still be reachable by [temp.deduct.guide] p1. +B(int) -> B<double>; + + +// Class-scope deduction guides should be reachable as well, even if +// the class body was not exported. +export template <typename T> struct C; + +template <typename T> +struct C { + template <typename U> + struct I { + template <typename V> I(V); + }; + + I(int) -> I<int>; + + template <typename P> + I(const P*) -> I<P>; +}; diff --git a/gcc/testsuite/g++.dg/modules/dguide-1_b.C b/gcc/testsuite/g++.dg/modules/dguide-1_b.C new file mode 100644 index 00000000000..97266986d8f --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/dguide-1_b.C @@ -0,0 +1,20 @@ +// PR c++/115231 +// { dg-additional-options "-fmodules-ts" } + +import M; + +int main() { + // Check that deduction guides are reachable, + // and that they declared the right type. + A a(1); + A<int> a2 = a; + + B b(2); + B<double> b2 = b; + + C<int>::I x(10); + C<int>::I<int> x2 = x; + + C<int>::I y("xyz"); + C<int>::I<char> y2 = y; +} diff --git a/gcc/testsuite/g++.dg/modules/dguide-2_a.C b/gcc/testsuite/g++.dg/modules/dguide-2_a.C new file mode 100644 index 00000000000..fcd6c579813 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/dguide-2_a.C @@ -0,0 +1,24 @@ +// PR c++/115231 +// { dg-additional-options "-fmodules-ts -Wno-global-module" } +// { dg-module-cmi M } + +module; + +template <typename T> +struct A { + template <typename U> A(U); +}; +template <typename T> A(T) -> A<T>; + +export module M; + +template <typename T> +struct B { + template <typename U> B(U); +}; +B(int) -> B<int>; + +// Accessing deduction guides should be possible, +// even if we can't name the type directly. +export A<void> f(); +export B<void> g(); diff --git a/gcc/testsuite/g++.dg/modules/dguide-2_b.C b/gcc/testsuite/g++.dg/modules/dguide-2_b.C new file mode 100644 index 00000000000..ca31306aea3 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/dguide-2_b.C @@ -0,0 +1,19 @@ +// PR c++/115231 +// { dg-additional-options "-fmodules-ts" } + +import M; + +template <typename> +struct U; + +template <template <typename> typename TT, typename Inner> +struct U<TT<Inner>> { + void go() { + TT t(10); + } +}; + +int main() { + U<decltype(f())>{}.go(); + U<decltype(g())>{}.go(); +} diff --git a/gcc/testsuite/g++.dg/modules/dguide-3_a.C b/gcc/testsuite/g++.dg/modules/dguide-3_a.C new file mode 100644 index 00000000000..33350ce9804 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/dguide-3_a.C @@ -0,0 +1,10 @@ +// { dg-additional-options "-fmodules-ts" } +// { dg-module-cmi A } + +export module A; + +extern "C++" { + template <typename T> struct S; + S(int) -> S<int>; + S(double) -> S<double>; +} diff --git a/gcc/testsuite/g++.dg/modules/dguide-3_b.C b/gcc/testsuite/g++.dg/modules/dguide-3_b.C new file mode 100644 index 00000000000..d23696c74bd --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/dguide-3_b.C @@ -0,0 +1,10 @@ +// { dg-additional-options "-fmodules-ts" } +// { dg-module-cmi B } + +export module B; + +extern "C++" { + template <typename T> struct S; + S(int) -> S<int>; + S(char) -> S<char>; +} diff --git a/gcc/testsuite/g++.dg/modules/dguide-3_c.C b/gcc/testsuite/g++.dg/modules/dguide-3_c.C new file mode 100644 index 00000000000..95d29db724d --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/dguide-3_c.C @@ -0,0 +1,6 @@ +// { dg-additional-options "-fmodules-ts -Wno-global-module" } + +module; +template <typename T> struct S; +export module C; +S(const char*) -> S<const char*>; diff --git a/gcc/testsuite/g++.dg/modules/dguide-3_d.C b/gcc/testsuite/g++.dg/modules/dguide-3_d.C new file mode 100644 index 00000000000..0d98517b7da --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/dguide-3_d.C @@ -0,0 +1,30 @@ +// { dg-additional-options "-fmodules-ts" } +// Test merging deduction guides. + +template <typename T> struct S { + template <typename U> S(U); +}; + +import A; +import B; +import C; + +int main() { + // declared in A and B + S x(123); + S<int> x2 = x; + + // declared only in A + S y(0.5); + S<double> y2 = y; + + // declared only in B + S z('c'); + S<char> z2 = z; + + // declared only in C (and attached to named module) + S w("hello"); + S<const char*> w2 = w; +} + +S(char) -> S<double>; // { dg-error "ambiguating" }