coroutines: Fix handling of ramp return value [PR94661]

Message ID 694AF2FE-7C34-4A86-BA68-9CA36C5B3197@sandoe.co.uk
State New
Headers show
Series
  • coroutines: Fix handling of ramp return value [PR94661]
Related show

Commit Message

Iain Sandoe April 19, 2020, 7:35 p.m.
Hi,

Coroutine ramp functions have synthesised return values (the
user-authored function body cannot have an explicit 'return').
The current implementation attempts to optimise by building
the return in-place, in the manner of C++17 code.  Clearly,
that was too ambitious and the fix builds a target expr for
the constructed version and passes that to finish_return_stmt.

This also means that we now get the same error messages as 
non-coroutines code, for implicit use of deleted CTORs etc.

-- 
2.24.1

Comments

Nathan Sidwell April 20, 2020, 4:54 p.m. | #1
On 4/19/20 3:35 PM, Iain Sandoe wrote:
> Hi,

> 

> Coroutine ramp functions have synthesised return values (the

> user-authored function body cannot have an explicit 'return').

> The current implementation attempts to optimise by building

> the return in-place, in the manner of C++17 code.  Clearly,

> that was too ambitious and the fix builds a target expr for

> the constructed version and passes that to finish_return_stmt.

> 

> This also means that we now get the same error messages as

> non-coroutines code, for implicit use of deleted CTORs etc.

> 

> ====

> 

> This is not part of PR94288, but the testcase for that also  has

> the issue, so it’s preferable to apply this first.

> 

> tested on x86_64-darwin16,

> OK if it passes regstrap on x86-64-Linux too?

> thanks

> Iain

> 

> 

> gcc/cp/ChangeLog:

> 

> 2020-04-19  Iain Sandoe  <iain@sandoe.co.uk>

> 

> 	PR c++/94661

> 	* coroutines.cc (morph_fn_to_coro): Simplify return

> 	value computation.

> 


ok, thanks

-- 
Nathan Sidwell

Patch

====

This is not part of PR94288, but the testcase for that also  has
the issue, so it’s preferable to apply this first.

tested on x86_64-darwin16,
OK if it passes regstrap on x86-64-Linux too?
thanks
Iain


gcc/cp/ChangeLog:

2020-04-19  Iain Sandoe  <iain@sandoe.co.uk>

	PR c++/94661
	* coroutines.cc (morph_fn_to_coro): Simplify return
	value computation.

gcc/testsuite/ChangeLog:

2020-04-19  Iain Sandoe  <iain@sandoe.co.uk>

        PR c++/94661
	* g++.dg/coroutines/ramp-return-a.C: New test.
	* g++.dg/coroutines/ramp-return-b.C: New test.
	* g++.dg/coroutines/ramp-return-c.C: New test.
---
 gcc/cp/coroutines.cc                          | 47 +++++---------
 .../g++.dg/coroutines/ramp-return-a.C         | 24 +++++++
 .../g++.dg/coroutines/ramp-return-b.C         | 22 +++++++
 .../g++.dg/coroutines/ramp-return-c.C         | 22 +++++++
 gcc/testsuite/g++.dg/coroutines/ramp-return.h | 64 +++++++++++++++++++
 5 files changed, 147 insertions(+), 32 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/ramp-return-a.C
 create mode 100644 gcc/testsuite/g++.dg/coroutines/ramp-return-b.C
 create mode 100644 gcc/testsuite/g++.dg/coroutines/ramp-return-c.C
 create mode 100644 gcc/testsuite/g++.dg/coroutines/ramp-return.h

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index 0a8e7521c4f..e1890ace956 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -3726,23 +3726,14 @@  morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
     }
 
   tree gro_context_body = push_stmt_list ();
-  tree gro, gro_bind_vars;
-  if (same_type_p (TREE_TYPE (get_ro), fn_return_type))
-    {
-      gro = DECL_RESULT (orig);
-      gro_bind_vars = NULL_TREE; /* We don't need a separate var.  */
-    }
-  else
-    {
-      gro = build_lang_decl (VAR_DECL, get_identifier ("coro.gro"),
-			     TREE_TYPE (TREE_OPERAND (get_ro, 0)));
-      DECL_CONTEXT (gro) = current_scope ();
-      r = build_stmt (fn_start, DECL_EXPR, gro);
-      add_stmt (r);
-      gro_bind_vars = gro; /* We need a temporary var.  */
-    }
-
-  /* Initialize our actual var.  */
+  tree gro = build_lang_decl (VAR_DECL, get_identifier ("coro.gro"), 
+			      TREE_TYPE (get_ro));
+  DECL_CONTEXT (gro) = current_scope ();
+  add_decl_expr (gro);
+  tree gro_bind_vars = gro;
+
+  /* We have to sequence the call to get_return_object before initial
+     suspend.  */
   r = build2_loc (fn_start, INIT_EXPR, TREE_TYPE (gro), gro, get_ro);
   r = coro_build_cvt_void_expr_stmt (r, fn_start);
   add_stmt (r);
@@ -3779,30 +3770,22 @@  morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
      logically doing things related to the end of the function.  */
 
   /* The ramp is done, we just need the return value.  */
-  if (!same_type_p (TREE_TYPE (gro), fn_return_type))
+  if (!same_type_p (TREE_TYPE (get_ro), fn_return_type))
     {
       /* construct the return value with a single GRO param.  */
       vec<tree, va_gc> *args = make_tree_vector_single (gro);
-      r = build_special_member_call (DECL_RESULT (orig),
+      r = build_special_member_call (NULL_TREE,
 				     complete_ctor_identifier, &args,
 				     fn_return_type, LOOKUP_NORMAL,
 				     tf_warning_or_error);
-      r = coro_build_cvt_void_expr_stmt (r, input_location);
-      add_stmt (r);
-      release_tree_vector (args);
+      r = build_cplus_new (fn_return_type, r, tf_warning_or_error);
     }
-  /* Else the GRO is the return and we already built it in place.  */
+  else
+    r = rvalue (gro); /* The GRO is the return value.  */
 
-  bool no_warning;
-  r = check_return_expr (DECL_RESULT (orig), &no_warning);
-  if (error_operand_p (r) && warn_return_type)
-    /* Suppress -Wreturn-type for the ramp.  */
-    TREE_NO_WARNING (orig) = true;
+  finish_return_stmt (r);
 
-  r = build_stmt (input_location, RETURN_EXPR, DECL_RESULT (orig));
-  TREE_NO_WARNING (r) |= no_warning;
-  r = maybe_cleanup_point_expr_void (r);
-  add_stmt (r);
+  /* Finish up the ramp function.  */
   BIND_EXPR_VARS (gro_context_bind) = gro_bind_vars;
   BIND_EXPR_BODY (gro_context_bind) = pop_stmt_list (gro_context_body);
   BIND_EXPR_BODY (ramp_bind) = pop_stmt_list (ramp_body);
diff --git a/gcc/testsuite/g++.dg/coroutines/ramp-return-a.C b/gcc/testsuite/g++.dg/coroutines/ramp-return-a.C
new file mode 100644
index 00000000000..c6e445e0529
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/ramp-return-a.C
@@ -0,0 +1,24 @@ 
+//  { dg-additional-options "-std=c++14" }
+
+#include "ramp-return.h"
+
+task<int>
+foo ()
+{
+ std::coroutine_handle<promise<int>> _handle;
+ return task<int> (_handle);
+}
+
+// This ICEd for the PR.
+
+task<int>
+bar ()
+{
+  co_return 0;
+}
+
+task<std::vector<int>>
+baz ()
+{
+  co_return std::vector<int>();
+}
diff --git a/gcc/testsuite/g++.dg/coroutines/ramp-return-b.C b/gcc/testsuite/g++.dg/coroutines/ramp-return-b.C
new file mode 100644
index 00000000000..d0e5d1f3c7f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/ramp-return-b.C
@@ -0,0 +1,22 @@ 
+//  { dg-options "-fcoroutines -std=c++14" }
+#define DELETE_COPY_CTOR 1
+#include "ramp-return.h"
+
+task<int>
+foo ()
+{
+ std::coroutine_handle<promise<int>> _handle;
+ return task<int> (_handle);  // { dg-error {use of deleted function 'task<T>::task\(const task<T>&\) \[with T = int\]'} }
+}
+
+task<int>
+bar ()
+{
+  co_return 0;
+} // { dg-error {use of deleted function 'task<T>::task\(const task<T>&\) \[with T = int\]'} }
+
+task<std::vector<int>>
+baz ()
+{
+  co_return std::vector<int>();
+} // { dg-error {use of deleted function 'task<T>::task\(const task<T>&\) \[with T = std::vector<int>\]'} }
diff --git a/gcc/testsuite/g++.dg/coroutines/ramp-return-c.C b/gcc/testsuite/g++.dg/coroutines/ramp-return-c.C
new file mode 100644
index 00000000000..e030ca1b7ae
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/ramp-return-c.C
@@ -0,0 +1,22 @@ 
+//  { dg-additional-options "-std=c++17" }
+#define DELETE_COPY_CTOR 1
+#include "ramp-return.h"
+
+task<int>
+foo ()
+{
+ std::coroutine_handle<promise<int>> _handle;
+ return task<int> (_handle); 
+}
+
+task<int>
+bar ()
+{
+  co_return 0;
+}
+
+task<std::vector<int>>
+baz ()
+{
+  co_return std::vector<int>();
+}
diff --git a/gcc/testsuite/g++.dg/coroutines/ramp-return.h b/gcc/testsuite/g++.dg/coroutines/ramp-return.h
new file mode 100644
index 00000000000..f41a07dafbe
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/ramp-return.h
@@ -0,0 +1,64 @@ 
+#include "coro.h"
+
+#include <exception>
+#include <vector>
+
+template <typename T>
+struct promise {
+  T _value;
+  coro::coroutine_handle<> _continuation = nullptr;
+
+  struct final_awaitable {
+    bool _has_continuation;
+    final_awaitable(bool has_continuation)
+        : _has_continuation(has_continuation) {}
+
+    bool await_ready() const noexcept { return !_has_continuation; }
+
+    template <typename Promise>
+    coro::coroutine_handle<>
+    await_suspend(coro::coroutine_handle<Promise> coro) noexcept {
+      return coro.promise()._continuation;
+    }
+
+    void await_resume() noexcept {}
+  };
+
+  auto get_return_object() noexcept {
+    return coro::coroutine_handle<promise>::from_promise(*this);
+  }
+
+  auto initial_suspend() noexcept { return coro::suspend_always(); }
+
+  auto final_suspend() noexcept {
+    return final_awaitable(_continuation != nullptr);
+  }
+
+  void return_value(T value) { _value = value; }
+
+  void unhandled_exception() { std::terminate(); }
+};
+
+template <typename T> 
+struct task {
+  using promise_type = promise<T>;
+  std::coroutine_handle<promise<T>> _handle;
+
+  task (coro::coroutine_handle<promise<T>> handle) : _handle(handle) {}
+#if DELETE_COPY_CTOR
+ task (const task &) = delete; // no copying
+#endif
+#if DELETE_MOVE_CTOR
+ task(task&& t) noexcept
+    : _handle(t._handle) { t._handle = nullptr; }
+#endif
+  bool await_ready() noexcept { return _handle.done(); }
+
+  std::coroutine_handle<>
+  await_suspend(std::coroutine_handle<> handle) noexcept {
+    _handle.promise()._continuation = handle;
+    return _handle;
+  }
+
+  T await_resume() noexcept { return _handle.promise()._value; }
+};