Tested on x86_64-darwin, x86_64, powerpc64-linux,
OK for trunk?
thanks
Iain
--- 8< ---
Recent changes in the C++ FE and the coroutines implementation have
exposed a latent issue in which a bind expression containing a var
that we need to capture in the coroutine state gets visited twice.
This causes an ICE (from a checking assert). Fixed by adding a pset
to the relevant tree walk. Exit the callback early when the tree is
not a BIND_EXPR.
PR c++/98935
gcc/cp/ChangeLog:
* coroutines.cc (register_local_var_uses): Add a pset to the
tree walk to avoid visiting the same BIND_EXPR twice. Make
an early exit for cases that the callback does not apply.
(cp_coroutine_transform::apply_transforms): Add a pset to the
tree walk for register_local_vars.
gcc/testsuite/ChangeLog:
* g++.dg/coroutines/pr98935.C: New test.
Signed-off-by: Iain Sandoe <i...@sandoe.co.uk>
---
gcc/cp/coroutines.cc | 158 +++++++++++-----------
gcc/testsuite/g++.dg/coroutines/pr98935.C | 27 ++++
2 files changed, 107 insertions(+), 78 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/coroutines/pr98935.C
diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index 5764475a7de..f6f6256114a 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -4055,6 +4055,9 @@ coro_make_frame_entry (tree *field_list, const char
*name, tree fld_type,
static tree
register_local_var_uses (tree *stmt, int *do_subtree, void *d)
{
+ if (TREE_CODE (*stmt) != BIND_EXPR)
+ return NULL_TREE;
+
local_vars_frame_data *lvd = (local_vars_frame_data *) d;
/* As we enter a bind expression - record the vars there and then recurse.
@@ -4062,88 +4065,86 @@ register_local_var_uses (tree *stmt, int *do_subtree,
void *d)
The bind index is a growing count of how many bind indices we've seen.
We build a space in the frame for each local var. */
- if (TREE_CODE (*stmt) == BIND_EXPR)
+ tree lvar;
+ unsigned serial = 0;
+ for (lvar = BIND_EXPR_VARS (*stmt); lvar != NULL; lvar = DECL_CHAIN (lvar))
{
- tree lvar;
- unsigned serial = 0;
- for (lvar = BIND_EXPR_VARS (*stmt); lvar != NULL;
- lvar = DECL_CHAIN (lvar))
- {
- bool existed;
- local_var_info &local_var
- = lvd->local_var_uses->get_or_insert (lvar, &existed);
- gcc_checking_assert (!existed);
- local_var.def_loc = DECL_SOURCE_LOCATION (lvar);
- tree lvtype = TREE_TYPE (lvar);
- local_var.frame_type = lvtype;
- local_var.field_idx = local_var.field_id = NULL_TREE;
-
- /* Make sure that we only present vars to the tests below. */
- if (TREE_CODE (lvar) != PARM_DECL
- && TREE_CODE (lvar) != VAR_DECL)
- continue;
-
- /* We don't move static vars into the frame. */
- local_var.is_static = TREE_STATIC (lvar);
- if (local_var.is_static)
- continue;
-
- poly_uint64 size;
- if (TREE_CODE (lvtype) == ARRAY_TYPE
- && !poly_int_tree_p (DECL_SIZE_UNIT (lvar), &size))
- {
- sorry_at (local_var.def_loc, "variable length arrays are not"
- " yet supported in coroutines");
- /* Ignore it, this is broken anyway. */
- continue;
- }
+ bool existed;
+ local_var_info &local_var
+ = lvd->local_var_uses->get_or_insert (lvar, &existed);
+ gcc_checking_assert (!existed);
+ local_var.def_loc = DECL_SOURCE_LOCATION (lvar);
+ tree lvtype = TREE_TYPE (lvar);
+ local_var.frame_type = lvtype;
+ local_var.field_idx = local_var.field_id = NULL_TREE;
+
+ /* Make sure that we only present vars to the tests below. */
+ if (TREE_CODE (lvar) != PARM_DECL
+ && TREE_CODE (lvar) != VAR_DECL)
+ continue;
- lvd->local_var_seen = true;
- /* If this var is a lambda capture proxy, we want to leave it alone,
- and later rewrite the DECL_VALUE_EXPR to indirect through the
- frame copy of the pointer to the lambda closure object. */
- local_var.is_lambda_capture = is_capture_proxy (lvar);
- if (local_var.is_lambda_capture)
- continue;
-
- /* If a variable has a value expression, then that's what needs
- to be processed. */
- local_var.has_value_expr_p = DECL_HAS_VALUE_EXPR_P (lvar);
- if (local_var.has_value_expr_p)
- continue;
-
- /* Make names depth+index unique, so that we can support nested
- scopes with identically named locals and still be able to
- identify them in the coroutine frame. */
- tree lvname = DECL_NAME (lvar);
- char *buf = NULL;
-
- /* The outermost bind scope contains the artificial variables that
- we inject to implement the coro state machine. We want to be able
- to inspect these in debugging. */
- if (lvname != NULL_TREE && lvd->nest_depth == 0)
- buf = xasprintf ("%s", IDENTIFIER_POINTER (lvname));
- else if (lvname != NULL_TREE)
- buf = xasprintf ("%s_%u_%u", IDENTIFIER_POINTER (lvname),
- lvd->nest_depth, lvd->bind_indx);
- else
- buf = xasprintf ("_D%u_%u_%u", lvd->nest_depth, lvd->bind_indx,
- serial++);
+ /* We don't move static vars into the frame. */
+ local_var.is_static = TREE_STATIC (lvar);
+ if (local_var.is_static)
+ continue;
- /* TODO: Figure out if we should build a local type that has any
- excess alignment or size from the original decl. */
- local_var.field_id = coro_make_frame_entry (lvd->field_list, buf,
- lvtype, lvd->loc);
- free (buf);
- /* We don't walk any of the local var sub-trees, they won't contain
- any bind exprs. */
+ poly_uint64 size;
+ if (TREE_CODE (lvtype) == ARRAY_TYPE
+ && !poly_int_tree_p (DECL_SIZE_UNIT (lvar), &size))
+ {
+ sorry_at (local_var.def_loc, "variable length arrays are not"
+ " yet supported in coroutines");
+ /* Ignore it, this is broken anyway. */
+ continue;
}
- lvd->bind_indx++;
- lvd->nest_depth++;
- cp_walk_tree (&BIND_EXPR_BODY (*stmt), register_local_var_uses, d, NULL);
- *do_subtree = 0; /* We've done this. */
- lvd->nest_depth--;
+
+ lvd->local_var_seen = true;
+ /* If this var is a lambda capture proxy, we want to leave it alone,
+ and later rewrite the DECL_VALUE_EXPR to indirect through the
+ frame copy of the pointer to the lambda closure object. */
+ local_var.is_lambda_capture = is_capture_proxy (lvar);
+ if (local_var.is_lambda_capture)
+ continue;
+
+ /* If a variable has a value expression, then that's what needs
+ to be processed. */
+ local_var.has_value_expr_p = DECL_HAS_VALUE_EXPR_P (lvar);
+ if (local_var.has_value_expr_p)
+ continue;
+
+ /* Make names depth+index unique, so that we can support nested
+ scopes with identically named locals and still be able to
+ identify them in the coroutine frame. */
+ tree lvname = DECL_NAME (lvar);
+ char *buf = NULL;
+
+ /* The outermost bind scope contains the artificial variables that
+ we inject to implement the coro state machine. We want to be able
+ to inspect these in debugging. */
+ if (lvname != NULL_TREE && lvd->nest_depth == 0)
+ buf = xasprintf ("%s", IDENTIFIER_POINTER (lvname));
+ else if (lvname != NULL_TREE)
+ buf = xasprintf ("%s_%u_%u", IDENTIFIER_POINTER (lvname),
+ lvd->nest_depth, lvd->bind_indx);
+ else
+ buf = xasprintf ("_D%u_%u_%u", lvd->nest_depth, lvd->bind_indx,
+ serial++);
+
+ /* TODO: Figure out if we should build a local type that has any
+ excess alignment or size from the original decl. */
+ local_var.field_id = coro_make_frame_entry (lvd->field_list, buf,
+ lvtype, lvd->loc);
+ free (buf);
+ /* We don't walk any of the local var sub-trees, they won't contain
+ any bind exprs. */
}
+ lvd->bind_indx++;
+ lvd->nest_depth++;
+ /* Ensure we only visit each expression once. */
+ hash_set<tree> pset;
+ cp_walk_tree (&BIND_EXPR_BODY (*stmt), register_local_var_uses, d, &pset);
+ *do_subtree = 0; /* We've done this. */
+ lvd->nest_depth--;
return NULL_TREE;
}
@@ -5332,7 +5333,8 @@ cp_coroutine_transform::apply_transforms ()
/* Determine the fields for the coroutine state. */
tree field_list = NULL_TREE;
local_vars_frame_data local_vars_data (&field_list, &local_var_uses);
- cp_walk_tree (&coroutine_body, register_local_var_uses, &local_vars_data,
NULL);
+ hash_set<tree> pset;
+ cp_walk_tree (&coroutine_body, register_local_var_uses, &local_vars_data,
&pset);
/* Conservative computation of the coroutine frame content. */
frame_type = begin_class_definition (frame_type);
diff --git a/gcc/testsuite/g++.dg/coroutines/pr98935.C
b/gcc/testsuite/g++.dg/coroutines/pr98935.C
new file mode 100644
index 00000000000..57c7d7d12a8
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/pr98935.C
@@ -0,0 +1,27 @@
+// { dg-additional-options "-Wno-pedantic" }
+
+#if __has_include(<coroutine>)
+#include <coroutine>
+#elif __has_include(<experimental/coroutine>)
+#include <experimental/coroutine>
+namespace std {
+using namespace std::experimental;
+}
+#endif
+
+struct future {
+ struct promise_type {
+ void return_void() {}
+ std::suspend_always initial_suspend() noexcept { return {}; }
+ std::suspend_always final_suspend() noexcept { return {}; }
+ void unhandled_exception() {}
+ future get_return_object() { return {}; }
+ };
+ bool await_ready() { return false; }
+ void await_suspend(std::coroutine_handle<>) {}
+ int await_resume() { return 42; }
+};
+
+future failcase() {
+ co_await ({auto x = future{}; x;});
+}
--
2.39.2 (Apple Git-143)
