coroutines: Correct handling of references in parm copies [PR95350].

Message ID 9A53E840-5651-4971-A928-DCC98D257A98@sandoe.co.uk
State New
Headers show
Series
  • coroutines: Correct handling of references in parm copies [PR95350].
Related show

Commit Message

Iain Sandoe June 1, 2020, 7:59 a.m.
(resending, this didn’t appear to make it to the list)

Hi,

I had implemented a move out of rvalue refs for such ramp values (since
these are most likely to be dangling references).  However this does cause
a divergence with the clang implementation - and the patch fixes that.

tested on x86_64,powerpc64-linux, x86_64-darwin
OK for master?
OK for 10.2?
thanks
Iain

-------

Adjust to handle rvalue refs the same way as clang, and to correct
the handling of moves when a copy CTOR is present.  This is one area
where we could make things easier for the end-user (as was implemented
before this change), however there needs to be agreement about when the
full statement containing a coroutine call ends (i.e. when the ramp
terminates or when the coroutine terminates).

gcc/cp/ChangeLog:

	PR c++/95350
	* coroutines.cc (struct param_info): Remove rv_ref field.
	(build_actor_fn): Remove specifial rvalue ref handling.
	(morph_fn_to_coro): Likewise.

gcc/testsuite/ChangeLog:

	PR c++/95350
	* g++.dg/coroutines/torture/func-params-08.C: Adjust test to
	reflect that all rvalue refs are dangling.
	* g++.dg/coroutines/torture/func-params-09-awaitable-parms.C:
	Likewise.
	* g++.dg/coroutines/pr95350.C: New test.
---
gcc/cp/coroutines.cc                          | 41 +++++--------------
gcc/testsuite/g++.dg/coroutines/pr95350.C     | 28 +++++++++++++
.../coroutines/torture/func-params-08.C       | 11 ++---
.../torture/func-params-09-awaitable-parms.C  | 11 ++---
4 files changed, 50 insertions(+), 41 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/coroutines/pr95350.C

-- 
2.24.1

Comments

Nathan Sidwell June 1, 2020, 2:52 p.m. | #1
On 6/1/20 3:59 AM, Iain Sandoe wrote:
> (resending, this didn’t appear to make it to the list)

> 

> Hi,

> 

> I had implemented a move out of rvalue refs for such ramp values (since

> these are most likely to be dangling references).  However this does cause

> a divergence with the clang implementation - and the patch fixes that.

> 

> tested on x86_64,powerpc64-linux, x86_64-darwin

> OK for master?

> OK for 10.2?


ok for both


> Iain

> 

> -------

> 

> Adjust to handle rvalue refs the same way as clang, and to correct

> the handling of moves when a copy CTOR is present.  This is one area

> where we could make things easier for the end-user (as was implemented

> before this change), however there needs to be agreement about when the

> full statement containing a coroutine call ends (i.e. when the ramp

> terminates or when the coroutine terminates).

> 

> gcc/cp/ChangeLog:

> 

> 	PR c++/95350

> 	* coroutines.cc (struct param_info): Remove rv_ref field.

> 	(build_actor_fn): Remove specifial rvalue ref handling.

> 	(morph_fn_to_coro): Likewise.

> 

> gcc/testsuite/ChangeLog:

> 

> 	PR c++/95350

> 	* g++.dg/coroutines/torture/func-params-08.C: Adjust test to

> 	reflect that all rvalue refs are dangling.

> 	* g++.dg/coroutines/torture/func-params-09-awaitable-parms.C:

> 	Likewise.

> 	* g++.dg/coroutines/pr95350.C: New test.

> ---

> gcc/cp/coroutines.cc                          | 41 +++++--------------

> gcc/testsuite/g++.dg/coroutines/pr95350.C     | 28 +++++++++++++

> .../coroutines/torture/func-params-08.C       | 11 ++---

> .../torture/func-params-09-awaitable-parms.C  | 11 ++---

> 4 files changed, 50 insertions(+), 41 deletions(-)

> create mode 100644 gcc/testsuite/g++.dg/coroutines/pr95350.C

> 

> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc

> index 969f4a66f2f..8746927577a 100644

> --- a/gcc/cp/coroutines.cc

> +++ b/gcc/cp/coroutines.cc

> @@ -1807,7 +1807,6 @@ struct param_info

>    tree frame_type;   /* The type used to represent this parm in the frame.  */

>    tree orig_type;    /* The original type of the parm (not as passed).  */

>    bool by_ref;       /* Was passed by reference.  */

> -  bool rv_ref;       /* Was an rvalue reference.  */

>    bool pt_ref;       /* Was a pointer to object.  */

>    bool trivial_dtor; /* The frame type has a trivial DTOR.  */

>    bool this_ptr;     /* Is 'this' */

> @@ -2077,12 +2076,6 @@ build_actor_fn (location_t loc, tree coro_frame_type, tree actor, tree fnbody,

> 	  if (parm.pt_ref)

> 	    fld_idx = build1_loc (loc, CONVERT_EXPR, TREE_TYPE (arg), fld_idx);

> 

> -	  /* We expect an rvalue ref. here.  */

> -	  if (parm.rv_ref)

> -	    fld_idx = convert_to_reference (DECL_ARG_TYPE (arg), fld_idx,

> -					    CONV_STATIC, LOOKUP_NORMAL,

> -					    NULL_TREE, tf_warning_or_error);

> -

> 	  int i;

> 	  tree *puse;

> 	  FOR_EACH_VEC_ELT (*parm.body_uses, i, puse)

> @@ -3770,15 +3763,8 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)

> 	  if (actual_type == NULL_TREE)

> 	    actual_type = error_mark_node;

> 	  parm.orig_type = actual_type;

> -	  parm.by_ref = parm.rv_ref = parm.pt_ref = false;

> -	  if (TREE_CODE (actual_type) == REFERENCE_TYPE

> -	      && TYPE_REF_IS_RVALUE (DECL_ARG_TYPE (arg)))

> -	    {

> -	      parm.rv_ref = true;

> -	      actual_type = TREE_TYPE (actual_type);

> -	      parm.frame_type = actual_type;

> -	    }

> -	  else if (TREE_CODE (actual_type) == REFERENCE_TYPE)

> +	  parm.by_ref = parm.pt_ref = false;

> +	  if (TREE_CODE (actual_type) == REFERENCE_TYPE)

> 	    {

> 	      /* If the user passes by reference, then we will save the

> 		 pointer to the original.  As noted in

> @@ -3786,16 +3772,12 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)

> 		 referenced item ends and then the coroutine is resumed,

> 		 we have UB; well, the user asked for it.  */

> 	      actual_type = build_pointer_type (TREE_TYPE (actual_type));

> -	      parm.frame_type = actual_type;

> 	      parm.pt_ref = true;

> 	    }

> 	  else if (TYPE_REF_P (DECL_ARG_TYPE (arg)))

> -	    {

> -	      parm.by_ref = true;

> -	      parm.frame_type = actual_type;

> -	    }

> -	  else

> -	    parm.frame_type = actual_type;

> +	    parm.by_ref = true;

> +

> +	  parm.frame_type = actual_type;

> 

> 	  parm.this_ptr = is_this_parameter (arg);

> 	  if (lambda_p)

> @@ -4170,17 +4152,16 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)

> 	    }

> 	  else if (parm.by_ref)

> 	    vec_safe_push (promise_args, fld_idx);

> -	  else if (parm.rv_ref)

> -	    vec_safe_push (promise_args, rvalue (fld_idx));

> 	  else

> 	    vec_safe_push (promise_args, arg);

> 

> 	  if (TYPE_NEEDS_CONSTRUCTING (parm.frame_type))

> 	    {

> 	      vec<tree, va_gc> *p_in;

> -	      if (parm.by_ref

> -		  && classtype_has_non_deleted_move_ctor (parm.frame_type)

> -		  && !classtype_has_non_deleted_copy_ctor (parm.frame_type))

> +	      if (CLASS_TYPE_P (parm.frame_type)

> +		  && classtype_has_non_deleted_move_ctor (parm.frame_type))

> +		p_in = make_tree_vector_single (move (arg));

> +	      else if (lvalue_p (arg))

> 		p_in = make_tree_vector_single (rvalue (arg));

> 	      else

> 		p_in = make_tree_vector_single (arg);

> @@ -4193,9 +4174,7 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)

> 	    }

> 	  else

> 	    {

> -	      if (parm.rv_ref)

> -		r = convert_from_reference (arg);

> -	      else if (!same_type_p (parm.frame_type, DECL_ARG_TYPE (arg)))

> +	      if (!same_type_p (parm.frame_type, DECL_ARG_TYPE (arg)))

> 		r = build1_loc (DECL_SOURCE_LOCATION (arg), CONVERT_EXPR,

> 				parm.frame_type, arg);

> 	      else

> diff --git a/gcc/testsuite/g++.dg/coroutines/pr95350.C b/gcc/testsuite/g++.dg/coroutines/pr95350.C

> new file mode 100644

> index 00000000000..1915032c471

> --- /dev/null

> +++ b/gcc/testsuite/g++.dg/coroutines/pr95350.C

> @@ -0,0 +1,28 @@

> +#if __has_include(<coroutine>)

> +#include <coroutine>

> +#else

> +#include <experimental/coroutine>

> +namespace std { using namespace experimental; }

> +#endif

> +#include <utility>

> +

> +struct task {

> +    struct promise_type {

> +        task get_return_object();

> +        void return_void();

> +        void unhandled_exception();

> +        std::suspend_always initial_suspend() noexcept;

> +        std::suspend_always final_suspend() noexcept;

> +    };

> +};

> +

> +struct move_only {

> +    move_only();

> +    move_only(const move_only&) = delete;

> +    move_only(move_only&) = delete;

> +    move_only(move_only&&) = default;

> +};

> +

> +task f(move_only x) {

> +    co_return;

> +}

> diff --git a/gcc/testsuite/g++.dg/coroutines/torture/func-params-08.C b/gcc/testsuite/g++.dg/coroutines/torture/func-params-08.C

> index c166745e052..cce1521b226 100644

> --- a/gcc/testsuite/g++.dg/coroutines/torture/func-params-08.C

> +++ b/gcc/testsuite/g++.dg/coroutines/torture/func-params-08.C

> @@ -69,8 +69,9 @@ my_coro (Foo t_lv, Foo& t_ref, Foo&& t_rv_ref)

>    PRINT ("my_coro 1");

>    sum += co_await t_ref;

>    PRINT ("my_coro 2");

> -  sum += co_await t_rv_ref;

> -  PRINT ("my_coro 3");

> +  // This can't work for the rvalue ref, it's always dangling.

> +  //sum += co_await t_rv_ref;

> +  //PRINT ("my_coro 3");

>    co_return sum;

> }

> 

> @@ -90,17 +91,17 @@ int main ()

>    // now do the three co_awaits.

>    while(!x.handle.done())

>      x.handle.resume();

> -  PRINT ("main: after resuming 3 co_awaits");

> +  PRINT ("main: after resuming 2 co_awaits");

> 

>    /* Now we should have the co_returned value.  */

>    int y = x.handle.promise().get_value();

> -  if (y != 14)

> +  if (y != 10)

>      {

>        PRINTF ("main: wrong result (%d).", y);

>        abort ();

>      }

> 

> -  if (regular != 3 || copy != 1 || move != 1)

> +  if (regular != 3 || copy != 0 || move != 1)

>      {

>        PRINTF ("main: unexpected ctor use (R:%d, C:%d, M:%d)\n",

> 	      regular, copy, move);

> diff --git a/gcc/testsuite/g++.dg/coroutines/torture/func-params-09-awaitable-parms.C b/gcc/testsuite/g++.dg/coroutines/torture/func-params-09-awaitable-parms.C

> index 7d376b91f13..81430bf4d54 100644

> --- a/gcc/testsuite/g++.dg/coroutines/torture/func-params-09-awaitable-parms.C

> +++ b/gcc/testsuite/g++.dg/coroutines/torture/func-params-09-awaitable-parms.C

> @@ -62,8 +62,9 @@ my_coro (FooAwaitable t_lv, FooAwaitable& t_ref, FooAwaitable&& t_rv_ref)

>    PRINT ("my_coro 1");

>    sum += co_await t_ref;

>    PRINT ("my_coro 2");

> -  sum += co_await t_rv_ref;

> -  PRINT ("my_coro 3");

> +  // This can't work for the rvalue ref, it's always dangling.

> +  //sum += co_await t_rv_ref;

> +  //PRINT ("my_coro 3");

>    co_return sum;

> }

> 

> @@ -83,17 +84,17 @@ int main ()

>    // now do the three co_awaits.

>    while(!x.handle.done())

>      x.handle.resume();

> -  PRINT ("main: after resuming 3 co_awaits");

> +  PRINT ("main: after resuming 2 co_awaits");

> 

>    /* Now we should have the co_returned value.  */

>    int y = x.handle.promise().get_value();

> -  if (y != 14)

> +  if (y != 10)

>      {

>        PRINTF ("main: wrong result (%d).", y);

>        abort ();

>      }

> 

> -  if (regular != 3 || copy != 1 || move != 1)

> +  if (regular != 3 || copy != 0 || move != 1)

>      {

>        PRINTF ("main: unexpected ctor use (R:%d, C:%d, M:%d)\n",

> 	      regular, copy, move);

> 



-- 
Nathan Sidwell

Patch

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index 969f4a66f2f..8746927577a 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -1807,7 +1807,6 @@  struct param_info
  tree frame_type;   /* The type used to represent this parm in the frame.  */
  tree orig_type;    /* The original type of the parm (not as passed).  */
  bool by_ref;       /* Was passed by reference.  */
-  bool rv_ref;       /* Was an rvalue reference.  */
  bool pt_ref;       /* Was a pointer to object.  */
  bool trivial_dtor; /* The frame type has a trivial DTOR.  */
  bool this_ptr;     /* Is 'this' */
@@ -2077,12 +2076,6 @@  build_actor_fn (location_t loc, tree coro_frame_type, tree actor, tree fnbody,
	  if (parm.pt_ref)
	    fld_idx = build1_loc (loc, CONVERT_EXPR, TREE_TYPE (arg), fld_idx);

-	  /* We expect an rvalue ref. here.  */
-	  if (parm.rv_ref)
-	    fld_idx = convert_to_reference (DECL_ARG_TYPE (arg), fld_idx,
-					    CONV_STATIC, LOOKUP_NORMAL,
-					    NULL_TREE, tf_warning_or_error);
-
	  int i;
	  tree *puse;
	  FOR_EACH_VEC_ELT (*parm.body_uses, i, puse)
@@ -3770,15 +3763,8 @@  morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
	  if (actual_type == NULL_TREE)
	    actual_type = error_mark_node;
	  parm.orig_type = actual_type;
-	  parm.by_ref = parm.rv_ref = parm.pt_ref = false;
-	  if (TREE_CODE (actual_type) == REFERENCE_TYPE
-	      && TYPE_REF_IS_RVALUE (DECL_ARG_TYPE (arg)))
-	    {
-	      parm.rv_ref = true;
-	      actual_type = TREE_TYPE (actual_type);
-	      parm.frame_type = actual_type;
-	    }
-	  else if (TREE_CODE (actual_type) == REFERENCE_TYPE)
+	  parm.by_ref = parm.pt_ref = false;
+	  if (TREE_CODE (actual_type) == REFERENCE_TYPE)
	    {
	      /* If the user passes by reference, then we will save the
		 pointer to the original.  As noted in
@@ -3786,16 +3772,12 @@  morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
		 referenced item ends and then the coroutine is resumed,
		 we have UB; well, the user asked for it.  */
	      actual_type = build_pointer_type (TREE_TYPE (actual_type));
-	      parm.frame_type = actual_type;
	      parm.pt_ref = true;
	    }
	  else if (TYPE_REF_P (DECL_ARG_TYPE (arg)))
-	    {
-	      parm.by_ref = true;
-	      parm.frame_type = actual_type;
-	    }
-	  else
-	    parm.frame_type = actual_type;
+	    parm.by_ref = true;
+
+	  parm.frame_type = actual_type;

	  parm.this_ptr = is_this_parameter (arg);
	  if (lambda_p)
@@ -4170,17 +4152,16 @@  morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
	    }
	  else if (parm.by_ref)
	    vec_safe_push (promise_args, fld_idx);
-	  else if (parm.rv_ref)
-	    vec_safe_push (promise_args, rvalue (fld_idx));
	  else
	    vec_safe_push (promise_args, arg);

	  if (TYPE_NEEDS_CONSTRUCTING (parm.frame_type))
	    {
	      vec<tree, va_gc> *p_in;
-	      if (parm.by_ref
-		  && classtype_has_non_deleted_move_ctor (parm.frame_type)
-		  && !classtype_has_non_deleted_copy_ctor (parm.frame_type))
+	      if (CLASS_TYPE_P (parm.frame_type)
+		  && classtype_has_non_deleted_move_ctor (parm.frame_type))
+		p_in = make_tree_vector_single (move (arg));
+	      else if (lvalue_p (arg))
		p_in = make_tree_vector_single (rvalue (arg));
	      else
		p_in = make_tree_vector_single (arg);
@@ -4193,9 +4174,7 @@  morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
	    }
	  else
	    {
-	      if (parm.rv_ref)
-		r = convert_from_reference (arg);
-	      else if (!same_type_p (parm.frame_type, DECL_ARG_TYPE (arg)))
+	      if (!same_type_p (parm.frame_type, DECL_ARG_TYPE (arg)))
		r = build1_loc (DECL_SOURCE_LOCATION (arg), CONVERT_EXPR,
				parm.frame_type, arg);
	      else
diff --git a/gcc/testsuite/g++.dg/coroutines/pr95350.C b/gcc/testsuite/g++.dg/coroutines/pr95350.C
new file mode 100644
index 00000000000..1915032c471
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/pr95350.C
@@ -0,0 +1,28 @@ 
+#if __has_include(<coroutine>)
+#include <coroutine>
+#else
+#include <experimental/coroutine>
+namespace std { using namespace experimental; }
+#endif
+#include <utility>
+
+struct task {
+    struct promise_type {
+        task get_return_object();
+        void return_void();
+        void unhandled_exception();
+        std::suspend_always initial_suspend() noexcept;
+        std::suspend_always final_suspend() noexcept;
+    };
+};
+
+struct move_only {
+    move_only();
+    move_only(const move_only&) = delete;
+    move_only(move_only&) = delete;
+    move_only(move_only&&) = default;
+};
+
+task f(move_only x) {
+    co_return;
+}
diff --git a/gcc/testsuite/g++.dg/coroutines/torture/func-params-08.C b/gcc/testsuite/g++.dg/coroutines/torture/func-params-08.C
index c166745e052..cce1521b226 100644
--- a/gcc/testsuite/g++.dg/coroutines/torture/func-params-08.C
+++ b/gcc/testsuite/g++.dg/coroutines/torture/func-params-08.C
@@ -69,8 +69,9 @@  my_coro (Foo t_lv, Foo& t_ref, Foo&& t_rv_ref)
  PRINT ("my_coro 1");
  sum += co_await t_ref;
  PRINT ("my_coro 2");
-  sum += co_await t_rv_ref;
-  PRINT ("my_coro 3");
+  // This can't work for the rvalue ref, it's always dangling.
+  //sum += co_await t_rv_ref;
+  //PRINT ("my_coro 3");
  co_return sum;
}

@@ -90,17 +91,17 @@  int main ()
  // now do the three co_awaits.
  while(!x.handle.done())
    x.handle.resume();
-  PRINT ("main: after resuming 3 co_awaits");
+  PRINT ("main: after resuming 2 co_awaits");

  /* Now we should have the co_returned value.  */
  int y = x.handle.promise().get_value();
-  if (y != 14)
+  if (y != 10)
    {
      PRINTF ("main: wrong result (%d).", y);
      abort ();
    }

-  if (regular != 3 || copy != 1 || move != 1)
+  if (regular != 3 || copy != 0 || move != 1)
    {
      PRINTF ("main: unexpected ctor use (R:%d, C:%d, M:%d)\n",
	      regular, copy, move);
diff --git a/gcc/testsuite/g++.dg/coroutines/torture/func-params-09-awaitable-parms.C b/gcc/testsuite/g++.dg/coroutines/torture/func-params-09-awaitable-parms.C
index 7d376b91f13..81430bf4d54 100644
--- a/gcc/testsuite/g++.dg/coroutines/torture/func-params-09-awaitable-parms.C
+++ b/gcc/testsuite/g++.dg/coroutines/torture/func-params-09-awaitable-parms.C
@@ -62,8 +62,9 @@  my_coro (FooAwaitable t_lv, FooAwaitable& t_ref, FooAwaitable&& t_rv_ref)
  PRINT ("my_coro 1");
  sum += co_await t_ref;
  PRINT ("my_coro 2");
-  sum += co_await t_rv_ref;
-  PRINT ("my_coro 3");
+  // This can't work for the rvalue ref, it's always dangling.
+  //sum += co_await t_rv_ref;
+  //PRINT ("my_coro 3");
  co_return sum;
}

@@ -83,17 +84,17 @@  int main ()
  // now do the three co_awaits.
  while(!x.handle.done())
    x.handle.resume();
-  PRINT ("main: after resuming 3 co_awaits");
+  PRINT ("main: after resuming 2 co_awaits");

  /* Now we should have the co_returned value.  */
  int y = x.handle.promise().get_value();
-  if (y != 14)
+  if (y != 10)
    {
      PRINTF ("main: wrong result (%d).", y);
      abort ();
    }

-  if (regular != 3 || copy != 1 || move != 1)
+  if (regular != 3 || copy != 0 || move != 1)
    {
      PRINTF ("main: unexpected ctor use (R:%d, C:%d, M:%d)\n",
	      regular, copy, move);