From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp002.apm-internet.net (smtp002.apm-internet.net [85.119.248.221]) by sourceware.org (Postfix) with ESMTPS id 3E6BE3854568 for ; Mon, 28 Nov 2022 20:44:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 3E6BE3854568 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=sandoe.co.uk Authentication-Results: sourceware.org; spf=none smtp.mailfrom=sandoe.co.uk Received: (qmail 83119 invoked from network); 28 Nov 2022 20:44:40 -0000 X-APM-Out-ID: 16696682808311 X-APM-Authkey: 257869/1(257869/1) 11 Received: from unknown (HELO smtpclient.apple) (81.138.1.83) by smtp002.apm-internet.net with SMTP; 28 Nov 2022 20:44:40 -0000 Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.120.41.1.1\)) Subject: Re: [PATCH] coroutines: Fix promotion of class members in co_await statements [PR99576] From: Iain Sandoe In-Reply-To: Date: Mon, 28 Nov 2022 20:44:40 +0000 Cc: GCC Patches , Jason Merrill Content-Transfer-Encoding: quoted-printable Message-Id: References: To: Adrian Perl X-Mailer: Apple Mail (2.3696.120.41.1.1) X-Spam-Status: No, score=-14.2 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_COUK,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,KAM_SHORT,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hi Adrian, Again thanks for working on this and getting it into a suitable state = for posting. It=E2=80=99s a good idea to CC relevant maintainer(s) on patches [see = MAINTAINERS] and I=E2=80=99d welcome cc on any coroutines ones (it=E2=80=99= s easy to miss a patch otherwise). > On 28 Nov 2022, at 11:55, Adrian Perl via Gcc-patches = 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. >=20 > To summarize my post in = https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D99576#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. >=20 > 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). >=20 > 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. >=20 > Bootstrapping and running the testsuite on x86_64 was successfull. No = regression occured. This looks resonable to me, as said in the PR. I=E2=80=99d 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=E2=80=99d need to do one of: 1. get an FSF copyright assignment 2. release your patch under the DCO. See: =E2=80=9CLegal Prerequisites=E2=80=9D here = https://gcc.gnu.org/contribute.html. thanks again for the patch Iain > Please let me know if you need more information. >=20 > PR 100611 > PR 101367 > PR 101976 > PR 99576 >=20 > gcc/cp/ChangeLog: >=20 > * coroutines.cc (find_interesting_subtree): Prevent = recursion into constructor >=20 > gcc/testsuite/ChangeLog: >=20 > * 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. >=20 > --- > 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 >=20 > 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) =3D=3D CONSTRUCTOR) > + *dosub =3D 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 > +#include > + > +static unsigned int struct_Foo_destructor_counter =3D 0; > +static bool lambda_was_executed =3D false; > + > +class Task { > +public: > + struct promise_type { > + Task get_return_object() { > + return = {std::coroutine_handle::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 handle) : handle_(handle) = {} > + > + std::coroutine_handle 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 =3D=3D 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 > +#include > +#include > + > +struct resource { > + template resource(Func fn) { > + fn(); > + std::cout << "resource() " << (void *)this << std::endl; > + } > + ~resource() { std::cout << "~resource() " << (void *)this << = std::endl; } > + resource(resource&&) =3D delete; > +}; > + > +template struct generator { > + struct promise_type { > + generator get_return_object() { > + return generator{ > + std::coroutine_handle::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 coro) : coro(coro) {} > + > + generator(generator&& g) noexcept : coro(std::exchange(g.coro, {})) = {} > + > + ~generator() { > + if (coro) { > + coro.destroy(); > + } > + } > + > + std::coroutine_handle coro; > +}; > + > +generator 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 =3D 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 > +#include > + > +static unsigned int struct_nontrivial_move_destructor_counter =3D 0; > + > +struct task { > + struct promise_type { > + task get_return_object() { > + return = {std::coroutine_handle::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 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 > +#include > +#include > +#include > + > +static unsigned int struct_Foo_destructor_counter =3D 0; > +static bool lambda_was_executed =3D false; > + > +class Task { > +public: > + struct promise_type { > + struct final_awaitable { > + bool await_ready() noexcept { return false; } > + auto await_suspend(std::coroutine_handle coro) = noexcept { > + return coro.promise().continuation; > + } > + void await_resume() noexcept {} > + }; > + Task get_return_object() { > + return = Task(std::coroutine_handle::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 continuation =3D = std::noop_coroutine(); > + }; > + > + Task(Task const&) =3D delete; > + Task(Task&& other) noexcept > + : handle_(std::exchange(other.handle_, nullptr)) {} > + Task& operator=3D(Task const&) =3D delete; > + Task& operator=3D(Task&& other) noexcept { > + handle_ =3D std::exchange(other.handle_, nullptr); > + return *this; > + } > + ~Task() { > + if (handle_) { > + handle_.destroy(); > + } > + } > + > + bool await_ready() const { return false; } > + auto await_suspend(std::coroutine_handle continuation) { > + handle_.promise().continuation =3D continuation; > + return handle_; > + } > + void await_resume() {} > + > +private: > + explicit Task(std::coroutine_handle handle) : = handle_(handle) {} > + > + std::coroutine_handle 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&&) =3D delete; > + Foo(Foo const&) =3D delete; > + Foo& operator=3D(Foo&&) =3D delete; > + Foo& operator=3D(Foo const&) =3D delete; > +}; > + > +Task DoAsync() { > + std::cout << "START TASK\n"; > + co_await [foo =3D 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 =3D true; > + co_return; > + }(); > + // After the co_await statement the temporary lambda and foo > + // must now have been destroyed > + if (struct_Foo_destructor_counter =3D=3D 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 > +#include > + > + > +static unsigned int struct_A_destructor_counter =3D 0; > + > +struct task : std::coroutine_handle<> { > + struct promise_type; > +}; > + > +struct task::promise_type { > + task get_return_object() { > + return = {std::coroutine_handle::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&&) =3D delete; > + A(A const&) =3D delete; > + A& operator=3D(A&&) =3D delete; > + A& operator=3D(A const&) =3D 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 =3D coroutine(); > + while (!task.done()) { > + task(); > + } > + task.destroy(); > + > + return 0; > +} > -- > 2.34.1