coroutines : Handle rethrow from unhandled_exception [PR98704].
authorIain Sandoe <iain@sandoe.co.uk>
Thu, 11 Mar 2021 17:04:14 +0000 (17:04 +0000)
committerIain Sandoe <iain@sandoe.co.uk>
Mon, 15 Mar 2021 15:54:29 +0000 (15:54 +0000)
Although there is still some discussion in CWG 2451 on this, the
implementors are agreed on the intent.

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 CWG 2451 discussion.

gcc/cp/ChangeLog:

PR c++/98704
* 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
gcc/testsuite/g++.dg/coroutines/torture/pr98704.C [new file with mode: 0644]

index 438538a..ea714da 100644 (file)
@@ -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 (file)
index 0000000..15db250
--- /dev/null
@@ -0,0 +1,91 @@
+//  { dg-do run }
+#include "../coro.h"
+
+#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 ();
+    }
+}