From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 44B753858D20 for ; Sat, 3 Dec 2022 20:19:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 44B753858D20 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1670098759; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=940mbk38Kyu7noUebUXl7PECq1HuYeSArBPUamcZHUQ=; b=hoUrSySwpvIysiz3ymDgizO59V73tMIjyro9ZrBfZds7AqkFvm9V37RziGbSVixWjZU8tG HBj2aWogX6UA+piXDUWT7BQ+S2R3+5faphtNd2lvDQVQxL8gajCR/QHSaTKq96//cbC/VE J4ynkWEq0dgeTw3Fdkc9bKN6SQyEutk= Received: from mail-qk1-f198.google.com (mail-qk1-f198.google.com [209.85.222.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-626-fv1kjG4UMM-7j0ULGOLJAg-1; Sat, 03 Dec 2022 15:19:18 -0500 X-MC-Unique: fv1kjG4UMM-7j0ULGOLJAg-1 Received: by mail-qk1-f198.google.com with SMTP id bp11-20020a05620a458b00b006fc8fa99f8eso10085522qkb.11 for ; Sat, 03 Dec 2022 12:19:18 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=940mbk38Kyu7noUebUXl7PECq1HuYeSArBPUamcZHUQ=; b=XxB2eAjROqcklvswvb1+Hbm7x14y1IiHv9ahS3+EH9Zpb7dtmOOurrI9Fin2EgTZmd mph6KavTYhnQ3m2RdtPCQZCcnaGiKHzw7bT3IG8KarLB5IhCzTebp5NZuQ5EycZKtMQd N4HSvmcFW2pjka8YXpk8Z5X29fcs8IKxs1/XNkkRmZ+9ulmGrM2XyZ4kC/+k4zntwDoN y41OzRLSNkUAnvFEDumalgHKujtPXrexnzHTRIOfkUT1O3AW598lPmxwEwAwnh0nkond 5Uit+L9uvwVZ5l7gKQfSz2oTWbAL5lj6jwHnCj78QKUMVlOzeQ026Ojma9b9cZGB0Ul2 5k0w== X-Gm-Message-State: ANoB5pneTIp8PVZO/zNXjW8zSVOrEtRuZBtXwYP619fKoh1fLFS+i46r Y4QGmX7Tim7wA5CjKwkE+UoCGDI3RellTrnyPE28nbHCyZ4LpH05dBGJGo/rJ5SJz+W4jePIvY5 rieZCoU7f1Etyxs0KmA== X-Received: by 2002:a05:622a:130c:b0:3a5:95d0:a51d with SMTP id v12-20020a05622a130c00b003a595d0a51dmr72548911qtk.348.1670098758151; Sat, 03 Dec 2022 12:19:18 -0800 (PST) X-Google-Smtp-Source: AA0mqf5YJpuOYWB3o+Gi+yi+wGC65lxJ5nKocGbcWwCb6st1puWiWxid8s2p9OB4EeB/5wxZVO6U0g== X-Received: by 2002:a05:622a:130c:b0:3a5:95d0:a51d with SMTP id v12-20020a05622a130c00b003a595d0a51dmr72548892qtk.348.1670098757729; Sat, 03 Dec 2022 12:19:17 -0800 (PST) Received: from [192.168.1.108] (130-44-159-43.s15913.c3-0.arl-cbr1.sbo-arl.ma.cable.rcncustomer.com. [130.44.159.43]) by smtp.gmail.com with ESMTPSA id w18-20020a05620a445200b006fafc111b12sm8853224qkp.83.2022.12.03.12.19.16 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 03 Dec 2022 12:19:16 -0800 (PST) Message-ID: <9968449f-06a3-338b-901c-282b73665e35@redhat.com> Date: Sat, 3 Dec 2022 15:19:15 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.5.1 Subject: Re: [PATCH] coroutines: Do not promote temporaries that will be elided. To: iain@sandoe.co.uk, gcc-patches@gcc.gnu.org Cc: adrian.perl@web.de References: <20221202202526.10504-1-iain@sandoe.co.uk> From: Jason Merrill In-Reply-To: <20221202202526.10504-1-iain@sandoe.co.uk> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,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: On 12/2/22 15:25, Iain Sandoe wrote: > 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). OK, glad the TARGET_EXPR_ELIDING_P work is being useful! > 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?) Backporting TARGET_EXPR_ELIDING_P doesn't seem feasible for 12. If you wanted to try to handle it within coroutines, find_interesting_subtree could recognize various cases where the TARGET_EXPR is used as an initializer, i.e. RHS of an INIT_EXPR, TARGET_EXPR_INITIAL, element of CONSTRUCTOR, and maybe add them to temps_used like you already do for an INIT_EXPR at the top of a co_await. > 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; > +}