On Sat, Nov 15, 2025 at 10:44:53AM +1100, Nathaniel Shead wrote: > On Sat, Nov 15, 2025 at 12:11:39AM +1100, Nathaniel Shead wrote: > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? > > > > An alternative implementation would be to do the additional lookups in > > modules.cc during tree walk at the point we know we need to check for > > any CALL_EXPR with KOENIG_LOOKUP_P or DEPENDENT_OPERATOR_TYPE, and then > > discard the results. > > > > I felt this was the slightly neater approach but that would possibly be > > less risky; any thoughts? > > Here's the other approach doing it just during module walk. I actually > think I prefer this approach. > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? >
Ping for this patch. > -- >8 -- > > [module.global.frag] p3.3 says "A declaration D is decl-reachable from a > declaration S in the same translation unit if ... S contains a dependent > call E ([temp.dep]) and D is found by any name lookup performed for an > expression synthesized from E by replacing each type-dependent argument > or operand with a value of a placeholder type with no associated > namespaces or entities". > > This requires doing partial ADL ondependent calls, in case there are > non-dependent arguments that would cause new functions to become > decl-reachable. This patch implements this with an additional lookup > during modules streaming to find any such entities. > > This causes us to do ADL in more circumstances; this means also that we > might instantiate templates in cases we didn't use to. This could cause > issues given we have already started our modules walk at this point, or > break any otherwise valid existing code. To fix this patch adds a flag > to do a "tentative" ADL pass which doesn't attempt to complete any types > (and hence cause instantiations to occur); this means that we might miss > some associated entities however. During a tentative walk we can also > skip entities that we know won't contribute to the missing > decl-reachable set, as an optimisation. > > One implementation limitation is that both modules tree walking and > name lookup marks tree nodes as TREE_VISITED for different purposes; to > avoid conflicts this patch caches calls that will require lookup in a > separate worklist to be processed after the walk is done. > > gcc/cp/ChangeLog: > > * module.cc (depset::hash::dep_adl_info): New type. > (depset::hash::dep_adl_entity_list): New work list. > (depset::hash::hash): Create it. > (depset::hash::~hash): Release it. > (trees_out::tree_value): Cache possibly dependent > calls during tree walk. > (depset::hash::add_dependent_adl_entities): New function. > (depset::hash::find_dependencies): Process cached entities. > * name-lookup.cc (name_lookup::tentative): New member. > (name_lookup::name_lookup): Initialize it. > (name_lookup::preserve_state): Propagate tentative from previous > lookup. > (name_lookup::adl_namespace_fns): Don't search imported bindings > during tentative lookup. > (name_lookup::adl_class): Don't attempt to complete class types > during tentative lookup. > (name_lookup::search_adl): Skip type-dependent args and avoid > unnecessary work during tentative lookup. > (lookup_arg_dependent): Add tentative parameter. > * name-lookup.h (lookup_arg_dependent): Likewise. > > gcc/testsuite/ChangeLog: > > * g++.dg/modules/adl-12_a.C: New test. > * g++.dg/modules/adl-12_b.C: New test. > > Signed-off-by: Nathaniel Shead <[email protected]> > --- > gcc/cp/module.cc | 129 ++++++++++++++++++++++++ > gcc/cp/name-lookup.cc | 36 +++++-- > gcc/cp/name-lookup.h | 3 +- > gcc/testsuite/g++.dg/modules/adl-12_a.C | 74 ++++++++++++++ > gcc/testsuite/g++.dg/modules/adl-12_b.C | 27 +++++ > 5 files changed, 260 insertions(+), 9 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/modules/adl-12_a.C > create mode 100644 gcc/testsuite/g++.dg/modules/adl-12_b.C > > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc > index ccabd640757..8889735c730 100644 > --- a/gcc/cp/module.cc > +++ b/gcc/cp/module.cc > @@ -2628,6 +2628,20 @@ public: > bool ignore_tu_local; /* In a context where referencing a TU-local > entity is not an exposure. */ > > + private: > + /* Information needed to do dependent ADL for discovering > + more decl-reachable entities. Cached during walking to > + prevent tree marking from interfering with lookup. */ > + struct dep_adl_info { > + /* The name of the call or operator. */ > + tree name = NULL_TREE; > + /* If not ERROR_MARK, a rewrite candidate for this operator. */ > + tree_code rewrite = ERROR_MARK; > + /* Argument list for the call. */ > + vec<tree, va_gc>* args = make_tree_vector (); > + }; > + vec<dep_adl_info> dep_adl_entity_list; > + > public: > hash (size_t size, hash *c = NULL) > : parent (size), chain (c), current (NULL), section (0), > @@ -2635,10 +2649,12 @@ public: > ignore_tu_local (false) > { > worklist.create (size); > + dep_adl_entity_list.create (16); > } > ~hash () > { > worklist.release (); > + dep_adl_entity_list.release (); > } > > public: > @@ -2671,6 +2687,7 @@ public: > void add_specializations (bool decl_p); > void add_partial_entities (vec<tree, va_gc> *); > void add_class_entities (vec<tree, va_gc> *); > + void add_dependent_adl_entities (tree expr); > > private: > void add_deduction_guides (tree decl); > @@ -9771,6 +9788,9 @@ trees_out::tree_value (tree t) > && !DECL_CONTEXT (t))) > && TREE_CODE (t) != FUNCTION_DECL)); > > + if (is_initial_scan () && EXPR_P (t)) > + dep_hash->add_dependent_adl_entities (t); > + > if (streaming_p ()) > { > /* A new node -> tt_node. */ > @@ -14913,6 +14933,90 @@ depset::hash::add_class_entities (vec<tree, va_gc> > *class_members) > } > } > > +/* Add any entities found via dependent ADL. */ > + > +void > +depset::hash::add_dependent_adl_entities (tree expr) > +{ > + gcc_checking_assert (!is_key_order ()); > + if (TREE_CODE (current->get_entity ()) != TEMPLATE_DECL) > + return; > + > + dep_adl_info info; > + switch (TREE_CODE (expr)) > + { > + case CALL_EXPR: > + if (!KOENIG_LOOKUP_P (expr)) > + return; > + info.name = CALL_EXPR_FN (expr); > + if (!info.name) > + return; > + if (TREE_CODE (info.name) == TEMPLATE_ID_EXPR) > + info.name = TREE_OPERAND (info.name, 0); > + if (TREE_CODE (info.name) == TU_LOCAL_ENTITY) > + return; > + if (!identifier_p (info.name)) > + info.name = OVL_NAME (info.name); > + for (int ix = 0; ix < call_expr_nargs (expr); ix++) > + vec_safe_push (info.args, CALL_EXPR_ARG (expr, ix)); > + break; > + > + case LE_EXPR: > + case GE_EXPR: > + case LT_EXPR: > + case GT_EXPR: > + info.rewrite = SPACESHIP_EXPR; > + goto overloadable_expr; > + > + case NE_EXPR: > + info.rewrite = EQ_EXPR; > + goto overloadable_expr; > + > + case EQ_EXPR: > + /* Not strictly a rewrite candidate, but we need to ensure > + that lookup of a matching NE_EXPR can succeed if that > + would inhibit a rewrite with reversed parameters. */ > + info.rewrite = NE_EXPR; > + goto overloadable_expr; > + > + case COMPOUND_EXPR: > + case MEMBER_REF: > + case MULT_EXPR: > + case TRUNC_DIV_EXPR: > + case TRUNC_MOD_EXPR: > + case PLUS_EXPR: > + case MINUS_EXPR: > + case LSHIFT_EXPR: > + case RSHIFT_EXPR: > + case SPACESHIP_EXPR: > + case BIT_AND_EXPR: > + case BIT_XOR_EXPR: > + case BIT_IOR_EXPR: > + case TRUTH_ANDIF_EXPR: > + case TRUTH_ORIF_EXPR: > + overloadable_expr: > + info.name = ovl_op_identifier (TREE_CODE (expr)); > + gcc_checking_assert (tree_operand_length (expr) == 2); > + vec_safe_push (info.args, TREE_OPERAND (expr, 0)); > + vec_safe_push (info.args, TREE_OPERAND (expr, 1)); > + break; > + > + default: > + return; > + } > + > + /* If all arguments are type-dependent we don't need to do > + anything further, we won't find new entities. */ > + processing_template_decl_sentinel ptds; > + ++processing_template_decl; > + if (!any_type_dependent_arguments_p (info.args)) > + return; > + > + /* We need to defer name lookup until after walking, otherwise > + we get confused by stray TREE_VISITEDs. */ > + dep_adl_entity_list.safe_push (info); > +} > + > /* We add the partial & explicit specializations, and the explicit > instantiations. */ > > @@ -15274,6 +15378,31 @@ depset::hash::find_dependencies (module_state > *module) > && deduction_guide_p (decl)) > add_deduction_guides (TYPE_NAME (TREE_TYPE (TREE_TYPE (decl)))); > > + /* Handle dependent ADL for [module.global.frag] p3.3. */ > + if (!is_key_order () && !dep_adl_entity_list.is_empty ()) > + { > + processing_template_decl_sentinel ptds; > + ++processing_template_decl; > + for (auto &info : dep_adl_entity_list) > + { > + tree lookup = lookup_arg_dependent (info.name, NULL_TREE, > + info.args, true); > + for (tree fn : lkp_range (lookup)) > + add_dependency (make_dependency (fn, EK_DECL)); > + > + if (info.rewrite) > + { > + tree rewrite_name = ovl_op_identifier (info.rewrite); > + lookup = lookup_arg_dependent (rewrite_name, > NULL_TREE, > + info.args, true); > + for (tree fn : lkp_range (lookup)) > + add_dependency (make_dependency (fn, EK_DECL)); > + } > + release_tree_vector (info.args); > + } > + dep_adl_entity_list.truncate (0); > + } > + > if (!is_key_order () > && TREE_CODE (decl) == TEMPLATE_DECL > && !DECL_UNINSTANTIATED_TEMPLATE_FRIEND_P (decl)) > diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc > index 984d37c2089..54743ad1761 100644 > --- a/gcc/cp/name-lookup.cc > +++ b/gcc/cp/name-lookup.cc > @@ -449,6 +449,8 @@ public: > > LOOK_want want; /* What kind of entity we want. */ > > + bool tentative; /* The lookup is just to find additional decl-reachable > + entities in this TU during modules streaming. */ > bool deduping; /* Full deduping is needed because using declarations > are in play. */ > vec<tree, va_heap, vl_embed> *scopes; > @@ -463,7 +465,7 @@ protected: > public: > name_lookup (tree n, LOOK_want w = LOOK_want::NORMAL) > : name (n), value (NULL_TREE), type (NULL_TREE), > - want (w), > + want (w), tentative (false), > deduping (false), scopes (NULL), previous (NULL) > { > preserve_state (); > @@ -602,6 +604,8 @@ name_lookup::preserve_state () > } > } > > + tentative = previous->tentative; > + > /* Unmark the outer partial lookup. */ > if (previous->deduping) > lookup_mark (previous->value, false); > @@ -1252,6 +1256,11 @@ name_lookup::adl_namespace_fns (tree scope, bitmap > imports, > add_fns (ovl_skip_hidden (MAYBE_STAT_DECL (bind))); > } > > + /* When doing tentative name lookup we only care about entities > + in the current TU. */ > + if (tentative) > + return; > + > /* Scan the imported bindings. */ > unsigned ix = BINDING_VECTOR_NUM_CLUSTERS (val); > if (BINDING_VECTOR_SLOTS_PER_CLUSTER == BINDING_SLOTS_FIXED) > @@ -1484,7 +1493,10 @@ name_lookup::adl_class (tree type) > if (found_p (type)) > return; > > - complete_type (type); > + /* Don't instantiate if we don't have to. */ > + if (!tentative) > + complete_type (type); > + > adl_bases (type); > mark_found (type); > > @@ -1679,7 +1691,10 @@ name_lookup::search_adl (tree fns, vec<tree, va_gc> > *args) > first arg. */ > if (TYPE_P (arg)) > adl_type (arg); > - else > + /* When processing a module CMI we might get a type-dependent > + argument: treat as a placeholder with no associated namespace > + or entities. */ > + else if (!tentative || !type_dependent_expression_p (arg)) > adl_expr (arg); > > if (vec_safe_length (scopes)) > @@ -1690,10 +1705,11 @@ name_lookup::search_adl (tree fns, vec<tree, va_gc> > *args) > dedup (true); > > /* First get the attached modules for each innermost non-inline > - namespace of an associated entity. */ > + namespace of an associated entity. This isn't needed for > + tentative lookup, as we're only interested in the current TU. */ > bitmap_obstack_initialize (NULL); > hash_map<tree, bitmap> ns_mod_assocs; > - if (modules_p ()) > + if (modules_p () && !tentative) > { > for (tree scope : scopes) > if (TYPE_P (scope)) > @@ -1732,7 +1748,7 @@ name_lookup::search_adl (tree fns, vec<tree, va_gc> > *args) > adl_namespace_fns (scope, visible, inst_path, > assocs ? *assocs : NULL); > } > - else if (RECORD_OR_UNION_TYPE_P (scope)) > + else if (RECORD_OR_UNION_TYPE_P (scope) && !tentative) > adl_class_fns (scope); > } > > @@ -1756,13 +1772,17 @@ static void consider_binding_level (tree name, > > /* ADL lookup of NAME. FNS is the result of regular lookup, and we > don't add duplicates to it. ARGS is the vector of call > - arguments (which will not be empty). */ > + arguments (which will not be empty). TENTATIVE is true when > + this is early lookup only for the purpose of finding more > + decl-reachable declarations. */ > > tree > -lookup_arg_dependent (tree name, tree fns, vec<tree, va_gc> *args) > +lookup_arg_dependent (tree name, tree fns, vec<tree, va_gc> *args, > + bool tentative/*=false*/) > { > auto_cond_timevar tv (TV_NAME_LOOKUP); > name_lookup lookup (name); > + lookup.tentative = tentative; > return lookup.search_adl (fns, args); > } > > diff --git a/gcc/cp/name-lookup.h b/gcc/cp/name-lookup.h > index 3815b8c1c96..46cbf8e1016 100644 > --- a/gcc/cp/name-lookup.h > +++ b/gcc/cp/name-lookup.h > @@ -461,7 +461,8 @@ extern void push_decl_namespace (tree); > extern void pop_decl_namespace (void); > extern void do_namespace_alias (location_t, tree, tree); > extern tree do_class_using_decl (tree, tree); > -extern tree lookup_arg_dependent (tree, tree, vec<tree, va_gc> *); > +extern tree lookup_arg_dependent (tree, tree, vec<tree, va_gc> *, > + bool tentative = false); > extern tree search_anon_aggr (tree, tree, bool = false); > extern tree get_class_binding_direct (tree, tree, bool want_type = false); > extern tree get_class_binding (tree, tree, bool want_type = false); > diff --git a/gcc/testsuite/g++.dg/modules/adl-12_a.C > b/gcc/testsuite/g++.dg/modules/adl-12_a.C > new file mode 100644 > index 00000000000..47c3c21a2a5 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/adl-12_a.C > @@ -0,0 +1,74 @@ > +// { dg-additional-options "-fmodules -Wno-global-module -fdump-lang-module" > } > +// { dg-module-cmi M } > +// Check we implement [module.global.frag] p3.3. > + > +module; > +namespace Q { > + struct X {}; > + void g_impl(X, X); > + void operator-(X, X); > + void go_partial(X, int); > + void operator/(X, int); > +} > +namespace P { > + struct X {}; > +} > +#if __cpp_impl_three_way_comparison >= 201907L > +namespace ops1 { > + struct Y {}; > + int operator<=>(Y, int); > + bool operator==(Y, int); > +} > +namespace ops2 { > + struct Y {}; > + bool operator==(Y, int); > + bool operator!=(Y, int); > +} > +#endif > +namespace Incomplete { > + template <typename T> struct Holder { T t; }; > + struct Z; > + void go(int, void*); > +} > +export module M; > + > +export template <typename T> > +void g(T t) { > + g_impl(t, Q::X{}); // ADL in definition finds Q::g_impl, g_impl not > discarded > + // { dg-final { scan-lang-dump "Bindings '::Q::g_impl'" module } } > + > + t - Q::X{}; // Similarly for Q::operator- > + // { dg-final { scan-lang-dump "Bindings '::Q::operator-'" module } } > +} > + > +export template <typename T> struct Partial { > + template <typename U> static decltype(go_partial(T{}, U{})) f(); > + template <typename U> static decltype(T{} / U{}) o(); > +}; > +// The instantantiation of Partial<Q::X> should emit go_partial and operator/ > +template struct Partial<Q::X>; > +// { dg-final { scan-lang-dump "Bindings '::Q::go_partial'" module } } > +// { dg-final { scan-lang-dump "Bindings '::Q::operator/'" module } } > + > +#if __cpp_impl_three_way_comparison >= 201907L > +export template <typename T> > +void rewrite_ops(T t) { > + // Rewritten operators should also look for the ops they rewrite to. > + t < ops1::Y{}; > + t != ops1::Y{}; > + // { dg-final { scan-lang-dump {Bindings '::ops1::operator<=>'} module { > target c++20 } } } > + // { dg-final { scan-lang-dump {Bindings '::ops1::operator=='} module { > target c++20 } } } > +} > +export template <typename T> > +void rewrite_ops_error(T t) { > + // Test we bind != to prevent considering == as a rewrite target. > + t == ops2::Y{}; > + // { dg-final { scan-lang-dump "Bindings '::ops2::operator!='" module { > target c++20 } } } > +} > +#endif > + > +export template <typename T> > +void incomplete(T t) { > + Incomplete::Holder<Incomplete::Z>* holder; > + go(t, holder); // Shouldn't attempt to instantiate unnecessarily here > +} > diff --git a/gcc/testsuite/g++.dg/modules/adl-12_b.C > b/gcc/testsuite/g++.dg/modules/adl-12_b.C > new file mode 100644 > index 00000000000..fd960c9ae42 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/adl-12_b.C > @@ -0,0 +1,27 @@ > +// { dg-additional-options "-fmodules" } > +// Example from [temp.dep.candidate] > + > +namespace Q { > + struct X {}; > +} > +namespace Incomplete { > + struct Z {}; > +}; > + > +import M; > + > +void test(Q::X x) { > + g(x); // OK > + Partial<Q::X>::f<int>(); > + Partial<Q::X>::o<int>(); > + incomplete(0); // OK > + > +#if __cpp_impl_three_way_comparison >= 201907L > + rewrite_ops(0); // OK > + > + // This should error, but lookup_qualified_name in add_candidates > + // doesn't look in the instantiation context of this call, so > + // we don't see the operator!= and think we can validly rewrite. > + rewrite_ops_error(0); // { dg-error "required from here" "" { xfail *-*-* > } } > +#endif > +} > -- > 2.51.0 >
