Re: [PATCH] coroutines: Fix promotion of class members in co_await statements [PR99576]
On 11/30/22 03:51, Iain Sandoe wrote: Hi Adrian, On 28 Nov 2022, at 20:44, Iain Sandoe wrote: Bootstrapping and running the testsuite on x86_64 was successfull. No regression occured. This looks resonable to me, as said in the PR. I’d like to test a little wider with some larger codebases, if you could bear with me for a few days. So wider testing (in this case folly) reveals that, although the analysis seems reasonable, this is not quite the right patch to fix the issue. It can be that CONSTRUCTORS contain nested await expressions, so we cannot simply punt on seeing one. My hunch is that the real solution lies in (correctly) deciding whether to promote the temporary or not. Jason recently made a change that identifies whether a target expression is expected to be elided (i.e. it is a direct intializer for another object) - I think this might help in this case. My concern is whether I should read “expected to be elided” to be a guarantee (“expected” to me could also be read “but it might not be”). You should be able to rely on that flag. I believe all TARGET_EXPRs with TARGET_EXPR_ELIDING_P set are indeed elided. As an optimization, occasionally TARGET_EXPRs without the flag are elided anyway, but it's still safe to promote them; the optimization just won't happen then. Jason
Re: [PATCH] coroutines: Fix promotion of class members in co_await statements [PR99576]
Hi Adrian, > On 28 Nov 2022, at 20:44, Iain Sandoe wrote: >> Bootstrapping and running the testsuite on x86_64 was successfull. No >> regression occured. > > This looks resonable to me, as said in the PR. I’d like to test a little > wider with some larger > codebases, if you could bear with me for a few days. So wider testing (in this case folly) reveals that, although the analysis seems reasonable, this is not quite the right patch to fix the issue. It can be that CONSTRUCTORS contain nested await expressions, so we cannot simply punt on seeing one. My hunch is that the real solution lies in (correctly) deciding whether to promote the temporary or not. Jason recently made a change that identifies whether a target expression is expected to be elided (i.e. it is a direct intializer for another object) - I think this might help in this case. My concern is whether I should read “expected to be elided” to be a guarantee (“expected” to me could also be read “but it might not be”). As is, the patch is not OK since it regresses cases with nested await expressions in CONSTRUCTORS. sorry for not spotting that sooner, Iain
Re: [PATCH] coroutines: Fix promotion of class members in co_await statements [PR99576]
Hi Adrian,
Again thanks for working on this and getting it into a suitable state for
posting.
It’s a good idea to CC relevant maintainer(s) on patches [see MAINTAINERS] and
I’d welcome cc on any coroutines ones (it’s easy to miss a patch otherwise).
> On 28 Nov 2022, at 11:55, Adrian Perl via Gcc-patches
> wrote:
> 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.
This looks resonable to me, as said in the PR. I’d like to test a little wider
with some larger
codebases, if you could bear with me for a few days.
I think this patch is small enough to accept without any copyright machinery,
but (for reference and
in the hope that larger patches might be coming) .. at some stage, you’d need
to do one of:
1. get an FSF copyright assignment
2. release your patch under the DCO.
See: “Legal Prerequisites” here https://gcc.gnu.org/contribute.html.
thanks again for the patch
Iain
> 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
>
> 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 000..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; }
>
[PATCH] coroutines: Fix promotion of class members in co_await statements [PR99576]
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
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 000..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;
+ }();
+
+
