Hi Bin,
bin.cheng <bin.ch...@linux.alibaba.com> wrote:
> By standard, coroutine body should be encapsulated in try-catch block
> as following:
> try {
> // coroutine body
> } catch(...) {
> promise.unhandled_exception();
> }
> Given above try-catch block is implemented in the coroutine actor
> function called by coroutine ramp function, so the promise should
> be accessed via actor function's coroutine frame pointer argument,
> rather than the ramp function's coroutine frame variable.
thanks, good catch!
it has not triggered for me (including on some more complex test-cases that are
not useable
in the testsuite).
> This patch also refactored code to make the fix easy, unfortunately,
see below,
> I failed to reduce a test case from cpproro.
it would be good if we could try to find a reproducer. I’m happy to try and
throw
creduce at a preprocessed file, if you have one.
> gcc/cp
> 2020-01-20 Bin Cheng <bin.li...@linux.alibaba.com>
>
> * coroutines.cc (act_des_fn, wrap_coroutine_body): New.
> (morph_fn_to_coro): Call act_des_fn to build actor/destroy decls, as
> well as access promise via actor function's frame pointer argument.
> Refactor code into above functions.
> (build_actor_fn, build_destroy_fn): Use frame pointer argument
diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index b04baae..1b0338c5 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -1827,11 +1827,7 @@ build_actor_fn (location_t loc, tree coro_frame_type,
tree actor, tree fnbody,
tree act_des_fn_ptr = build_pointer_type (act_des_fn_type);
/* One param, the coro frame pointer. */
- tree actor_fp
- = build_lang_decl (PARM_DECL, get_identifier ("frame_ptr"),
coro_frame_ptr);
- DECL_CONTEXT (actor_fp) = actor;
- DECL_ARG_TYPE (actor_fp) = type_passed_as (coro_frame_ptr);
- DECL_ARGUMENTS (actor) = actor_fp;
+ tree actor_fp = DECL_ARGUMENTS (actor);
/* A void return. */
tree resdecl = build_decl (loc, RESULT_DECL, 0, void_type_node);
@@ -2218,12 +2214,7 @@ build_destroy_fn (location_t loc, tree coro_frame_type,
tree destroy,
tree actor)
{
/* One param, the coro frame pointer. */
- tree coro_frame_ptr = build_pointer_type (coro_frame_type);
- tree destr_fp
- = build_lang_decl (PARM_DECL, get_identifier ("frame_ptr"),
coro_frame_ptr);
- DECL_CONTEXT (destr_fp) = destroy;
- DECL_ARG_TYPE (destr_fp) = type_passed_as (coro_frame_ptr);
- DECL_ARGUMENTS (destroy) = destr_fp;
+ tree destr_fp = DECL_ARGUMENTS (destroy);
/* A void return. */
tree resdecl = build_decl (loc, RESULT_DECL, 0, void_type_node);
@@ -2861,6 +2852,111 @@ register_local_var_uses (tree *stmt, int *do_subtree,
void *d)
return NULL_TREE;
}
+/* Build, return FUNCTION_DECL node with its coroutine frame pointer argument
+ for either actor or destroy functions. */
+
+static tree
+act_des_fn (tree orig, tree fn_type, tree coro_frame_ptr, const char* name)
+{
+ tree fn_name = get_fn_local_identifier (orig, name);
+ tree fn = build_lang_decl (FUNCTION_DECL, fn_name, fn_type);
+ DECL_CONTEXT (fn) = DECL_CONTEXT (orig);
+ DECL_INITIAL (fn) = error_mark_node;
+ tree id = get_identifier ("frame_ptr");
+ tree fp = build_lang_decl (PARM_DECL, id, coro_frame_ptr);
+ DECL_CONTEXT (fp) = fn;
+ DECL_ARG_TYPE (fp) = type_passed_as (coro_frame_ptr);
+ DECL_ARGUMENTS (fn) = fp;
+ return fn;
+}
^^^ these (and the hunk in morph_fn_to_coro) or an equivalent are prequisite
for the actual
fix and a nice tidy-up, thanks.
+/* Wrap and return the function body in a try {} catch (...) {} block if
+ exceptions are needed. */
+
+static tree
+wrap_coroutine_body (tree coro_frame_type, tree actor, tree fnbody, tree orig)
+{
+ /* First make a new block for the body - that will be embedded in the
+ re-written function. */
+ tree first = expr_first (fnbody);
+ bool orig_fn_has_outer_bind = false;
+ tree replace_blk = NULL_TREE;
+ if (first && TREE_CODE (first) == BIND_EXPR)
+ {
+ orig_fn_has_outer_bind = true;
+ tree block = BIND_EXPR_BLOCK (first);
+ replace_blk = make_node (BLOCK);
+ if (block) // missing block is probably an error.
+ {
+ gcc_assert (BLOCK_SUPERCONTEXT (block) == NULL_TREE);
+ gcc_assert (BLOCK_CHAIN (block) == NULL_TREE);
+ BLOCK_VARS (replace_blk) = BLOCK_VARS (block);
+ BLOCK_SUBBLOCKS (replace_blk) = BLOCK_SUBBLOCKS (block);
+ for (tree b = BLOCK_SUBBLOCKS (replace_blk); b; b = BLOCK_CHAIN (b))
+ BLOCK_SUPERCONTEXT (b) = replace_blk;
+ }
+ BIND_EXPR_BLOCK (first) = replace_blk;
+ }
+
+ location_t loc = DECL_SOURCE_LOCATION (orig);
+ if (flag_exceptions)
+ {
+ tree ueh_meth
+ = lookup_promise_method (orig, coro_unhandled_exception_identifier,
+ loc, /*musthave=*/ true);
+ /* actor's version of the promise. */
+ tree actor_frame = build1_loc (loc, INDIRECT_REF, coro_frame_type,
+ DECL_ARGUMENTS (actor));
+ tree ap_m = lookup_member (coro_frame_type, get_identifier ("__p"), 1, 0,
+ tf_warning_or_error);
+ tree ap = build_class_member_access_expr (actor_frame, ap_m, NULL_TREE,
+ false, tf_warning_or_error);
+ /* Build promise.unhandled_exception(); */
+ tree ueh = build_new_method_call (ap, ueh_meth, NULL, NULL_TREE,
+ LOOKUP_NORMAL, NULL,
+ tf_warning_or_error);
+
+ /* The try block is just the original function, there's no real
+ need to call any function to do this. */
+ fnbody = build_stmt (loc, TRY_BLOCK, fnbody, NULL_TREE);
+ TRY_HANDLERS (fnbody) = push_stmt_list ();
+ /* Mimic what the parser does for the catch. */
+ tree handler = begin_handler ();
+ finish_handler_parms (NULL_TREE, handler); /* catch (...) */
+ ueh = maybe_cleanup_point_expr_void (ueh);
+ add_stmt (ueh);
+ finish_handler (handler);
+ TRY_HANDLERS (fnbody) = pop_stmt_list (TRY_HANDLERS (fnbody));
+ /* If the function starts with a BIND_EXPR, then we need to create
+ one here to contain the try-catch and to link up the scopes. */
+ if (orig_fn_has_outer_bind)
+ {
+ fnbody = build3 (BIND_EXPR, void_type_node, NULL, fnbody, NULL);
+ /* Make and connect the scope blocks. */
+ tree tcb_block = make_node (BLOCK);
+ /* .. and connect it here. */
+ BLOCK_SUPERCONTEXT (replace_blk) = tcb_block;
+ BLOCK_SUBBLOCKS (tcb_block) = replace_blk;
+ BIND_EXPR_BLOCK (fnbody) = tcb_block;
+ }
+ }
+ else if (pedantic)
+ {
+ /* We still try to look for the promise method and warn if it's not
+ present. */
+ tree ueh_meth
+ = lookup_promise_method (orig, coro_unhandled_exception_identifier,
+ loc, /*musthave=*/ false);
+ if (!ueh_meth || ueh_meth == error_mark_node)
+ warning_at (loc, 0, "no member named %qE in %qT",
+ coro_unhandled_exception_identifier,
+ get_coroutine_promise_type (orig));
+ }
+ /* Else we don't check and don't care if the method is missing. */
+
+ return fnbody;
+}
IMO this ^^^ obfuscates the fix, and I don’t think it should be done at the
same time.
I don’t (personally) think it has to be oulined, since the functionality is
only used once
(but other maintainers might take a different view).
/* Here we:
a) Check that the function and promise type are valid for a
coroutine.
@@ -2988,17 +3084,9 @@ morph_fn_to_coro (tree orig, tree *resumer, tree
*destroyer)
= build_function_type_list (void_type_node, coro_frame_ptr, NULL_TREE);
tree act_des_fn_ptr = build_pointer_type (act_des_fn_type);
- /* Declare the actor function. */
- tree actor_name = get_fn_local_identifier (orig, "actor");
- tree actor = build_lang_decl (FUNCTION_DECL, actor_name, act_des_fn_type);
- DECL_CONTEXT (actor) = DECL_CONTEXT (orig);
- DECL_INITIAL (actor) = error_mark_node;
-
- /* Declare the destroyer function. */
- tree destr_name = get_fn_local_identifier (orig, "destroy");
- tree destroy = build_lang_decl (FUNCTION_DECL, destr_name, act_des_fn_type);
- DECL_CONTEXT (destroy) = DECL_CONTEXT (orig);
- DECL_INITIAL (destroy) = error_mark_node;
+ /* Declare the actor and destroyer function. */
+ tree actor = act_des_fn (orig, act_des_fn_type, coro_frame_ptr, "actor");
+ tree destroy = act_des_fn (orig, act_des_fn_type, coro_frame_ptr, "destroy");
^^^^ but this is needed (or the equivalent) to enable the fix.
thanks
Iain
