[PATCH coroutines] Set side effects flag for BIND_EXPR which build in maybe_promote_captured_temps

[PATCH coroutines] Set side effects flag for BIND_EXPR which build in maybe_promote_captured_temps

[JunMa]

Hi
As title. in maybe_promote_captured_temps, we promote captured temporaries
and co_await_expr into a new BIND_EXPR. As the BIND_EXPR contains
co_await_expr and maybe other function calls, the side effects flag should
be set.

This patch fix one mismatch in cppcoro, the testcase comes from cppcoro
and is reduced by creduce.

Bootstrap and test on X86_64, is it OK?

Regards
JunMa

gcc/cp
2020-02-11  Jun Ma <JunMa@linux.alibaba.com>

         * coroutines.cc (maybe_promote_captured_temps): Set side effects
         flag for BIND_EXPR.

gcc/testsuite
2020-02-11  Jun Ma <JunMa@linux.alibaba.com>

         * g++.dg/coroutines/torture/lambda-10-co-await-lambda.C: New test.

Re: [PATCH coroutines] Set side effects flag for BIND_EXPR which build in maybe_promote_captured_temps

[JunMa]

[-- Attachment #1: Type: text/plain, Size: 912 bytes --]

在 2020/2/11 上午10:50, JunMa 写道:
> Hi
> As title. in maybe_promote_captured_temps, we promote captured 
> temporaries
> and co_await_expr into a new BIND_EXPR. As the BIND_EXPR contains
> co_await_expr and maybe other function calls, the side effects flag 
> should
> be set.
>
> This patch fix one mismatch in cppcoro, the testcase comes from cppcoro
> and is reduced by creduce.
>
> Bootstrap and test on X86_64, is it OK?
>
> Regards
> JunMa
>
> gcc/cp
> 2020-02-11  Jun Ma <JunMa@linux.alibaba.com>
>
>         * coroutines.cc (maybe_promote_captured_temps): Set side effects
>         flag for BIND_EXPR.
>
> gcc/testsuite
> 2020-02-11  Jun Ma <JunMa@linux.alibaba.com>
>
>         * g++.dg/coroutines/torture/lambda-10-co-await-lambda.C: New 
> test.
Hi

Sorry for forgetting the patch. Here it is.

Regards
JunMa

[-- Attachment #2: 0001-Set-side-effects-flag-when-the-BIND_EXPR-contains-co.patch --]
[-- Type: text/plain, Size: 2193 bytes --]

---
 gcc/cp/coroutines.cc                          |  2 +
 .../torture/lambda-10-co-await-lambda.C       | 47 +++++++++++++++++++
 2 files changed, 49 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/torture/lambda-10-co-await-lambda.C

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index decec4550af..004e0a1a659 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -2735,6 +2735,8 @@ maybe_promote_captured_temps (tree *stmt, void *d)
 	}
       BIND_EXPR_BLOCK (aw_bind) = b_block;
 
+      /* Set side effects flag since the BIND_EXPR contains co_await expr.  */
+      TREE_SIDE_EFFECTS (aw_bind) = 1;
       *stmt = aw_bind;
     }
   return res;
diff --git a/gcc/testsuite/g++.dg/coroutines/torture/lambda-10-co-await-lambda.C b/gcc/testsuite/g++.dg/coroutines/torture/lambda-10-co-await-lambda.C
new file mode 100644
index 00000000000..43d4ff6b77e
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/torture/lambda-10-co-await-lambda.C
@@ -0,0 +1,47 @@
+//  { dg-do run }
+
+#include "../coro.h"
+template <typename> struct a;
+struct b {
+  struct c {
+    int await_ready() {return 0;}
+    template <typename d> void await_suspend(d) {}
+    void await_resume() noexcept {}
+  };
+  auto initial_suspend() { return std::suspend_never{}; }
+  auto final_suspend() { return c{}; }
+};
+template <typename> struct e {
+  int f() {return 1;}
+};
+template <> struct e<void> : b {
+  a<void> get_return_object() noexcept;
+  void unhandled_exception();
+};
+template <typename g = void> struct a {
+  using promise_type = e<g>;
+  struct h {
+    std::coroutine_handle<promise_type> j;
+    int await_ready() {return 0;}
+    bool await_suspend(std::coroutine_handle<>) { return false;}
+  };
+  auto operator co_await() {
+    struct awaitable : h {
+      auto await_resume() { return this->j.promise().f(); }
+    };
+    return awaitable{};
+  }
+};
+a<> e<void>::get_return_object() noexcept { return a<>{};}
+
+int main() {
+  auto k = []() -> a<int> { return a<int>{};};
+  int l = 0 ;
+  auto m = [&]() -> a<> {
+    for (int i; i < 100; ++i)
+      l += co_await k();
+  };
+  m();
+  if (l != 100)
+    abort();
+}
-- 
2.19.1.3.ge56e4f7

Re: [PING PATCH coroutines] Set side effects flag for BIND_EXPR which build in maybe_promote_captured_temps

[JunMa]

在 2020/2/11 上午10:50, JunMa 写道:
Hi
kindly ping~

Regards
JunMa
> Hi
> As title. in maybe_promote_captured_temps, we promote captured 
> temporaries
> and co_await_expr into a new BIND_EXPR. As the BIND_EXPR contains
> co_await_expr and maybe other function calls, the side effects flag 
> should
> be set.
>
> This patch fix one mismatch in cppcoro, the testcase comes from cppcoro
> and is reduced by creduce.
>
> Bootstrap and test on X86_64, is it OK?
>
> Regards
> JunMa
>
> gcc/cp
> 2020-02-11  Jun Ma <JunMa@linux.alibaba.com>
>
>         * coroutines.cc (maybe_promote_captured_temps): Set side effects
>         flag for BIND_EXPR.
>
> gcc/testsuite
> 2020-02-11  Jun Ma <JunMa@linux.alibaba.com>
>
>         * g++.dg/coroutines/torture/lambda-10-co-await-lambda.C: New 
> test.

Re: [PING PATCH coroutines] Set side effects flag for BIND_EXPR which build in maybe_promote_captured_temps

[JunMa]

Ping

Regards
JunMa
在 2020/2/27 上午10:17, JunMa 写道:
> 在 2020/2/11 上午10:50, JunMa 写道:
> Hi
> kindly ping~
>
> Regards
> JunMa
>> Hi
>> As title. in maybe_promote_captured_temps, we promote captured 
>> temporaries
>> and co_await_expr into a new BIND_EXPR. As the BIND_EXPR contains
>> co_await_expr and maybe other function calls, the side effects flag 
>> should
>> be set.
>>
>> This patch fix one mismatch in cppcoro, the testcase comes from cppcoro
>> and is reduced by creduce.
>>
>> Bootstrap and test on X86_64, is it OK?
>>
>> Regards
>> JunMa
>>
>> gcc/cp
>> 2020-02-11  Jun Ma <JunMa@linux.alibaba.com>
>>
>>         * coroutines.cc (maybe_promote_captured_temps): Set side effects
>>         flag for BIND_EXPR.
>>
>> gcc/testsuite
>> 2020-02-11  Jun Ma <JunMa@linux.alibaba.com>
>>
>>         * g++.dg/coroutines/torture/lambda-10-co-await-lambda.C: New 
>> test.
>
>

Re: [PING PATCH coroutines] Set side effects flag for BIND_EXPR which build in maybe_promote_captured_temps

[Iain Sandoe]

Hello JunMa,

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

> Ping

Thanks for your patch(es) and I am sorry this has taken some time to review.

(right now, we’re trying to ensure that we have the latest standard  
represented in
  GCC10, so updating to n4849).
>
> 在 2020/2/27 上午10:17, JunMa 写道:
>> 在 2020/2/11 上午10:50, JunMa 写道:
>> Hi
>> kindly ping~
>>
>> Regards
>> JunMa
>>> Hi
>>> As title. in maybe_promote_captured_temps, we promote captured  
>>> temporaries
>>> and co_await_expr into a new BIND_EXPR. As the BIND_EXPR contains
>>> co_await_expr and maybe other function calls, the side effects flag  
>>> should
>>> be set.
>>>
>>> This patch fix one mismatch in cppcoro, the testcase comes from cppcoro
>>> and is reduced by creduce.

With the following test conditions;

r10-7040-ga2ec7c4aafbcd517
  + the two approved patches by Bin Cheng applied.

  1/ the test case in this patch (lambda-10-co-await-lambda.C) fails both with and without the patch.
  2/ the patch regresses one of my local testcases.

So, it appears that the testcase might show a bug - but the fix is not the  
right one for current trunk?

Please could you re-check ?

thanks
Iain

Re: [PING PATCH coroutines] Set side effects flag for BIND_EXPR which build in maybe_promote_captured_temps

[JunMa]

在 2020/3/5 下午9:51, Iain Sandoe 写道:
> Hello JunMa,
>
> JunMa <JunMa@linux.alibaba.com> wrote:
>
>> Ping
>
> Thanks for your patch(es) and I am sorry this has taken some time to 
> review.
>
> (right now, we’re trying to ensure that we have the latest standard 
> represented in
>  GCC10, so updating to n4849).
>>
>> 在 2020/2/27 上午10:17, JunMa 写道:
>>> 在 2020/2/11 上午10:50, JunMa 写道:
>>> Hi
>>> kindly ping~
>>>
>>> Regards
>>> JunMa
>>>> Hi
>>>> As title. in maybe_promote_captured_temps, we promote captured 
>>>> temporaries
>>>> and co_await_expr into a new BIND_EXPR. As the BIND_EXPR contains
>>>> co_await_expr and maybe other function calls, the side effects flag 
>>>> should
>>>> be set.
>>>>
>>>> This patch fix one mismatch in cppcoro, the testcase comes from 
>>>> cppcoro
>>>> and is reduced by creduce.
>
> With the following test conditions;
>
> r10-7040-ga2ec7c4aafbcd517
>  + the two approved patches by Bin Cheng applied.
>
>  1/ the test case in this patch (lambda-10-co-await-lambda.C) fails 
> both with and without the patch.
>  2/ the patch regresses one of my local testcases.
>
The test case fails because of ICE which is fixed by the
[PING PATCH coroutines] Do not strip cleanup_point when promote 
temporaries out of current stmt
This patch fix the runtime mismatch.

The extra regression is co-await-syntax-11.C which comes from Bin's 
patch and also is fixed by that
patch.

Regards
JunMa
> So, it appears that the testcase might show a bug - but the fix is not 
> the right one for current trunk?
>
> Please could you re-check ?
>
> thanks
> Iain