c++, coroutines:Ensure bind exprs are visited once [PR98935].

Message ID 20241129134532.51940-1-iain@sandoe.co.uk
State New
Headers
Series c++, coroutines:Ensure bind exprs are visited once [PR98935]. |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gcc_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Build passed

Commit Message

Iain Sandoe Nov. 29, 2024, 1:45 p.m. UTC
  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 <iain@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
  

Patch

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;});
+}