coroutines: Handle initial awaiters with non-void returns [PR 100127].
Commit Message
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
Comments
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
@@ -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);
new file mode 100644
@@ -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(); }