Re: [PATCH] coroutines: Do not promote temporaries that will be elided.

[Jason Merrill <jason@redhat.com>]

On 12/2/22 15:25, Iain Sandoe wrote:
> Thanks to Adrian for working on this and producing the revised test-cases.
> 
> This has been tested on x86_64-darwin21 with our testsuite, cppcoro and
> a patched version of folly (that enables the experimental coroutines tests)
> without regresssion (actually a handful of progressions on folly, plus the
> fixes to our suite).

OK, glad the TARGET_EXPR_ELIDING_P work is being useful!

> I am not sure how best to backport this; I think we want it at least on 12
> and preferably earlier, but the TARGET_EXPR_ELIDING_P flag is not available
> there, perhaps we can construct some local function in coroutines.cc that
> emulates it? (or maybe the TARGET_EXPR_ELIDING_P and associated fixes is
> suitable for backport?)

Backporting TARGET_EXPR_ELIDING_P doesn't seem feasible for 12.

If you wanted to try to handle it within coroutines, 
find_interesting_subtree could recognize various cases where the 
TARGET_EXPR is used as an initializer, i.e. RHS of an INIT_EXPR, 
TARGET_EXPR_INITIAL, element of CONSTRUCTOR, and maybe add them to 
temps_used like you already do for an INIT_EXPR at the top of a co_await.

> thanks
> Iain
> 
> -- *< --
> 
> We usually need to 'promote' (i.e. save to the coroutine frame) any temporary
> variable that is in a target expression that must persist across an await
> expression.  However, if the TE is just used as a direct initializer for
> another object it will be elided - and we should not promote it since that
> would lead to a DTOR call for something that is never constructed.
> 
> Since we now have a mechanism to tell if TEs will be elided, use that.
> 
> Although the PRs referenced initially appear to be different issues, they all
> stem from this.
> 
> Co-Authored-By: Adrian Perl <adrian.perl@web.de>
> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
> 
> 	PR c++/100611
> 	PR c++/101367
> 	PR c++/101976
> 	PR c++/99576
> 
> gcc/cp/ChangeLog:
> 
> 	* coroutines.cc (find_interesting_subtree): Do not promote temporaries
> 	that are only used as direct initializers for some other object.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/coroutines/pr100611.C: New test.
> 	* g++.dg/coroutines/pr101367.C: New test.
> 	* g++.dg/coroutines/pr101976.C: New test.
> 	* g++.dg/coroutines/pr99576_1.C: New test.
> 	* g++.dg/coroutines/pr99576_2.C: New test.
> ---
>   gcc/cp/coroutines.cc                        |   1 +
>   gcc/testsuite/g++.dg/coroutines/pr100611.C  |  94 +++++++++++++++
>   gcc/testsuite/g++.dg/coroutines/pr101367.C  |  72 ++++++++++++
>   gcc/testsuite/g++.dg/coroutines/pr101976.C  |  78 ++++++++++++
>   gcc/testsuite/g++.dg/coroutines/pr99576_1.C | 124 ++++++++++++++++++++
>   gcc/testsuite/g++.dg/coroutines/pr99576_2.C |  72 ++++++++++++
>   6 files changed, 441 insertions(+)
>   create mode 100644 gcc/testsuite/g++.dg/coroutines/pr100611.C
>   create mode 100644 gcc/testsuite/g++.dg/coroutines/pr101367.C
>   create mode 100644 gcc/testsuite/g++.dg/coroutines/pr101976.C
>   create mode 100644 gcc/testsuite/g++.dg/coroutines/pr99576_1.C
>   create mode 100644 gcc/testsuite/g++.dg/coroutines/pr99576_2.C
> 
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index 01a3e831ee5..3f23317a315 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -2685,6 +2685,7 @@ find_interesting_subtree (tree *expr_p, int *dosub, void *d)
>   	}
>       }
>     else if (tmp_target_expr_p (expr)
> +	   && !TARGET_EXPR_ELIDING_P (expr)
>   	   && !p->temps_used->contains (expr))
>       {
>         p->entry = expr_p;
> diff --git a/gcc/testsuite/g++.dg/coroutines/pr100611.C b/gcc/testsuite/g++.dg/coroutines/pr100611.C
> new file mode 100644
> index 00000000000..14edf4870a1
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/pr100611.C
> @@ -0,0 +1,94 @@
> +// { dg-do run }
> +/*
> +  Test that instances created in capture clauses within co_await statements do not
> +  get 'promoted'. This would lead to the members destructor getting called more
> +  than once.
> +
> +  Correct output should look like:
> +  Foo(23) 0xf042d8
> +  Foo(const& 23) 0xf042ec
> +  ~Foo(23) 0xf042ec
> +  After co_await
> +  ~Foo(23) 0xf042d8
> +*/
> +#include <coroutine>
> +#include <iostream>
> +
> +static unsigned int struct_Foo_destructor_counter = 0;
> +static bool lambda_was_executed = false;
> +
> +class Task {
> +public:
> +  struct promise_type {
> +    Task get_return_object() {
> +      return {std::coroutine_handle<promise_type>::from_promise(*this)};
> +    }
> +
> +    std::suspend_never initial_suspend() { return {}; }
> +    std::suspend_always final_suspend() noexcept { return {}; }
> +    void unhandled_exception() {}
> +    void return_void() {}
> +  };
> +
> +  ~Task() {
> +    if (handle_) {
> +      handle_.destroy();
> +    }
> +  }
> +
> +  bool await_ready() { return false; }
> +  bool await_suspend(std::coroutine_handle<>) { return false; }
> +  bool await_resume() { return false; }
> +
> +private:
> +  Task(std::coroutine_handle<promise_type> handle) : handle_(handle) {}
> +
> +  std::coroutine_handle<promise_type> handle_;
> +};
> +
> +class Foo {
> +public:
> +  Foo(int id) : id_(id) {
> +    std::cout << "Foo(" << id_ << ") " << (void*)this << std::endl;
> +  }
> +
> +  Foo(Foo const& other) : id_(other.id_) {
> +    std::cout << "Foo(const& " << id_ << ") " << (void*)this << std::endl;
> +  }
> +
> +  Foo(Foo&& other) : id_(other.id_) {
> +    std::cout << "Foo(&& " << id_ << ") " << (void*)this << std::endl;
> +  }
> +
> +  ~Foo() {
> +    std::cout << "~Foo(" << id_ << ") " << (void*)this << std::endl;
> +    struct_Foo_destructor_counter++;
> +
> +    if (struct_Foo_destructor_counter > 2){
> +      std::cout << "Foo was destroyed more than two times!\n";
> +      __builtin_abort();
> +    }
> +    }
> +
> +private:
> +  int id_;
> +};
> +
> +Task test() {
> +  Foo foo(23);
> +
> +  co_await [foo]() -> Task { // A copy of foo is captured. This copy must not get 'promoted'.
> +    co_return;
> +  }();
> +
> +  std::cout << "After co_await\n";
> +  if (struct_Foo_destructor_counter == 0){
> +    std::cout << "The captured copy of foo was not destroyed after the co_await statement!\n";
> +    __builtin_abort();
> +  }
> +}
> +
> +int main() {
> +  test();
> +  return 0;
> +}
> diff --git a/gcc/testsuite/g++.dg/coroutines/pr101367.C b/gcc/testsuite/g++.dg/coroutines/pr101367.C
> new file mode 100644
> index 00000000000..0a9e5bee7d1
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/pr101367.C
> @@ -0,0 +1,72 @@
> +// { dg-do run }
> +
> +#include <coroutine>
> +using namespace std;
> +#include <cstdio>
> +#include <utility>
> +#include <string>
> +
> +struct resource {
> +    template<typename Func>
> +    resource(Func fn) { fn(); /*std::printf("resource()\n"); */}
> +    ~resource() { /*std::printf("~resource()\n"); */}
> +    resource(resource&&) = delete;
> +};
> +
> +template<typename T>
> +struct generator {
> +    struct promise_type {
> +        generator get_return_object() {
> +            return generator{coroutine_handle<promise_type>::from_promise(*this)};
> +        }
> +
> +        void return_void() {}
> +        void unhandled_exception() {}
> +        suspend_always initial_suspend() { return {}; }
> +        suspend_always final_suspend() noexcept { return {}; }
> +
> +        struct awaitable {
> +            resource& r;
> +
> +            awaitable(resource&& r) : r(r) {}
> +
> +            bool await_ready() noexcept { return false; }
> +
> +            void await_suspend(coroutine_handle<> h) noexcept {
> +                //std::printf("awaitable::await_suspend()\n");
> +            }
> +
> +            void await_resume() noexcept {
> +                //std::printf("awaitable::await_resume()\n");
> +            }
> +        };
> +
> +        awaitable yield_value(resource&& r) {
> +            return awaitable{std::move(r)};
> +        }
> +    };
> +
> +    generator(coroutine_handle<promise_type> coro) : coro(coro)
> +    {}
> +
> +    generator(generator&& g) noexcept : coro(std::exchange(g.coro, {}))
> +    {}
> +
> +    ~generator() {
> +        if (coro) { coro.destroy(); }
> +    }
> +
> +    coroutine_handle<promise_type> coro;
> +};
> +
> +generator<int> f() {
> +    std::string s;
> +    // if `s` isn't captured things work ok
> +    co_yield resource{[s]{}};
> +}
> +
> +int main() {
> +    generator x = f();
> +    x.coro.resume();
> +    x.coro.resume();
> +}
> diff --git a/gcc/testsuite/g++.dg/coroutines/pr101976.C b/gcc/testsuite/g++.dg/coroutines/pr101976.C
> new file mode 100644
> index 00000000000..1854ba001bb
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/pr101976.C
> @@ -0,0 +1,78 @@
> +// { dg-do run }
> +
> +/*
> +  Test that members of temporary instances in co_await statements do not get
> +  'promoted'. This would lead to the members destructor getting called more
> +  than once.
> +
> +  Correct output should look like:
> +  Before co_await
> +  nontrivial_move() 0x6ec2e1
> +  nontrivial_move(nontrivial_move&&) 0x6ed320
> +  In subtask
> +  ~nontrivial_move() 0x6ed320
> +  ~nontrivial_move() 0x6ec2e1
> +  After co_await
> +*/
> +#include <coroutine>
> +#include <iostream>
> +
> +static unsigned int struct_nontrivial_move_destructor_counter = 0;
> +
> +struct task {
> +  struct promise_type {
> +    task get_return_object() {
> +      return {std::coroutine_handle<promise_type>::from_promise(*this)};
> +    }
> +    std::suspend_never initial_suspend() { return {}; }
> +    std::suspend_never final_suspend() noexcept { return {}; }
> +    void unhandled_exception() {}
> +    void return_void() {}
> +  };
> +
> +  bool await_ready() { return true; }
> +  void await_suspend(std::coroutine_handle<>) {}
> +  void await_resume() {}
> +
> +  std::coroutine_handle<promise_type> m_handle;
> +};
> +
> +struct nontrivial_move {
> +  nontrivial_move() {
> +    std::cout << "nontrivial_move() " << (void *)this << std::endl;
> +  }
> +  nontrivial_move(nontrivial_move&&) {
> +    std::cout << "nontrivial_move(nontrivial_move&&) " << (void *)this
> +              << std::endl;
> +  }
> +  ~nontrivial_move() {
> +    std::cout << "~nontrivial_move() " << (void *)this << std::endl;
> +    struct_nontrivial_move_destructor_counter++;
> +    if (struct_nontrivial_move_destructor_counter > 2){
> +      std::cerr << "The destructor of nontrivial_move was called more than two times!\n";
> +      __builtin_abort();
> +    }
> +  }
> +
> +  char buf[128]{}; // Example why the move could be non trivial
> +};
> +
> +struct wrapper {
> +  nontrivial_move member;
> +};
> +
> +task subtask(wrapper /* unused */) {
> +  std::cout << "In subtask\n";
> +  co_return;
> +}
> +
> +task main_task() {
> +  std::cout << "Before co_await\n";
> +  co_await subtask({}); // wrapper must get 'promoted', but not its member
> +  std::cout << "After co_await\n";
> +}
> +
> +int main() {
> +  main_task();
> +  return 0;
> +}
> diff --git a/gcc/testsuite/g++.dg/coroutines/pr99576_1.C b/gcc/testsuite/g++.dg/coroutines/pr99576_1.C
> new file mode 100644
> index 00000000000..612f0cda2b1
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/pr99576_1.C
> @@ -0,0 +1,124 @@
> +// { dg-do run }
> +/*
> +  Test that instances created in capture clauses within co_await statements do not get
> +  'promoted'. This would lead to their members destructors getting called more
> +  than once.
> +
> +  Correct output should look like:
> +  START TASK
> +  Foo() 0x4f9320
> +  IN LAMBDA
> +  ~Foo() 0x4f9320
> +  TASK RETURN
> +*/
> +#include <coroutine>
> +#include <exception>
> +#include <iostream>
> +#include <utility>
> +
> +static unsigned int struct_Foo_destructor_counter = 0;
> +static bool lambda_was_executed = false;
> +
> +class Task {
> +public:
> +  struct promise_type {
> +    struct final_awaitable {
> +      bool await_ready() noexcept { return false; }
> +      auto await_suspend(std::coroutine_handle<promise_type> coro) noexcept {
> +        return coro.promise().continuation;
> +      }
> +      void await_resume() noexcept {}
> +    };
> +    Task get_return_object() {
> +      return Task(std::coroutine_handle<promise_type>::from_promise(*this));
> +    }
> +    std::suspend_always initial_suspend() { return {}; }
> +    final_awaitable final_suspend() noexcept { return {}; }
> +    void unhandled_exception() { std::terminate(); }
> +    void return_void() {}
> +
> +    std::coroutine_handle<void> continuation = std::noop_coroutine();
> +  };
> +
> +  Task(Task const&) = delete;
> +  Task(Task&& other) noexcept
> +      : handle_(std::exchange(other.handle_, nullptr)) {}
> +  Task& operator=(Task const&) = delete;
> +  Task& operator=(Task&& other) noexcept {
> +    handle_ = std::exchange(other.handle_, nullptr);
> +    return *this;
> +  }
> +  ~Task() {
> +    if (handle_) {
> +      handle_.destroy();
> +    }
> +  }
> +
> +  bool await_ready() const { return false; }
> +  auto await_suspend(std::coroutine_handle<void> continuation) {
> +    handle_.promise().continuation = continuation;
> +    return handle_;
> +  }
> +  void await_resume() {}
> +
> +private:
> +  explicit Task(std::coroutine_handle<promise_type> handle) : handle_(handle) {}
> +
> +  std::coroutine_handle<promise_type> handle_;
> +};
> +
> +struct RunTask {
> +  struct promise_type {
> +    RunTask get_return_object() { return {}; }
> +    std::suspend_never initial_suspend() { return {}; }
> +    std::suspend_never final_suspend() noexcept { return {}; }
> +    void return_void() {}
> +    void unhandled_exception() { std::terminate(); }
> +  };
> +};
> +
> +struct Foo {
> +  Foo() {
> +    std::cout << "Foo() " << (void *)this << std::endl;
> +  }
> +
> +  ~Foo() {
> +    std::cout << "~Foo() " << (void *)this << std::endl;
> +    struct_Foo_destructor_counter++;
> +
> +    if (struct_Foo_destructor_counter > 1 || !lambda_was_executed) {
> +      std::cout << "The destructor of Foo was called more than once or too early!\n";
> +      __builtin_abort();
> +    }
> +  }
> +
> +  Foo(Foo&&) = delete;
> +  Foo(Foo const&) = delete;
> +  Foo& operator=(Foo&&) = delete;
> +  Foo& operator=(Foo const&) = delete;
> +};
> +
> +Task DoAsync() {
> +  std::cout << "START TASK\n";
> +  co_await [foo = Foo{}]() -> Task { // foo is constructed inplace, no copy/move is performed.
> +                                     // foo itself must not get 'promoted'.
> +    std::cout << "IN LAMBDA\n";
> +    lambda_was_executed = true;
> +    co_return;
> +  }();
> +  // After the co_await statement the temporary lambda and foo
> +  // must now have been destroyed
> +  if (struct_Foo_destructor_counter == 0){
> +    std::cout << "foo was not destroyed after the co_await statement!\n";
> +    __builtin_abort();
> +  }
> +  std::cout << "TASK RETURN\n";
> +  co_return;
> +}
> +
> +RunTask Main() { co_await DoAsync(); }
> +
> +int main() {
> +  Main();
> +  return 0;
> +}
> diff --git a/gcc/testsuite/g++.dg/coroutines/pr99576_2.C b/gcc/testsuite/g++.dg/coroutines/pr99576_2.C
> new file mode 100644
> index 00000000000..b7371d64480
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/pr99576_2.C
> @@ -0,0 +1,72 @@
> +// { dg-do run }
> +/*
> +  Test that members of temporary awaitables in co_await statements do not get
> +  'promoted'. This would lead to the members destructor getting called more
> +  than once.
> +
> +  Correct output should look like:
> +  A 0x4f82d6
> +  ~A 0x4f82d6
> +*/
> +#include <coroutine>
> +#include <iostream>
> +
> +
> +static unsigned int struct_A_destructor_counter = 0;
> +
> +struct task : std::coroutine_handle<> {
> +  struct promise_type;
> +};
> +
> +struct task::promise_type {
> +  task get_return_object() {
> +    return {std::coroutine_handle<promise_type>::from_promise(*this)};
> +  }
> +  std::suspend_always initial_suspend() { return {}; }
> +  std::suspend_always final_suspend() noexcept { return {}; }
> +  void unhandled_exception() { std::terminate(); }
> +  void return_void() {}
> +};
> +
> +struct A {
> +  void log(const char *str) { std::cout << str << " " << (void *)this << std::endl; }
> +
> +  A() { log(__func__); }
> +
> +  ~A() {
> +    log(__func__);
> +    struct_A_destructor_counter++;
> +
> +    if (struct_A_destructor_counter > 1) {
> +      std::cout << "The destructor of A was called more than once!\n";
> +      __builtin_abort();
> +    }
> +  }
> +
> +  A(A&&) = delete;
> +  A(A const&) = delete;
> +  A& operator=(A&&) = delete;
> +  A& operator=(A const&) = delete;
> +};
> +
> +struct Awaitable {
> +  A a{}; // <- This member must NOT get 'promoted'
> +  bool await_ready() { return false; }
> +  void await_suspend(std::coroutine_handle<> handle) {}
> +  void await_resume() {}
> +};
> +
> +task coroutine() {
> +  co_await Awaitable{}; // <- This temporary must get 'promoted'
> +}
> +
> +int main() {
> +
> +  auto task = coroutine();
> +  while (!task.done()) {
> +    task();
> +  }
> +  task.destroy();
> +
> +  return 0;
> +}