Re: [PATCH] coroutines: Handle initial awaiters with non-void returns [PR 100127].
On 11/19/21 12:40, Iain Sandoe wrote: On 18 Nov 2021, at 23:42, Iain Sandoe wrote: On 18 Nov 2021, at 22:13, Jason Merrill via Gcc-patches 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
Re: [PATCH] coroutines: Handle initial awaiters with non-void returns [PR 100127].
Hi Jason, > On 18 Nov 2021, at 23:42, Iain Sandoe wrote: > > > >> On 18 Nov 2021, at 22:13, Jason Merrill via Gcc-patches >> 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
Re: [PATCH] coroutines: Handle initial awaiters with non-void returns [PR 100127].
> On 18 Nov 2021, at 22:13, Jason Merrill via Gcc-patches
> 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
>> 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 000..374cd710077
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/coroutines/pr100127.C
>> @@ -0,0 +1,65 @@
>> +#ifdef __clang__
>> +#include
>> +namespace std {
>> + using namespace std::experimental;
>> +}
>> +#else
>> +#include
>> +#endif
>> +#include
>> +
>> +struct future
>> +{
>> +using value_type = int;
>> +struct promise_type;
>> +using handle_type = std::coroutine_handle;
>> +
>> +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 = std::nullopt;
>> +
>> +future get_return_object() {
>> +return future{handle_type::from_promise(*this)};
>> +}
>> +void return_value(value_type val) {
>> +_value = static_cast(val);
>> +}
>> +auto initial_suspend() noexcept {
>> +class awaiter {
>> +std::optional & value;
>> +public:
>> +explicit awaiter(std::optional & 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(); }
Re: [PATCH] coroutines: Handle initial awaiters with non-void returns [PR 100127].
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
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 000..374cd710077
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/pr100127.C
@@ -0,0 +1,65 @@
+#ifdef __clang__
+#include
+namespace std {
+ using namespace std::experimental;
+}
+#else
+#include
+#endif
+#include
+
+struct future
+{
+using value_type = int;
+struct promise_type;
+using handle_type = std::coroutine_handle;
+
+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 = std::nullopt;
+
+future get_return_object() {
+return future{handle_type::from_promise(*this)};
+}
+void return_value(value_type val) {
+_value = static_cast(val);
+}
+auto initial_suspend() noexcept {
+class awaiter {
+std::optional & value;
+public:
+explicit awaiter(std::optional & 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(); }
