Re: [PATCH] coroutines: Do not promote temporaries that will be elided.
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 000..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) :
[PATCH] coroutines: Do not promote temporaries that will be elided.
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 000..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_;
