Hi Nathaniel. > On 10 Jan 2026, at 03:06, Nathaniel Shead <[email protected]> wrote: > > And here's a v2 of [1] that should avoid any potential ABI issues with > coroutines, by just special-casing the transform functions during > modules streaming in has_definition.
Thanks; I think that this is the right mental model for the coroutine - it is a single entity that happens to be implemented in three pieces. Anything else is likely to open an ODR-flavoured can of worms. A couple of small additional points below, otherwise this looks good to me (but I cannot approve it), thanks Iain > [1]: https://gcc.gnu.org/pipermail/gcc-patches/2026-January/705304.html > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? > > -- >8 -- > > While working on another issue I found that currently modules do not > work with coroutines at all. This patch fixes a number of issues in > both the coroutines logic and modules logic to ensure that they play > well together. To summarize: > > - The coroutine proxy objects did not have a DECL_CONTEXT set (required > for modules to merge declarations). > > - The coroutine transformation functions are always non-inline, even > for an inline ramp function, which means that modules need an override > to ensure the definitions are available where needed. > > - Coroutine transformation functions were not marked DECL_COROUTINE_P, > despite accessors implying that they were. > > - In an importing TU we had lost the connection between the ramp > functions and the transform functions, as they were kept in a pair > of global maps. > > - Modules streaming couldn't discriminate between the actor or destroy > functions when merging. > > - Modules streaming wasn't setting the cfun->coroutine_component flag, > needed to activate the middle-end coroutine lowering pass. > > This patch also separates the coroutine_info_table initialization from > the ensure_coro_initialized function. If the first time we see a > coroutine is from a module import, we need to register the > transformation functions now but calling ensure_coro_initialized would > lookup e.g. std::coroutine_traits, which may only be visible from this > module that we're currently reading, causing a recursive load. > Separating the concerns allows this to work correctly. > > gcc/cp/ChangeLog: > > * coroutines.cc (create_coroutine_info_table): New function. > (get_or_insert_coroutine_info): Mark static. > (ensure_coro_initialized): Likewise; use > create_coroutine_info_table. > (coro_promise_type_found_p): Set DECL_CONTEXT for proxies. > (coro_set_ramp_function): New function. > (coro_set_transform_functions): New function. > (coro_build_actor_or_destroy_function): Use > coro_set_ramp_function, mark as DECL_COROUTINE_P. > * cp-tree.h (coro_set_transform_functions): Declare. > (coro_set_ramp_function): Declare. > * module.cc (struct merge_key): New field coro_disc. > (dumper::impl::nested_name): Distinguish coroutine transform > functions. > (get_coroutine_discriminator): New function. > (trees_out::key_mergeable): Stream coroutine discriminator. > (check_mergeable_decl): Adjust comment, check for matching > coroutine discriminator. > (trees_in::key_mergeable): Read coroutine discriminator. > (has_definition): Override for coroutine transform functions. > (trees_out::write_function_def): Stream linked ramp, actor, and > destroy functions for coroutines. > (trees_in::read_function_def): Read them. > (module_state::read_cluster): Set cfun->coroutine_component. > > gcc/testsuite/ChangeLog: > > * g++.dg/modules/coro-1_a.C: New test. > * g++.dg/modules/coro-1_b.C: New test. > > Reviewed-by: Iain Sandoe <[email protected]> > Signed-off-by: Nathaniel Shead <[email protected]> > --- > gcc/cp/coroutines.cc | 59 +++++++++++++--- > gcc/cp/cp-tree.h | 4 ++ > gcc/cp/module.cc | 90 +++++++++++++++++++++++-- > gcc/testsuite/g++.dg/modules/coro-1_a.C | 34 ++++++++++ > gcc/testsuite/g++.dg/modules/coro-1_b.C | 19 ++++++ > 5 files changed, 189 insertions(+), 17 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/modules/coro-1_a.C > create mode 100644 gcc/testsuite/g++.dg/modules/coro-1_b.C > > diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc > index f0485a95073..2056b184d99 100644 > --- a/gcc/cp/coroutines.cc > +++ b/gcc/cp/coroutines.cc > @@ -353,10 +353,20 @@ coroutine_info_hasher::equal (coroutine_info *lhs, > const compare_type& rhs) > return lhs->function_decl == rhs; > } > > +/* Initialize the coroutine info table, to hold state per coroutine decl, > + if not already created. */ > + > +static void > +create_coroutine_info_table () > +{ > + if (!coroutine_info_table) > + coroutine_info_table = hash_table<coroutine_info_hasher>::create_ggc > (11); > +} > + > /* Get the existing coroutine_info for FN_DECL, or insert a new one if the > entry does not yet exist. */ > > -coroutine_info * > +static coroutine_info * > get_or_insert_coroutine_info (tree fn_decl) > { > gcc_checking_assert (coroutine_info_table != NULL); > @@ -375,7 +385,7 @@ get_or_insert_coroutine_info (tree fn_decl) > > /* Get the existing coroutine_info for FN_DECL, fail if it doesn't exist. */ > > -coroutine_info * > +static coroutine_info * > get_coroutine_info (tree fn_decl) > { > if (coroutine_info_table == NULL) > @@ -757,11 +767,7 @@ ensure_coro_initialized (location_t loc) > if (!void_coro_handle_address) > return false; > > - /* A table to hold the state, per coroutine decl. */ > - gcc_checking_assert (coroutine_info_table == NULL); > - coroutine_info_table = > - hash_table<coroutine_info_hasher>::create_ggc (11); > - > + create_coroutine_info_table (); > if (coroutine_info_table == NULL) > return false; > > @@ -873,11 +879,13 @@ coro_promise_type_found_p (tree fndecl, location_t loc) > coro_info->self_h_proxy > = build_lang_decl (VAR_DECL, coro_self_handle_id, > coro_info->handle_type); > + DECL_CONTEXT (coro_info->self_h_proxy) = fndecl; > > /* Build a proxy for the promise so that we can perform lookups. */ > coro_info->promise_proxy > = build_lang_decl (VAR_DECL, coro_promise_id, > coro_info->promise_type); > + DECL_CONTEXT (coro_info->promise_proxy) = fndecl; > > /* Note where we first saw a coroutine keyword. */ > coro_info->first_coro_keyword = loc; > @@ -902,6 +910,17 @@ coro_get_ramp_function (tree decl) > return NULL_TREE; > } > > +/* Given a DECL, an actor or destroyer, build a link from that to the ramp > + function. Used by modules streaming. */ > + > +void > +coro_set_ramp_function (tree decl, tree ramp) > +{ > + if (!to_ramp) > + to_ramp = hash_map<tree, tree>::create_ggc (10); > + to_ramp->put (decl, ramp); > +} > + > /* Given the DECL for a ramp function (the user's original declaration) return > the actor function if it has been defined. */ > > @@ -926,6 +945,27 @@ coro_get_destroy_function (tree decl) > return NULL_TREE; > } > > +/* For a given ramp function DECL, set the actor and destroy functions. > + This is only used by modules streaming. */ > + > +void > +coro_set_transform_functions (tree decl, tree actor, tree destroy) > +{ > + /* Only relevant with modules. */ > + gcc_assert (modules_p ()); Could we make these asserts gcc_checking_asserts() ? > > + /* This should only be called for newly streamed declarations. */ > + gcc_assert (!get_coroutine_info (decl)); > + > + /* This might be the first use of coroutine info in the TU, so > + create the coroutine info table if needed. */ > + create_coroutine_info_table (); > + > + coroutine_info *coroutine = get_or_insert_coroutine_info (decl); > + coroutine->actor_decl = actor; > + coroutine->destroy_decl = destroy; > +} > + > /* Given a CO_AWAIT_EXPR AWAIT_EXPR, return its resume call. */ > > tree > @@ -4393,14 +4433,13 @@ coro_build_actor_or_destroy_function (tree orig, tree > fn_type, > = build_lang_decl (FUNCTION_DECL, copy_node (DECL_NAME (orig)), fn_type); > > /* Allow for locating the ramp (original) function from this one. */ > - if (!to_ramp) > - to_ramp = hash_map<tree, tree>::create_ggc (10); > - to_ramp->put (fn, orig); > + coro_set_ramp_function (fn, orig); > > DECL_CONTEXT (fn) = DECL_CONTEXT (orig); > DECL_SOURCE_LOCATION (fn) = loc; > DECL_ARTIFICIAL (fn) = true; > DECL_INITIAL (fn) = error_mark_node; > + DECL_COROUTINE_P (fn) = true; > > tree id = get_identifier ("frame_ptr"); > tree fp = build_lang_decl (PARM_DECL, id, coro_frame_ptr); > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > index b8470fc256c..1d40b387d6e 100644 > --- a/gcc/cp/cp-tree.h > +++ b/gcc/cp/cp-tree.h > @@ -9144,6 +9144,10 @@ extern tree coro_get_ramp_function (tree); > > extern tree co_await_get_resume_call (tree await_expr); > > +/* Only for use by modules. */ > +extern void coro_set_transform_functions (tree, tree, tree); > +extern void coro_set_ramp_function (tree, tree); > + > /* Inline bodies. */ > > inline tree > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc > index 174bebff01c..e6b2b2f7858 100644 > --- a/gcc/cp/module.cc > +++ b/gcc/cp/module.cc > @@ -2969,6 +2969,7 @@ static char const *const merge_kind_name[MK_hwm] = > /* Mergeable entity location data. */ > struct merge_key { > cp_ref_qualifier ref_q : 2; > + unsigned coro_disc : 2; /* Discriminator for coroutine transforms. */ > unsigned index; > > tree ret; /* Return type, if appropriate. */ > @@ -2977,7 +2978,7 @@ struct merge_key { > tree constraints; /* Constraints. */ > > merge_key () > - :ref_q (REF_QUAL_NONE), index (0), > + :ref_q (REF_QUAL_NONE), coro_disc (0), index (0), > ret (NULL_TREE), args (NULL_TREE), > constraints (NULL_TREE) > { > @@ -4590,6 +4591,17 @@ dumper::impl::nested_name (tree t) > else > fputs ("#null#", stream); > > + if (t && TREE_CODE (t) == FUNCTION_DECL && DECL_COROUTINE_P (t)) > + if (tree ramp = DECL_RAMP_FN (t)) > + { > + if (DECL_ACTOR_FN (ramp) == t) > + fputs (".actor", stream); > + else if (DECL_DESTROY_FN (ramp) == t) > + fputs (".destroy", stream); > + else > + gcc_unreachable (); > + } > + > if (origin >= 0) > { > const module_state *module = (*modules)[origin]; > @@ -11712,6 +11724,25 @@ trees_in::decl_container () > return container; > } > > +/* Gets a 2-bit discriminator to distinguish coroutine actor or destroy > + functions from a normal function. */ > + > +static int > +get_coroutine_discriminator (tree inner) > +{ > + if (DECL_COROUTINE_P (inner)) > + if (tree ramp = DECL_RAMP_FN (inner)) > + { > + if (DECL_ACTOR_FN (ramp) == inner) > + return 1; > + else if (DECL_DESTROY_FN (ramp) == inner) > + return 2; > + else > + gcc_unreachable (); > + } > + return 0; > +} > + > /* Write out key information about a mergeable DEP. Does not write > the contents of DEP itself. The context has already been > written. The container has already been streamed. */ > @@ -11803,6 +11834,7 @@ trees_out::key_mergeable (int tag, merge_kind mk, > tree decl, tree inner, > tree fn_type = TREE_TYPE (inner); > > key.ref_q = type_memfn_rqual (fn_type); > + key.coro_disc = get_coroutine_discriminator (inner); > key.args = TYPE_ARG_TYPES (fn_type); > > if (tree reqs = get_constraints (inner)) > @@ -11939,7 +11971,12 @@ trees_out::key_mergeable (int tag, merge_kind mk, > tree decl, tree inner, > tree_node (name); > if (streaming_p ()) > { > - unsigned code = (key.ref_q << 0) | (key.index << 2); > + /* Check we have enough bits for the index. */ > + gcc_checking_assert (key.index < (1u << (sizeof (unsigned) * 8 - 4))); > + > + unsigned code = ((key.ref_q << 0) > + | (key.coro_disc << 2) > + | (key.index << 4)); > u (code); > } > > @@ -11963,8 +12000,8 @@ trees_out::key_mergeable (int tag, merge_kind mk, > tree decl, tree inner, > } > } > > -/* DECL is a new declaration that may be duplicated in OVL. Use RET & > - ARGS to find its clone, or NULL. If DECL's DECL_NAME is NULL, this > +/* DECL is a new declaration that may be duplicated in OVL. Use KEY > + to find its clone, or NULL. If DECL's DECL_NAME is NULL, this > has been found by a proxy. It will be an enum type located by its > first member. > > @@ -12019,6 +12056,7 @@ check_mergeable_decl (merge_kind mk, tree decl, tree > ovl, merge_key const &key) > || same_type_p (key.ret, fndecl_declared_return_type (m_inner))) > && type_memfn_rqual (m_type) == key.ref_q > && compparms (key.args, TYPE_ARG_TYPES (m_type)) > + && get_coroutine_discriminator (m_inner) == key.coro_disc > /* Reject if old is a "C" builtin and new is not "C". > Matches decls_match behaviour. */ > && (!DECL_IS_UNDECLARED_BUILTIN (m_inner) > @@ -12141,7 +12179,8 @@ trees_in::key_mergeable (int tag, merge_kind mk, tree > decl, tree inner, > merge_key key; > unsigned code = u (); > key.ref_q = cp_ref_qualifier ((code >> 0) & 3); > - key.index = code >> 2; > + key.coro_disc = (code >> 2) & 3; > + key.index = code >> 4; > > if (mk == MK_enum) > key.ret = tree_node (); > @@ -12874,6 +12913,12 @@ has_definition (tree decl) > if (use_tpl < 2) > return true; > } > + > + /* Coroutine transform functions always need to be emitted > + into the importing TU if the ramp function will be. */ > + if (DECL_COROUTINE_P (decl)) > + if (tree ramp = DECL_RAMP_FN (decl)) > + return has_definition (ramp); > break; > > case TYPE_DECL: > @@ -13057,6 +13102,17 @@ trees_out::write_function_def (tree decl) > state->write_location (*this, f->function_start_locus); > state->write_location (*this, f->function_end_locus); > } > + > + if (DECL_COROUTINE_P (decl)) > + { > + tree ramp = DECL_RAMP_FN (decl); > + tree_node (ramp); What does this ^ do when ramp is nullptr? (Is it a NOP?) > + if (!ramp) > + { > + tree_node (DECL_ACTOR_FN (decl)); > + tree_node (DECL_DESTROY_FN (decl)); > + } > + } > } > > void > @@ -13072,13 +13128,13 @@ trees_in::read_function_def (tree decl, tree > maybe_template) > tree initial = tree_node (); > tree saved = tree_node (); > tree context = tree_node (); > - constexpr_fundef cexpr; > post_process_data pdata {}; > pdata.decl = maybe_template; > > tree maybe_dup = odr_duplicate (maybe_template, DECL_SAVED_TREE (decl)); > bool installing = maybe_dup && !DECL_SAVED_TREE (decl); > > + constexpr_fundef cexpr; > if (u ()) > { > cexpr.parms = chained_decls (); > @@ -13090,7 +13146,6 @@ trees_in::read_function_def (tree decl, tree > maybe_template) > cexpr.decl = NULL_TREE; > > unsigned flags = u (); > - > if (flags & 2) > { > pdata.start_locus = state->read_location (*this); > @@ -13101,6 +13156,21 @@ trees_in::read_function_def (tree decl, tree > maybe_template) > pdata.infinite_loop = flags & 32; > } > > + tree coro_actor = NULL_TREE; > + tree coro_destroy = NULL_TREE; > + tree coro_ramp = NULL_TREE; > + if (DECL_COROUTINE_P (decl)) > + { > + coro_ramp = tree_node (); > + if (!coro_ramp) > + { > + coro_actor = tree_node (); > + coro_destroy = tree_node (); > + if ((coro_actor == NULL_TREE) != (coro_destroy == NULL_TREE)) > + set_overrun (); > + } > + } > + > if (get_overrun ()) > return NULL_TREE; > > @@ -13116,6 +13186,11 @@ trees_in::read_function_def (tree decl, tree > maybe_template) > if (cexpr.decl) > register_constexpr_fundef (cexpr); > > + if (coro_ramp) > + coro_set_ramp_function (decl, coro_ramp); > + else if (coro_actor && coro_destroy) > + coro_set_transform_functions (decl, coro_actor, coro_destroy); > + > if (DECL_LOCAL_DECL_P (decl)) > /* Block-scope OMP UDRs aren't real functions, and don't need a > function structure to be allocated or to be expanded. */ > @@ -17572,6 +17647,7 @@ module_state::read_cluster (unsigned snum) > cfun->language->returns_null = pdata.returns_null; > cfun->language->returns_abnormally = pdata.returns_abnormally; > cfun->language->infinite_loop = pdata.infinite_loop; > + cfun->coroutine_component = DECL_COROUTINE_P (decl); > > /* Make sure we emit explicit instantiations. > FIXME do we want to do this in expand_or_defer_fn instead? */ > diff --git a/gcc/testsuite/g++.dg/modules/coro-1_a.C > b/gcc/testsuite/g++.dg/modules/coro-1_a.C > new file mode 100644 > index 00000000000..165c643fd7f > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/coro-1_a.C > @@ -0,0 +1,34 @@ > +// { dg-do compile { target c++20 } } > +// { dg-additional-options "-fmodules -fdump-lang-module" } > +// { dg-module-cmi M } > + > +module; > +#include <coroutine> > +export module M; > + > +struct simple_promise; > +struct simple_coroutine : std::coroutine_handle<simple_promise> { > + using promise_type = ::simple_promise; > +}; > +struct simple_promise { > + simple_coroutine get_return_object() { return { > simple_coroutine::from_promise(*this) }; } > + std::suspend_always initial_suspend() noexcept { return {}; } > + std::suspend_always final_suspend() noexcept { return {}; } > + void return_void() {} > + void unhandled_exception() {} > +}; > +export simple_coroutine coroutine() { > + co_return; > +} > +export inline simple_coroutine inline_coroutine() { > + co_return; > +} > +export template <typename T> simple_coroutine template_coroutine() { > + co_return; > +} > + > +// The actor and destroy functions should have definitions streamed only for > inline coroutines > +// { dg-final { scan-lang-dump-not {Writing definition > function_decl:'::coroutine.actor'} module } } > +// { dg-final { scan-lang-dump-not {Writing definition > function_decl:'::coroutine.destroy'} module } } > +// { dg-final { scan-lang-dump {Writing definition > function_decl:'::inline_coroutine.actor'} module } } > +// { dg-final { scan-lang-dump {Writing definition > function_decl:'::inline_coroutine.destroy'} module } } > diff --git a/gcc/testsuite/g++.dg/modules/coro-1_b.C > b/gcc/testsuite/g++.dg/modules/coro-1_b.C > new file mode 100644 > index 00000000000..e1384a7d639 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/coro-1_b.C > @@ -0,0 +1,19 @@ > +// { dg-module-do run { target c++20 } } > +// { dg-additional-options "-fmodules" } > + > +#include <coroutine> > +import M; > + > +int main() { > + auto a = coroutine(); > + a.resume(); > + a.destroy(); > + > + auto b = inline_coroutine(); > + b.resume(); > + b.destroy(); > + > + auto c = template_coroutine<int>(); > + c.resume(); > + c.destroy(); > +} > -- > 2.51.0 >
