* [PATCH RFC] c++: fix broken conversion in coroutines @ 2022-09-30 22:06 Jason Merrill 2022-09-30 22:50 ` Iain Sandoe 0 siblings, 1 reply; 5+ messages in thread From: Jason Merrill @ 2022-09-30 22:06 UTC (permalink / raw) To: gcc-patches, Iain Sandoe You can't use CONVERT_EXPR to convert between two class types, and it was breaking copy elision. Unfortunately, this patch breaks symmetric-transfer-00-basic.C, where susp_type is Loopy<int>::handle_type. How is this supposed to work? gcc/cp/ChangeLog: * coroutines.cc (expand_one_await_expression): Change conversion to assert. --- gcc/cp/coroutines.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc index eca01abcb7a..568f2edf67d 100644 --- a/gcc/cp/coroutines.cc +++ b/gcc/cp/coroutines.cc @@ -1728,7 +1728,9 @@ expand_one_await_expression (tree *stmt, tree *await_expr, void *d) } else { - r = build1_loc (loc, CONVERT_EXPR, void_coro_handle_type, suspend); + gcc_checking_assert (same_type_ignoring_top_level_qualifiers_p + (void_coro_handle_type, susp_type)); + r = suspend; r = build2_loc (loc, INIT_EXPR, void_coro_handle_type, data->conthand, r); r = build1 (CONVERT_EXPR, void_type_node, r); append_to_statement_list (r, &body_list); base-commit: 43faf3e5445b571731e52faa1be085ecd0a09323 -- 2.31.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RFC] c++: fix broken conversion in coroutines 2022-09-30 22:06 [PATCH RFC] c++: fix broken conversion in coroutines Jason Merrill @ 2022-09-30 22:50 ` Iain Sandoe 2022-10-04 3:53 ` Jason Merrill 0 siblings, 1 reply; 5+ messages in thread From: Iain Sandoe @ 2022-09-30 22:50 UTC (permalink / raw) To: Jason Merrill; +Cc: GCC Patches Hi Jason, > On 30 Sep 2022, at 23:06, Jason Merrill <jason@redhat.com> wrote: > > You can't use CONVERT_EXPR to convert between two class types, and it was > breaking copy elision. > > Unfortunately, this patch breaks symmetric-transfer-00-basic.C, where > susp_type is Loopy<int>::handle_type. How is this supposed to work? We are trying to save a type-erased handle (which the symmetric transfer makes and indirect call through, nothing else). so, I suppose the equivalent could be: conthand = coroutine_handle::from_address (suspend.address()) or, is there some cast version that would be valid here? Iain > > gcc/cp/ChangeLog: > > * coroutines.cc (expand_one_await_expression): Change conversion > to assert. > --- > gcc/cp/coroutines.cc | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc > index eca01abcb7a..568f2edf67d 100644 > --- a/gcc/cp/coroutines.cc > +++ b/gcc/cp/coroutines.cc > @@ -1728,7 +1728,9 @@ expand_one_await_expression (tree *stmt, tree *await_expr, void *d) > } > else > { > - r = build1_loc (loc, CONVERT_EXPR, void_coro_handle_type, suspend); > + gcc_checking_assert (same_type_ignoring_top_level_qualifiers_p > + (void_coro_handle_type, susp_type)); > + r = suspend; > r = build2_loc (loc, INIT_EXPR, void_coro_handle_type, data->conthand, r); > r = build1 (CONVERT_EXPR, void_type_node, r); > append_to_statement_list (r, &body_list); > > base-commit: 43faf3e5445b571731e52faa1be085ecd0a09323 > -- > 2.31.1 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RFC] c++: fix broken conversion in coroutines 2022-09-30 22:50 ` Iain Sandoe @ 2022-10-04 3:53 ` Jason Merrill 2022-10-06 21:44 ` Jason Merrill 0 siblings, 1 reply; 5+ messages in thread From: Jason Merrill @ 2022-10-04 3:53 UTC (permalink / raw) To: Iain Sandoe; +Cc: GCC Patches On 9/30/22 18:50, Iain Sandoe wrote: > Hi Jason, > >> On 30 Sep 2022, at 23:06, Jason Merrill <jason@redhat.com> wrote: >> >> You can't use CONVERT_EXPR to convert between two class types, and it was >> breaking copy elision. >> >> Unfortunately, this patch breaks symmetric-transfer-00-basic.C, where >> susp_type is Loopy<int>::handle_type. How is this supposed to work? > > We are trying to save a type-erased handle (which the symmetric transfer makes > and indirect call through, nothing else). The problem is you're treating one class directly as another class here, without the indirection involved in usual type-erasure idioms. It does seem that the gimplifier handles this fine, but it doesn't correspond to anything in the language and much of the front end assumes that CONVERT_EXPR is only used for scalars. VIEW_CONVERT_EXPR would better express that we're not doing anything to the value, just cheating the type system. That's still dodgy from a language perspective, but probably safe enough in this case. Note that I was wrong to mention copy elision above; it's irrelevant to codegen here since the handle type returns in a register. > so, I suppose the equivalent could be: > > conthand = coroutine_handle::from_address (suspend.address()) That sounds more correct, yes. Jason ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RFC] c++: fix broken conversion in coroutines 2022-10-04 3:53 ` Jason Merrill @ 2022-10-06 21:44 ` Jason Merrill 2022-10-06 22:47 ` Iain Sandoe 0 siblings, 1 reply; 5+ messages in thread From: Jason Merrill @ 2022-10-06 21:44 UTC (permalink / raw) To: Iain Sandoe; +Cc: GCC Patches [-- Attachment #1: Type: text/plain, Size: 1139 bytes --] On 10/3/22 23:53, Jason Merrill wrote: > On 9/30/22 18:50, Iain Sandoe wrote: >> Hi Jason, >> >>> On 30 Sep 2022, at 23:06, Jason Merrill <jason@redhat.com> wrote: >>> >>> You can't use CONVERT_EXPR to convert between two class types, and it >>> was >>> breaking copy elision. >>> >>> Unfortunately, this patch breaks symmetric-transfer-00-basic.C, where >>> susp_type is Loopy<int>::handle_type. How is this supposed to work? >> >> We are trying to save a type-erased handle (which the symmetric >> transfer makes >> and indirect call through, nothing else). > > The problem is you're treating one class directly as another class here, > without the indirection involved in usual type-erasure idioms. > > It does seem that the gimplifier handles this fine, but it doesn't > correspond to anything in the language and much of the front end assumes > that CONVERT_EXPR is only used for scalars. VIEW_CONVERT_EXPR would > better express that we're not doing anything to the value, just cheating > the type system. That's still dodgy from a language perspective, but > probably safe enough in this case. So I'm applying this: [-- Attachment #2: 0001-c-fix-broken-conversion-in-coroutines.patch --] [-- Type: text/x-patch, Size: 1891 bytes --] From 0cd9dafcfa8cf69b3a1eae91bea82d388c5328d2 Mon Sep 17 00:00:00 2001 From: Jason Merrill <jason@redhat.com> Date: Fri, 30 Sep 2022 10:04:22 -0400 Subject: [PATCH] c++: fix broken conversion in coroutines To: gcc-patches@gcc.gnu.org You can't use CONVERT_EXPR to convert between two class types. VIEW_CONVERT_EXPR takes liberties with the C++ type system, but is probably safe in this context. Let's also only use it when the type isn't already what we want. gcc/cp/ChangeLog: * coroutines.cc (expand_one_await_expression): Change conversion to VIEW_CONVERT_EXPR. * cp-gimplify.cc (cp_genericize_r) [CONVERT_EXPR]: Add assert. --- gcc/cp/coroutines.cc | 5 ++++- gcc/cp/cp-gimplify.cc | 1 + 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc index eca01abcb7a..60b846600b9 100644 --- a/gcc/cp/coroutines.cc +++ b/gcc/cp/coroutines.cc @@ -1728,7 +1728,10 @@ expand_one_await_expression (tree *stmt, tree *await_expr, void *d) } else { - r = build1_loc (loc, CONVERT_EXPR, void_coro_handle_type, suspend); + r = suspend; + if (!same_type_ignoring_top_level_qualifiers_p (susp_type, + void_coro_handle_type)) + r = build1_loc (loc, VIEW_CONVERT_EXPR, void_coro_handle_type, r); r = build2_loc (loc, INIT_EXPR, void_coro_handle_type, data->conthand, r); r = build1 (CONVERT_EXPR, void_type_node, r); append_to_statement_list (r, &body_list); diff --git a/gcc/cp/cp-gimplify.cc b/gcc/cp/cp-gimplify.cc index b4599fc34d8..5d26e59d098 100644 --- a/gcc/cp/cp-gimplify.cc +++ b/gcc/cp/cp-gimplify.cc @@ -1589,6 +1589,7 @@ cp_genericize_r (tree *stmt_p, int *walk_subtrees, void *data) break; case CONVERT_EXPR: + gcc_checking_assert (!AGGREGATE_TYPE_P (TREE_TYPE (stmt))); gcc_assert (!CONVERT_EXPR_VBASE_PATH (stmt)); break; -- 2.31.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RFC] c++: fix broken conversion in coroutines 2022-10-06 21:44 ` Jason Merrill @ 2022-10-06 22:47 ` Iain Sandoe 0 siblings, 0 replies; 5+ messages in thread From: Iain Sandoe @ 2022-10-06 22:47 UTC (permalink / raw) To: Jason Merrill; +Cc: GCC Patches > On 6 Oct 2022, at 22:44, Jason Merrill <jason@redhat.com> wrote: > > On 10/3/22 23:53, Jason Merrill wrote: >> On 9/30/22 18:50, Iain Sandoe wrote: >>> Hi Jason, >>> >>>> On 30 Sep 2022, at 23:06, Jason Merrill <jason@redhat.com> wrote: >>>> >>>> You can't use CONVERT_EXPR to convert between two class types, and it was >>>> breaking copy elision. >>>> >>>> Unfortunately, this patch breaks symmetric-transfer-00-basic.C, where >>>> susp_type is Loopy<int>::handle_type. How is this supposed to work? >>> >>> We are trying to save a type-erased handle (which the symmetric transfer makes >>> and indirect call through, nothing else). >> The problem is you're treating one class directly as another class here, without the indirection involved in usual type-erasure idioms. >> It does seem that the gimplifier handles this fine, but it doesn't correspond to anything in the language and much of the front end assumes that CONVERT_EXPR is only used for scalars. VIEW_CONVERT_EXPR would better express that we're not doing anything to the value, just cheating the type system. That's still dodgy from a language perspective, but probably safe enough in this case. > > So I'm applying this:<0001-c-fix-broken-conversion-in-coroutines.patch> thanks, I have not had any cycles to look at this. however, when I next do - was planning on looking at the: cont = handle.from_address(await_suspend().address()) approach, since both .address() and .from_address() are constexpr, cp_fold_function should turn that into essentially a NOP. Iain ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-10-06 22:47 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-30 22:06 [PATCH RFC] c++: fix broken conversion in coroutines Jason Merrill 2022-09-30 22:50 ` Iain Sandoe 2022-10-04 3:53 ` Jason Merrill 2022-10-06 21:44 ` Jason Merrill 2022-10-06 22:47 ` Iain Sandoe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).