From patchwork Mon Nov 28 11:55:51 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adrian Perl X-Patchwork-Id: 61152 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 404443857C51 for ; Mon, 28 Nov 2022 11:56:24 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 404443857C51 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1669636584; bh=n3p5wJFzeouHNruBrfLAIdfI+WAn6os/igpv5CCPpwk=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=NXWFr+K7a0eKNUPY6sOsL99O9jB0x/68qY95ObGqCeuPPoRdE0DRb8gAdXYHEe98R YpV+WhIaQAkdmFyAskIXVeZ6EQD17KbUBJBrCxJ0+yvO1mfwWzfqwXbK3NWXALW+Ak lKUxtE3yzkU6UpQiIbc8q5rbBEs8xU63PAuE8uuc= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org 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 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: To: gcc-patches@gcc.gnu.org Subject: [PATCH] coroutines: Fix promotion of class members in co_await statements [PR99576] 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== 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 X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Adrian Perl via Gcc-patches From: Adrian Perl Reply-To: Adrian Perl Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" 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 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 +#include + +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::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 == 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&&) = 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 = 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 = 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 = 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 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 = 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 continuation) { + handle_.promise().continuation = 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&&) = 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 +#include + + +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::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; +}