[C PATCH] Fix up file-scope _Atomic expansion (PR c/65345)

[C PATCH] Fix up file-scope _Atomic expansion (PR c/65345)

[Marek Polacek]

The PR shows that the compiler ICEs whenever it tries to expand an atomic
operation at the file scope.  That happens because it creates temporaries 
via create_tmp_var, which also pushes the variable into the current binding,
but that can't work if current_function_decl is NULL.  The fix is I think to
only generate the temporaries during gimplification.  Turned out that the
TARGET_EXPRs are tailor-made for this, so I've used them along with changing
create_tmp_var calls to create_tmp_var_raw that does not push the variable
into the current binding.

But this wasn't enough to handle the following case:
_Atomic int i = 5;
void f (int a[i += 1]) {}
To make it work I had to tweak the artificial labels that build_atomic_assign
creates to not ICE in gimplification.  The comment in store_parm_decls sums
it up.  It uses walk_tree, but I think this will be only rarely exercised in
practice, if ever; I think programs using such a construction are thin on the
ground.

I tried comparing .gimple dumps with/without the patch on

_Atomic int q = 4;
void
f (void)
{
  q += 2;
}

and I see no code changes.

This is not a regression, so not sure if I shouldn't defer this patch to the
next stage1 at this juncture...

Comments?

Bootstrapped/regtested on x86_64-linux.

2015-03-12  Marek Polacek  <polacek@redhat.com>

	PR c/65345
	* c-decl.c (set_labels_context_r): New function.
	(store_parm_decls): Call it via walk_tree_without_duplicates.
	* c-typeck.c (convert_lvalue_to_rvalue): Use create_tmp_var_raw
	instead of create_tmp_var.  Build TARGET_EXPR instead of
	COMPOUND_EXPR.
	(build_atomic_assign): Use create_tmp_var_raw instead of
	create_tmp_var.  Build TARGET_EXPRs instead of MODIFY_EXPR.

	* gcc.dg/pr65345-1.c: New test.
	* gcc.dg/pr65345-2.c: New test.

--- gcc/c/c-decl.c
+++ gcc/c/c-decl.c
@@ -8799,6 +8799,21 @@ store_parm_decls_from (struct c_arg_info *arg_info)
   store_parm_decls ();
 }
 
+/* Called by walk_tree to look for and update context-less labels.  */
+
+static tree
+set_labels_context_r (tree *tp, int *walk_subtrees, void *data)
+{
+  if (TREE_CODE (*tp) == LABEL_EXPR
+      && DECL_CONTEXT (LABEL_EXPR_LABEL (*tp)) == NULL_TREE)
+    {
+      DECL_CONTEXT (LABEL_EXPR_LABEL (*tp)) = static_cast<tree>(data);
+      *walk_subtrees = 0;
+    }
+
+  return NULL_TREE;
+}
+
 /* Store the parameter declarations into the current function declaration.
    This is called after parsing the parameter declarations, before
    digesting the body of the function.
@@ -8853,7 +8868,21 @@ store_parm_decls (void)
      thus won't naturally see the SAVE_EXPR containing the increment.  All
      other pending sizes would be handled by gimplify_parameters.  */
   if (arg_info->pending_sizes)
-    add_stmt (arg_info->pending_sizes);
+    {
+      /* In very special circumstances, e.g. for code like
+	   _Atomic int i = 5;
+	   void f (int a[i += 2]) {}
+	 we need to execute the atomic assignment on function entry.
+	 But in this case, it is not just a straight store, it has the
+	 op= form, which means that build_atomic_assign has generated
+	 gotos, labels, etc.  Because at that time the function decl
+	 for F has not been created yet, those labels do not have any
+	 function context.  But we have the fndecl now, so update the
+	 labels accordingly.  gimplify_expr would crash otherwise.  */
+      walk_tree_without_duplicates (&arg_info->pending_sizes,
+				    set_labels_context_r, fndecl);
+      add_stmt (arg_info->pending_sizes);
+    }
 }
 
 /* Store PARM_DECLs in PARMS into scope temporarily.  Used for
--- gcc/c/c-typeck.c
+++ gcc/c/c-typeck.c
@@ -2039,7 +2039,7 @@ convert_lvalue_to_rvalue (location_t loc, struct c_expr exp,
       /* Remove the qualifiers for the rest of the expressions and
 	 create the VAL temp variable to hold the RHS.  */
       nonatomic_type = build_qualified_type (expr_type, TYPE_UNQUALIFIED);
-      tmp = create_tmp_var (nonatomic_type);
+      tmp = create_tmp_var_raw (nonatomic_type);
       tmp_addr = build_unary_op (loc, ADDR_EXPR, tmp, 0);
       TREE_ADDRESSABLE (tmp) = 1;
       TREE_NO_WARNING (tmp) = 1;
@@ -2055,7 +2055,8 @@ convert_lvalue_to_rvalue (location_t loc, struct c_expr exp,
       mark_exp_read (exp.value);
 
       /* Return tmp which contains the value loaded.  */
-      exp.value = build2 (COMPOUND_EXPR, nonatomic_type, func_call, tmp);
+      exp.value = build4 (TARGET_EXPR, nonatomic_type, tmp, func_call,
+			  NULL_TREE, NULL_TREE);
     }
   return exp;
 }
@@ -3686,10 +3687,11 @@ build_atomic_assign (location_t loc, tree lhs, enum tree_code modifycode,
      the VAL temp variable to hold the RHS.  */
   nonatomic_lhs_type = build_qualified_type (lhs_type, TYPE_UNQUALIFIED);
   nonatomic_rhs_type = build_qualified_type (rhs_type, TYPE_UNQUALIFIED);
-  val = create_tmp_var (nonatomic_rhs_type);
+  val = create_tmp_var_raw (nonatomic_rhs_type);
   TREE_ADDRESSABLE (val) = 1;
   TREE_NO_WARNING (val) = 1;
-  rhs = build2 (MODIFY_EXPR, nonatomic_rhs_type, val, rhs);
+  rhs = build4 (TARGET_EXPR, nonatomic_rhs_type, val, rhs, NULL_TREE,
+		NULL_TREE);
   SET_EXPR_LOCATION (rhs, loc);
   add_stmt (rhs);
 
@@ -3715,12 +3717,12 @@ build_atomic_assign (location_t loc, tree lhs, enum tree_code modifycode,
     }
 
   /* Create the variables and labels required for the op= form.  */
-  old = create_tmp_var (nonatomic_lhs_type);
+  old = create_tmp_var_raw (nonatomic_lhs_type);
   old_addr = build_unary_op (loc, ADDR_EXPR, old, 0);
   TREE_ADDRESSABLE (old) = 1;
   TREE_NO_WARNING (old) = 1;
 
-  newval = create_tmp_var (nonatomic_lhs_type);
+  newval = create_tmp_var_raw (nonatomic_lhs_type);
   newval_addr = build_unary_op (loc, ADDR_EXPR, newval, 0);
   TREE_ADDRESSABLE (newval) = 1;
 
@@ -3736,7 +3738,9 @@ build_atomic_assign (location_t loc, tree lhs, enum tree_code modifycode,
   params->quick_push (old_addr);
   params->quick_push (seq_cst);
   func_call = c_build_function_call_vec (loc, vNULL, fndecl, params, NULL);
-  add_stmt (func_call);
+  old = build4 (TARGET_EXPR, nonatomic_lhs_type, old, func_call, NULL_TREE,
+		NULL_TREE);
+  add_stmt (old);
   params->truncate (0);
 
   /* Create the expressions for floating-point environment
@@ -3755,12 +3759,14 @@ build_atomic_assign (location_t loc, tree lhs, enum tree_code modifycode,
 
   /* newval = old + val;  */
   rhs = build_binary_op (loc, modifycode, old, val, 1);
+  rhs = c_fully_fold (rhs, false, NULL);
   rhs = convert_for_assignment (loc, UNKNOWN_LOCATION, nonatomic_lhs_type,
 				rhs, NULL_TREE, ic_assign, false, NULL_TREE,
 				NULL_TREE, 0);
   if (rhs != error_mark_node)
     {
-      rhs = build2 (MODIFY_EXPR, nonatomic_lhs_type, newval, rhs);
+      rhs = build4 (TARGET_EXPR, nonatomic_lhs_type, newval, rhs, NULL_TREE,
+		    NULL_TREE);
       SET_EXPR_LOCATION (rhs, loc);
       add_stmt (rhs);
     }
@@ -3782,7 +3788,7 @@ build_atomic_assign (location_t loc, tree lhs, enum tree_code modifycode,
   stmt = build3 (COND_EXPR, void_type_node, func_call, goto_stmt, NULL_TREE);
   SET_EXPR_LOCATION (stmt, loc);
   add_stmt (stmt);
-  
+
   if (clear_call)
     add_stmt (clear_call);
 
@@ -3790,7 +3796,7 @@ build_atomic_assign (location_t loc, tree lhs, enum tree_code modifycode,
   goto_stmt  = build1 (GOTO_EXPR, void_type_node, loop_decl);
   SET_EXPR_LOCATION (goto_stmt, loc);
   add_stmt (goto_stmt);
- 
+
   /* done:  */
   add_stmt (done_label);
 
--- gcc/testsuite/gcc.dg/pr65345-1.c
+++ gcc/testsuite/gcc.dg/pr65345-1.c
@@ -0,0 +1,35 @@
+/* PR c/65345 */
+/* { dg-do compile } */
+/* { dg-options "" } */
+
+_Atomic int i = 3;
+
+int a1 = sizeof (i + 1);
+int a2 = sizeof (i = 0);
+int a3 = sizeof (i++);
+int a4 = sizeof (i--);
+int a5 = sizeof (-i);
+
+int b1 = _Alignof (i + 1);
+int b2 = _Alignof (i = 0);
+int b3 = _Alignof (i++);
+int b4 = _Alignof (i--);
+int b5 = _Alignof (-i);
+
+int c1 = i; /* { dg-error "initializer element is not constant" } */
+int c2 = (i ? 1 : 2); /* { dg-error "initializer element is not constant" } */
+int c3[i]; /* { dg-error "variably modified" } */
+int c4 = 0 || i; /* { dg-error "initializer element is not constant" } */
+int c5 = (i += 10); /* { dg-error "initializer element is not constant" } */
+
+_Static_assert (_Generic (i, int: 1, default: 0) == 1, "1");
+_Static_assert (_Generic (i + 1, int: 1, default: 0) == 1, "2");
+_Static_assert (_Generic (i = 0, int: 1, default: 0) == 1, "3");
+_Static_assert (_Generic (i++, int: 1, default: 0) == 1, "4");
+_Static_assert (_Generic (i--, int: 1, default: 0) == 1, "5");
+
+void fn1 (int a[i + 1]);
+void fn2 (int a[i = 0]);
+void fn3 (int a[i++]);
+void fn4 (int a[i--]);
+void fn5 (int a[-i]);
--- gcc/testsuite/gcc.dg/pr65345-2.c
+++ gcc/testsuite/gcc.dg/pr65345-2.c
@@ -0,0 +1,59 @@
+/* PR c/65345 */
+/* { dg-do run } */
+/* { dg-options "" } */
+
+#define CHECK(X) if (!(X)) __builtin_abort ()
+
+_Atomic int i = 5;
+_Atomic int j = 2;
+
+void
+fn1 (int a[i = 0])
+{
+}
+
+void
+fn2 (int a[i += 2])
+{
+}
+
+void
+fn3 (int a[++i])
+{
+}
+
+void
+fn4 (int a[++i])
+{
+}
+
+void
+fn5 (int a[++i][j = 10])
+{
+}
+
+void
+fn6 (int a[i = 7][j--])
+{
+}
+
+int
+main ()
+{
+  int a[10];
+  int aa[10][10];
+  fn1 (a);
+  CHECK (i == 0);
+  fn2 (a);
+  CHECK (i == 2);
+  fn3 (a);
+  CHECK (i == 3);
+  fn4 (a);
+  CHECK (i == 4);
+  fn5 (aa);
+  CHECK (i == 5);
+  CHECK (j == 10);
+  fn6 (aa);
+  CHECK (i == 7);
+  CHECK (j == 9);
+}

	Marek

Re: [C PATCH] Fix up file-scope _Atomic expansion (PR c/65345)

[Jeff Law]

On 03/12/2015 11:37 AM, Marek Polacek wrote:
> The PR shows that the compiler ICEs whenever it tries to expand an atomic
> operation at the file scope.  That happens because it creates temporaries
> via create_tmp_var, which also pushes the variable into the current binding,
> but that can't work if current_function_decl is NULL.  The fix is I think to
> only generate the temporaries during gimplification.  Turned out that the
> TARGET_EXPRs are tailor-made for this, so I've used them along with changing
> create_tmp_var calls to create_tmp_var_raw that does not push the variable
> into the current binding.
>
> But this wasn't enough to handle the following case:
> _Atomic int i = 5;
> void f (int a[i += 1]) {}
> To make it work I had to tweak the artificial labels that build_atomic_assign
> creates to not ICE in gimplification.  The comment in store_parm_decls sums
> it up.  It uses walk_tree, but I think this will be only rarely exercised in
> practice, if ever; I think programs using such a construction are thin on the
> ground.
>
> I tried comparing .gimple dumps with/without the patch on
>
> _Atomic int q = 4;
> void
> f (void)
> {
>    q += 2;
> }
>
> and I see no code changes.
>
> This is not a regression, so not sure if I shouldn't defer this patch to the
> next stage1 at this juncture...
>
> Comments?
>
> Bootstrapped/regtested on x86_64-linux.
>
> 2015-03-12  Marek Polacek  <polacek@redhat.com>
>
> 	PR c/65345
> 	* c-decl.c (set_labels_context_r): New function.
> 	(store_parm_decls): Call it via walk_tree_without_duplicates.
> 	* c-typeck.c (convert_lvalue_to_rvalue): Use create_tmp_var_raw
> 	instead of create_tmp_var.  Build TARGET_EXPR instead of
> 	COMPOUND_EXPR.
> 	(build_atomic_assign): Use create_tmp_var_raw instead of
> 	create_tmp_var.  Build TARGET_EXPRs instead of MODIFY_EXPR.
>
> 	* gcc.dg/pr65345-1.c: New test.
> 	* gcc.dg/pr65345-2.c: New test.
My inclination is to defer unless it's painfully safe (like your removal 
of unused functions) or it's a regression of some sorts.

jeff

Re: [C PATCH] Fix up file-scope _Atomic expansion (PR c/65345)

[Marek Polacek]

On Tue, Mar 17, 2015 at 01:32:20PM -0600, Jeff Law wrote:
> My inclination is to defer unless it's painfully safe (like your removal of
> unused functions) or it's a regression of some sorts.

I think I also slightly prefer deferring this patch; it's not exactly in the
"painfully safe" category.

	Marek

Re: [C PATCH] Fix up file-scope _Atomic expansion (PR c/65345)

[Joseph Myers]

On Thu, 12 Mar 2015, Marek Polacek wrote:

> 2015-03-12  Marek Polacek  <polacek@redhat.com>
> 
> 	PR c/65345
> 	* c-decl.c (set_labels_context_r): New function.
> 	(store_parm_decls): Call it via walk_tree_without_duplicates.
> 	* c-typeck.c (convert_lvalue_to_rvalue): Use create_tmp_var_raw
> 	instead of create_tmp_var.  Build TARGET_EXPR instead of
> 	COMPOUND_EXPR.
> 	(build_atomic_assign): Use create_tmp_var_raw instead of
> 	create_tmp_var.  Build TARGET_EXPRs instead of MODIFY_EXPR.
> 
> 	* gcc.dg/pr65345-1.c: New test.
> 	* gcc.dg/pr65345-2.c: New test.

OK for stage 1, but I think you may need a further patch that fixes the 
TARGET_ATOMIC_ASSIGN_EXPAND_FENV implementations to use create_tmp_var_raw 
and TARGET_EXPR as well, along with adding tests for floating-point 
compound assignment / increment / decrement in the problem contexts.  A 
backport to GCC 5 branch could be considered after a while on trunk.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [C PATCH] Fix up file-scope _Atomic expansion (PR c/65345)

[Marek Polacek]

On Thu, Mar 12, 2015 at 06:37:48PM +0100, Marek Polacek wrote:
> This is not a regression, so not sure if I shouldn't defer this patch to the
> next stage1 at this juncture...

I've committed this patch now after another regtest/bootstrap (x86_64-linux).

	Marek