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
