coroutines: Update handling and failure for g-r-o-o-a-f [PR95505]

Message ID E195B717-19F3-4DD2-9ADE-862BD0D3060E@sandoe.co.uk
State New
Headers show
Series
  • coroutines: Update handling and failure for g-r-o-o-a-f [PR95505]
Related show

Commit Message

Iain Sandoe June 11, 2020, 7:50 p.m.
Hi,

The reason that the code fails is that (by a somewhat complex
implied route), when a user adds a 'get-return-on-alloc-fail’
to their coroutine promise, this implies the use of some content
from <new>; we should not ICE because the user forgot that tho.

Jonathan and I were discussing whether there’s a better way than
including <new> in <coroutines> to make this less likely to happen.

The issue is that neither <coroutine> nor the user’s code overtly
use <new>; one has to know that the standard makes use of it
by implication… anyway that’s not the bug, but the background.

tested on Linux, and Darwin.
OK for master?
10.2?
thanks
Iain

-- 
2.24.1

Comments

Nathan Sidwell June 15, 2020, 1:54 p.m. | #1
On 6/11/20 3:50 PM, Iain Sandoe wrote:
> Hi,

> 

> The reason that the code fails is that (by a somewhat complex

> implied route), when a user adds a 'get-return-on-alloc-fail’

> to their coroutine promise, this implies the use of some content

> from <new>; we should not ICE because the user forgot that tho.

> 

> Jonathan and I were discussing whether there’s a better way than

> including <new> in <coroutines> to make this less likely to happen.

> 

> The issue is that neither <coroutine> nor the user’s code overtly

> use <new>; one has to know that the standard makes use of it

> by implication… anyway that’s not the bug, but the background.

> 

> tested on Linux, and Darwin.

> OK for master?

> 10.2?

> thanks

> Iain


ok, a couple of nits

> ========

> 

> The actual issue is that (in the testcase) std::nothrow is not

> available.  So update the handling of the get-return-on-alloc-fail

> to include the possibility that std::nothrow might not be

> available.

> 

> gcc/cp/ChangeLog:

> 

> 	* coroutines.cc (morph_fn_to_coro): Update handling of

> 	get-return-object-on-allocation-fail and diagnose missing

> 	std::nothrow.

> 

> gcc/testsuite/ChangeLog:

> 

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

> ---

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

>   gcc/testsuite/g++.dg/coroutines/pr95505.C | 26 ++++++++++++++

>   2 files changed, 47 insertions(+), 22 deletions(-)

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

> 

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

> index f0b7e71633d..93f1e5ca30d 100644

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

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

> @@ -3913,30 +3913,25 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)

>     tree grooaf = NULL_TREE;

>     tree dummy_promise = build_dummy_object (get_coroutine_promise_type (orig));

>   

> -  /* We don't require this, so lookup_promise_method can return NULL...  */

> +  /* We don't require this, so lookup_promise_method can return NULL,

> +     but, if the lookup succeeds, then the function must be usable.*/

  .<MISSING-SPACES>*/

> -  if (fns && BASELINK_P (fns))

> +  /* We don't require this, so lookup_promise_method can return NULL...  */

... to what?  Is this a repeat of the above rationale or something different?


nathan
-- 
Nathan Sidwell

Patch

========

The actual issue is that (in the testcase) std::nothrow is not
available.  So update the handling of the get-return-on-alloc-fail
to include the possibility that std::nothrow might not be
available.

gcc/cp/ChangeLog:

	* coroutines.cc (morph_fn_to_coro): Update handling of
	get-return-object-on-allocation-fail and diagnose missing
	std::nothrow.

gcc/testsuite/ChangeLog:

	* g++.dg/coroutines/pr95505.C: New test.
---
 gcc/cp/coroutines.cc                      | 43 +++++++++++------------
 gcc/testsuite/g++.dg/coroutines/pr95505.C | 26 ++++++++++++++
 2 files changed, 47 insertions(+), 22 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/pr95505.C

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index f0b7e71633d..93f1e5ca30d 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -3913,30 +3913,25 @@  morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
   tree grooaf = NULL_TREE;
   tree dummy_promise = build_dummy_object (get_coroutine_promise_type (orig));
 
-  /* We don't require this, so lookup_promise_method can return NULL...  */
+  /* We don't require this, so lookup_promise_method can return NULL,
+     but, if the lookup succeeds, then the function must be usable.*/
   if (grooaf_meth && BASELINK_P (grooaf_meth))
-    {
-      /* ... but, if the lookup succeeds, then the function must be
-	 usable.
-	 build_new_method_call () wants a valid pointer to (an empty)  args
-	 list in this case.  */
-      vec<tree, va_gc> *args = make_tree_vector ();
-      grooaf = build_new_method_call (dummy_promise, grooaf_meth, &args,
-				      NULL_TREE, LOOKUP_NORMAL, NULL,
-				      tf_warning_or_error);
-      release_tree_vector (args);
-    }
+    grooaf = build_new_method_call (dummy_promise, grooaf_meth, NULL,
+				    NULL_TREE, LOOKUP_NORMAL, NULL,
+				    tf_warning_or_error);
 
   /* Allocate the frame, this has several possibilities:
      [dcl.fct.def.coroutine] / 9 (part 1)
      The allocation function’s name is looked up in the scope of the promise
      type.  It's not a failure for it to be absent see part 4, below.  */
+
   tree nwname = ovl_op_identifier (false, NEW_EXPR);
-  tree fns = lookup_promise_method (orig, nwname, fn_start,
-				    /*musthave=*/false);
   tree new_fn = NULL_TREE;
-  if (fns && BASELINK_P (fns))
+  /* We don't require this, so lookup_promise_method can return NULL...  */
+  if (TYPE_HAS_NEW_OPERATOR (promise_type))
     {
+      tree fns = lookup_promise_method (orig, nwname, fn_start,
+				    /*musthave=*/true);
       /* [dcl.fct.def.coroutine] / 9 (part 2)
 	If the lookup finds an allocation function in the scope of the promise
 	type, overload resolution is performed on a function call created by
@@ -3973,14 +3968,13 @@  morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
 				      LOOKUP_NORMAL, &func, tf_none);
       release_tree_vector (args);
 
-      if (!new_fn || new_fn == error_mark_node)
+      if (new_fn == error_mark_node)
 	{
 	  /* [dcl.fct.def.coroutine] / 9 (part 3)
 	    If no viable function is found, overload resolution is performed
 	    again on a function call created by passing just the amount of
 	    space required as an argument of type std::size_t.  */
-	  args = make_tree_vector ();
-	  vec_safe_push (args, resizeable); /* Space needed.  */
+	  args = make_tree_vector_single (resizeable); /* Space needed.  */
 	  new_fn = build_new_method_call (dummy_promise, fns, &args,
 					  NULL_TREE, LOOKUP_NORMAL, &func,
 					  tf_none);
@@ -3989,7 +3983,7 @@  morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
 
      /* However, if the initial lookup succeeded, then one of these two
 	options must be available.  */
-    if (!new_fn || new_fn == error_mark_node)
+    if (new_fn == error_mark_node)
       {
 	error_at (fn_start, "%qE is provided by %qT but is not usable with"
 		  " the function signature %qD", nwname, promise_type, orig);
@@ -3999,7 +3993,7 @@  morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
       error_at (fn_start, "%qE is provided by %qT but %qE is not marked"
 		" %<throw()%> or %<noexcept%>", grooaf, promise_type, nwname);
     }
-  else
+  else /* No operator new in the promise.  */
     {
       /* [dcl.fct.def.coroutine] / 9 (part 4)
 	 If this lookup fails, the allocation function’s name is looked up in
@@ -4009,7 +4003,6 @@  morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
       /* build_operator_new_call () will insert size needed as element 0 of
 	 this, and we might need to append the std::nothrow constant.  */
       vec_alloc (args, 2);
-
       if (grooaf)
 	{
 	  /* [dcl.fct.def.coroutine] / 10 (part 2)
@@ -4023,6 +4016,9 @@  morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
 	  tree std_nt = lookup_qualified_name (std_node,
 					       get_identifier ("nothrow"),
 					       0, /*complain=*/true, false);
+	  if (!std_nt || std_nt == error_mark_node)
+	    error_at (fn_start, "%qE is provided by %qT but %<std::nothrow%> "
+		      "cannot be found", grooaf, promise_type);
 	  vec_safe_push (args, std_nt);
 	}
 
@@ -4037,7 +4033,10 @@  morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
 					tf_warning_or_error);
       resizeable = build_call_expr_internal_loc
 	(fn_start, IFN_CO_FRAME, size_type_node, 2, frame_size, coro_fp);
-      CALL_EXPR_ARG (new_fn, 0) = resizeable;
+      /* If the operator call fails for some reason, then don't try to
+	 amend it.  */
+      if (new_fn != error_mark_node)
+	CALL_EXPR_ARG (new_fn, 0) = resizeable;
 
       release_tree_vector (args);
     }
diff --git a/gcc/testsuite/g++.dg/coroutines/pr95505.C b/gcc/testsuite/g++.dg/coroutines/pr95505.C
new file mode 100644
index 00000000000..a76b827cae4
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/pr95505.C
@@ -0,0 +1,26 @@ 
+#if __has_include (<coroutine>)
+#include <coroutine>
+using namespace std;
+#elif defined (__clang__) && __has_include (<experimental/coroutine>)
+#include <experimental/coroutine>
+namespace std { using namespace experimental; }
+#endif
+
+struct dummy
+{
+    struct promise_type
+    {
+        dummy get_return_object() const noexcept { return {}; }
+        static dummy get_return_object_on_allocation_failure() noexcept { return {}; }
+        std::suspend_always initial_suspend() const noexcept { return {}; }
+        std::suspend_never final_suspend() const noexcept { return {}; }
+        void return_void() const noexcept {}
+        void unhandled_exception() const noexcept {}
+    };
+};
+
+dummy foo() // { dg-error {dummy::promise_type::get_return_object_on_allocation_failure.*but 'std::nothrow' cannot be found} }
+{
+    co_return;
+}
+