From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mout.web.de (mout.web.de [212.227.15.4]) by sourceware.org (Postfix) with ESMTPS id 57C5F3858C62 for ; Mon, 28 Nov 2022 11:55:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 57C5F3858C62 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=web.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=web.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=web.de; s=s29768273; t=1669636551; bh=KOz9tkMy78WKRiVSd6rSla5aeSDV+ULeBB5xfqhzpzM=; h=X-UI-Sender-Class:From:To:Subject:Date; b=lHq0ng4/5m3ppQt1zHC/6PhlPwGAC0M5QMintAms6W4Acoq76894z5tKVMIvav8CD hYhqUnsXWLXNjOGJhItjWQX+ciZeVTtZZ/BZZQL+5qdp0zm8QVw5I2NVbKaTEPdR6r CCSQcmY6yNjbpjyjodsgXYHZUBcGBjYDiLoj0fu0WNKr8SyGfu/LIRi+7PVK1Cjijy h/KkhXidMOW5BjoQRUwlctFUIjiKxC0Upg9lnpcqDZ6Ya2sK2h3XxlmD+T/6xGpwvI 1PTBjHGUry0ITGdqMmnBn+2oEKYOw+x/PEOgWmz33xw28/py89Lt3EUcjl4oOVYBHP vIJIlcejoOGiw== X-UI-Sender-Class: 814a7b36-bfc1-4dae-8640-3722d8ec6cd6 Received: from [188.192.243.161] ([188.192.243.161]) by web-mail.web.de (3c-app-webde-bs19.server.lan [172.19.170.19]) (via HTTP); Mon, 28 Nov 2022 12:55:51 +0100 MIME-Version: 1.0 Message-ID: From: Adrian Perl To: gcc-patches@gcc.gnu.org Subject: [PATCH] coroutines: Fix promotion of class members in co_await statements [PR99576] Content-Type: text/plain; charset=UTF-8 Date: Mon, 28 Nov 2022 12:55:51 +0100 Importance: normal Sensitivity: Normal X-Priority: 3 X-Provags-ID: V03:K1:Swge9UxCz3+LqNhg39DH+SsDiiPGHULNRYnUZGdNXBc6Gq7Cieb6PLr9mLtv4336Kl9Ml +4OT1b5D9rpKtghGkrIBCJ6fWSr6aJqFP/xUSIhbEkaO4o3yoWa9u6tNsee0F6guImMUDwN7D/6a KAVqM+wsZWTauFx0cej+lScgiGGPpE8Bltg9XJANCj+dS2Of59NAO9Zqturo6focfiWUJqpLmRZL YydfINB4rZDMDcecRzOtddyP5t1yafwl0YlTLZZhSosLgTpTciK+c3akVVUBhlSvzJS7xQvYYhQy dE= UI-OutboundReport: notjunk:1;M01:P0:E9LmEjzi7Yo=;usq0JaI1mm99ChUhNZ3wuB+0Wkd 0U6VPgtNQxhkcgvAvHA1g2JDDrR0HuA4vEohAo72WIvAA9uOAZnc5STj4Ugnke/lOCIy8JCB3 ixzuU8OtARhLQFCTFh0aTRvP11DZex47hjz7RSAaWBvR1OGO/s6OYtkf1tsGeaP62heSxm+cm khJchbwqBXamqD8mJPmzMdMvFgP83pF/Bdg+QPrf9LeuODN4E/t+yCwHswphyJZ+zFZjbmQ7f qRZh7c+k334WIV+JdNzfGnjxXEUmqhrqa9tJN+WDmSSv8/XimB0WLfgmYAjAH+ezQNQcv9IBZ xPiHIwXMgCTZNl1i3pNlhJYSbuA21AeQQiPurzHQ7zmgmQm/RgPv84+6PoAbWpg1qNjh4xCQJ gNciCUylp1YF6Wfg9FCDuBvIboIoniXSvBD19X1v51TDgCLZQ95VSriCWqFGzGx64jycK/91T ad1VwKtbBQY/od4wMep06qLov5RaAY2FxO6FRHBXH9am9AcxsW553dLVRjMoQJcDygInRRr3O GYm0Ugif5EY8aIGCMsjjtS2gmq7Mf5OzGFJlOx4f6NUlta+Cqnahf/zrvBULEezsFjY8jwhOK 55lUgsk50pXWPaO0CEUSCbA5dYso0YViMGmh86fBH1dkUZq1hhYdFgcyZAcvtjfzRKLwIivSD kRDnC4QNcYMUMozncGjz9T3YeW9BvPU7wS4QQkOzlk+F3AzZ6Gr6WalUW6Igp0vLWQvyVt4zC G9ZiGnmQUwU6JIL5OngXN+Lz6AzF4jN0GRqcGZVz2Cg9SSvuF+BWsTPqJeDPWo6EHcXUz+CMi qU/uaDFtCK+RiMVviosicjXQ== Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-14.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_LOW,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_PASS,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, please have a look at the patch below for a potential fix that addresses t= he 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=3D995= 76#c5, the recursive promotion of temporaries is too eager and also recurses into constructor s= tatements where it finds the initialization of members. This patch prevents the recursion int= o 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 cal= ls). 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 basicall= y quiet similar. The main difference is how the temporaries are used in the co_await (and c= o_yield) statements. Bootstrapping and running the testsuite on x86_64 was successfull. No regr= ession 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. =2D-- 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 =2D-- 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 =2D-- /dev/null +++ b/gcc/testsuite/g++.dg/coroutines/pr100611.C @@ -0,0 +1,93 @@ +/* + Test that instances created in capture clauses within co_await statemen= ts do not + get 'promoted'. This would lead to the members destructor getting calle= d 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 =2D-- /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 'promote= d'. + + 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 promo= ted. + // If it gets promoted, the string will be f= reed + // 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 =2D-- /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 mo= re + 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 tha= n 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 membe= r + 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 =2D-- /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 statemen= ts 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) noexce= pt { + 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_(han= dle) {} + + 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 to= o 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 =2D-- /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 mo= re + 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; +} =2D- 2.34.1