From patchwork Fri Dec 2 20:25:26 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Iain Sandoe X-Patchwork-Id: 61401 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 999E13858C53 for ; Fri, 2 Dec 2022 20:26:02 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 999E13858C53 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1670012762; bh=z5mU6ScIZ/i8Gm/QD15cjIEDJVA5fkU7lBw9i6Rq/ek=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=SY6vcbr4ppRTNSB/qFkIDGuJsTKU/368NceIBx1GKPV5Xb7H5ilgqh5BjvMVr5z8c Zp2yz8xB8g93OnF+tK8U3iRp2doVJQRNGtLH/O83JiIVKQRYfGpCGsvWjU5tKeSfyl VQ/+BfrA+BkS2523ekiPU8c58TudZzZ3ai2399OA= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mail-wm1-x333.google.com (mail-wm1-x333.google.com [IPv6:2a00:1450:4864:20::333]) by sourceware.org (Postfix) with ESMTPS id 8D1E53858D20 for ; Fri, 2 Dec 2022 20:25:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8D1E53858D20 Received: by mail-wm1-x333.google.com with SMTP id 125-20020a1c0283000000b003d076ee89d6so4653827wmc.0 for ; Fri, 02 Dec 2022 12:25:32 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:reply-to:message-id:date :subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=z5mU6ScIZ/i8Gm/QD15cjIEDJVA5fkU7lBw9i6Rq/ek=; b=raGVNHVMEp9ygTt5L7ip77M/02tzr6uQtHWEz532jVOqxKjf2Z79ouERfBLYI0LgHV wCJKHtqjbHy0Y2FJVygrygtpk/1qEKVDIi/0hr+oAQuJDdnaw1P2cVrbkb0ZziZC0cB7 a/AYgfbTxkGvXdt5dQI0fN4K0Hgow28gUrbaYf8e1t/IHQaR99yk8NySKc0MSY9u3af7 RZHKjLybbXaUt9SxL+HV8B5CbaDTRb3FmgfXwVwoxYcIl48W66F+R5YPeKzMj1PX1RWW 8t37zjmXiqeHMa6WHx08dV7dqIqrZKE3/lX6Y4rUgrTLprNalRSzTeMi49qWlB+w1wHL W99g== X-Gm-Message-State: ANoB5pnBGuiaNYDRqMm5O54HqoIOEJgoGbBiyPbLWXu7JpVpTRd1Sh6Y IjiYYPeS/YbRa5Q6g9f0iT7r1ZDBoHA= X-Google-Smtp-Source: AA0mqf5WjBASLzBobMJSVZfuW9qC055zkGIYFoN1qL40DHFrvKrDbn1ogWZWs/6yjWDfB0EfT/a2FQ== X-Received: by 2002:a05:600c:4920:b0:3cf:8b23:549c with SMTP id f32-20020a05600c492000b003cf8b23549cmr44442034wmp.174.1670012731100; Fri, 02 Dec 2022 12:25:31 -0800 (PST) Received: from localhost.localdomain (host81-138-1-83.in-addr.btopenworld.com. [81.138.1.83]) by smtp.gmail.com with ESMTPSA id fn9-20020a05600c688900b003cf75213bb9sm12799616wmb.8.2022.12.02.12.25.30 (version=TLS1_3 cipher=TLS_CHACHA20_POLY1305_SHA256 bits=256/256); Fri, 02 Dec 2022 12:25:30 -0800 (PST) X-Google-Original-From: Iain Sandoe To: gcc-patches@gcc.gnu.org Subject: [PATCH] coroutines: Do not promote temporaries that will be elided. Date: Fri, 2 Dec 2022 20:25:26 +0000 Message-Id: <20221202202526.10504-1-iain@sandoe.co.uk> X-Mailer: git-send-email 2.37.1 (Apple Git-137.1) MIME-Version: 1.0 X-Spam-Status: No, score=-8.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, 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: Iain Sandoe via Gcc-patches From: Iain Sandoe Reply-To: iain@sandoe.co.uk Cc: Iain Sandoe , jason@redhat.com, adrian.perl@web.de Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" Thanks to Adrian for working on this and producing the revised test-cases. This has been tested on x86_64-darwin21 with our testsuite, cppcoro and a patched version of folly (that enables the experimental coroutines tests) without regresssion (actually a handful of progressions on folly, plus the fixes to our suite). I am not sure how best to backport this; I think we want it at least on 12 and preferably earlier, but the TARGET_EXPR_ELIDING_P flag is not available there, perhaps we can construct some local function in coroutines.cc that emulates it? (or maybe the TARGET_EXPR_ELIDING_P and associated fixes is suitable for backport?) thanks Iain -- *< -- We usually need to 'promote' (i.e. save to the coroutine frame) any temporary variable that is in a target expression that must persist across an await expression. However, if the TE is just used as a direct initializer for another object it will be elided - and we should not promote it since that would lead to a DTOR call for something that is never constructed. Since we now have a mechanism to tell if TEs will be elided, use that. Although the PRs referenced initially appear to be different issues, they all stem from this. Co-Authored-By: Adrian Perl Signed-off-by: Iain Sandoe PR c++/100611 PR c++/101367 PR c++/101976 PR c++/99576 gcc/cp/ChangeLog: * coroutines.cc (find_interesting_subtree): Do not promote temporaries that are only used as direct initializers for some other object. 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 | 1 + gcc/testsuite/g++.dg/coroutines/pr100611.C | 94 +++++++++++++++ gcc/testsuite/g++.dg/coroutines/pr101367.C | 72 ++++++++++++ gcc/testsuite/g++.dg/coroutines/pr101976.C | 78 ++++++++++++ gcc/testsuite/g++.dg/coroutines/pr99576_1.C | 124 ++++++++++++++++++++ gcc/testsuite/g++.dg/coroutines/pr99576_2.C | 72 ++++++++++++ 6 files changed, 441 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..3f23317a315 100644 --- a/gcc/cp/coroutines.cc +++ b/gcc/cp/coroutines.cc @@ -2685,6 +2685,7 @@ find_interesting_subtree (tree *expr_p, int *dosub, void *d) } } else if (tmp_target_expr_p (expr) + && !TARGET_EXPR_ELIDING_P (expr) && !p->temps_used->contains (expr)) { p->entry = expr_p; diff --git a/gcc/testsuite/g++.dg/coroutines/pr100611.C b/gcc/testsuite/g++.dg/coroutines/pr100611.C new file mode 100644 index 00000000000..14edf4870a1 --- /dev/null +++ b/gcc/testsuite/g++.dg/coroutines/pr100611.C @@ -0,0 +1,94 @@ +// { dg-do run } +/* + 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..0a9e5bee7d1 --- /dev/null +++ b/gcc/testsuite/g++.dg/coroutines/pr101367.C @@ -0,0 +1,72 @@ +// { dg-do run } + +#include +using namespace std; +#include +#include +#include + +struct resource { + template + resource(Func fn) { fn(); /*std::printf("resource()\n"); */} + ~resource() { /*std::printf("~resource()\n"); */} + resource(resource&&) = delete; +}; + +template +struct generator { + struct promise_type { + generator get_return_object() { + return generator{coroutine_handle::from_promise(*this)}; + } + + void return_void() {} + void unhandled_exception() {} + suspend_always initial_suspend() { return {}; } + suspend_always final_suspend() noexcept { return {}; } + + struct awaitable { + resource& r; + + awaitable(resource&& r) : r(r) {} + + bool await_ready() noexcept { return false; } + + void await_suspend(coroutine_handle<> h) noexcept { + //std::printf("awaitable::await_suspend()\n"); + } + + void await_resume() noexcept { + //std::printf("awaitable::await_resume()\n"); + } + }; + + awaitable yield_value(resource&& r) { + return awaitable{std::move(r)}; + } + }; + + generator(coroutine_handle coro) : coro(coro) + {} + + generator(generator&& g) noexcept : coro(std::exchange(g.coro, {})) + {} + + ~generator() { + if (coro) { coro.destroy(); } + } + + coroutine_handle coro; +}; + +generator f() { + std::string s; + // if `s` isn't captured things work ok + co_yield resource{[s]{}}; +} + +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..1854ba001bb --- /dev/null +++ b/gcc/testsuite/g++.dg/coroutines/pr101976.C @@ -0,0 +1,78 @@ +// { dg-do run } + +/* + 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..612f0cda2b1 --- /dev/null +++ b/gcc/testsuite/g++.dg/coroutines/pr99576_1.C @@ -0,0 +1,124 @@ +// { dg-do run } +/* + 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..b7371d64480 --- /dev/null +++ b/gcc/testsuite/g++.dg/coroutines/pr99576_2.C @@ -0,0 +1,72 @@ +// { dg-do run } +/* + 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; +}