* [PATCH] coroutines: Fix handling of artificial vars [PR94886]
@ 2020-04-30 13:24 Iain Sandoe
2020-04-30 13:46 ` Nathan Sidwell
0 siblings, 1 reply; 3+ messages in thread
From: Iain Sandoe @ 2020-04-30 13:24 UTC (permalink / raw)
To: GCC Patches; +Cc: Nathan Sidwell
Hi
Another case found when building the folly experimental coros stuff.
tested on x86_64-darwin so far,
OK for master after testing on x86_64-linux?
thanks
Iain
The testcase ICEs because the range-based for generates three
artificial variables that need to be allocated to the coroutine
frame but, when walking the BIND_EXR that contains these, the
DECL_INITIAL for one of them refers to an entry appearing later,
which means that the frame entry hasn't been allocated when that
INITIAL is walked.
The solution is to defer walking the DECL_INITIAL/SIZE etc. until
all the BIND_EXPR vars have been processed.
gcc/cp/ChangeLog:
2020-04-30 Iain Sandoe <iain@sandoe.co.uk>
PR c++/94886
* coroutines.cc (transform_local_var_uses): Defer walking
the DECL_INITIALs of BIND_EXPR vars until all the frame
allocations have been made.
gcc/testsuite/ChangeLog:
2020-04-30 Iain Sandoe <iain@sandoe.co.uk>
PR c++/94886
* g++.dg/coroutines/pr94886-folly-3.C: New test.
---
gcc/cp/coroutines.cc | 21 ++++++++++++-------
.../g++.dg/coroutines/pr94886-folly-3.C | 15 +++++++++++++
2 files changed, 28 insertions(+), 8 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/coroutines/pr94886-folly-3.C
diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index 7bb3e98fe6c..1bc4bf2e45b 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -1813,14 +1813,6 @@ transform_local_var_uses (tree *stmt, int *do_subtree, void *d)
/* Re-write the variable's context to be in the actor func. */
DECL_CONTEXT (lvar) = lvd->context;
- /* we need to walk some of the decl trees, which might contain
- references to vars replaced at a higher level. */
- cp_walk_tree (&DECL_INITIAL (lvar), transform_local_var_uses, d,
- NULL);
- cp_walk_tree (&DECL_SIZE (lvar), transform_local_var_uses, d, NULL);
- cp_walk_tree (&DECL_SIZE_UNIT (lvar), transform_local_var_uses, d,
- NULL);
-
/* For capture proxies, this could include the decl value expr. */
if (local_var.is_lambda_capture || local_var.has_value_expr_p)
{
@@ -1842,6 +1834,19 @@ transform_local_var_uses (tree *stmt, int *do_subtree, void *d)
lvd->actor_frame, fld_ref, NULL_TREE);
local_var.field_idx = fld_idx;
}
+ /* Now we've built the revised var in the frame, substitute uses of
+ it in initializers and the bind expr body. */
+ for (lvar = BIND_EXPR_VARS (*stmt); lvar != NULL;
+ lvar = DECL_CHAIN (lvar))
+ {
+ /* we need to walk some of the decl trees, which might contain
+ references to vars replaced at a higher level. */
+ cp_walk_tree (&DECL_INITIAL (lvar), transform_local_var_uses, d,
+ NULL);
+ cp_walk_tree (&DECL_SIZE (lvar), transform_local_var_uses, d, NULL);
+ cp_walk_tree (&DECL_SIZE_UNIT (lvar), transform_local_var_uses, d,
+ NULL);
+ }
cp_walk_tree (&BIND_EXPR_BODY (*stmt), transform_local_var_uses, d, NULL);
/* Now we have processed and removed references to the original vars,
diff --git a/gcc/testsuite/g++.dg/coroutines/pr94886-folly-3.C b/gcc/testsuite/g++.dg/coroutines/pr94886-folly-3.C
new file mode 100644
index 00000000000..d7bd2c17d4a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/pr94886-folly-3.C
@@ -0,0 +1,15 @@
+
+#include "coro.h"
+#include "coro1-ret-int-yield-int.h"
+
+#include <array>
+
+coro1
+my_coro ()
+{
+ const std::array<int, 5> expectedValues = {{0, 3, 1, 4, 2}};
+
+ for (int expectedValue : expectedValues) {
+ co_yield expectedValue;
+ }
+}
--
2.24.1
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] coroutines: Fix handling of artificial vars [PR94886]
2020-04-30 13:24 [PATCH] coroutines: Fix handling of artificial vars [PR94886] Iain Sandoe
@ 2020-04-30 13:46 ` Nathan Sidwell
2020-04-30 20:52 ` Iain Sandoe
0 siblings, 1 reply; 3+ messages in thread
From: Nathan Sidwell @ 2020-04-30 13:46 UTC (permalink / raw)
To: Iain Sandoe, GCC Patches
On 4/30/20 9:24 AM, Iain Sandoe wrote:
> Hi
>
> Another case found when building the folly experimental coros stuff.
> tested on x86_64-darwin so far,
> OK for master after testing on x86_64-linux?
> thanks
> Iain
>
> The testcase ICEs because the range-based for generates three
> artificial variables that need to be allocated to the coroutine
> frame but, when walking the BIND_EXR that contains these, the
> DECL_INITIAL for one of them refers to an entry appearing later,
> which means that the frame entry hasn't been allocated when that
> INITIAL is walked.
Hm, the error seems to be the range for's construction, did you look at
altering that so that such forward references do not occur? (range for
is described as a src->src xform, so I'm sure it's possible to xform it
into something a user could have written, which a forward-reference is not)
your patch is ok, if that for change's not simple.
>
> The solution is to defer walking the DECL_INITIAL/SIZE etc. until
> all the BIND_EXPR vars have been processed.
>
> gcc/cp/ChangeLog:
>
> 2020-04-30 Iain Sandoe <iain@sandoe.co.uk>
>
> PR c++/94886
> * coroutines.cc (transform_local_var_uses): Defer walking
> the DECL_INITIALs of BIND_EXPR vars until all the frame
> allocations have been made.
>
> gcc/testsuite/ChangeLog:
>
> 2020-04-30 Iain Sandoe <iain@sandoe.co.uk>
>
> PR c++/94886
> * g++.dg/coroutines/pr94886-folly-3.C: New test.
> ---
> gcc/cp/coroutines.cc | 21 ++++++++++++-------
> .../g++.dg/coroutines/pr94886-folly-3.C | 15 +++++++++++++
> 2 files changed, 28 insertions(+), 8 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/coroutines/pr94886-folly-3.C
>
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index 7bb3e98fe6c..1bc4bf2e45b 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -1813,14 +1813,6 @@ transform_local_var_uses (tree *stmt, int *do_subtree, void *d)
> /* Re-write the variable's context to be in the actor func. */
> DECL_CONTEXT (lvar) = lvd->context;
>
> - /* we need to walk some of the decl trees, which might contain
> - references to vars replaced at a higher level. */
> - cp_walk_tree (&DECL_INITIAL (lvar), transform_local_var_uses, d,
> - NULL);
> - cp_walk_tree (&DECL_SIZE (lvar), transform_local_var_uses, d, NULL);
> - cp_walk_tree (&DECL_SIZE_UNIT (lvar), transform_local_var_uses, d,
> - NULL);
> -
> /* For capture proxies, this could include the decl value expr. */
> if (local_var.is_lambda_capture || local_var.has_value_expr_p)
> {
> @@ -1842,6 +1834,19 @@ transform_local_var_uses (tree *stmt, int *do_subtree, void *d)
> lvd->actor_frame, fld_ref, NULL_TREE);
> local_var.field_idx = fld_idx;
> }
> + /* Now we've built the revised var in the frame, substitute uses of
> + it in initializers and the bind expr body. */
> + for (lvar = BIND_EXPR_VARS (*stmt); lvar != NULL;
> + lvar = DECL_CHAIN (lvar))
> + {
> + /* we need to walk some of the decl trees, which might contain
> + references to vars replaced at a higher level. */
> + cp_walk_tree (&DECL_INITIAL (lvar), transform_local_var_uses, d,
> + NULL);
> + cp_walk_tree (&DECL_SIZE (lvar), transform_local_var_uses, d, NULL);
> + cp_walk_tree (&DECL_SIZE_UNIT (lvar), transform_local_var_uses, d,
> + NULL);
> + }
> cp_walk_tree (&BIND_EXPR_BODY (*stmt), transform_local_var_uses, d, NULL);
>
> /* Now we have processed and removed references to the original vars,
> diff --git a/gcc/testsuite/g++.dg/coroutines/pr94886-folly-3.C b/gcc/testsuite/g++.dg/coroutines/pr94886-folly-3.C
> new file mode 100644
> index 00000000000..d7bd2c17d4a
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/pr94886-folly-3.C
> @@ -0,0 +1,15 @@
> +
> +#include "coro.h"
> +#include "coro1-ret-int-yield-int.h"
> +
> +#include <array>
> +
> +coro1
> +my_coro ()
> +{
> + const std::array<int, 5> expectedValues = {{0, 3, 1, 4, 2}};
> +
> + for (int expectedValue : expectedValues) {
> + co_yield expectedValue;
> + }
> +}
>
--
Nathan Sidwell
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] coroutines: Fix handling of artificial vars [PR94886]
2020-04-30 13:46 ` Nathan Sidwell
@ 2020-04-30 20:52 ` Iain Sandoe
0 siblings, 0 replies; 3+ messages in thread
From: Iain Sandoe @ 2020-04-30 20:52 UTC (permalink / raw)
To: Nathan Sidwell; +Cc: GCC Patches
Nathan Sidwell <nathan@acm.org> wrote:
> On 4/30/20 9:24 AM, Iain Sandoe wrote:
>> The testcase ICEs because the range-based for generates three
>> artificial variables that need to be allocated to the coroutine
>> frame but, when walking the BIND_EXR that contains these, the
>> DECL_INITIAL for one of them refers to an entry appearing later,
>> which means that the frame entry hasn't been allocated when that
>> INITIAL is walked.
>
> Hm, the error seems to be the range for's construction, did you look at
> altering that so that such forward references do not occur? (range for is
> described as a src->src xform, so I'm sure it's possible to xform it into
> something a user could have written, which a forward-reference is not)
It looks like the range-for lowering doesn’t quite do what its header
comment says.
Tracked as PR 94897.
> your patch is ok, if that for change's not simple.
not something that could have been done in the last hours before branching
10 ;)
thanks
Iain
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-04-30 20:52 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-30 13:24 [PATCH] coroutines: Fix handling of artificial vars [PR94886] Iain Sandoe
2020-04-30 13:46 ` Nathan Sidwell
2020-04-30 20:52 ` 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).