[Clang] Fix an unused-but-set-variable warning with volatile variable

[yonghong-song]

Summary

For the following code,

void test() {
    volatile int j = 0;
    for (int i = 0; i < 1000; i++)
            j += 1;
    return;
}

If compiled with

clang -g -Wall -Werror -S -emit-llvm test.c

we will see the following error:

test.c:2:6: error: variable 'j' set but not used [-Werror,-Wunused-but-set-variable]
        volatile int j = 0;
                         ^

This is not quite right since 'j' is indeed used due to '+=' operator.
gcc doesn't emit error either in this case.
Also if we change 'j += 1' to 'j++', the warning will disappear
with latest clang.

Note that clang will issue the warning if the volatile declaration
involves only simple assignment (var = ...).

To fix the issue, in function MaybeDecrementCount(), if the
operator is a compound assignment (i.e., +=, -=, etc.) and the
variable is volatile, the count for RefsMinusAssignments will be
decremented, similar to 'j++' case.

[yonghong-song]

@eli.friedman Please feel free adding other reviewers if needed. Thanks!

[efriedma]

clang/test/Sema/warn-unused-but-set-variables.c

80

The handling of this testcase without your patch seems fine. Even if the value of j is technically read, it's not "used" in any meaningful way.

I think the distinguishing factor for the testcase we discussed before is the use of a "volatile". Volatile is rare, and indicates the user is intentionally doing something unusual, so it probably doesn't make sense to warn.

[yonghong-song]

• only handle volatile variables with component assignment.

[yonghong-song]

clang/test/Sema/warn-unused-but-set-variables.c

80

@eli.friedman Just updated the patch to handle volatile variable only.

[efriedma]

LGTM

clang/lib/Sema/SemaDecl.cpp

2033 ↗

(On Diff #415665)

(Accidental change?)

[aaron.ballman]

clang/lib/Sema/SemaExprCXX.cpp

7928

clang/test/Sema/warn-unused-but-set-variables.c

35

I'd like to see another test:

typedef volatile int volint;
volint l = 0;
l += 1; // no warning

[xbolva00]

This is not quite right since 'j' is indeed used due to '+=' operator.

is “set” - load, increment, store - but not used after “store”. So technically this is okay no?

If there is j = j + 1 .. do we warn ?

[xbolva00]

void test() {
    int j = 0;
    for (int i = 0; i < 1000; i++)
            j++;
    return;
}

Should not we warn in this case? (Currently no warning)

[efriedma]

In D121715#3397215, @xbolva00 wrote:

If there is j = j + 1 .. do we warn ?

In D121715#3397237, @xbolva00 wrote:

j++;

Should not we warn in this case? (Currently no warning)

We probably should warn in both those cases; the variable clearly isn't "used" in any meaningful way.

[yonghong-song]

clang/lib/Sema/SemaDecl.cpp

2033 ↗

(On Diff #415665)

Right. Thanks for catching this. This is a leftover from my previous debugging code and forgot to remove it.

clang/lib/Sema/SemaExprCXX.cpp

7928

Thanks, really embarrassing typo.

clang/test/Sema/warn-unused-but-set-variables.c

35

Will do.

[yonghong-song]

In D121715#3397407, @efriedma wrote:

In D121715#3397215, @xbolva00 wrote:

If there is j = j + 1 .. do we warn ?

In D121715#3397237, @xbolva00 wrote:

j++;

Should not we warn in this case? (Currently no warning)

We probably should warn in both those cases; the variable clearly isn't "used" in any meaningful way.

The 'j++' is not warned as it is not a BinaryOperator or CXXOperatorCallExpr so it is not handled and considered no warning by default.
This patch intends to handle volatile variables. We can have another patch to deal with this specific case.

[yonghong-song]

• fix typo
• add a new test case with typedef of volatile type.

[efriedma]

In D121715#3397446, @yonghong-song wrote:

The 'j++' is not warned as it is not a BinaryOperator or CXXOperatorCallExpr so it is not handled and considered no warning by default.
This patch intends to handle volatile variables. We can have another patch to deal with this specific case.

Right, I wasn't suggesting you should add that in this patch. Just a general observation about cases we should consider diagnosing in the future.

clang/lib/Sema/SemaExprCXX.cpp

 ExprResult Sema::ActOnNoexceptExpr(SourceLocation KeyLoc, SourceLocation,
                                    Expr *Operand, SourceLocation RParen) {
   return BuildCXXNoexceptExpr(KeyLoc, Operand, RParen);
 }
 
 static void MaybeDecrementCount(
     Expr *E, llvm::DenseMap<const VarDecl *, int> &RefsMinusAssignments) {
   DeclRefExpr *LHS = nullptr;
+  bool IsCompoundAssign = false;
   if (BinaryOperator *BO = dyn_cast<BinaryOperator>(E)) {
     if (BO->getLHS()->getType()->isDependentType() ||
         BO->getRHS()->getType()->isDependentType()) {
       if (BO->getOpcode() != BO_Assign)
         return;
     } else if (!BO->isAssignmentOp())
       return;
+    else
+      IsCompoundAssign = BO->isCompoundAssignmentOp();
     LHS = dyn_cast<DeclRefExpr>(BO->getLHS());
   } else if (CXXOperatorCallExpr *COCE = dyn_cast<CXXOperatorCallExpr>(E)) {
     if (COCE->getOperator() != OO_Equal)
       return;
     LHS = dyn_cast<DeclRefExpr>(COCE->getArg(0));
   }
   if (!LHS)
     return;
   VarDecl *VD = dyn_cast<VarDecl>(LHS->getDecl());
   if (!VD)
     return;
+  // Don't decrement RefsMinusAssignments if volatile variable with compound
+  // assignment (+=, ...) to avoid potential unused-but-set-variable warning.
+  if (IsCompoundAssign && VD->getType().isVolatileQualified())
+    return;
   auto iter = RefsMinusAssignments.find(VD);
   if (iter == RefsMinusAssignments.end())
     return;
   iter->getSecond()--;
 }
 
 /// Perform the conversions required for an expression used in a
 /// context that ignores the result.

clang/test/Sema/warn-unused-but-set-variables.c

int f0(void) {
   // Don't warn for an extern variable.
   extern int w;
   w = 0;
 
   // Following gcc, this should not warn.
   int a;
   w = (a = 0);
 
+  int j = 0; // expected-warning{{variable 'j' set but not used}}
+  for (int i = 0; i < 1000; i++)
+    j += 1;
+
   // Following gcc, warn for a volatile variable.
   volatile int b; // expected-warning{{variable 'b' set but not used}}
   b = 0;
 
+  // volatile variable k is used, no warning.
+  volatile int k = 0;
+  for (int i = 0; i < 1000; i++)
+    k += 1;
+
+  // typedef of volatile type, no warning.
+  typedef volatile int volint;
+  volint l = 0;
+  l += 1;
+
   int x;
   x = 0;
   return x;
 }
 
 void f1(void) {
   (void)^() {
     int y; // expected-warning{{variable 'y' set but not used}}

 void for_cleanup(int *x) {
   *x = 0;
 }
 
 void f3(void) {
   // Don't warn if the __cleanup__ attribute is used.
   __attribute__((__cleanup__(for_cleanup))) int x;
   x = 5;
 }