[PATCH] coroutines: Implicitly movable objects should use move CTORs for co_return.

[PATCH] coroutines: Implicitly movable objects should use move CTORs for co_return.

[Iain Sandoe]

.. and now to the right list…

I came across a build failure in a folly experimental test case where,
at first, it appeared that GCC was DTRT … however, further
investigation concluded that this was a case of differing interpretations
between implementations.

It’s kinda unhelpful that the discussion appears in class elision
but it motivates the selection of the correct overload even in cases
where there’s no elision.

The (conflicting info) issue is being taken to WG21 / Core.

tested on x86_64-darwin so far,
OK master after regstrap on Linux?
for 10.2 after some bake time on master?
thanks
Iain

----

This is a case where the standard contains conflicting information.
after discussion between implementators, the accepted intent is of
[class.copy.elision].  This amends the handling of co_return statements
to follow that.

gcc/cp/ChangeLog:

2020-05-13  Iain Sandoe  <iain@sandoe.co.uk>

	* coroutines.cc (finish_co_return_stmt): Implement rules
	from [class.copy.elision] /3.

gcc/testsuite/ChangeLog:

2020-05-13  Iain Sandoe  <iain@sandoe.co.uk>

	* g++.dg/coroutines/co-return-syntax-10-movable.C: New test.
---
 gcc/cp/coroutines.cc                          | 56 +++++++++++-----
 .../coroutines/co-return-syntax-10-movable.C  | 67 +++++++++++++++++++
 2 files changed, 107 insertions(+), 16 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/co-return-syntax-10-movable.C

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index 730e6fef82a..423c37e1c9c 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -999,11 +999,10 @@ finish_co_return_stmt (location_t kw, tree expr)
 	 expression as it is.  */
       if (dependent_type_p (functype) || type_dependent_expression_p (expr))
 	{
-	  expr
-	    = build2_loc (kw, CO_RETURN_EXPR, void_type_node, expr, NULL_TREE);
+	  expr = build2_loc (kw, CO_RETURN_EXPR, unknown_type_node,
+			     expr, NULL_TREE);
 	  expr = maybe_cleanup_point_expr_void (expr);
-	  expr = add_stmt (expr);
-	  return expr;
+	  return add_stmt (expr);
 	}
     }
 
@@ -1022,7 +1021,7 @@ finish_co_return_stmt (location_t kw, tree expr)
 
   /* If the promise object doesn't have the correct return call then
      there's a mis-match between the co_return <expr> and this.  */
-  tree co_ret_call = NULL_TREE;
+  tree co_ret_call = error_mark_node;
   if (expr == NULL_TREE || VOID_TYPE_P (TREE_TYPE (expr)))
     {
       tree crv_meth
@@ -1045,25 +1044,50 @@ finish_co_return_stmt (location_t kw, tree expr)
       if (!crv_meth || crv_meth == error_mark_node)
 	return error_mark_node;
 
-      vec<tree, va_gc> *args = make_tree_vector_single (expr);
-      co_ret_call = build_new_method_call (
-	get_coroutine_promise_proxy (current_function_decl), crv_meth, &args,
-	NULL_TREE, LOOKUP_NORMAL, NULL, tf_warning_or_error);
+      /* [class.copy.elision] / 3.
+	 An implicitly movable entity is a variable of automatic storage
+	 duration that is either a non-volatile object or an rvalue reference
+	 to a non-volatile object type.  For such objects in the context of
+	 the co_return, the overload resolution should be carried out first
+	 treating the object as an rvalue, if that fails, then we fall back
+	 to regular overload resolution.  */
+      tree obj = STRIP_NOPS (expr);
+      if (TREE_TYPE (obj)
+	  && TYPE_REF_P (TREE_TYPE (obj))
+	  && TYPE_REF_IS_RVALUE (TREE_TYPE (obj)))
+	obj = TREE_OPERAND (obj, 0);
+
+      if (TREE_CODE (obj) == PARM_DECL
+	  || (VAR_P (obj)
+	      && decl_storage_duration (obj) == dk_auto
+	      && !TYPE_VOLATILE (TREE_TYPE (obj))))
+	{
+	  vec<tree, va_gc> *args = make_tree_vector_single (rvalue (expr));
+	  co_ret_call = build_new_method_call
+	    (get_coroutine_promise_proxy (current_function_decl), crv_meth,
+	     &args, NULL_TREE, LOOKUP_NORMAL, NULL, tf_none);
+	}
+
+      if (co_ret_call == error_mark_node )
+	{
+	  vec<tree, va_gc> *args = make_tree_vector_single (expr);
+	  co_ret_call = build_new_method_call
+	    (get_coroutine_promise_proxy (current_function_decl), crv_meth,
+	     &args,NULL_TREE, LOOKUP_NORMAL, NULL, tf_warning_or_error);
+	  release_tree_vector (args);
+	}
     }
 
+  expr = build2_loc (kw, CO_RETURN_EXPR, void_type_node, expr, co_ret_call);
+  expr = maybe_cleanup_point_expr_void (expr);
+
   /* Makes no sense for a co-routine really. */
   if (TREE_THIS_VOLATILE (current_function_decl))
     warning_at (kw, 0,
 		"function declared %<noreturn%> has a"
 		" %<co_return%> statement");
 
-  if (!co_ret_call || co_ret_call == error_mark_node)
-    return error_mark_node;
-
-  expr = build2_loc (kw, CO_RETURN_EXPR, void_type_node, expr, co_ret_call);
-  expr = maybe_cleanup_point_expr_void (expr);
-  expr = add_stmt (expr);
-  return expr;
+  return add_stmt (expr);
 }
 
 /* We need to validate the arguments to __builtin_coro_promise, since the
diff --git a/gcc/testsuite/g++.dg/coroutines/co-return-syntax-10-movable.C b/gcc/testsuite/g++.dg/coroutines/co-return-syntax-10-movable.C
new file mode 100644
index 00000000000..8c18e13bce2
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/co-return-syntax-10-movable.C
@@ -0,0 +1,67 @@
+// Check that we obey the extra rules for implicitly movable co_return
+// objects [class.copy.elision]/3.
+
+#include "coro.h"
+
+#include  <utility>
+
+template <typename T>
+struct coro1 {
+  struct promise_type;
+  using handle_type = coro::coroutine_handle<coro1::promise_type>;
+  handle_type handle;
+  coro1 () : handle(0) {}
+  coro1 (handle_type _handle)
+    : handle(_handle) { }
+  coro1 (const coro1 &) = delete; // no copying
+  coro1 (coro1 &&s) : handle(s.handle) { s.handle = nullptr;  }
+  coro1 &operator = (coro1 &&s) {
+    handle = s.handle;
+    s.handle = nullptr;
+    return *this;
+  }
+  ~coro1() {
+    if ( handle )
+      handle.destroy();
+  }
+
+  struct promise_type {
+  T value;
+  promise_type() {}
+  ~promise_type() {}
+
+  auto get_return_object () { return handle_type::from_promise (*this);}
+  std::suspend_always initial_suspend () const { return {}; }
+  std::suspend_always final_suspend () const {  return {}; }
+
+  void return_value(T&& v) noexcept { value = std::move(v); }
+  
+  T get_value (void) { return value; }
+  void unhandled_exception() { }
+  };
+};
+
+struct MoveOnlyType 
+{
+  int value_;
+
+  explicit MoveOnlyType() noexcept : value_(0) {}
+  explicit MoveOnlyType(int value) noexcept : value_(value) {}
+
+  MoveOnlyType(MoveOnlyType&& other) noexcept
+      : value_(std::exchange(other.value_, -1)) {}
+
+  MoveOnlyType& operator=(MoveOnlyType&& other) noexcept {
+    value_ = std::exchange(other.value_, -1);
+    return *this;
+  }
+
+  ~MoveOnlyType() { value_ = -2; }
+};
+
+coro1<MoveOnlyType> 
+my_coro ()
+{
+  MoveOnlyType x{10};
+  co_return x;
+}
-- 
2.24.1

Re: [PATCH] coroutines: Implicitly movable objects should use move CTORs for co_return.

[Nathan Sidwell]

On 5/13/20 6:59 AM, Iain Sandoe wrote:
> .. and now to the right list…
> 
> I came across a build failure in a folly experimental test case where,
> at first, it appeared that GCC was DTRT … however, further
> investigation concluded that this was a case of differing interpretations
> between implementations.
> 
> It’s kinda unhelpful that the discussion appears in class elision
> but it motivates the selection of the correct overload even in cases
> where there’s no elision.
> 
> The (conflicting info) issue is being taken to WG21 / Core.

Yeah, let's have an xrefing note at the co_return point.  not an 
intercal come-from at the eliding point!

> @@ -1045,25 +1044,50 @@ finish_co_return_stmt (location_t kw, tree expr)
>         if (!crv_meth || crv_meth == error_mark_node)
>   	return error_mark_node;
>   
> -      vec<tree, va_gc> *args = make_tree_vector_single (expr);
> -      co_ret_call = build_new_method_call (
> -	get_coroutine_promise_proxy (current_function_decl), crv_meth, &args,
> -	NULL_TREE, LOOKUP_NORMAL, NULL, tf_warning_or_error);
> +      /* [class.copy.elision] / 3.
> +	 An implicitly movable entity is a variable of automatic storage
> +	 duration that is either a non-volatile object or an rvalue reference
> +	 to a non-volatile object type.  For such objects in the context of
> +	 the co_return, the overload resolution should be carried out first
> +	 treating the object as an rvalue, if that fails, then we fall back
> +	 to regular overload resolution.  */
> +      tree obj = STRIP_NOPS (expr);
> +      if (TREE_TYPE (obj)
> +	  && TYPE_REF_P (TREE_TYPE (obj))
> +	  && TYPE_REF_IS_RVALUE (TREE_TYPE (obj)))
> +	obj = TREE_OPERAND (obj, 0);
> +
> +      if (TREE_CODE (obj) == PARM_DECL
> +	  || (VAR_P (obj)
> +	      && decl_storage_duration (obj) == dk_auto
> +	      && !TYPE_VOLATILE (TREE_TYPE (obj))))

this is wrong and insufficient. As I explained on IRC -- 'expressions of 
reference type are not a thing'.  We always bash them into an 
indirect-ref of the referency thing.

Look at check_return_value and how it handles NRVO.  There's a 
treat_lvalue_as_rvalue_p call that I'll bet you can use[*], along with a 
move wrapper, which maybe what you want rather than the below rvalue (expr).

a return_expr's operand is an INIT_EXPR or MODIFY_EXPR whose LHS is 
DECL_RESULT(fn).  So that's almost what you want here.  I think the 
general form is
  if (treat_lval_as_rval ())
    { silently try callexpr with moved operand }

if no joy either way
    { noisily try callexpr with unmoved operand }

[*] That is insufficient, because elision also applies to objects you've 
placed into the coro frame.  by the std those are still automatic 
storage objects, but inside the compiler they are of course 
COMPONENT_REFs (or do you cons up VAR_DECLS of reference type that refer 
to them?).   Further, the PARM_DECL case can;t occur here -- the actor 
fn's only parm is the coro frame pointer.
  Either way you'll need to teach TLAR a new trick, or clone it locally 
(probably the better choice).

in place of TLAR's (parm_ok && ...) I think you want
  (code == COMPONENT_REF && OPERAND (0) == coro_frame)
and of course move it outside the (irrelavant) context check

does that help?

nathan
-- 
Nathan Sidwell

Re: [PATCH] coroutines: Implicitly movable objects should use move CTORs for co_return.

[Iain Sandoe]

Nathan Sidwell <nathan@acm.org> wrote:

> On 5/13/20 6:59 AM, Iain Sandoe wrote:

>> @@ -1045,25 +1044,50 @@ finish_co_return_stmt (location_t kw, tree expr)
>>        if (!crv_meth || crv_meth == error_mark_node)
>>  	return error_mark_node;
>>  -      vec<tree, va_gc> *args = make_tree_vector_single (expr);
>> -      co_ret_call = build_new_method_call (
>> -	get_coroutine_promise_proxy (current_function_decl), crv_meth, &args,
>> -	NULL_TREE, LOOKUP_NORMAL, NULL, tf_warning_or_error);
>> +      /* [class.copy.elision] / 3.
>> +	 An implicitly movable entity is a variable of automatic storage
>> +	 duration that is either a non-volatile object or an rvalue reference
>> +	 to a non-volatile object type.  For such objects in the context of
>> +	 the co_return, the overload resolution should be carried out first
>> +	 treating the object as an rvalue, if that fails, then we fall back
>> +	 to regular overload resolution.  */
>> +      tree obj = STRIP_NOPS (expr);
>> +      if (TREE_TYPE (obj)
>> +	  && TYPE_REF_P (TREE_TYPE (obj))
>> +	  && TYPE_REF_IS_RVALUE (TREE_TYPE (obj)))
>> +	obj = TREE_OPERAND (obj, 0);
>> +
>> +      if (TREE_CODE (obj) == PARM_DECL
>> +	  || (VAR_P (obj)
>> +	      && decl_storage_duration (obj) == dk_auto
>> +	      && !TYPE_VOLATILE (TREE_TYPE (obj))))
>
> this is wrong and insufficient. As I explained on IRC -- 'expressions of  
> reference type are not a thing'.  We always bash them into an  
> indirect-ref of the referency thing.
>
> Look at check_return_value and how it handles NRVO.  There's a  
> treat_lvalue_as_rvalue_p call that I'll bet you can use[*], along with a  
> move wrapper, which maybe what you want rather than the below rvalue  
> (expr).
>
> a return_expr's operand is an INIT_EXPR or MODIFY_EXPR whose LHS is  
> DECL_RESULT(fn).  So that's almost what you want here.  I think the  
> general form is
> if (treat_lval_as_rval ())
>   { silently try callexpr with moved operand }
>
> if no joy either way
>   { noisily try callexpr with unmoved operand }
>
> [*] That is insufficient, because elision also applies to objects you've  
> placed into the coro frame.  by the std those are still automatic storage  
> objects, but inside the compiler they are of course COMPONENT_REFs (or do  
> you cons up VAR_DECLS of reference type that refer to them?).   Further,  
> the PARM_DECL case can;t occur here -- the actor fn's only parm is the  
> coro frame pointer.

This is the equivalent of finish_return_stmt () in the parser, it knows  
nothing of the eventual morphing of local vars (or parms) into frame  
references.

So I only need to handle what can be returned by "expr =  
cp_parser_expression (parser);”
dependent expressions are dealt with above, with an early return with  
“type_unknown_node”.

So, it might be I can just reuse the code you point to?

Iain

Re: [PATCH] coroutines: Implicitly movable objects should use move CTORs for co_return.

[Nathan Sidwell]

On 5/13/20 9:26 AM, Iain Sandoe wrote:
> Nathan Sidwell <nathan@acm.org> wrote:
> 
>> On 5/13/20 6:59 AM, Iain Sandoe wrote:
> 
>>> @@ -1045,25 +1044,50 @@ finish_co_return_stmt (location_t kw, tree expr)
>>>        if (!crv_meth || crv_meth == error_mark_node)
>>>      return error_mark_node;
>>>  -      vec<tree, va_gc> *args = make_tree_vector_single (expr);
>>> -      co_ret_call = build_new_method_call (
>>> -    get_coroutine_promise_proxy (current_function_decl), crv_meth, 
>>> &args,
>>> -    NULL_TREE, LOOKUP_NORMAL, NULL, tf_warning_or_error);
>>> +      /* [class.copy.elision] / 3.
>>> +     An implicitly movable entity is a variable of automatic storage
>>> +     duration that is either a non-volatile object or an rvalue 
>>> reference
>>> +     to a non-volatile object type.  For such objects in the context of
>>> +     the co_return, the overload resolution should be carried out first
>>> +     treating the object as an rvalue, if that fails, then we fall back
>>> +     to regular overload resolution.  */
>>> +      tree obj = STRIP_NOPS (expr);
>>> +      if (TREE_TYPE (obj)
>>> +      && TYPE_REF_P (TREE_TYPE (obj))
>>> +      && TYPE_REF_IS_RVALUE (TREE_TYPE (obj)))
>>> +    obj = TREE_OPERAND (obj, 0);
>>> +
>>> +      if (TREE_CODE (obj) == PARM_DECL
>>> +      || (VAR_P (obj)
>>> +          && decl_storage_duration (obj) == dk_auto
>>> +          && !TYPE_VOLATILE (TREE_TYPE (obj))))
>>
>> this is wrong and insufficient. As I explained on IRC -- 'expressions 
>> of reference type are not a thing'.  We always bash them into an 
>> indirect-ref of the referency thing.
>>
>> Look at check_return_value and how it handles NRVO.  There's a 
>> treat_lvalue_as_rvalue_p call that I'll bet you can use[*], along with 
>> a move wrapper, which maybe what you want rather than the below rvalue 
>> (expr).
>>
>> a return_expr's operand is an INIT_EXPR or MODIFY_EXPR whose LHS is 
>> DECL_RESULT(fn).  So that's almost what you want here.  I think the 
>> general form is
>> if (treat_lval_as_rval ())
>>   { silently try callexpr with moved operand }
>>
>> if no joy either way
>>   { noisily try callexpr with unmoved operand }
>>
>> [*] That is insufficient, because elision also applies to objects 
>> you've placed into the coro frame.  by the std those are still 
>> automatic storage objects, but inside the compiler they are of course 
>> COMPONENT_REFs (or do you cons up VAR_DECLS of reference type that 
>> refer to them?).   Further, the PARM_DECL case can;t occur here -- the 
>> actor fn's only parm is the coro frame pointer.
> 
> This is the equivalent of finish_return_stmt () in the parser, it knows 
> nothing of the eventual morphing of local vars (or parms) into frame 
> references.
> 
> So I only need to handle what can be returned by "expr = 
> cp_parser_expression (parser);”
> dependent expressions are dealt with above, with an early return with 
> “type_unknown_node”.
> 
> So, it might be I can just reuse the code you point to?

Yeah I think so.  I realized it was pre-xform so the component-ref stuff 
was irrelevant when out shopping.  In other news, flour's back in stock :)

nathan

-- 
Nathan Sidwell

[PATCH v2] coroutines: Implicitly movable objects should use move CTORs for co_return.

[Iain Sandoe]

Hi

thanks for the review and pointers ...

Nathan Sidwell <nathan@acm.org> wrote:

> On 5/13/20 9:26 AM, Iain Sandoe wrote:
>> Nathan Sidwell <nathan@acm.org> wrote:
>>> On 5/13/20 6:59 AM, Iain Sandoe wrote:
>>>> 

>> This is the equivalent of finish_return_stmt () in the parser, it knows nothing of the eventual morphing of local vars (or parms) into frame references.
>> So I only need to handle what can be returned by "expr = cp_parser_expression (parser);”
>> dependent expressions are dealt with above, with an early return with “type_unknown_node”.

Unfortunately, the code in finish_return_stmt / and check_return_expr is too retval-centric to be
re-used in this context.  However, I have taken from it in determining a sequence of operations,
and in the use of treat_lvalue_as_rvalue_p() - with the additional criterion that the object must
not be volatile (check_return_expr checks that in a different predicate, that’s not usable here).

tested on x86_64-darwin so far,
does this now look OK for master (after checking on Linux too)?
and for 10.2 after some bake time on master?

thanks
Iain

=====

This is a case where the standard contains conflicting information.
after discussion between implementators, the accepted intent is of
[class.copy.elision].  This amends the handling of co_return statements
to follow that.

gcc/cp/ChangeLog:

2020-05-14  Iain Sandoe  <iain@sandoe.co.uk>

	* coroutines.cc (finish_co_return_stmt): Implement rules
	from [class.copy.elision] /3.

gcc/testsuite/ChangeLog:

2020-05-14  Iain Sandoe  <iain@sandoe.co.uk>

	* g++.dg/coroutines/co-return-syntax-10-movable.C: New test.
---
 gcc/cp/coroutines.cc                          | 109 ++++++++++++------
 .../coroutines/co-return-syntax-10-movable.C  |  67 +++++++++++
 2 files changed, 141 insertions(+), 35 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/co-return-syntax-10-movable.C

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index 730e6fef82a..facfafaaa86 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -969,16 +969,23 @@ finish_co_yield_expr (location_t kw, tree expr)
   return op;
 }
 
-/* Check that it's valid to have a co_return keyword here.
+/* Check and build a co_return statememt.
+   First that it's valid to have a co_return keyword here.
    If it is, then check and build the p.return_{void(),value(expr)}.
-   These are built against the promise proxy, but saved for expand time.  */
+   These are built against a proxy for the promise, which will be filled
+   in with the actual frame version when the function is transformed.  */
 
 tree
 finish_co_return_stmt (location_t kw, tree expr)
 {
-  if (expr == error_mark_node)
+  if (expr)
+    STRIP_ANY_LOCATION_WRAPPER (expr);
+
+  if (error_operand_p (expr))
     return error_mark_node;
 
+  /* If it fails the following test, the function is not permitted to be a
+     coroutine, so the co_return statement is erroneous.  */
   if (!coro_common_keyword_context_valid_p (current_function_decl, kw,
 					    "co_return"))
     return error_mark_node;
@@ -987,32 +994,32 @@ finish_co_return_stmt (location_t kw, tree expr)
      already.  */
   DECL_COROUTINE_P (current_function_decl) = 1;
 
-  if (processing_template_decl)
-    {
-      current_function_returns_value = 1;
+  /* This function will appear to have no return statement, even if it
+     is declared to return non-void (most likely).  This is correct - we
+     synthesize the return for the ramp in the compiler.  So suppress any
+     extraneous warnings during substitution.  */
+  TREE_NO_WARNING (current_function_decl) = true;
 
-      if (check_for_bare_parameter_packs (expr))
-	return error_mark_node;
+  if (processing_template_decl
+      && check_for_bare_parameter_packs (expr))
+    return error_mark_node;
 
-      tree functype = TREE_TYPE (current_function_decl);
-      /* If we don't know the promise type, we can't proceed, return the
-	 expression as it is.  */
-      if (dependent_type_p (functype) || type_dependent_expression_p (expr))
-	{
-	  expr
-	    = build2_loc (kw, CO_RETURN_EXPR, void_type_node, expr, NULL_TREE);
-	  expr = maybe_cleanup_point_expr_void (expr);
-	  expr = add_stmt (expr);
-	  return expr;
-	}
+  /* If we don't know the promise type, we can't proceed, build the
+     co_return with the expression unchanged.  */
+  tree functype = TREE_TYPE (current_function_decl);
+  if (dependent_type_p (functype) || type_dependent_expression_p (expr))
+    {
+      /* co_return expressions are always void type, regardless of the
+	 expression type.  */
+      expr = build2_loc (kw, CO_RETURN_EXPR, void_type_node,
+			 expr, NULL_TREE);
+      expr = maybe_cleanup_point_expr_void (expr);
+      return add_stmt (expr);
     }
 
   if (!coro_promise_type_found_p (current_function_decl, kw))
     return error_mark_node;
 
-  if (error_operand_p (expr))
-    return error_mark_node;
-
   /* Suppress -Wreturn-type for co_return, we need to check indirectly
      whether the promise type has a suitable return_void/return_value.  */
   TREE_NO_WARNING (current_function_decl) = true;
@@ -1020,16 +1027,29 @@ finish_co_return_stmt (location_t kw, tree expr)
   if (!processing_template_decl && warn_sequence_point)
     verify_sequence_points (expr);
 
+  if (expr)
+    {
+      /* If we had an id-expression obfuscated by force_paren_expr, we need
+	 to undo it so we can try to treat it as an rvalue below.  */
+      expr = maybe_undo_parenthesized_ref (expr);
+
+      if (processing_template_decl)
+	expr = build_non_dependent_expr (expr);
+
+      if (error_operand_p (expr))
+	return error_mark_node;
+    }
+
   /* If the promise object doesn't have the correct return call then
      there's a mis-match between the co_return <expr> and this.  */
-  tree co_ret_call = NULL_TREE;
+  tree co_ret_call = error_mark_node;
   if (expr == NULL_TREE || VOID_TYPE_P (TREE_TYPE (expr)))
     {
       tree crv_meth
 	= lookup_promise_method (current_function_decl,
 				 coro_return_void_identifier, kw,
 				 /*musthave=*/true);
-      if (!crv_meth || crv_meth == error_mark_node)
+      if (crv_meth == error_mark_node)
 	return error_mark_node;
 
       co_ret_call = build_new_method_call (
@@ -1042,13 +1062,37 @@ finish_co_return_stmt (location_t kw, tree expr)
 	= lookup_promise_method (current_function_decl,
 				 coro_return_value_identifier, kw,
 				 /*musthave=*/true);
-      if (!crv_meth || crv_meth == error_mark_node)
+      if (crv_meth == error_mark_node)
 	return error_mark_node;
 
-      vec<tree, va_gc> *args = make_tree_vector_single (expr);
-      co_ret_call = build_new_method_call (
-	get_coroutine_promise_proxy (current_function_decl), crv_meth, &args,
-	NULL_TREE, LOOKUP_NORMAL, NULL, tf_warning_or_error);
+      /* [class.copy.elision] / 3.
+	 An implicitly movable entity is a variable of automatic storage
+	 duration that is either a non-volatile object or an rvalue reference
+	 to a non-volatile object type.  For such objects in the context of
+	 the co_return, the overload resolution should be carried out first
+	 treating the object as an rvalue, if that fails, then we fall back
+	 to regular overload resolution.  */
+
+      if (treat_lvalue_as_rvalue_p (expr, /*parm_ok*/true)
+	  && CLASS_TYPE_P (TREE_TYPE (expr))
+	  && !TYPE_VOLATILE (TREE_TYPE (expr)))
+	{
+	  vec<tree, va_gc> *args = make_tree_vector_single (move (expr));
+	  /* It's OK if this fails... */
+	  co_ret_call = build_new_method_call
+	    (get_coroutine_promise_proxy (current_function_decl), crv_meth,
+	     &args, NULL_TREE, LOOKUP_NORMAL|LOOKUP_PREFER_RVALUE,
+	     NULL, tf_none);
+	}
+
+      if (co_ret_call == error_mark_node)
+	{
+	  vec<tree, va_gc> *args = make_tree_vector_single (expr);
+	  /* ... but this must succeed if we didn't get the move variant.  */
+	  co_ret_call = build_new_method_call
+	    (get_coroutine_promise_proxy (current_function_decl), crv_meth,
+	     &args, NULL_TREE, LOOKUP_NORMAL, NULL, tf_warning_or_error);
+	}
     }
 
   /* Makes no sense for a co-routine really. */
@@ -1057,13 +1101,8 @@ finish_co_return_stmt (location_t kw, tree expr)
 		"function declared %<noreturn%> has a"
 		" %<co_return%> statement");
 
-  if (!co_ret_call || co_ret_call == error_mark_node)
-    return error_mark_node;
-
   expr = build2_loc (kw, CO_RETURN_EXPR, void_type_node, expr, co_ret_call);
-  expr = maybe_cleanup_point_expr_void (expr);
-  expr = add_stmt (expr);
-  return expr;
+  return finish_expr_stmt (expr);
 }
 
 /* We need to validate the arguments to __builtin_coro_promise, since the
diff --git a/gcc/testsuite/g++.dg/coroutines/co-return-syntax-10-movable.C b/gcc/testsuite/g++.dg/coroutines/co-return-syntax-10-movable.C
new file mode 100644
index 00000000000..e2c47a9ec1b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/co-return-syntax-10-movable.C
@@ -0,0 +1,67 @@
+// Check that we obey the extra rules for implicitly movable co_return
+// objects [class.copy.elision]/3.
+
+#include "coro.h"
+
+#include  <utility>
+
+template <typename T>
+struct coro1 {
+  struct promise_type;
+  using handle_type = coro::coroutine_handle<coro1::promise_type>;
+  handle_type handle;
+  coro1 () : handle(0) {}
+  coro1 (handle_type _handle)
+    : handle(_handle) { }
+  coro1 (const coro1 &) = delete; // no copying
+  coro1 (coro1 &&s) : handle(s.handle) { s.handle = nullptr;  }
+  coro1 &operator = (coro1 &&s) {
+    handle = s.handle;
+    s.handle = nullptr;
+    return *this;
+  }
+  ~coro1() {
+    if ( handle )
+      handle.destroy();
+  }
+
+  struct promise_type {
+  T value;
+  promise_type() {}
+  ~promise_type() {}
+
+  auto get_return_object () { return handle_type::from_promise (*this);}
+  coro::suspend_always initial_suspend () const { return {}; }
+  coro::suspend_always final_suspend () const {  return {}; }
+
+  void return_value(T&& v) noexcept { value = std::move(v); }
+  
+  T get_value (void) { return value; }
+  void unhandled_exception() { }
+  };
+};
+
+struct MoveOnlyType 
+{
+  int value_;
+
+  explicit MoveOnlyType() noexcept : value_(0) {}
+  explicit MoveOnlyType(int value) noexcept : value_(value) {}
+
+  MoveOnlyType(MoveOnlyType&& other) noexcept
+      : value_(std::exchange(other.value_, -1)) {}
+
+  MoveOnlyType& operator=(MoveOnlyType&& other) noexcept {
+    value_ = std::exchange(other.value_, -1);
+    return *this;
+  }
+
+  ~MoveOnlyType() { value_ = -2; }
+};
+
+coro1<MoveOnlyType> 
+my_coro ()
+{
+  MoveOnlyType x{10};
+  co_return x;
+}
-- 
2.24.1

Re: [PATCH v2] coroutines: Implicitly movable objects should use move CTORs for co_return.

[Nathan Sidwell]

On 5/14/20 9:04 AM, Iain Sandoe wrote:
> Hi
> 
> thanks for the review and pointers ...
> 
> Nathan Sidwell <nathan@acm.org> wrote:
> 
>> On 5/13/20 9:26 AM, Iain Sandoe wrote:
>>> Nathan Sidwell <nathan@acm.org> wrote:
>>>> On 5/13/20 6:59 AM, Iain Sandoe wrote:
>>>>>
> 
>>> This is the equivalent of finish_return_stmt () in the parser, it knows nothing of the eventual morphing of local vars (or parms) into frame references.
>>> So I only need to handle what can be returned by "expr = cp_parser_expression (parser);”
>>> dependent expressions are dealt with above, with an early return with “type_unknown_node”.
> 
> Unfortunately, the code in finish_return_stmt / and check_return_expr is too retval-centric to be
> re-used in this context.  However, I have taken from it in determining a sequence of operations,
> and in the use of treat_lvalue_as_rvalue_p() - with the additional criterion that the object must
> not be volatile (check_return_expr checks that in a different predicate, that’s not usable here).
> 
> tested on x86_64-darwin so far,
> does this now look OK for master (after checking on Linux too)?
> and for 10.2 after some bake time on master?
> 
> thanks
> Iain
> 
> =====
> 
> This is a case where the standard contains conflicting information.
> after discussion between implementators, the accepted intent is of
> [class.copy.elision].  This amends the handling of co_return statements
> to follow that.
> 
> gcc/cp/ChangeLog:
> 
> 2020-05-14  Iain Sandoe  <iain@sandoe.co.uk>
> 
> 	* coroutines.cc (finish_co_return_stmt): Implement rules
> 	from [class.copy.elision] /3.

Yeah, this is better, ok for master and 10.2 (when you're ready)

nathan

-- 
Nathan Sidwell