[Coroutines] Add error messages for missing methods of awaitable class

Message ID 661fe9cf-23eb-5d33-4fce-8edfab8a3d2a@linux.alibaba.com
State New
Headers show
Series
  • [Coroutines] Add error messages for missing methods of awaitable class
Related show

Commit Message

JunMa Jan. 20, 2020, 5:18 a.m.
Hi
This patch adds lookup_awaitable_member, it outputs error messages when 
any of
the await_ready/suspend/resume functions are missing in awaitable class.

This patch also add some error check on return value of build_co_await 
since we
may failed to build co_await_expr.

Bootstrap and test on X86_64, is it OK?

Regards
JunMa

gcc/cp
2020-01-20  Jun Ma <JunMa@linux.alibaba.com>

         * coroutines.cc (lookup_awaitable_member): Lookup an awaitable 
member.
         (build_co_await): Use lookup_awaitable_member instead of 
lookup_member.
         (finish_co_yield_expr, finish_co_await_expr): Add error check 
on return
         value of build_co_await.

gcc/testsuite
2020-01-20  Jun Ma <JunMa@linux.alibaba.com>

         * g++.dg/coroutines/coro1-missing-await-method.C: New test.
From 3979b29dcdb1000bbc819f69c1dc3ce7616f87cd Mon Sep 17 00:00:00 2001
From: "liangbin.mj" <liangbin.mj@alibaba-inc.com>
Date: Thu, 21 Nov 2019 08:51:22 +0800
Subject: [PATCH] to #23584419 Add some error messages when can not find method
 of awaitable class.

---
 gcc/cp/coroutines.cc                          | 49 ++++++++++++-------
 .../coroutines/coro1-missing-await-method.C   | 21 ++++++++
 .../coroutines/coro1-ret-int-yield-int.h      |  9 ++++
 3 files changed, 61 insertions(+), 18 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/coro1-missing-await-method.C

Comments

Iain Sandoe Jan. 20, 2020, 8:55 a.m. | #1
Hi JunMa,

JunMa <JunMa@linux.alibaba.com> wrote:

> gcc/cp

> 2020-01-20  Jun Ma <JunMa@linux.alibaba.com>

>

>         * coroutines.cc (lookup_awaitable_member): Lookup an awaitable member.

>         (build_co_await): Use lookup_awaitable_member instead of lookup_member.

>         (finish_co_yield_expr, finish_co_await_expr): Add error check on return

>         value of build_co_await.

>

> gcc/testsuite

> 2020-01-20  Jun Ma <JunMa@linux.alibaba.com>

>

>         * g++.dg/coroutines/coro1-missing-await-method.C: New test.

> <0001-Add-some-error-messages-when-missing.patch>


this LGTM, but you will have to wait for a C++ maintainer to approve.
thanks
Iain
JunMa Jan. 20, 2020, 9:36 a.m. | #2
在 2020/1/20 下午4:55, Iain Sandoe 写道:
> Hi JunMa,

>

> JunMa <JunMa@linux.alibaba.com> wrote:

>

>> gcc/cp

>> 2020-01-20  Jun Ma <JunMa@linux.alibaba.com>

>>

>>         * coroutines.cc (lookup_awaitable_member): Lookup an 

>> awaitable member.

>>         (build_co_await): Use lookup_awaitable_member instead of 

>> lookup_member.

>>         (finish_co_yield_expr, finish_co_await_expr): Add error check 

>> on return

>>         value of build_co_await.

>>

>> gcc/testsuite

>> 2020-01-20  Jun Ma <JunMa@linux.alibaba.com>

>>

>>         * g++.dg/coroutines/coro1-missing-await-method.C: New test.

>> <0001-Add-some-error-messages-when-missing.patch>

>

> this LGTM, but you will have to wait for a C++ maintainer to approve.

> thanks

> Iain

Thanks Iain,

+Jason and Nathan
could you have a look?

Regards
JunMa
Nathan Sidwell Jan. 20, 2020, 3:49 p.m. | #3
On 1/20/20 12:18 AM, JunMa wrote:
> Hi

> This patch adds lookup_awaitable_member, it outputs error messages when 

> any of

> the await_ready/suspend/resume functions are missing in awaitable class.

> 

> This patch also add some error check on return value of build_co_await 

> since we

> may failed to build co_await_expr.

> 

> Bootstrap and test on X86_64, is it OK?

> 

> Regards

> JunMa

> 

> gcc/cp

> 2020-01-20  Jun Ma <JunMa@linux.alibaba.com>

> 

>          * coroutines.cc (lookup_awaitable_member): Lookup an awaitable 

> member.

>          (build_co_await): Use lookup_awaitable_member instead of 

> lookup_member.

>          (finish_co_yield_expr, finish_co_await_expr): Add error check 

> on return

>          value of build_co_await.

> 

> gcc/testsuite

> 2020-01-20  Jun Ma <JunMa@linux.alibaba.com>

> 

>          * g++.dg/coroutines/coro1-missing-await-method.C: New test.



1) you're mixing two distinct changes, the one about the error message 
and the one about changing error_mark_node.

>    tree op = build_co_await (kw, a, CO_AWAIT_SUSPEND_POINT);

> -  TREE_SIDE_EFFECTS (op) = 1;

> -  SET_EXPR_LOCATION (op, kw);

> -

> +  if (op != error_mark_node)

> +    {

> +      TREE_SIDE_EFFECTS (op) = 1;

> +      SET_EXPR_LOCATION (op, kw);

> +    }

>    return op;

>  }


these two fragments are ok, as a separate patch.


> +/* Lookup an Awaitable member, which should be await_ready, await_suspend

> +   or await_resume  */

> +

> +static tree

> +lookup_awaitable_member (tree await_type, tree member_id, location_t loc)

> +{

> +  tree aw_memb

> +    = lookup_member (await_type, member_id,

> +		     /*protect*/ 1, /*want_type*/ 0, tf_warning_or_error);

fwiw '/*foo=*/expr' is the canonical form -- i know Iain wasn't 
consistent, and it looks like I may have missed a few places in review.

> +  if (aw_memb == NULL_TREE || aw_memb == error_mark_node)

> +    {

> +      error_at (loc, "no member named %qE in %qT", member_id, await_type);

> +      return error_mark_node;

> +    }

> +  return aw_memb;

> +}


This looks wrong.  lookup_member is being passed tf_warning_or_error, so 
it should already be emitting a diagnostic, for the cases where 
something is found, but is ambiguous or whatever.  So, I think you only 
want a diagnostic here when you get NULL_TREE back.

nathan

-- 
Nathan Sidwell
Nathan Sidwell Jan. 21, 2020, 12:06 p.m. | #4
On 1/20/20 10:38 PM, JunMa wrote:
> 在 2020/1/21 上午9:31, JunMa 写道:

>> 在 2020/1/20 下午11:49, Nathan Sidwell 写道:

>>> On 1/20/20 12:18 AM, JunMa wrote:

>>>> Hi

>>>> This patch adds lookup_awaitable_member, it outputs error messages 

>>>> when any of

>>>> the await_ready/suspend/resume functions are missing in awaitable 

>>>> class.

>>>>

>>>> This patch also add some error check on return value of 

>>>> build_co_await since we

>>>> may failed to build co_await_expr.

>>>>

>>>> Bootstrap and test on X86_64, is it OK?

>>>>

>>>> Regards

>>>> JunMa

>>>>

>>>> gcc/cp

>>>> 2020-01-20  Jun Ma <JunMa@linux.alibaba.com>

>>>>

>>>>          * coroutines.cc (lookup_awaitable_member): Lookup an 

>>>> awaitable member.

>>>>          (build_co_await): Use lookup_awaitable_member instead of 

>>>> lookup_member.

>>>>          (finish_co_yield_expr, finish_co_await_expr): Add error 

>>>> check on return

>>>>          value of build_co_await.

>>>>

>>>> gcc/testsuite

>>>> 2020-01-20  Jun Ma <JunMa@linux.alibaba.com>

>>>>

>>>>          * g++.dg/coroutines/coro1-missing-await-method.C: New test.

>>>

>>>

>>> 1) you're mixing two distinct changes, the one about the error 

>>> message and the one about changing error_mark_node.

>>>

>>>>    tree op = build_co_await (kw, a, CO_AWAIT_SUSPEND_POINT);

>>>> -  TREE_SIDE_EFFECTS (op) = 1;

>>>> -  SET_EXPR_LOCATION (op, kw);

>>>> -

>>>> +  if (op != error_mark_node)

>>>> +    {

>>>> +      TREE_SIDE_EFFECTS (op) = 1;

>>>> +      SET_EXPR_LOCATION (op, kw);

>>>> +    }

>>>>    return op;

>>>>  }

>>>

>>> these two fragments are ok, as a separate patch.

>>>

>>>

>> ok, I'll split it.

>>>> +/* Lookup an Awaitable member, which should be await_ready, 

>>>> await_suspend

>>>> +   or await_resume  */

>>>> +

>>>> +static tree

>>>> +lookup_awaitable_member (tree await_type, tree member_id, 

>>>> location_t loc)

>>>> +{

>>>> +  tree aw_memb

>>>> +    = lookup_member (await_type, member_id,

>>>> +             /*protect*/ 1, /*want_type*/ 0, tf_warning_or_error);

>>> fwiw '/*foo=*/expr' is the canonical form -- i know Iain wasn't 

>>> consistent, and it looks like I may have missed a few places in review.

>>>

>> ok.

>>>> +  if (aw_memb == NULL_TREE || aw_memb == error_mark_node)

>>>> +    {

>>>> +      error_at (loc, "no member named %qE in %qT", member_id, 

>>>> await_type);

>>>> +      return error_mark_node;

>>>> +    }

>>>> +  return aw_memb;

>>>> +}

>>>

>>> This looks wrong.  lookup_member is being passed tf_warning_or_error, 

>>> so it should already be emitting a diagnostic, for the cases where 

>>> something is found, but is ambiguous or whatever.  So, I think you 

>>> only want a diagnostic here when you get NULL_TREE back.

>>>

>> you are right, I follow with same code of lookup_promise_member, I'll 

>> update both.

>>

>> Regards

>> JunMa

>>> nathan

>>>

> Hi nathan,

> attachment is the updated patch which add error messages for lookup 

> awaitable member.


thanks this is ok.  Could you remove the space in '/*protect=*/ 1' and 
similar?  This appear to be one place where our coding style has fewer 
spaces than expected!

nathan

-- 
Nathan Sidwell
Nathan Sidwell Jan. 21, 2020, 12:07 p.m. | #5
On 1/20/20 10:44 PM, JunMa wrote:
> 在 2020/1/21 上午9:31, JunMa 写道:


>> Regards

>> JunMa

>>> nathan

>>>

> Hi nathan,

> attachment is the updated patch which check error_mark_node of 

> build_co_coawait.

> 

> Regards

> JunMa

> 

> gcc/cp

> 2020-01-21  Jun Ma <JunMa@linux.alibaba.com>

> 

>           * coroutines.cc (finish_co_await_expr): Add error check on return

>           value of build_co_await.

>           (finish_co_yield_expr,): Ditto.


ok, thanks

nathan

-- 
Nathan Sidwell
JunMa Jan. 21, 2020, 2:23 p.m. | #6
在 2020/1/21 下午8:06, Nathan Sidwell 写道:
> On 1/20/20 10:38 PM, JunMa wrote:

>> 在 2020/1/21 上午9:31, JunMa 写道:

>>> 在 2020/1/20 下午11:49, Nathan Sidwell 写道:

>>>> On 1/20/20 12:18 AM, JunMa wrote:

>>>>> Hi

>>>>> This patch adds lookup_awaitable_member, it outputs error messages 

>>>>> when any of

>>>>> the await_ready/suspend/resume functions are missing in awaitable 

>>>>> class.

>>>>>

>>>>> This patch also add some error check on return value of 

>>>>> build_co_await since we

>>>>> may failed to build co_await_expr.

>>>>>

>>>>> Bootstrap and test on X86_64, is it OK?

>>>>>

>>>>> Regards

>>>>> JunMa

>>>>>

>>>>> gcc/cp

>>>>> 2020-01-20  Jun Ma <JunMa@linux.alibaba.com>

>>>>>

>>>>>          * coroutines.cc (lookup_awaitable_member): Lookup an 

>>>>> awaitable member.

>>>>>          (build_co_await): Use lookup_awaitable_member instead of 

>>>>> lookup_member.

>>>>>          (finish_co_yield_expr, finish_co_await_expr): Add error 

>>>>> check on return

>>>>>          value of build_co_await.

>>>>>

>>>>> gcc/testsuite

>>>>> 2020-01-20  Jun Ma <JunMa@linux.alibaba.com>

>>>>>

>>>>>          * g++.dg/coroutines/coro1-missing-await-method.C: New test.

>>>>

>>>>

>>>> 1) you're mixing two distinct changes, the one about the error 

>>>> message and the one about changing error_mark_node.

>>>>

>>>>>    tree op = build_co_await (kw, a, CO_AWAIT_SUSPEND_POINT);

>>>>> -  TREE_SIDE_EFFECTS (op) = 1;

>>>>> -  SET_EXPR_LOCATION (op, kw);

>>>>> -

>>>>> +  if (op != error_mark_node)

>>>>> +    {

>>>>> +      TREE_SIDE_EFFECTS (op) = 1;

>>>>> +      SET_EXPR_LOCATION (op, kw);

>>>>> +    }

>>>>>    return op;

>>>>>  }

>>>>

>>>> these two fragments are ok, as a separate patch.

>>>>

>>>>

>>> ok, I'll split it.

>>>>> +/* Lookup an Awaitable member, which should be await_ready, 

>>>>> await_suspend

>>>>> +   or await_resume  */

>>>>> +

>>>>> +static tree

>>>>> +lookup_awaitable_member (tree await_type, tree member_id, 

>>>>> location_t loc)

>>>>> +{

>>>>> +  tree aw_memb

>>>>> +    = lookup_member (await_type, member_id,

>>>>> +             /*protect*/ 1, /*want_type*/ 0, tf_warning_or_error);

>>>> fwiw '/*foo=*/expr' is the canonical form -- i know Iain wasn't 

>>>> consistent, and it looks like I may have missed a few places in 

>>>> review.

>>>>

>>> ok.

>>>>> +  if (aw_memb == NULL_TREE || aw_memb == error_mark_node)

>>>>> +    {

>>>>> +      error_at (loc, "no member named %qE in %qT", member_id, 

>>>>> await_type);

>>>>> +      return error_mark_node;

>>>>> +    }

>>>>> +  return aw_memb;

>>>>> +}

>>>>

>>>> This looks wrong.  lookup_member is being passed 

>>>> tf_warning_or_error, so it should already be emitting a diagnostic, 

>>>> for the cases where something is found, but is ambiguous or 

>>>> whatever.  So, I think you only want a diagnostic here when you get 

>>>> NULL_TREE back.

>>>>

>>> you are right, I follow with same code of lookup_promise_member, 

>>> I'll update both.

>>>

>>> Regards

>>> JunMa

>>>> nathan

>>>>

>> Hi nathan,

>> attachment is the updated patch which add error messages for lookup 

>> awaitable member.

>

> thanks this is ok.  Could you remove the space in '/*protect=*/ 1' and 

> similar?  This appear to be one place where our coding style has fewer 

> spaces than expected!

>

ok, I'll update it.

Regards
JunMa
> nathan

>

Patch

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index d3aacd7ad3b..49e509f4384 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -505,6 +505,23 @@  lookup_promise_method (tree fndecl, tree member_id, location_t loc,
   return pm_memb;
 }
 
+/* Lookup an Awaitable member, which should be await_ready, await_suspend
+   or await_resume  */
+
+static tree
+lookup_awaitable_member (tree await_type, tree member_id, location_t loc)
+{
+  tree aw_memb
+    = lookup_member (await_type, member_id,
+		     /*protect*/ 1, /*want_type*/ 0, tf_warning_or_error);
+  if (aw_memb == NULL_TREE || aw_memb == error_mark_node)
+    {
+      error_at (loc, "no member named %qE in %qT", member_id, await_type);
+      return error_mark_node;
+    }
+  return aw_memb;
+}
+
 /* Here we check the constraints that are common to all keywords (since the
    presence of a coroutine keyword makes the function into a coroutine).  */
 
@@ -643,25 +660,18 @@  build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind)
 
   /* Check for required awaitable members and their types.  */
   tree awrd_meth
-    = lookup_member (o_type, coro_await_ready_identifier,
-		     /* protect */ 1, /*want_type=*/0, tf_warning_or_error);
-
+    = lookup_awaitable_member (o_type, coro_await_ready_identifier, loc);
   if (!awrd_meth || awrd_meth == error_mark_node)
     return error_mark_node;
-
   tree awsp_meth
-    = lookup_member (o_type, coro_await_suspend_identifier,
-		     /* protect */ 1, /*want_type=*/0, tf_warning_or_error);
-
+    = lookup_awaitable_member (o_type, coro_await_suspend_identifier, loc);
   if (!awsp_meth || awsp_meth == error_mark_node)
     return error_mark_node;
 
   /* The type of the co_await is the return type of the awaitable's
-     co_resume(), so we need to look that up.  */
+     await_resume(), so we need to look that up.  */
   tree awrs_meth
-    = lookup_member (o_type, coro_await_resume_identifier,
-		     /* protect */ 1, /*want_type=*/0, tf_warning_or_error);
-
+    = lookup_awaitable_member (o_type, coro_await_resume_identifier, loc);
   if (!awrs_meth || awrs_meth == error_mark_node)
     return error_mark_node;
 
@@ -800,9 +810,11 @@  finish_co_await_expr (location_t kw, tree expr)
 
   /* Now we want to build co_await a.  */
   tree op = build_co_await (kw, a, CO_AWAIT_SUSPEND_POINT);
-  TREE_SIDE_EFFECTS (op) = 1;
-  SET_EXPR_LOCATION (op, kw);
-
+  if (op != error_mark_node)
+    {
+      TREE_SIDE_EFFECTS (op) = 1;
+      SET_EXPR_LOCATION (op, kw);
+    }
   return op;
 }
 
@@ -864,10 +876,11 @@  finish_co_yield_expr (location_t kw, tree expr)
      promise transform_await().  */
 
   tree op = build_co_await (kw, yield_call, CO_YIELD_SUSPEND_POINT);
-
-  op = build2_loc (kw, CO_YIELD_EXPR, TREE_TYPE (op), expr, op);
-  TREE_SIDE_EFFECTS (op) = 1;
-
+  if (op != error_mark_node)
+    {
+      op = build2_loc (kw, CO_YIELD_EXPR, TREE_TYPE (op), expr, op);
+      TREE_SIDE_EFFECTS (op) = 1;
+    }
   return op;
 }
 
diff --git a/gcc/testsuite/g++.dg/coroutines/coro1-missing-await-method.C b/gcc/testsuite/g++.dg/coroutines/coro1-missing-await-method.C
new file mode 100644
index 00000000000..c1869e0654c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/coro1-missing-await-method.C
@@ -0,0 +1,21 @@ 
+//  { dg-additional-options "-fsyntax-only -w" }
+#include "coro.h"
+
+#define MISSING_AWAIT_READY
+#define MISSING_AWAIT_SUSPEND
+#define MISSING_AWAIT_RESUME
+#include "coro1-ret-int-yield-int.h"
+
+coro1
+bar0 () // { dg-error {no member named 'await_suspend' in 'coro1::suspend_always_prt'} }
+{
+  co_await coro1::suspend_never_prt{}; // { dg-error {no member named 'await_ready' in 'coro1::suspend_never_prt'} }
+  co_yield 5; // { dg-error {no member named 'await_suspend' in 'coro1::suspend_always_prt'} }
+  co_await coro1::suspend_always_intprt(5); // { dg-error {no member named 'await_resume' in 'coro1::suspend_always_intprt'} }
+  co_return 0;
+}
+
+int main (int ac, char *av[]) {
+  struct coro1 x0 = bar0 ();
+  return 0;
+}
diff --git a/gcc/testsuite/g++.dg/coroutines/coro1-ret-int-yield-int.h b/gcc/testsuite/g++.dg/coroutines/coro1-ret-int-yield-int.h
index b961755e472..abf625869fa 100644
--- a/gcc/testsuite/g++.dg/coroutines/coro1-ret-int-yield-int.h
+++ b/gcc/testsuite/g++.dg/coroutines/coro1-ret-int-yield-int.h
@@ -27,14 +27,20 @@  struct coro1 {
   // Some awaitables to use in tests.
   // With progress printing for debug.
   struct suspend_never_prt {
+#ifdef MISSING_AWAIT_READY
+#else
   bool await_ready() const noexcept { return true; }
+#endif
   void await_suspend(handle_type) const noexcept { PRINT ("susp-never-susp");}
   void await_resume() const noexcept { PRINT ("susp-never-resume");}
   };
 
   struct  suspend_always_prt {
   bool await_ready() const noexcept { return false; }
+#ifdef MISSING_AWAIT_SUSPEND
+#else
   void await_suspend(handle_type) const noexcept { PRINT ("susp-always-susp");}
+#endif
   void await_resume() const noexcept { PRINT ("susp-always-resume");}
   ~suspend_always_prt() { PRINT ("susp-always-dtor"); }
   };
@@ -46,7 +52,10 @@  struct coro1 {
     ~suspend_always_intprt() {}
     bool await_ready() const noexcept { return false; }
     void await_suspend(coro::coroutine_handle<>) const noexcept { PRINT ("susp-always-susp-intprt");}
+#ifdef MISSING_AWAIT_RESUME
+#else
     int await_resume() const noexcept { PRINT ("susp-always-resume-intprt"); return x;}
+#endif
   };
   
   /* This returns the square of the int that it was constructed with.  */