[PATCH] coroutines : Handle rethrow from unhandled_exception [PR98704].

Iain Sandoe iain@sandoe.co.uk
Mon Mar 15 00:25:45 GMT 2021


Hi

Although there is still some discussion in CWG2451 on this, the
implementors are agreed on the intent (thus it is wording that is
expected to change - the implementations should be brought into
sync).

tested on x86_64-darwin, x86_64-linux-gnu and with cppcoro and
folly/coroutines.

OK for master / 10.x?
thanks
Iain

---

When promise.unhandled_exception () is entered, the coroutine is
considered to be still running - returning from the method will
cause the final await expression to be evaluated.

If the method throws, that action is considered to make the
coroutine suspend (since, otherwise, it would be impossible to
reclaim its resources, since one cannot destroy a running coro).

The wording issue is to do with how to represent the place at
which the coroutine should be considered suspended.

For the implementation here, that place is immediately before the
promise life-time ends. A handler for the rethrown exception, can
thus call xxxx.destroy() which will run DTORs for the promise and
any parameter copies [as needed] then the coroutine frame will be
deallocated.

At present, we also set "done=true" in this case (for compatibility
with other current implementations).  One might consider 'done()'
to be misleading in the case of an abnormal termination - that is
also part of the CGW 2451 discussion.

gcc/cp/ChangeLog:

	PR c++/98740
	* coroutines.cc (build_actor_fn): Make destroy index 1
	correspond to the abnormal unhandled_exception() exit.
	Substitute the proxy for the resume index.
	(coro_rewrite_function_body): Arrange to reset the resume
	index and make done = true for a rethrown exception from
	unhandled_exception ().
	(morph_fn_to_coro): Adjust calls to build_actor_fn and
	coro_rewrite_function_body.

gcc/testsuite/ChangeLog:

	PR c++/98704
	* g++.dg/coroutines/torture/pr98704.C: New test.
---
 gcc/cp/coroutines.cc                          | 75 ++++++++++-----
 .../g++.dg/coroutines/torture/pr98704.C       | 93 +++++++++++++++++++
 2 files changed, 147 insertions(+), 21 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/torture/pr98704.C

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index 438538a0b4f..ea714da146b 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -2145,7 +2145,7 @@ build_actor_fn (location_t loc, tree coro_frame_type, tree actor, tree fnbody,
 		tree orig, hash_map<tree, param_info> *param_uses,
 		hash_map<tree, local_var_info> *local_var_uses,
 		vec<tree, va_gc> *param_dtor_list, tree resume_fn_field,
-		unsigned body_count, tree frame_size)
+		tree resume_idx_field, unsigned body_count, tree frame_size)
 {
   verify_stmt_tree (fnbody);
   /* Some things we inherit from the original function.  */
@@ -2267,6 +2267,17 @@ build_actor_fn (location_t loc, tree coro_frame_type, tree actor, tree fnbody,
   b = coro_build_cvt_void_expr_stmt (b, loc);
   add_stmt (b);
 
+  /* The destroy point numbered #1 is special, in that it is reached from a
+     coroutine that is suspended after re-throwing from unhandled_exception().
+     This label just invokes the cleanup of promise, param copies and the
+     frame itself.  */
+  tree del_promise_label
+    = create_named_label_with_ctx (loc, "coro.delete.promise", actor);
+  b = build_case_label (build_int_cst (short_unsigned_type_node, 1), NULL_TREE,
+			create_anon_label_with_ctx (loc, actor));
+  add_stmt (b);
+  add_stmt (build_stmt (loc, GOTO_EXPR, del_promise_label));
+
   short unsigned lab_num = 3;
   for (unsigned destr_pt = 0; destr_pt < body_count; destr_pt++)
     {
@@ -2371,9 +2382,10 @@ build_actor_fn (location_t loc, tree coro_frame_type, tree actor, tree fnbody,
   p_data.to = ap;
   cp_walk_tree (&fnbody, replace_proxy, &p_data, NULL);
 
-  /* Set the actor pointer to null, so that 'done' will work.
-     Resume from here is UB anyway - although a 'ready' await will
-     branch to the final resume, and fall through to the destroy.  */
+  /* The rewrite of the function adds code to set the __resume field to
+     nullptr when the coroutine is done and also the index to zero when
+     calling an unhandled exception.  These are represented by two proxies
+     in the function, so rewrite them to the proper frame access.  */
   tree resume_m
     = lookup_member (coro_frame_type, get_identifier ("__resume"),
 		     /*protect=*/1, /*want_type=*/0, tf_warning_or_error);
@@ -2383,12 +2395,14 @@ build_actor_fn (location_t loc, tree coro_frame_type, tree actor, tree fnbody,
   p_data.to = res_x;
   cp_walk_tree (&fnbody, replace_proxy, &p_data, NULL);
 
+  p_data.from = resume_idx_field;
+  p_data.to = rat;
+  cp_walk_tree (&fnbody, replace_proxy, &p_data, NULL);
+
   /* Add in our function body with the co_returns rewritten to final form.  */
   add_stmt (fnbody);
 
   /* now do the tail of the function.  */
-  tree del_promise_label
-    = create_named_label_with_ctx (loc, "coro.delete.promise", actor);
   r = build_stmt (loc, LABEL_EXPR, del_promise_label);
   add_stmt (r);
 
@@ -4022,9 +4036,9 @@ act_des_fn (tree orig, tree fn_type, tree coro_frame_ptr, const char* name)
 /* Re-write the body as per [dcl.fct.def.coroutine] / 5.  */
 
 static tree
-coro_rewrite_function_body (location_t fn_start, tree fnbody,
-			    tree orig, tree resume_fn_ptr_type,
-			    tree& resume_fn_field, tree& fs_label)
+coro_rewrite_function_body (location_t fn_start, tree fnbody, tree orig,
+			    tree resume_fn_ptr_type, tree& resume_fn_field,
+			    tree& resume_idx_field, tree& fs_label)
 {
   /* This will be our new outer scope.  */
   tree update_body = build3 (BIND_EXPR, void_type_node, NULL, NULL, NULL);
@@ -4068,6 +4082,25 @@ coro_rewrite_function_body (location_t fn_start, tree fnbody,
   tree return_void
     = get_coroutine_return_void_expr (current_function_decl, fn_start, false);
 
+  /* We will need to be able to set the resume function pointer to nullptr
+     to signal that the coroutine is 'done'.  */
+  resume_fn_field
+    = build_lang_decl (VAR_DECL, get_identifier ("resume.fn.ptr.proxy"),
+		       resume_fn_ptr_type);
+  DECL_ARTIFICIAL (resume_fn_field) = true;
+  tree zero_resume
+    = build1 (CONVERT_EXPR, resume_fn_ptr_type, integer_zero_node);
+  zero_resume
+    = build2 (INIT_EXPR, resume_fn_ptr_type, resume_fn_field, zero_resume);
+  /* Likewise, the resume index needs to be reset.  */
+  resume_idx_field
+    = build_lang_decl (VAR_DECL, get_identifier ("resume.index.proxy"),
+		       short_unsigned_type_node);
+  DECL_ARTIFICIAL (resume_idx_field) = true;
+  tree zero_resume_idx = build_int_cst (short_unsigned_type_node, 0);
+  zero_resume_idx = build2 (INIT_EXPR, short_unsigned_type_node,
+			    resume_idx_field, zero_resume_idx);
+
   if (flag_exceptions)
     {
       /* Build promise.unhandled_exception();  */
@@ -4126,7 +4159,13 @@ coro_rewrite_function_body (location_t fn_start, tree fnbody,
       IF_SCOPE (not_iarc_if) = NULL;
       not_iarc_if = do_poplevel (iarc_scope);
       add_stmt (not_iarc_if);
-      /* ... else call the promise unhandled exception method.  */
+      /* ... else call the promise unhandled exception method
+	 but first we set done = true and the resume index to 0.
+	 If the unhandled exception method returns, then we continue
+	 to the final await expression (which duplicates the clearing of
+	 the field). */
+      finish_expr_stmt (zero_resume);
+      finish_expr_stmt (zero_resume_idx);
       ueh = maybe_cleanup_point_expr_void (ueh);
       add_stmt (ueh);
       finish_handler (handler);
@@ -4163,14 +4202,6 @@ coro_rewrite_function_body (location_t fn_start, tree fnbody,
   /* Before entering the final suspend point, we signal that this point has
      been reached by setting the resume function pointer to zero (this is
      what the 'done()' builtin tests) as per the current ABI.  */
-  resume_fn_field
-    = build_lang_decl (VAR_DECL, get_identifier ("resume.fn.ptr.proxy"),
-		       resume_fn_ptr_type);
-  DECL_ARTIFICIAL (resume_fn_field) = true;
-  tree zero_resume
-    = build1 (CONVERT_EXPR, resume_fn_ptr_type, integer_zero_node);
-  zero_resume
-    = build2 (INIT_EXPR, resume_fn_ptr_type, resume_fn_field, zero_resume);
   finish_expr_stmt (zero_resume);
   finish_expr_stmt (build_init_or_final_await (fn_start, true));
   BIND_EXPR_BODY (update_body) = pop_stmt_list (BIND_EXPR_BODY (update_body));
@@ -4314,9 +4345,11 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
      the requirements for the coroutine frame.  */
 
   tree resume_fn_field = NULL_TREE;
+  tree resume_idx_field = NULL_TREE;
   tree fs_label = NULL_TREE;
-  fnbody = coro_rewrite_function_body (fn_start, fnbody, orig, act_des_fn_ptr,
-				       resume_fn_field, fs_label);
+  fnbody = coro_rewrite_function_body (fn_start, fnbody, orig,
+				       act_des_fn_ptr, resume_fn_field,
+				       resume_idx_field, fs_label);
   /* Build our dummy coro frame layout.  */
   coro_frame_type = begin_class_definition (coro_frame_type);
 
@@ -5198,7 +5231,7 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
   /* Build the actor...  */
   build_actor_fn (fn_start, coro_frame_type, actor, fnbody, orig, param_uses,
 		  &local_var_uses, param_dtor_list, resume_fn_field,
-		  body_aw_points.await_number, frame_size);
+		  resume_idx_field, body_aw_points.await_number, frame_size);
 
   /* Destroyer ... */
   build_destroy_fn (fn_start, coro_frame_type, destroy, actor);
diff --git a/gcc/testsuite/g++.dg/coroutines/torture/pr98704.C b/gcc/testsuite/g++.dg/coroutines/torture/pr98704.C
new file mode 100644
index 00000000000..84ebfb2e601
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/torture/pr98704.C
@@ -0,0 +1,93 @@
+//  { dg-do run }
+#include "../coro.h"
+
+//#include <iostream>
+//#include <coroutine>
+#include <stdexcept>
+
+int frame_live = 0;
+int promise_live = 0;
+int task_live = 0;
+
+struct Task
+{
+    struct promise_type;
+    using handle = std::coroutine_handle<promise_type>;
+
+    struct promise_type
+    {
+        promise_type () { promise_live++; PRINT ("promise_type ()"); }
+        ~promise_type () { promise_live--; PRINT ("~promise_type ()"); }
+        void* operator new(size_t sz) {
+            PRINT("operator new()");
+            frame_live++;
+            return ::operator new(sz);
+        }
+        void operator delete(void* p, size_t sz) {
+            PRINT("operator delete");
+            frame_live--;
+            return ::operator delete(p, sz);
+        }
+
+        Task get_return_object() { return handle::from_promise(*this); }
+        auto initial_suspend() noexcept { return std::suspend_always{}; }
+        auto final_suspend() noexcept { return std::suspend_always{}; }
+        void return_void() noexcept {}
+
+        auto yield_value(int x) noexcept
+        {
+            PRINTF ("yield_value(%d)\n", x);
+            return std::suspend_always{};
+        }
+
+        void unhandled_exception()
+        {
+            PRINT ("unhandled_exception()");
+            throw;
+        }
+    };
+
+    Task(handle h) : coro(h) { task_live++; PRINT ("Task(handle h)"); }
+    ~Task() { task_live--; PRINT ("~Task()"); if (coro) coro.destroy(); }
+
+    handle coro;
+};
+
+Task myco()
+{
+    co_yield 42;
+    throw std::out_of_range("TEST EXCEPTION");
+}
+
+int main()
+{
+  {
+    Task task = myco();
+    PRINT ("START");
+    try {
+        PRINTF ("done #0 = %d\n", task.coro.done());
+        if (task.coro.done())
+          abort();
+        task.coro.resume(); // will yield 42
+        PRINTF ("done #1 = %d\n", task.coro.done());
+        if (task.coro.done())
+          abort();        
+        task.coro.resume(); // will throw exception
+        PRINT ("should not be reached");
+        abort ();
+    }
+    catch (const std::exception&) {
+        PRINTF ("done exc = %d\n", task.coro.done());
+        if (!task.coro.done())
+          abort();        
+    }
+    if (!task.coro.done())
+      abort();        
+  } // should cause cause the destroy () to run.
+  if (task_live || promise_live || frame_live)
+    {
+      PRINTF ("task_live = %d, promise_live = %d, frame_live = %d\n",
+               task_live, promise_live, frame_live);
+      abort ();
+    }
+}
\ No newline at end of file
-- 
2.24.1



More information about the Gcc-patches mailing list