coroutines: Fix promotion of class members in co_await statements [PR99576]

  Hi,

please have a look at the patch below for a potential fix that addresses the incorrect 'promotion'
of members found in temporaries within co_await statements.

To summarize my post in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99576#c5, the recursive
promotion of temporaries is too eager and also recurses into constructor statements where it
finds the initialization of members. This patch prevents the recursion into constructors and
skips the remaining subtree.

It fixes the issues in bug reports [PR99576, PR100611, PR101976, PR101367, PR107288] which are all
related to incorrect 'promotions' (and manifest in too many destructor calls).

I have added test applications based on examples in the PRs, which I have only slightly
annotated and refactored to allow automatic testing. They are all basically quiet similar.
The main difference is how the temporaries are used in the co_await (and co_yield) statements.

Bootstrapping and running the testsuite on x86_64 was successfull. No regression occured.

Please let me know if you need more information.

            PR 100611
            PR 101367
            PR 101976
            PR 99576

    gcc/cp/ChangeLog:

            * coroutines.cc (find_interesting_subtree): Prevent recursion into constructor

    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                        |   2 +
 gcc/testsuite/g++.dg/coroutines/pr100611.C  |  93 +++++++++++++++
 gcc/testsuite/g++.dg/coroutines/pr101367.C  |  78 +++++++++++++
 gcc/testsuite/g++.dg/coroutines/pr101976.C  |  76 ++++++++++++
 gcc/testsuite/g++.dg/coroutines/pr99576_1.C | 123 ++++++++++++++++++++
 gcc/testsuite/g++.dg/coroutines/pr99576_2.C |  71 +++++++++++
 6 files changed, 443 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

--
2.34.1
  

Hi Adrian,

Again thanks for working on this and getting it into a suitable state for posting.
It’s a good idea to CC relevant maintainer(s) on patches [see MAINTAINERS] and I’d welcome cc on any coroutines ones (it’s easy to miss a patch otherwise).

> On 28 Nov 2022, at 11:55, Adrian Perl via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:

> please have a look at the patch below for a potential fix that addresses the incorrect 'promotion'
> of members found in temporaries within co_await statements.
> 
> To summarize my post in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99576#c5, the recursive
> promotion of temporaries is too eager and also recurses into constructor statements where it
> finds the initialization of members. This patch prevents the recursion into constructors and
> skips the remaining subtree.
> 
> It fixes the issues in bug reports [PR99576, PR100611, PR101976, PR101367, PR107288] which are all
> related to incorrect 'promotions' (and manifest in too many destructor calls).
> 
> I have added test applications based on examples in the PRs, which I have only slightly
> annotated and refactored to allow automatic testing. They are all basically quiet similar.
> The main difference is how the temporaries are used in the co_await (and co_yield) statements.
> 
> Bootstrapping and running the testsuite on x86_64 was successfull. No regression occured.

This looks resonable to me, as said in the PR.  I’d like to test a little wider with some larger
codebases, if you could bear with me for a few days.

I think this patch is small enough to accept without any copyright machinery, but (for reference and
in the hope that larger patches might be coming) .. at some stage, you’d need to do one of:

1. get an FSF copyright assignment
2. release your patch under the DCO.

See: “Legal Prerequisites” here https://gcc.gnu.org/contribute.html.

thanks again for the patch
Iain

> Please let me know if you need more information.
> 
>            PR 100611
>            PR 101367
>            PR 101976
>            PR 99576
> 
>    gcc/cp/ChangeLog:
> 
>            * coroutines.cc (find_interesting_subtree): Prevent recursion into constructor
> 
>    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                        |   2 +
> gcc/testsuite/g++.dg/coroutines/pr100611.C  |  93 +++++++++++++++
> gcc/testsuite/g++.dg/coroutines/pr101367.C  |  78 +++++++++++++
> gcc/testsuite/g++.dg/coroutines/pr101976.C  |  76 ++++++++++++
> gcc/testsuite/g++.dg/coroutines/pr99576_1.C | 123 ++++++++++++++++++++
> gcc/testsuite/g++.dg/coroutines/pr99576_2.C |  71 +++++++++++
> 6 files changed, 443 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..a87ea7fe60a 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -2684,6 +2684,8 @@ find_interesting_subtree (tree *expr_p, int *dosub, void *d)
> 	  return expr;
> 	}
>     }
> +  else if (TREE_CODE(expr) == CONSTRUCTOR)
> +    *dosub = 0; /* We don't need to consider this any further.  */
>   else if (tmp_target_expr_p (expr)
> 	   && !p->temps_used->contains (expr))
>     {
> diff --git a/gcc/testsuite/g++.dg/coroutines/pr100611.C b/gcc/testsuite/g++.dg/coroutines/pr100611.C
> new file mode 100644
> index 00000000000..5fbcfa7e6ec
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/pr100611.C
> @@ -0,0 +1,93 @@
> +/*
> +  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..694e1e7783b
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/pr101367.C
> @@ -0,0 +1,78 @@
> +/*
> +  Test that captured instances in co_yield statements do not get 'promoted'.
> +
> +  Correct output should look like:
> +  resource() 0x18c8300
> +  awaitable::await_suspend() 0x18c82f8
> +  awaitable::await_resume() 0x18c82f8
> +  ~resource() 0x18c8300
> +*/
> +#include <coroutine>
> +#include <iostream>
> +#include <utility>
> +
> +struct resource {
> +  template <typename Func> resource(Func fn) {
> +    fn();
> +    std::cout << "resource() " << (void *)this << std::endl;
> +  }
> +  ~resource() { std::cout << "~resource() " << (void *)this << std::endl; }
> +  resource(resource&&) = delete;
> +};
> +
> +template <typename T> struct generator {
> +  struct promise_type {
> +    generator get_return_object() {
> +      return generator{
> +          std::coroutine_handle<promise_type>::from_promise(*this)};
> +    }
> +
> +    void return_void() {}
> +    void unhandled_exception() {}
> +    std::suspend_always initial_suspend() { return {}; }
> +    std::suspend_always final_suspend() noexcept { return {}; }
> +
> +    struct awaitable {
> +      resource &r;
> +
> +      awaitable(resource&& r) : r(r) {}
> +
> +      bool await_ready() noexcept { return false; }
> +
> +      void await_suspend(std::coroutine_handle<> h) noexcept {
> +        std::cout << "awaitable::await_suspend() " << (void *)this << std::endl;
> +      }
> +
> +      void await_resume() noexcept {
> +        std::cout << "awaitable::await_resume() " << (void *)this << std::endl;
> +      }
> +    };
> +
> +    awaitable yield_value(resource&& r) { return awaitable{std::move(r)}; }
> +  };
> +
> +  generator(std::coroutine_handle<promise_type> coro) : coro(coro) {}
> +
> +  generator(generator&& g) noexcept : coro(std::exchange(g.coro, {})) {}
> +
> +  ~generator() {
> +    if (coro) {
> +      coro.destroy();
> +    }
> +  }
> +
> +  std::coroutine_handle<promise_type> coro;
> +};
> +
> +generator<int> f() {
> +  std::string s;
> +  co_yield resource{[s] {}}; // the captured copy of s must not get promoted.
> +                             // If it gets promoted, the string will be freed
> +                             // twice which leads to an abort
> +}
> +
> +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..27453380b1b
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/pr101976.C
> @@ -0,0 +1,76 @@
> +/*
> +  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..c5fa3a46ee5
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/pr99576_1.C
> @@ -0,0 +1,123 @@
> +/*
> +  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..854941c5f73
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/pr99576_2.C
> @@ -0,0 +1,71 @@
> +/*
> +  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;
> +}
> --
> 2.34.1
  

Hi Adrian,

> On 28 Nov 2022, at 20:44, Iain Sandoe <iain@sandoe.co.uk> wrote:

>> Bootstrapping and running the testsuite on x86_64 was successfull. No regression occured.
> 
> This looks resonable to me, as said in the PR.  I’d like to test a little wider with some larger
> codebases, if you could bear with me for a few days.

So wider testing (in this case folly) reveals that, although the analysis seems reasonable, this is not quite the right patch to fix the issue.  It can be that CONSTRUCTORS contain nested await expressions, so we cannot simply punt on seeing one.

My hunch is that the real solution lies in (correctly) deciding whether to promote the temporary or not.  Jason recently made a change that identifies whether a target expression is expected to be elided (i.e. it is a direct intializer for another object) - I think this might help in this case.  My concern is whether I should read “expected to be elided” to be a guarantee (“expected” to me could also be read “but it might not be”).

As is, the patch is not OK since it regresses cases with nested await expressions in CONSTRUCTORS.
sorry for not spotting that sooner,

Iain
  

On 11/30/22 03:51, Iain Sandoe wrote:
> Hi Adrian,
> 
>> On 28 Nov 2022, at 20:44, Iain Sandoe <iain@sandoe.co.uk> wrote:
> 
>>> Bootstrapping and running the testsuite on x86_64 was successfull. No regression occured.
>>
>> This looks resonable to me, as said in the PR.  I’d like to test a little wider with some larger
>> codebases, if you could bear with me for a few days.
> 
> So wider testing (in this case folly) reveals that, although the analysis seems reasonable, this is not quite the right patch to fix the issue.  It can be that CONSTRUCTORS contain nested await expressions, so we cannot simply punt on seeing one.
> 
> My hunch is that the real solution lies in (correctly) deciding whether to promote the temporary or not.  Jason recently made a change that identifies whether a target expression is expected to be elided (i.e. it is a direct intializer for another object) - I think this might help in this case.  My concern is whether I should read “expected to be elided” to be a guarantee (“expected” to me could also be read “but it might not be”).

You should be able to rely on that flag.  I believe all TARGET_EXPRs 
with TARGET_EXPR_ELIDING_P set are indeed elided.  As an optimization, 
occasionally TARGET_EXPRs without the flag are elided anyway, but it's 
still safe to promote them; the optimization just won't happen then.

Jason