[PATCH Coroutines]Insert the default return_void call at correct position

[PATCH Coroutines]Insert the default return_void call at correct position

[bin.cheng]

[-- Attachment #1: Type: text/plain, Size: 919 bytes --]

Hi,

Exception in coroutine is not correctly handled because the default
return_void call is now inserted before the finish suspend point,
rather than at the end of the original coroutine body.  This patch
fixes the issue by generating following code:
  co_await promise.initial_suspend();
  try {
    // The original coroutine body

    promise.return_void(); // The default return_void call.
  } catch (...) {
    promise.unhandled_exception();
  }
  final_suspend:
  // ...

Bootstrap and test on x86_64.  Is it OK?

Thanks,
bin

gcc/cp
2020-02-03  Bin Cheng  <bin.cheng@linux.alibaba.com>

        * coroutines.cc (build_actor_fn): Factor out code inserting the
        default return_void call to...
        (morph_fn_to_coro): ...here, also hoist local var declarations.

gcc/testsuite
2020-02-03  Bin Cheng  <bin.cheng@linux.alibaba.com>

        * g++.dg/coroutines/torture/co-ret-15-default-return_void.C: New.

[-- Attachment #2: 0001-Insert-default-return_void-at-the-end-of-coroutine-b.patch --]
[-- Type: application/octet-stream, Size: 6686 bytes --]

Subject: [PATCH] Insert default return_void at the end of coroutine body

Exception in coroutine is not correctly handled because the default
return_void call is now inserted before the finish suspend point,
rather than at the end of the original coroutine body.  This patch
fixes the issue by expanding code as following:
  co_await promise.initial_suspend();
  try {
    // The original coroutine body

    promise.return_void(); // The default return_void call.
  } catch (...) {
    promise.unhandled_exception();
  }
  final_suspend:
  // ...
---
 gcc/cp/coroutines.cc                               | 68 ++++++++++++----------
 .../torture/co-ret-15-default-return_void.C        | 55 +++++++++++++++++
 2 files changed, 92 insertions(+), 31 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/torture/co-ret-15-default-return_void.C

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index f7f85cb..a606769 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -2025,21 +2025,6 @@ build_actor_fn (location_t loc, tree coro_frame_type, tree actor, tree fnbody,
   r = coro_build_expr_stmt (initial_await, loc);
   add_stmt (r);
 
-  /* Now we've built the promise etc, process fnbody for co_returns.
-     We want the call to return_void () below and it has no params so
-     we can create it once here.
-     Calls to return_value () will have to be checked and created as
-     required.  */
-
-  tree return_void = NULL_TREE;
-  tree rvm
-    = lookup_promise_method (orig, coro_return_void_identifier, loc,
-			     /*musthave=*/false);
-  if (rvm && rvm != error_mark_node)
-    return_void
-      = build_new_method_call (ap, rvm, NULL, NULL_TREE, LOOKUP_NORMAL, NULL,
-			       tf_warning_or_error);
-
   /* co_return branches to the final_suspend label, so declare that now.  */
   tree fs_label = create_named_label_with_ctx (loc, "final.suspend", actor);
 
@@ -2054,15 +2039,6 @@ build_actor_fn (location_t loc, tree coro_frame_type, tree actor, tree fnbody,
   /* Add in our function body with the co_returns rewritten to final form.  */
   add_stmt (fnbody);
 
-  /* [stmt.return.coroutine] (2.2 : 3) if p.return_void() is a valid
-     expression, flowing off the end of a coroutine is equivalent to
-     co_return; otherwise UB.
-     We just inject the call to p.return_void() here, and fall through to
-     the final_suspend: label (eliding the goto).  If the function body has
-     a co_return, then this statement will be unreachable and DCEd.  */
-  if (return_void != NULL_TREE)
-    add_stmt (return_void);
-
   /* Final suspend starts here.  */
   r = build_stmt (loc, LABEL_EXPR, fs_label);
   add_stmt (r);
@@ -3594,18 +3570,48 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
       BIND_EXPR_BLOCK (first) = replace_blk;
     }
 
+  /* actor's version of the promise.  */
+  tree actor_frame = build1_loc (fn_start, 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);
+
+  /* Now we've built the promise etc, process fnbody for co_returns.
+     We want the call to return_void () below and it has no params so
+     we can create it once here.
+     Calls to return_value () will have to be checked and created as
+     required.  */
+
+  tree return_void = NULL_TREE;
+  tree rvm
+    = lookup_promise_method (orig, coro_return_void_identifier, fn_start,
+			     /*musthave=*/false);
+  if (rvm && rvm != error_mark_node)
+    return_void
+      = build_new_method_call (ap, rvm, NULL, NULL_TREE, LOOKUP_NORMAL, NULL,
+			       tf_warning_or_error);
+
+  /* [stmt.return.coroutine] (2.2 : 3) if p.return_void() is a valid
+     expression, flowing off the end of a coroutine is equivalent to
+     co_return; otherwise UB.
+     We just inject the call to p.return_void() here, and fall through to
+     the final_suspend: label (eliding the goto).  If the function body has
+     a co_return, then this statement will be unreachable and DCEd.  */
+  if (return_void != NULL_TREE)
+    {
+      tree append = push_stmt_list ();
+      add_stmt (fnbody);
+      add_stmt (return_void);
+      fnbody = pop_stmt_list(append);
+    }
+
   if (flag_exceptions)
     {
       tree ueh_meth
 	= lookup_promise_method (orig, coro_unhandled_exception_identifier,
 				 fn_start, /*musthave=*/true);
-      /* actor's version of the promise.  */
-      tree actor_frame = build1_loc (fn_start, 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,
diff --git a/gcc/testsuite/g++.dg/coroutines/torture/co-ret-15-default-return_void.C b/gcc/testsuite/g++.dg/coroutines/torture/co-ret-15-default-return_void.C
new file mode 100644
index 0000000..e600fea
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/torture/co-ret-15-default-return_void.C
@@ -0,0 +1,55 @@
+// { dg-do run }
+//
+// Check if default return_void is insert at correct position.
+#include <cassert>
+#include "../coro.h"
+
+class resumable {
+public:
+  class promise_type;
+  using coro_handle = std::coroutine_handle<promise_type>;
+  resumable(coro_handle handle) : handle_(handle) { assert(handle); }
+  resumable(resumable&) = delete;
+  resumable(resumable&&) = delete;
+  bool resume() {
+    if (!handle_.done())
+      handle_.resume();
+    return !handle_.done();
+  }
+  int recent_val();
+  ~resumable() { handle_.destroy(); }
+private:
+  coro_handle handle_;
+};
+
+class resumable::promise_type {
+public:
+  friend class resumable;
+  using coro_handle = std::coroutine_handle<promise_type>;
+  auto get_return_object() { return coro_handle::from_promise(*this); }
+  auto initial_suspend() { return std::suspend_always(); }
+  auto final_suspend() { return std::suspend_always(); }
+  void return_void() { value_ = -1; }
+  void unhandled_exception() {}
+private:
+  int value_ = 0;
+};
+
+int resumable::recent_val() {return handle_.promise().value_;}
+
+resumable foo(int n){
+  co_await std::suspend_always();
+  throw 1;
+}
+
+int bar (int n) {
+  resumable res = foo(n);
+  while(res.resume());
+  return res.recent_val();
+}
+
+int main() {
+  int res = bar(3);
+  assert(res == 0);
+  return 0;
+}
-- 
1.8.3.1

Re: [PATCH Coroutines]Insert the default return_void call at correct position

[Bin.Cheng]

Ping.

Thanks,
bin

On Mon, Feb 3, 2020 at 1:55 PM bin.cheng <bin.cheng@linux.alibaba.com> wrote:
>
> Hi,
>
> Exception in coroutine is not correctly handled because the default
> return_void call is now inserted before the finish suspend point,
> rather than at the end of the original coroutine body.  This patch
> fixes the issue by generating following code:
>   co_await promise.initial_suspend();
>   try {
>     // The original coroutine body
>
>     promise.return_void(); // The default return_void call.
>   } catch (...) {
>     promise.unhandled_exception();
>   }
>   final_suspend:
>   // ...
>
> Bootstrap and test on x86_64.  Is it OK?
>
> Thanks,
> bin
>
> gcc/cp
> 2020-02-03  Bin Cheng  <bin.cheng@linux.alibaba.com>
>
>         * coroutines.cc (build_actor_fn): Factor out code inserting the
>         default return_void call to...
>         (morph_fn_to_coro): ...here, also hoist local var declarations.
>
> gcc/testsuite
> 2020-02-03  Bin Cheng  <bin.cheng@linux.alibaba.com>
>
>         * g++.dg/coroutines/torture/co-ret-15-default-return_void.C: New.

Re: [PATCH Coroutines]Insert the default return_void call at correct position

[Iain Sandoe]

Hi Bin,

Bin.Cheng <amker.cheng@gmail.com> wrote:

> Ping.

We are seeking to clarify the standard wording around this (and the cases where
unhandled_exception() returns - hopefully during the WG21 meeting this week,

FWIW, I think your interpretation makes sense here.

thanks
Iain

> 
> Thanks,
> bin
> 
> On Mon, Feb 3, 2020 at 1:55 PM bin.cheng <bin.cheng@linux.alibaba.com> wrote:
>> Hi,
>> 
>> Exception in coroutine is not correctly handled because the default
>> return_void call is now inserted before the finish suspend point,
>> rather than at the end of the original coroutine body.  This patch
>> fixes the issue by generating following code:
>>  co_await promise.initial_suspend();
>>  try {
>>    // The original coroutine body
>> 
>>    promise.return_void(); // The default return_void call.
>>  } catch (...) {
>>    promise.unhandled_exception();
>>  }
>>  final_suspend:
>>  // ...
>> 
>> Bootstrap and test on x86_64.  Is it OK?
>> 
>> Thanks,
>> bin
>> 
>> gcc/cp
>> 2020-02-03  Bin Cheng  <bin.cheng@linux.alibaba.com>
>> 
>>        * coroutines.cc (build_actor_fn): Factor out code inserting the
>>        default return_void call to...
>>        (morph_fn_to_coro): ...here, also hoist local var declarations.
>> 
>> gcc/testsuite
>> 2020-02-03  Bin Cheng  <bin.cheng@linux.alibaba.com>
>> 
>>        * g++.dg/coroutines/torture/co-ret-15-default-return_void.C: New.

Re: [PATCH Coroutines]Insert the default return_void call at correct position

[Iain Sandoe]

Hello Bin, Nathan,

Iain Sandoe <iain@sandoe.co.uk> wrote:
>
>
> We are seeking to clarify the standard wording around this (and the cases  
> where
> unhandled_exception() returns - hopefully during the WG21 meeting this  
> week,
>
> FWIW, I think your interpretation makes sense here.

It’s not clear if the committee will have time to process wording changes  
needed for this during the current session, however, it is agreed that  
injecting the return_void at the closing brace of the user’s original  
function (your solution) is the correct approach.

So, from my point of view this patch DTRT and should be applied,

thanks
Iain


>> Thanks,
>> bin
>>
>> On Mon, Feb 3, 2020 at 1:55 PM bin.cheng <bin.cheng@linux.alibaba.com>  
>> wrote:
>>> Hi,
>>>
>>> Exception in coroutine is not correctly handled because the default
>>> return_void call is now inserted before the finish suspend point,
>>> rather than at the end of the original coroutine body.  This patch
>>> fixes the issue by generating following code:
>>> co_await promise.initial_suspend();
>>> try {
>>>  // The original coroutine body
>>>
>>>  promise.return_void(); // The default return_void call.
>>> } catch (...) {
>>>  promise.unhandled_exception();
>>> }
>>> final_suspend:
>>> // ...
>>>
>>> Bootstrap and test on x86_64.  Is it OK?
>>>
>>> Thanks,
>>> bin
>>>
>>> gcc/cp
>>> 2020-02-03  Bin Cheng  <bin.cheng@linux.alibaba.com>
>>>
>>>      * coroutines.cc (build_actor_fn): Factor out code inserting the
>>>      default return_void call to...
>>>      (morph_fn_to_coro): ...here, also hoist local var declarations.
>>>
>>> gcc/testsuite
>>> 2020-02-03  Bin Cheng  <bin.cheng@linux.alibaba.com>
>>>
>>>      * g++.dg/coroutines/torture/co-ret-15-default-return_void.C: New.

Re: [PATCH Coroutines]Insert the default return_void call at correct position

[Nathan Sidwell]

On 2/3/20 12:55 AM, bin.cheng wrote:
> Hi,
> 
> Exception in coroutine is not correctly handled because the default
> return_void call is now inserted before the finish suspend point,
> rather than at the end of the original coroutine body.  This patch
> fixes the issue by generating following code:
>    co_await promise.initial_suspend();
>    try {
>      // The original coroutine body
> 
>      promise.return_void(); // The default return_void call.
>    } catch (...) {
>      promise.unhandled_exception();
>    }
>    final_suspend:
>    // ...
> 
> Bootstrap and test on x86_64.  Is it OK?
> 
> Thanks,
> bin
> 
> gcc/cp
> 2020-02-03  Bin Cheng  <bin.cheng@linux.alibaba.com>
> 
>          * coroutines.cc (build_actor_fn): Factor out code inserting the
>          default return_void call to...
>          (morph_fn_to_coro): ...here, also hoist local var declarations.
> 
> gcc/testsuite
> 2020-02-03  Bin Cheng  <bin.cheng@linux.alibaba.com>
> 
>          * g++.dg/coroutines/torture/co-ret-15-default-return_void.C: New.

ok, thanks!

nathan

-- 
Nathan Sidwell