diff mbox series

[coroutines] member slicing fix

Message ID 406a8452-08f5-19ac-8071-06ca842b2dea@acm.org
State New
Headers show
Series [coroutines] member slicing fix | expand

Commit Message

Nathan Sidwell Feb. 28, 2022, 5:42 p.m. UTC
Iain,
this is the first of 2 patches I needed on top of your github WIP 
series.  It is against GCC-10, but AFAICT will also be needed on trunk.

We were not descending into the object operand of COMPONENT_REF exprs, 
and end up bitcopying a field out of a temporary, leading to badness. 
The breaking out of replace_unary_preamble is merely coincidental 
simplification.

I also think one needs to descend the second operand of COMPOUND_EXPR, 
but when I did that badness happens, probably because of the artificial 
COMPOUND_EXPRs that get generated, which you mentioned?

The bug manifested itself in Folly, and it was too difficult to extract 
a testcase.

nathan
diff mbox series

Patch


Summary:
The ported coroutine patches still contained a bug.  It could
   cause object slicing and bit copying when the compiler got confused
   about the lifetime of a member access.  It needs to look at the object
   being accessed, not just the data member.


diff --git a/9.x/src/gcc-10.x/gcc/cp/coroutines.cc b/9.x/src/gcc-10.x/gcc/cp/coroutines.cc
index d0f292f2a6..ee004d5b98 100644
--- a/9.x/src/gcc-10.x/gcc/cp/coroutines.cc
+++ b/9.x/src/gcc-10.x/gcc/cp/coroutines.cc
@@ -3007,6 +3007,7 @@  struct coro_flattened_statement {
 
   static tree *find_unary_preamble (tree, bool&);
   static bool expr_result_needs_frame_temp (tree);
+  static tree replace_unary_preamble (tree unary, tree replace);
 
   tree get_flattened_statement (tree);
   void build_flattened_statement ();
@@ -3565,15 +3566,15 @@  tree *
 coro_flattened_statement::find_unary_preamble (tree expr, bool& addr_taken)
 {
   tree *non_u_ptr = NULL;
-  bool done = false;
 
   /* Find the first non-unary operation in an expression.  */
-  while (!done)
+  for (;;)
     if (UNARY_CLASS_P (expr)
-	|| (EXPRESSION_CLASS_P (expr)
-	    && TREE_CODE_LENGTH (TREE_CODE (expr)) == 1)
 	|| TREE_CODE (expr) == INDIRECT_REF
-	|| TREE_CODE (expr) == VIEW_CONVERT_EXPR)
+	|| TREE_CODE (expr) == VIEW_CONVERT_EXPR
+	|| TREE_CODE (expr) == COMPONENT_REF
+	|| (EXPRESSION_CLASS_P (expr)
+	    && TREE_CODE_LENGTH (TREE_CODE (expr)) == 1))
       {
 	if (TREE_CODE (expr) == STMT_EXPR
 	    || TREE_CODE (expr) == CLEANUP_POINT_EXPR)
@@ -3582,14 +3583,24 @@  coro_flattened_statement::find_unary_preamble (tree expr, bool& addr_taken)
 	if (TREE_CODE (expr) == ADDR_EXPR)
 	  addr_taken = true;
 	non_u_ptr = &TREE_OPERAND (expr, 0);
-	expr = TREE_OPERAND (expr, 0);
+	expr = *non_u_ptr;
       }
     else
-      done = true;
+      break;
 
   return non_u_ptr;
 }
 
+tree coro_flattened_statement::replace_unary_preamble (tree unary, tree replace)
+{
+  tree *slot = &unary;
+  while (*slot)
+    slot = &TREE_OPERAND (*slot, 0);
+  *slot = replace;
+
+  return unary;
+}
+
 /* EXPR is an expression with a result that must persist across one or more
    suspension points, does this need a frame var?  */
 
@@ -3639,15 +3650,7 @@  coro_flattened_statement::flatten_aggr_init (var_nest_node *t, tree expr,
     handle_call_param (t, &AGGR_INIT_EXPR_ARG (expr, p_num), "AI");
 
   /* [A = ] <unary ops> revised_call.  */
-  if (unary_preamble)
-    {
-      gcc_checking_assert (!VOID_TYPE_P (TREE_TYPE (expr)));
-      tree x_op = unary_preamble;
-      while (TREE_OPERAND (x_op, 0))
-	x_op = TREE_OPERAND (x_op, 0);
-      TREE_OPERAND (x_op, 0) = expr;
-      expr = unary_preamble;
-    }
+  expr = replace_unary_preamble (unary_preamble, expr);
 
   if (!discarded)
     expr = build2_loc (loc, expr_code, TREE_TYPE (t->var), t->var, expr);
@@ -3710,14 +3713,7 @@  coro_flattened_statement::flatten_await (var_nest_node *t, tree expr,
   flatten_await_inner (t, expr);
 
   /* [A = ] <unary ops> expr.  */
-  if (unary_preamble)
-    {
-      tree x_op = unary_preamble;
-      while (TREE_OPERAND (x_op, 0))
-	x_op = TREE_OPERAND (x_op, 0);
-      TREE_OPERAND (x_op, 0) = expr;
-      expr = unary_preamble;
-    }
+  expr = replace_unary_preamble (unary_preamble, expr);
 
   if (!discarded)
     expr = build2_loc (loc, expr_code, TREE_TYPE (t->var), t->var, expr);
@@ -3790,14 +3786,7 @@  coro_flattened_statement::flatten_binary (var_nest_node *t, tree expr,
       flatten_expression (ins);  /* Recurse into the second sub-expr...  */
     }
 
-  if (unary_preamble)
-    {
-      tree x_op = unary_preamble;
-      while (TREE_OPERAND (x_op, 0))
-	x_op = TREE_OPERAND (x_op, 0);
-      TREE_OPERAND (x_op, 0) = expr;
-      expr = unary_preamble;
-    }
+  expr = replace_unary_preamble (unary_preamble, expr);
 
   if (!discarded)
     expr = build2_loc (loc, expr_code, TREE_TYPE (t->var), t->var, expr);
@@ -3890,14 +3879,7 @@  coro_flattened_statement::flatten_call (var_nest_node *t, tree expr,
     handle_call_param (t, &CALL_EXPR_ARG(expr, p_num), "CT");
 
   /* [A = ] <unary ops> revised_call.  */
-  if (unary_preamble)
-    {
-      tree x_op = unary_preamble;
-      while (TREE_OPERAND (x_op, 0))
-	x_op = TREE_OPERAND (x_op, 0);
-      TREE_OPERAND (x_op, 0) = expr;
-      expr = unary_preamble;
-    }
+  expr = replace_unary_preamble (unary_preamble, expr);
 
   if (!discarded)
     expr = build2_loc (loc, expr_code, TREE_TYPE (t->var), t->var, expr);
@@ -3933,16 +3915,7 @@  coro_flattened_statement::flatten_cleanup (var_nest_node *t, tree expr,
   flatten_expression (t->then_cl);  /* Recurse into the sub-expr...  */
 
   /* [A = ] <unary ops> tmp.  */
-  if (unary_preamble)
-    {
-      tree x_op = unary_preamble;
-      while (TREE_OPERAND (x_op, 0))
-	x_op = TREE_OPERAND (x_op, 0);
-      TREE_OPERAND (x_op, 0) = tmp;
-      expr = unary_preamble;
-    }
-  else
-    expr = tmp;
+  expr = replace_unary_preamble (unary_preamble, tmp);
   if (!discarded)
     expr = build2_loc (loc, expr_code, TREE_TYPE (t->var), t->var, tmp);
   t->init = expr;
@@ -3988,14 +3961,7 @@  coro_flattened_statement::flatten_compound (var_nest_node *t, tree expr,
 
   /* The second operand of the compound becomes the new expression.  */
   tree second = TREE_OPERAND (expr, 1);
-  if (unary_preamble)
-    {
-      tree x_op = unary_preamble;
-      while (TREE_OPERAND (x_op, 0))
-	x_op = TREE_OPERAND (x_op, 0);
-      TREE_OPERAND (x_op, 0) = second;
-      second = unary_preamble;
-    }
+  second = replace_unary_preamble (unary_preamble, second);
 
   if (!discarded)
     second = build2_loc (loc, expr_code, TREE_TYPE (t->var), t->var, second);
@@ -4031,14 +3997,7 @@  coro_flattened_statement::flatten_constructor (var_nest_node *t, tree expr,
     }
 
   /* Reassemble the original.  */
-  if (unary_preamble)
-    {
-      tree x_op = unary_preamble;
-      while (TREE_OPERAND (x_op, 0))
-	x_op = TREE_OPERAND (x_op, 0);
-      TREE_OPERAND (x_op, 0) = expr;
-      expr = unary_preamble;
-    }
+  expr = replace_unary_preamble (unary_preamble, expr);
 
   if (!discarded)
     expr = build2_loc (loc, expr_code, TREE_TYPE (t->var), t->var, expr);
@@ -4107,10 +4066,8 @@  coro_flattened_statement::flatten_target (var_nest_node *t, tree expr,
       /* There must be an await expression in here, so recurse with a 'bare'
 	 target expr.  */
       flatten_expression (ins);
-      tree x_op = unary_preamble;
-      while (TREE_OPERAND (x_op, 0))
-	x_op = TREE_OPERAND (x_op, 0);
-      TREE_OPERAND (x_op, 0) = var;
+      gcc_assert (unary_preamble);
+      replace_unary_preamble (unary_preamble, var);
       expr = unary_preamble;
       if (!discarded)
 	expr = build2_loc (loc, expr_code, TREE_TYPE (t->var), t->var, expr);
@@ -4225,21 +4182,10 @@  coro_flattened_statement::flatten_ternary (var_nest_node *t, tree expr,
 		      ? copy_node (unary_preamble)
 		      : unary_preamble;
 	  if (apply_uops_then)
-	    {
-	      tree x_op = t_op;
-	      while (TREE_OPERAND (x_op, 0))
-		x_op = TREE_OPERAND (x_op, 0);
-	      TREE_OPERAND (x_op, 0) = then_;
-	      then_ = t_op;
-	    }
+	    then_ = replace_unary_preamble (t_op, then_);
+
 	  if (apply_uops_else)
-	    {
-	      tree x_op = unary_preamble;
-	      while (TREE_OPERAND (x_op, 0))
-		x_op = TREE_OPERAND (x_op, 0);
-	      TREE_OPERAND (x_op, 0) = else_;
-	      else_ = unary_preamble;
-	    }
+	    else_ = replace_unary_preamble (unary_preamble, else_);
 	}
 
       if (!discarded && apply_uops_then)
@@ -4270,14 +4216,7 @@  coro_flattened_statement::flatten_ternary (var_nest_node *t, tree expr,
     {
       /* The only replacement is the condition, so that we preserve the ternary
 	 and any unary preamble.  */
-      if (unary_preamble)
-	{
-	  tree x_op = unary_preamble;
-	  while (TREE_OPERAND (x_op, 0))
-	    x_op = TREE_OPERAND (x_op, 0);
-	  TREE_OPERAND (x_op, 0) = expr;
-	  expr = unary_preamble;
-	}
+      expr = replace_unary_preamble (unary_preamble, expr);
       if (!discarded)
 	expr = build2_loc (loc, expr_code, res_type, t->var, expr);
     }
@@ -4361,10 +4300,7 @@  coro_flattened_statement::flatten_truth_if (var_nest_node *t, tree expr,
       ins->then_cl = then_cl;
       ins->synthesized_var_p = synthetic;
       /* <unary ops> x.  */
-      tree x_op = unary_preamble;
-      while (TREE_OPERAND (x_op, 0))
-	x_op = TREE_OPERAND (x_op, 0);
-      TREE_OPERAND (x_op, 0) = var;
+      replace_unary_preamble (unary_preamble, var);
       if (!discarded)
 	init = build2_loc (loc, expr_code, expr_type, t->var, unary_preamble);
       else
@@ -4454,11 +4390,7 @@  coro_flattened_statement::flatten_expression (var_nest_node *t)
       else if (unary_preamble)
 	{
 	  /* Undo the split.  */
-	  tree x_op = unary_preamble;
-	  while (TREE_OPERAND (x_op, 0))
-	    x_op = TREE_OPERAND (x_op, 0);
-	  TREE_OPERAND (x_op, 0) = expr;
-	  expr = unary_preamble;
+	  expr = replace_unary_preamble (unary_preamble, expr);
 	}
       if (!discarded)
 	expr = build2_loc (loc, expr_code, TREE_TYPE (t->var), t->var, expr);
-- 
2.30.2