From daaed0199ee57013ae011421a7e90b7bdd295373 Mon Sep 17 00:00:00 2001 From: Iain Sandoe Date: Sat, 27 Jun 2020 08:18:34 +0100 Subject: [PATCH] coroutines: Handle awaiters that are sub-objects [PR95736] Move deciding on initializers for awaitables to the build of the co_await, this allows us to analyse cases that do not need a temporary at that point. As the PR shows, the late analysis meant that we were not checking properly for the case that an awaiter is a sub-object of an existing variable outside the current function scope (and therefore does not need to be duplicated in the frame). gcc/cp/ChangeLog: PR c++/95736 * coroutines.cc (get_awaitable_var): New helper. (build_co_await): Check more carefully before copying an awaitable. (expand_one_await_expression): No initializer is required when the awaitable is not a temp. (register_awaits): Remove handling that is now completed when the await expression is built. gcc/testsuite/ChangeLog: PR c++/95736 * g++.dg/coroutines/pr95736.C: New test. --- gcc/cp/coroutines.cc | 112 ++++++++++++++++------ gcc/testsuite/g++.dg/coroutines/pr95736.C | 84 ++++++++++++++++ 2 files changed, 166 insertions(+), 30 deletions(-) create mode 100644 gcc/testsuite/g++.dg/coroutines/pr95736.C diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc index 8b8d00e8e0c..bab03d44863 100644 --- a/gcc/cp/coroutines.cc +++ b/gcc/cp/coroutines.cc @@ -740,6 +740,30 @@ enum suspend_point_kind { FINAL_SUSPEND_POINT }; +/* Helper function to build a named variable for the temps we use for each + await point. The root of the name is determined by SUSPEND_KIND, and + the variable is of type V_TYPE. The awaitable number is reset each time + we encounter a final suspend. */ + +static tree +get_awaitable_var (suspend_point_kind suspend_kind, tree v_type) +{ + static int awn = 0; + char *buf; + switch (suspend_kind) + { + default: buf = xasprintf ("Aw%d", awn++); break; + case CO_YIELD_SUSPEND_POINT: buf = xasprintf ("Yd%d", awn++); break; + case INITIAL_SUSPEND_POINT: buf = xasprintf ("Is"); break; + case FINAL_SUSPEND_POINT: buf = xasprintf ("Fs"); awn = 0; break; + } + tree ret = get_identifier (buf); + free (buf); + ret = build_lang_decl (VAR_DECL, ret, v_type); + DECL_ARTIFICIAL (ret) = true; + return ret; +} + /* This performs [expr.await] bullet 3.3 and validates the interface obtained. It is also used to build the initial and final suspend points. @@ -798,23 +822,57 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind) return error_mark_node; /* To complete the lookups, we need an instance of 'e' which is built from - 'o' according to [expr.await] 3.4. However, we don't want to materialize - 'e' here (it might need to be placed in the coroutine frame) so we will - make a temp placeholder instead. If 'o' is a parameter or a local var, - then we do not need an additional var (parms and local vars are already - copied into the frame and will have lifetimes according to their original - scope). */ + 'o' according to [expr.await] 3.4. + + If we need to materialize this as a temporary, then that will have to be + 'promoted' to a coroutine frame var. However, if the awaitable is a + user variable, parameter or comes from a scope outside this function, + then we must use it directly - or we will see unnecessary copies. + + If o is a variable, find the underlying var. */ tree e_proxy = STRIP_NOPS (o); if (INDIRECT_REF_P (e_proxy)) e_proxy = TREE_OPERAND (e_proxy, 0); + while (TREE_CODE (e_proxy) == COMPONENT_REF) + { + e_proxy = TREE_OPERAND (e_proxy, 0); + if (INDIRECT_REF_P (e_proxy)) + e_proxy = TREE_OPERAND (e_proxy, 0); + if (TREE_CODE (e_proxy) == CALL_EXPR) + { + /* We could have operator-> here too. */ + tree op = TREE_OPERAND (CALL_EXPR_FN (e_proxy), 0); + if (DECL_OVERLOADED_OPERATOR_P (op) + && DECL_OVERLOADED_OPERATOR_IS (op, COMPONENT_REF)) + { + e_proxy = CALL_EXPR_ARG (e_proxy, 0); + STRIP_NOPS (e_proxy); + gcc_checking_assert (TREE_CODE (e_proxy) == ADDR_EXPR); + e_proxy = TREE_OPERAND (e_proxy, 0); + } + } + STRIP_NOPS (e_proxy); + } + + /* Only build a temporary if we need it. */ if (TREE_CODE (e_proxy) == PARM_DECL - || (VAR_P (e_proxy) && (!DECL_ARTIFICIAL (e_proxy) - || DECL_HAS_VALUE_EXPR_P (e_proxy)))) - e_proxy = o; + || (VAR_P (e_proxy) && !is_local_temp (e_proxy))) + { + e_proxy = o; + o = NULL_TREE; /* The var is already present. */ + } + else if (CLASS_TYPE_P (o_type) || TYPE_NEEDS_CONSTRUCTING (o_type)) + { + e_proxy = get_awaitable_var (suspend_kind, o_type); + releasing_vec arg (make_tree_vector_single (rvalue (o))); + o = build_special_member_call (e_proxy, complete_ctor_identifier, + &arg, o_type, LOOKUP_NORMAL, + tf_warning_or_error); + } else { - e_proxy = build_lang_decl (VAR_DECL, NULL_TREE, o_type); - DECL_ARTIFICIAL (e_proxy) = true; + e_proxy = get_awaitable_var (suspend_kind, o_type); + o = build2 (INIT_EXPR, o_type, e_proxy, rvalue (o)); } /* I suppose we could check that this is contextually convertible to bool. */ @@ -1531,16 +1589,14 @@ expand_one_await_expression (tree *stmt, tree *await_expr, void *d) tree await_type = TREE_TYPE (var); tree stmt_list = NULL; - tree t_expr = STRIP_NOPS (expr); tree r; tree *await_init = NULL; - if (t_expr == var) - needs_dtor = false; + + if (!expr) + needs_dtor = false; /* No need, the var's lifetime is managed elsewhere. */ else { - /* Initialize the var from the provided 'o' expression. */ - r = build2 (INIT_EXPR, await_type, var, expr); - r = coro_build_cvt_void_expr_stmt (r, loc); + r = coro_build_cvt_void_expr_stmt (expr, loc); append_to_statement_list_force (r, &stmt_list); /* We have an initializer, which might itself contain await exprs. */ await_init = tsi_stmt_ptr (tsi_last (stmt_list)); @@ -2840,15 +2896,12 @@ register_awaits (tree *stmt, int *do_subtree ATTRIBUTE_UNUSED, void *d) /* If the awaitable is a parm or a local variable, then we already have a frame copy, so don't make a new one. */ tree aw = TREE_OPERAND (aw_expr, 1); + tree o = TREE_OPERAND (aw_expr, 2); /* Initializer for the frame var. */ + /* If we have an initializer, then the var is a temp and we need to make + it in the frame. */ tree aw_field_type = TREE_TYPE (aw); tree aw_field_nam = NULL_TREE; - if (INDIRECT_REF_P (aw)) - aw = TREE_OPERAND (aw, 0); - if (TREE_CODE (aw) == PARM_DECL - || (VAR_P (aw) && (!DECL_ARTIFICIAL (aw) - || DECL_HAS_VALUE_EXPR_P (aw)))) - ; /* Don't make an additional copy. */ - else + if (o) { /* The required field has the same type as the proxy stored in the await expr. */ @@ -2856,13 +2909,12 @@ register_awaits (tree *stmt, int *do_subtree ATTRIBUTE_UNUSED, void *d) aw_field_nam = coro_make_frame_entry (data->field_list, nam, aw_field_type, aw_loc); free (nam); - } - tree o = TREE_OPERAND (aw_expr, 2); /* Initialiser for the frame var. */ - /* If this is a target expression, then we need to remake it to strip off - any extra cleanups added. */ - if (TREE_CODE (o) == TARGET_EXPR) - TREE_OPERAND (aw_expr, 2) = get_target_expr (TREE_OPERAND (o, 1)); + /* If the init is a target expression, then we need to remake it to + strip off any extra cleanups added. */ + if (o && TREE_CODE (o) == TARGET_EXPR) + TREE_OPERAND (aw_expr, 2) = get_target_expr (TREE_OPERAND (o, 1)); + } tree v = TREE_OPERAND (aw_expr, 3); o = TREE_VEC_ELT (v, 1); diff --git a/gcc/testsuite/g++.dg/coroutines/pr95736.C b/gcc/testsuite/g++.dg/coroutines/pr95736.C new file mode 100644 index 00000000000..0be5168a8d2 --- /dev/null +++ b/gcc/testsuite/g++.dg/coroutines/pr95736.C @@ -0,0 +1,84 @@ +#include +#include +#include + +#if __has_include("coroutine") +#include +namespace stdcoro = std; +#else +#include +namespace stdcoro = std::experimental; +#endif + +struct footable : stdcoro::suspend_always { + footable() noexcept = default; + ~footable() { assert(released); } + footable(const footable&) = delete; + + using coro_handle = stdcoro::coroutine_handle<>; + + void await_suspend(coro_handle awaiter) noexcept { + std::cout << "suspending to footable " << this << std::endl; + assert(!handle); + handle = awaiter; + } + void await_resume() noexcept { + std::cout << "resuming from footable " << this << std::endl; + assert(handle); + handle = {}; + } + + void operator()() noexcept { + std::cout << "operator() on " << this << std::endl; + assert(handle); + handle.resume(); + handle = {}; + } + + void release() noexcept { released = true; } +private: + coro_handle handle; + bool released = false; +}; + +struct footask { + struct promise_type { + using coro_handle = stdcoro::coroutine_handle; + + stdcoro::suspend_never initial_suspend() noexcept { return {}; } + stdcoro::suspend_never final_suspend() noexcept { std::cout << "final suspend" << std::endl; return {}; } + void unhandled_exception() {} + void return_void() noexcept { std::cout << "coro returns" << std::endl; } + + footask get_return_object() { return footask{ coro_handle::from_promise(*this) }; } + }; + + footask(promise_type::coro_handle handle) : handle(handle) {} + ~footask() { assert(handle.done()); } + + promise_type::coro_handle handle; +}; + +struct bar { + bar() = default; + bar(const bar&) = delete; + + footable foo{}; + footask task = taskfun(); + + footask taskfun() noexcept { + std::cout << "coro begin" << std::endl; + co_await foo; + std::cout << "coro end" << std::endl; + } +}; + +int main() { + bar foobar; + foobar.foo(); + assert(foobar.task.handle.done()); + std::cout << "releasing" << std::endl; + foobar.foo.release(); + std::cout << "done" << std::endl; + return 0; +} -- 2.30.2