diff mbox series

c: FIx up -Wunused-but-set-* warnings for _Atomics [PR99588]

Message ID 20210319093447.GE231854@tucnak
State New
Headers show
Series c: FIx up -Wunused-but-set-* warnings for _Atomics [PR99588] | expand

Commit Message

Jakub Jelinek March 19, 2021, 9:34 a.m. UTC 
Hi!

As the following testcases show, compared to -D_Atomic= case we have many
-Wunused-but-set-* warning false positives.
When an _Atomic variable/parameter is read, we call mark_exp_read on it in
convert_lvalue_to_rvalue, but build_atomic_assign does not.
For consistency with the non-_Atomic case where we mark_exp_read the lhs
for lhs op= ... but not for lhs = ..., this patch does that too.
But furthermore we need to pattern match the trees emitted by _Atomic store,
so that _Atomic store itself is not marked as being a variable read, but
when the result of the store is used, we mark it.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2021-03-18  Jakub Jelinek  <jakub@redhat.com>

	PR c/99588
	* c-typeck.c (mark_exp_read): Recognize what build_atomic_assign
	with modifycode NOP_EXPR produces and mark the _Atomic var as read
	if found.
	(build_atomic_assign): For modifycode of NOP_EXPR, use COMPOUND_EXPRs
	rather than STATEMENT_LIST.  Otherwise call mark_exp_read on lhs.
	Set TREE_SIDE_EFFECTS on the TARGET_EXPR.

	* gcc.dg/Wunused-var-5.c: New test.
	* gcc.dg/Wunused-var-6.c: New test.


	Jakub

Comments

Joseph Myers March 19, 2021, 9:50 p.m. UTC | #1 
On Fri, 19 Mar 2021, Jakub Jelinek via Gcc-patches wrote:

> Hi!
> 
> As the following testcases show, compared to -D_Atomic= case we have many
> -Wunused-but-set-* warning false positives.
> When an _Atomic variable/parameter is read, we call mark_exp_read on it in
> convert_lvalue_to_rvalue, but build_atomic_assign does not.
> For consistency with the non-_Atomic case where we mark_exp_read the lhs
> for lhs op= ... but not for lhs = ..., this patch does that too.
> But furthermore we need to pattern match the trees emitted by _Atomic store,
> so that _Atomic store itself is not marked as being a variable read, but
> when the result of the store is used, we mark it.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

OK.
diff mbox series

Patch

--- gcc/c/c-typeck.c.jj	2021-03-16 18:45:39.643043766 +0100
+++ gcc/c/c-typeck.c	2021-03-18 13:16:07.370260914 +0100
@@ -1968,6 +1968,50 @@  mark_exp_read (tree exp)
       mark_exp_read (TREE_OPERAND (exp, 0));
       break;
     case COMPOUND_EXPR:
+      /* Pattern match what build_atomic_assign produces with modifycode
+	 NOP_EXPR.  */
+      if (VAR_P (TREE_OPERAND (exp, 1))
+	  && DECL_ARTIFICIAL (TREE_OPERAND (exp, 1))
+	  && TREE_CODE (TREE_OPERAND (exp, 0)) == COMPOUND_EXPR)
+	{
+	  tree t1 = TREE_OPERAND (TREE_OPERAND (exp, 0), 0);
+	  tree t2 = TREE_OPERAND (TREE_OPERAND (exp, 0), 1);
+	  if (TREE_CODE (t1) == TARGET_EXPR
+	      && TARGET_EXPR_SLOT (t1) == TREE_OPERAND (exp, 1)
+	      && TREE_CODE (t2) == CALL_EXPR)
+	    {
+	      tree fndecl = get_callee_fndecl (t2);
+	      tree arg = NULL_TREE;
+	      if (fndecl
+		  && TREE_CODE (fndecl) == FUNCTION_DECL
+		  && fndecl_built_in_p (fndecl, BUILT_IN_NORMAL)
+		  && call_expr_nargs (t2) >= 2)
+		switch (DECL_FUNCTION_CODE (fndecl))
+		  {
+		  case BUILT_IN_ATOMIC_STORE:
+		    arg = CALL_EXPR_ARG (t2, 1);
+		    break;
+		  case BUILT_IN_ATOMIC_STORE_1:
+		  case BUILT_IN_ATOMIC_STORE_2:
+		  case BUILT_IN_ATOMIC_STORE_4:
+		  case BUILT_IN_ATOMIC_STORE_8:
+		  case BUILT_IN_ATOMIC_STORE_16:
+		    arg = CALL_EXPR_ARG (t2, 0);
+		    break;
+		  default:
+		    break;
+		  }
+	      if (arg)
+		{
+		  STRIP_NOPS (arg);
+		  if (TREE_CODE (arg) == ADDR_EXPR
+		      && DECL_P (TREE_OPERAND (arg, 0))
+		      && TYPE_ATOMIC (TREE_TYPE (TREE_OPERAND (arg, 0))))
+		    mark_exp_read (TREE_OPERAND (arg, 0));
+		}
+	    }
+	}
+      /* FALLTHRU */
     case C_MAYBE_CONST_EXPR:
       mark_exp_read (TREE_OPERAND (exp, 1));
       break;
@@ -4065,7 +4109,7 @@  build_atomic_assign (location_t loc, tre
   vec<tree, va_gc> *params;
   tree val, nonatomic_lhs_type, nonatomic_rhs_type, newval, newval_addr;
   tree old, old_addr;
-  tree compound_stmt;
+  tree compound_stmt = NULL_TREE;
   tree stmt, goto_stmt;
   tree loop_label, loop_decl, done_label, done_decl;
 
@@ -4086,7 +4130,15 @@  build_atomic_assign (location_t loc, tre
 
   /* Create a compound statement to hold the sequence of statements
      with a loop.  */
-  compound_stmt = c_begin_compound_stmt (false);
+  if (modifycode != NOP_EXPR)
+    {
+      compound_stmt = c_begin_compound_stmt (false);
+
+      /* For consistency with build_modify_expr on non-_Atomic,
+	 mark the lhs as read.  Also, it would be very hard to match
+	 such expressions in mark_exp_read.  */
+      mark_exp_read (lhs);
+    }
 
   /* Remove any excess precision (which is only present here in the
      case of compound assignments).  */
@@ -4112,13 +4164,16 @@  build_atomic_assign (location_t loc, tre
   TREE_NO_WARNING (val) = 1;
   rhs = build4 (TARGET_EXPR, nonatomic_rhs_type, val, rhs, NULL_TREE,
 		NULL_TREE);
+  TREE_SIDE_EFFECTS (rhs) = 1;
   SET_EXPR_LOCATION (rhs, loc);
-  add_stmt (rhs);
+  if (modifycode != NOP_EXPR)
+    add_stmt (rhs);
 
   /* NOP_EXPR indicates it's a straight store of the RHS. Simply issue
      an atomic_store.  */
   if (modifycode == NOP_EXPR)
     {
+      compound_stmt = rhs;
       /* Build __atomic_store (&lhs, &val, SEQ_CST)  */
       rhs = build_unary_op (loc, ADDR_EXPR, val, false);
       fndecl = builtin_decl_explicit (BUILT_IN_ATOMIC_STORE);
@@ -4126,10 +4181,9 @@  build_atomic_assign (location_t loc, tre
       params->quick_push (rhs);
       params->quick_push (seq_cst);
       func_call = c_build_function_call_vec (loc, vNULL, fndecl, params, NULL);
-      add_stmt (func_call);
 
-      /* Finish the compound statement.  */
-      compound_stmt = c_end_compound_stmt (loc, compound_stmt, false);
+      compound_stmt = build2 (COMPOUND_EXPR, void_type_node,
+			      compound_stmt, func_call);
 
       /* VAL is the value which was stored, return a COMPOUND_STMT of
 	 the statement and that value.  */
--- gcc/testsuite/gcc.dg/Wunused-var-5.c.jj	2021-03-18 14:01:21.276508342 +0100
+++ gcc/testsuite/gcc.dg/Wunused-var-5.c	2021-03-18 14:03:32.586073970 +0100
@@ -0,0 +1,23 @@ 
+/* PR c/99588 */
+/* { dg-do compile } */
+/* { dg-options "-std=c11 -Wunused-but-set-variable" } */
+
+void bar (int, ...);
+void f1 (void) { static _Atomic int x = 0; bar (0, x); }
+void f2 (void) { static _Atomic int x = 0; bar (0, x += 1); }
+void f3 (void) { static _Atomic int x = 0; bar (x); }
+void f4 (void) { static _Atomic int x = 0; bar (x += 1); }
+void f5 (void) { static _Atomic int x = 0; bar (x = 1); }
+void f6 (void) { static _Atomic int x = 0; x = 1; }	/* { dg-warning "variable 'x' set but not used" } */
+void f7 (void) { static _Atomic int x = 0; x += 3; }
+void f8 (void) { _Atomic int x = 0; bar (0, x); }
+void f9 (void) { _Atomic int x = 0; bar (0, x += 1); }
+void f10 (void) { _Atomic int x = 0; bar (x); }
+void f11 (void) { _Atomic int x = 0; bar (x += 1); }
+void f12 (void) { _Atomic int x = 0; bar (x = 1); }
+void f13 (void) { _Atomic int x = 0; x = 1; }		/* { dg-warning "variable 'x' set but not used" } */
+void f14 (void) { _Atomic int x = 0; x += 3; }
+void f15 (void) { _Atomic int x = 0; int y = 3; x += y; }
+void f16 (void) { _Atomic int x = 0; int y = 3; bar (x += y); }
+void f17 (void) { _Atomic int x = 0; int y = 3; x = y; }	/* { dg-warning "variable 'x' set but not used" } */
+void f18 (void) { _Atomic int x = 0; int y = 3; bar (x = y); }
--- gcc/testsuite/gcc.dg/Wunused-var-6.c.jj	2021-03-18 14:02:59.538434534 +0100
+++ gcc/testsuite/gcc.dg/Wunused-var-6.c	2021-03-18 14:03:39.031003655 +0100
@@ -0,0 +1,14 @@ 
+/* PR c/99588 */
+/* { dg-do compile } */
+/* { dg-options "-std=c11 -Wunused-but-set-variable" } */
+
+void bar (int, ...);
+struct S { int a, b, c; };
+typedef _Atomic struct S T;
+
+void
+foo (void)
+{
+  static T x = (struct S) { 0, 0, 0 };	/* { dg-bogus "set but not used" } */
+  bar (0, x = (struct S) { 1, 1, 1 });
+}