Hi Jason,
>>>Moving this initialization doesn't seem connected to the type of gro, or
>>>mentioned above?
>>A fly-by tidy up.. removed.
>>I still see it in the new patch?
Apologies, I obviously fat-fingered something - done now.
>>...return object from an object that has already been destroyed.
>This message doesn't quote my comment
I added that now - with the folly part elided since we understand that is
not affected by this.
>>+ = finish_decltype_type (get_ro, /
>>*id_expression_or_member_access_p*/false,
>>+ tf_warning_or_error); // TREE_TYPE (get_ro);
>Let's drop this comment, it looks vestigial.
Indeed, done.
>So please xfail the test than test for the defect.
I have done this and added a header comment explaining why we expect it
to fail.
OK for trunk now?
thanks
Iain
--- 8< ---
The revised wording for coroutines, uses decltype(auto) for the
type of the get return object, which preserves references.
It is quite reasonable for a coroutine body implementation to
complete before control is returned to the ramp - and in that
case we would be creating the ramp return object from an already-
deleted promise object.
Jason observes that this is a terrible situation and we should
seek a resolution to it via core.
Since the test added here explicitly performs the unsafe action
dscribed above we expect it to fail (until a resolution is found).
gcc/cp/ChangeLog:
* coroutines.cc
(cp_coroutine_transform::build_ramp_function): Use
decltype(auto) to determine the type of the temporary
get_return_object.
gcc/testsuite/ChangeLog:
* g++.dg/coroutines/pr115908.C: Count promise construction
and destruction. Run the test and XFAIL it.
Signed-off-by: Iain Sandoe <i...@sandoe.co.uk>
---
gcc/cp/coroutines.cc | 12 ++-
gcc/testsuite/g++.dg/coroutines/pr115908.C | 86 ++++++++++++++++------
2 files changed, 72 insertions(+), 26 deletions(-)
diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index bc5fb9381db..5c4133a42b7 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -5120,8 +5120,11 @@ cp_coroutine_transform::build_ramp_function ()
/* Check for a bad get return object type.
[dcl.fct.def.coroutine] / 7 requires:
The expression promise.get_return_object() is used to initialize the
- returned reference or prvalue result object ... */
- tree gro_type = TREE_TYPE (get_ro);
+ returned reference or prvalue result object ...
+ When we use a local to hold this, it is decltype(auto). */
+ tree gro_type
+ = finish_decltype_type (get_ro, /*id_expression_or_member_access_p*/false,
+ tf_warning_or_error);
if (VOID_TYPE_P (gro_type) && !void_ramp_p)
{
error_at (fn_start, "no viable conversion from %<void%> provided by"
@@ -5159,7 +5162,7 @@ cp_coroutine_transform::build_ramp_function ()
= coro_build_and_push_artificial_var (loc, "_Coro_gro", gro_type,
orig_fn_decl, NULL_TREE);
- r = cp_build_init_expr (coro_gro, get_ro);
+ r = cp_build_init_expr (coro_gro, STRIP_REFERENCE_REF (get_ro));
finish_expr_stmt (r);
tree coro_gro_cleanup
= cxx_maybe_build_cleanup (coro_gro, tf_warning_or_error);
@@ -5181,7 +5184,8 @@ cp_coroutine_transform::build_ramp_function ()
/* The ramp is done, we just need the return statement, which we build from
the return object we constructed before we called the function body. */
- finish_return_stmt (void_ramp_p ? NULL_TREE : coro_gro);
+ r = void_ramp_p ? NULL_TREE : convert_from_reference (coro_gro);
+ finish_return_stmt (r);
if (flag_exceptions)
{
diff --git a/gcc/testsuite/g++.dg/coroutines/pr115908.C
b/gcc/testsuite/g++.dg/coroutines/pr115908.C
index ac27d916de2..a40cece1143 100644
--- a/gcc/testsuite/g++.dg/coroutines/pr115908.C
+++ b/gcc/testsuite/g++.dg/coroutines/pr115908.C
@@ -1,3 +1,16 @@
+// { dg-do run }
+
+// With the changes to deal with CWG2563 (and PR119916) we now use the
+// referenced promise in the return expression. It is quite reasonable
+// for a body implementation to complete before control is returned to
+// the ramp - and in that case we would be creating the ramp return object
+// from an already-deleted promise object.
+// This is recognised to be a poor situation and resolution via a core
+// issue is planned.
+
+// In this test we explicitly trigger the circumstance mentioned above.
+// { dg-xfail-run-if "" { *-*-* } }
+
#include <coroutine>
#ifdef OUTPUT
@@ -6,23 +19,25 @@
struct Promise;
-bool promise_live = false;
+int promise_life = 0;
struct Handle : std::coroutine_handle<Promise> {
+
Handle(Promise &p) :
std::coroutine_handle<Promise>(Handle::from_promise(p)) {
- if (!promise_live)
- __builtin_abort ();
#ifdef OUTPUT
- std::cout << "Handle(Promise &)\n";
+ std::cout << "Handle(Promise &) " << promise_life << std::endl;
#endif
- }
- Handle(Promise &&p) :
std::coroutine_handle<Promise>(Handle::from_promise(p)) {
- if (!promise_live)
+ if (promise_life <= 0)
__builtin_abort ();
+ }
+
+ Handle(Promise &&p) :
std::coroutine_handle<Promise>(Handle::from_promise(p)) {
#ifdef OUTPUT
- std::cout << "Handle(Promise &&)\n";
+ std::cout << "Handle(Promise &&) " << promise_life << std::endl;
#endif
- }
+ if (promise_life <= 0)
+ __builtin_abort ();
+ }
using promise_type = Promise;
};
@@ -30,46 +45,73 @@ struct Handle : std::coroutine_handle<Promise> {
struct Promise {
Promise() {
#ifdef OUTPUT
- std::cout << "Promise()\n";
+ std::cout << "Promise()" << std::endl;
+#endif
+ promise_life++;
+ }
+
+ Promise(Promise& p){
+#ifdef OUTPUT
+ std::cout << "Promise(Promise&)" << std::endl;
#endif
- promise_live = true;
+ promise_life++;
}
+
~Promise() {
#ifdef OUTPUT
- std::cout << "~Promise()\n";
+ std::cout << "~Promise()" << std::endl;
#endif
- if (!promise_live)
+ if (promise_life <= 0)
__builtin_abort ();
- promise_live = false;
+ promise_life--;
}
+
Promise& get_return_object() noexcept {
#ifdef OUTPUT
- std::cout << "get_return_object()\n";
+ std::cout << "get_return_object() " << promise_life << std::endl;
#endif
- if (!promise_live)
+ if (promise_life <= 0)
__builtin_abort ();
return *this;
}
- std::suspend_never initial_suspend() const noexcept { return {}; }
- std::suspend_never final_suspend() const noexcept { return {}; }
+
+ std::suspend_never initial_suspend() const noexcept {
+#ifdef OUTPUT
+ std::cout << "initial_suspend()" << std::endl;
+#endif
+ return {};
+ }
+ std::suspend_never final_suspend() const noexcept {
+#ifdef OUTPUT
+ std::cout << "final_suspend()" << std::endl;
+#endif
+ return {};
+ }
void return_void() const noexcept {
- if (!promise_live)
+ if (!promise_life)
__builtin_abort ();
#ifdef OUTPUT
- std::cout << "return_void()\n";
+ std::cout << "return_void()" << std::endl;
#endif
}
void unhandled_exception() const noexcept {}
};
Handle Coro() {
+
+#ifdef OUTPUT
+ std::cout << "Coro()" << std::endl;
+#endif
co_return;
}
int main() {
- Coro();
- if (promise_live)
+ Coro();
+#ifdef OUTPUT
+ std::cout << "done Coro()" << std::endl;
+#endif
+ if (promise_life)
__builtin_abort ();
return 0;
}
--
2.39.2 (Apple Git-143)
