Message ID | 20211105154651.77786-1-iain@sandoe.co.uk |
---|---|
State | New |
Headers | show |
Series | coroutines: Handle initial awaiters with non-void returns [PR 100127]. | expand |
On 11/5/21 11:46, Iain Sandoe wrote: > The way in which a C++20 coroutine is specified discards any value > that might be returned from the initial or final await expressions. > > This PR ICE was caused by an initial await expression with an > await_resume () returning a reference, the function rewrite code > was not set up to expect this. > > Fixed by looking through any indirection present and by explicitly > discarding the value, if any, returned by await_resume(). > > It does not seem useful to make a diagnostic for this, since > the user could define a generic awaiter that usefully returns > values when used in a different position from the initial (or > final) await expressions. > > tested on x86_64 darwin, linux, > OK for master and backports? > thanks > Iain > > Signed-off-by: Iain Sandoe <iain@sandoe.co.uk> > > PR c++/100127 > > gcc/cp/ChangeLog: > > * coroutines.cc (coro_rewrite_function_body): Handle initial > await expressions that try to produce a reference value. > > gcc/testsuite/ChangeLog: > > * g++.dg/coroutines/pr100127.C: New test. > --- > gcc/cp/coroutines.cc | 9 ++- > gcc/testsuite/g++.dg/coroutines/pr100127.C | 65 ++++++++++++++++++++++ > 2 files changed, 73 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/g++.dg/coroutines/pr100127.C > > diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc > index 9017902e6fb..6db4b70f028 100644 > --- a/gcc/cp/coroutines.cc > +++ b/gcc/cp/coroutines.cc > @@ -4211,9 +4211,16 @@ coro_rewrite_function_body (location_t fn_start, tree fnbody, tree orig, > { > /* Build a compound expression that sets the > initial-await-resume-called variable true and then calls the > - initial suspend expression await resume. */ > + initial suspend expression await resume. > + In the case that the user decides to make the initial await > + await_resume() return a value, we need to discard it and, it is > + a reference type, look past the indirection. */ > + if (INDIRECT_REF_P (initial_await)) > + initial_await = TREE_OPERAND (initial_await, 0); > tree vec = TREE_OPERAND (initial_await, 3); > tree aw_r = TREE_VEC_ELT (vec, 2); > + if (!VOID_TYPE_P (TREE_TYPE (aw_r))) > + aw_r = build1 (CONVERT_EXPR, void_type_node, aw_r); Is there a reason not to use convert_to_void? > tree update = build2 (MODIFY_EXPR, boolean_type_node, i_a_r_c, > boolean_true_node); > aw_r = cp_build_compound_expr (update, aw_r, tf_warning_or_error); > diff --git a/gcc/testsuite/g++.dg/coroutines/pr100127.C b/gcc/testsuite/g++.dg/coroutines/pr100127.C > new file mode 100644 > index 00000000000..374cd710077 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/coroutines/pr100127.C > @@ -0,0 +1,65 @@ > +#ifdef __clang__ > +#include <experimental/coroutine> > +namespace std { > + using namespace std::experimental; > +} > +#else > +#include <coroutine> > +#endif > +#include <optional> > + > +struct future > +{ > + using value_type = int; > + struct promise_type; > + using handle_type = std::coroutine_handle<promise_type>; > + > + handle_type _coroutine; > + > + future(handle_type h) : _coroutine{h} {} > + > + ~future() noexcept{ > + if (_coroutine) { > + _coroutine.destroy(); > + } > + } > + > + value_type get() { > + auto ptr = _coroutine.promise()._value; > + return *ptr; > + } > + > + struct promise_type { > + std::optional<value_type> _value = std::nullopt; > + > + future get_return_object() { > + return future{handle_type::from_promise(*this)}; > + } > + void return_value(value_type val) { > + _value = static_cast<value_type &&>(val); > + } > + auto initial_suspend() noexcept { > + class awaiter { > + std::optional<value_type> & value; > + public: > + explicit awaiter(std::optional<value_type> & val) noexcept : value{val} {} > + bool await_ready() noexcept { return value.has_value(); } > + void await_suspend(handle_type) noexcept { } > + value_type & await_resume() noexcept { return *value; } > + }; > + > + return awaiter{_value}; > + } > + std::suspend_always final_suspend() noexcept { > + return {}; > + } > + //void return_void() {} > + void unhandled_exception() {} > + }; > +}; > + > +future create_future() > +{ co_return 2021; } > + > +int main() > +{ auto f = create_future(); } >
> On 18 Nov 2021, at 22:13, Jason Merrill via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > On 11/5/21 11:46, Iain Sandoe wrote: >> The way in which a C++20 coroutine is specified discards any value >> that might be returned from the initial or final await expressions. >> This PR ICE was caused by an initial await expression with an >> await_resume () returning a reference, the function rewrite code >> was not set up to expect this. >> Fixed by looking through any indirection present and by explicitly >> discarding the value, if any, returned by await_resume(). >> It does not seem useful to make a diagnostic for this, since >> the user could define a generic awaiter that usefully returns >> values when used in a different position from the initial (or >> final) await expressions. >> tested on x86_64 darwin, linux, >> OK for master and backports? >> thanks >> Iain >> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk> >> PR c++/100127 >> gcc/cp/ChangeLog: >> * coroutines.cc (coro_rewrite_function_body): Handle initial >> await expressions that try to produce a reference value. >> gcc/testsuite/ChangeLog: >> * g++.dg/coroutines/pr100127.C: New test. >> --- >> gcc/cp/coroutines.cc | 9 ++- >> gcc/testsuite/g++.dg/coroutines/pr100127.C | 65 ++++++++++++++++++++++ >> 2 files changed, 73 insertions(+), 1 deletion(-) >> create mode 100644 gcc/testsuite/g++.dg/coroutines/pr100127.C >> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc >> index 9017902e6fb..6db4b70f028 100644 >> --- a/gcc/cp/coroutines.cc >> +++ b/gcc/cp/coroutines.cc >> @@ -4211,9 +4211,16 @@ coro_rewrite_function_body (location_t fn_start, tree fnbody, tree orig, >> { >> /* Build a compound expression that sets the >> initial-await-resume-called variable true and then calls the >> - initial suspend expression await resume. */ >> + initial suspend expression await resume. >> + In the case that the user decides to make the initial await >> + await_resume() return a value, we need to discard it and, it is >> + a reference type, look past the indirection. */ >> + if (INDIRECT_REF_P (initial_await)) >> + initial_await = TREE_OPERAND (initial_await, 0); >> tree vec = TREE_OPERAND (initial_await, 3); >> tree aw_r = TREE_VEC_ELT (vec, 2); >> + if (!VOID_TYPE_P (TREE_TYPE (aw_r))) >> + aw_r = build1 (CONVERT_EXPR, void_type_node, aw_r); > > Is there a reason not to use convert_to_void? no, just me still learning APIs… I’ll do a revised and check it. Iain > >> tree update = build2 (MODIFY_EXPR, boolean_type_node, i_a_r_c, >> boolean_true_node); >> aw_r = cp_build_compound_expr (update, aw_r, tf_warning_or_error); >> diff --git a/gcc/testsuite/g++.dg/coroutines/pr100127.C b/gcc/testsuite/g++.dg/coroutines/pr100127.C >> new file mode 100644 >> index 00000000000..374cd710077 >> --- /dev/null >> +++ b/gcc/testsuite/g++.dg/coroutines/pr100127.C >> @@ -0,0 +1,65 @@ >> +#ifdef __clang__ >> +#include <experimental/coroutine> >> +namespace std { >> + using namespace std::experimental; >> +} >> +#else >> +#include <coroutine> >> +#endif >> +#include <optional> >> + >> +struct future >> +{ >> + using value_type = int; >> + struct promise_type; >> + using handle_type = std::coroutine_handle<promise_type>; >> + >> + handle_type _coroutine; >> + >> + future(handle_type h) : _coroutine{h} {} >> + >> + ~future() noexcept{ >> + if (_coroutine) { >> + _coroutine.destroy(); >> + } >> + } >> + >> + value_type get() { >> + auto ptr = _coroutine.promise()._value; >> + return *ptr; >> + } >> + >> + struct promise_type { >> + std::optional<value_type> _value = std::nullopt; >> + >> + future get_return_object() { >> + return future{handle_type::from_promise(*this)}; >> + } >> + void return_value(value_type val) { >> + _value = static_cast<value_type &&>(val); >> + } >> + auto initial_suspend() noexcept { >> + class awaiter { >> + std::optional<value_type> & value; >> + public: >> + explicit awaiter(std::optional<value_type> & val) noexcept : value{val} {} >> + bool await_ready() noexcept { return value.has_value(); } >> + void await_suspend(handle_type) noexcept { } >> + value_type & await_resume() noexcept { return *value; } >> + }; >> + >> + return awaiter{_value}; >> + } >> + std::suspend_always final_suspend() noexcept { >> + return {}; >> + } >> + //void return_void() {} >> + void unhandled_exception() {} >> + }; >> +}; >> + >> +future create_future() >> +{ co_return 2021; } >> + >> +int main() >> +{ auto f = create_future(); }
Hi Jason, > On 18 Nov 2021, at 23:42, Iain Sandoe <iain@sandoe.co.uk> wrote: > > > >> On 18 Nov 2021, at 22:13, Jason Merrill via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: >> >> On 11/5/21 11:46, Iain Sandoe wrote: >>> The way in which a C++20 coroutine is specified discards any value >>> tree aw_r = TREE_VEC_ELT (vec, 2); >>> + if (!VOID_TYPE_P (TREE_TYPE (aw_r))) >>> + aw_r = build1 (CONVERT_EXPR, void_type_node, aw_r); >> >> Is there a reason not to use convert_to_void? > > no, just me still learning APIs… I’ll do a revised and check it. So I’m testing this replacement: aw_r = convert_to_void (aw_r, ICV_CAST, tf_warning_or_error); OK if the testing succeeds? Iain
On 11/19/21 12:40, Iain Sandoe wrote: >> On 18 Nov 2021, at 23:42, Iain Sandoe <iain@sandoe.co.uk> wrote: >>> On 18 Nov 2021, at 22:13, Jason Merrill via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: >>> >>> On 11/5/21 11:46, Iain Sandoe wrote: >>>> The way in which a C++20 coroutine is specified discards any value > >>>> tree aw_r = TREE_VEC_ELT (vec, 2); >>>> + if (!VOID_TYPE_P (TREE_TYPE (aw_r))) >>>> + aw_r = build1 (CONVERT_EXPR, void_type_node, aw_r); >>> >>> Is there a reason not to use convert_to_void? >> >> no, just me still learning APIs… I’ll do a revised and check it. > > So I’m testing this replacement: > > aw_r = convert_to_void (aw_r, ICV_CAST, tf_warning_or_error); Why ICV_CAST? I'd think ICV_STATEMENT, so that we get [[nodiscard]] warnings. OK with that change. Jason
diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc index 9017902e6fb..6db4b70f028 100644 --- a/gcc/cp/coroutines.cc +++ b/gcc/cp/coroutines.cc @@ -4211,9 +4211,16 @@ coro_rewrite_function_body (location_t fn_start, tree fnbody, tree orig, { /* Build a compound expression that sets the initial-await-resume-called variable true and then calls the - initial suspend expression await resume. */ + initial suspend expression await resume. + In the case that the user decides to make the initial await + await_resume() return a value, we need to discard it and, it is + a reference type, look past the indirection. */ + if (INDIRECT_REF_P (initial_await)) + initial_await = TREE_OPERAND (initial_await, 0); tree vec = TREE_OPERAND (initial_await, 3); tree aw_r = TREE_VEC_ELT (vec, 2); + if (!VOID_TYPE_P (TREE_TYPE (aw_r))) + aw_r = build1 (CONVERT_EXPR, void_type_node, aw_r); tree update = build2 (MODIFY_EXPR, boolean_type_node, i_a_r_c, boolean_true_node); aw_r = cp_build_compound_expr (update, aw_r, tf_warning_or_error); diff --git a/gcc/testsuite/g++.dg/coroutines/pr100127.C b/gcc/testsuite/g++.dg/coroutines/pr100127.C new file mode 100644 index 00000000000..374cd710077 --- /dev/null +++ b/gcc/testsuite/g++.dg/coroutines/pr100127.C @@ -0,0 +1,65 @@ +#ifdef __clang__ +#include <experimental/coroutine> +namespace std { + using namespace std::experimental; +} +#else +#include <coroutine> +#endif +#include <optional> + +struct future +{ + using value_type = int; + struct promise_type; + using handle_type = std::coroutine_handle<promise_type>; + + handle_type _coroutine; + + future(handle_type h) : _coroutine{h} {} + + ~future() noexcept{ + if (_coroutine) { + _coroutine.destroy(); + } + } + + value_type get() { + auto ptr = _coroutine.promise()._value; + return *ptr; + } + + struct promise_type { + std::optional<value_type> _value = std::nullopt; + + future get_return_object() { + return future{handle_type::from_promise(*this)}; + } + void return_value(value_type val) { + _value = static_cast<value_type &&>(val); + } + auto initial_suspend() noexcept { + class awaiter { + std::optional<value_type> & value; + public: + explicit awaiter(std::optional<value_type> & val) noexcept : value{val} {} + bool await_ready() noexcept { return value.has_value(); } + void await_suspend(handle_type) noexcept { } + value_type & await_resume() noexcept { return *value; } + }; + + return awaiter{_value}; + } + std::suspend_always final_suspend() noexcept { + return {}; + } + //void return_void() {} + void unhandled_exception() {} + }; +}; + +future create_future() +{ co_return 2021; } + +int main() +{ auto f = create_future(); }
The way in which a C++20 coroutine is specified discards any value that might be returned from the initial or final await expressions. This PR ICE was caused by an initial await expression with an await_resume () returning a reference, the function rewrite code was not set up to expect this. Fixed by looking through any indirection present and by explicitly discarding the value, if any, returned by await_resume(). It does not seem useful to make a diagnostic for this, since the user could define a generic awaiter that usefully returns values when used in a different position from the initial (or final) await expressions. tested on x86_64 darwin, linux, OK for master and backports? thanks Iain Signed-off-by: Iain Sandoe <iain@sandoe.co.uk> PR c++/100127 gcc/cp/ChangeLog: * coroutines.cc (coro_rewrite_function_body): Handle initial await expressions that try to produce a reference value. gcc/testsuite/ChangeLog: * g++.dg/coroutines/pr100127.C: New test. --- gcc/cp/coroutines.cc | 9 ++- gcc/testsuite/g++.dg/coroutines/pr100127.C | 65 ++++++++++++++++++++++ 2 files changed, 73 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/g++.dg/coroutines/pr100127.C