[Clang] -Wunused-but-set-variable warning - handle also pre/post unary operators

[xbolva00]

Summary

Clang fails to diagnose:

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

Reason: Missing support for UnaryOperator.

We should not warn with volatile variables... so add check for it.

[yonghong-song]

LGTM. Maybe @efriedma can look at this as well.

[efriedma]

LGTM

[kamaub]

This option appears to incorrectly warn warning that unsigned NumEntries = getNumEntries(); is "set but not used" in llvm/include/llvm/ADT/DenseMap.h:129 and so it is breaking the ppc64le-lld-multistage-test bot.
This could be because the only use is in an assert which is in a corner case maybe?

const KeyT EmptyKey = getEmptyKey(), TombstoneKey = getTombstoneKey();
if (std::is_trivially_destructible<ValueT>::value) {
  // Use a simpler loop when values don't need destruction.
  for (BucketT *P = getBuckets(), *E = getBucketsEnd(); P != E; ++P)
    P->getFirst() = EmptyKey;
} else {
  unsigned NumEntries = getNumEntries();
  for (BucketT *P = getBuckets(), *E = getBucketsEnd(); P != E; ++P) {
    if (!KeyInfoT::isEqual(P->getFirst(), EmptyKey)) {
      if (!KeyInfoT::isEqual(P->getFirst(), TombstoneKey)) {
        P->getSecond().~ValueT();
        --NumEntries;
      }
      P->getFirst() = EmptyKey;
    }
  }
  assert(NumEntries == 0 && "Node count imbalance!");
}

I noticed you were commit NFCI changes to makes sure you did not break any builds before reapplying but it appears you missed llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:3924 which is causing build breaks to both the sanitizer-ppc64be-linux and sanitizer-ppc64le-linux bots. Is there an NFCI planned for it by you? If not I'll submit an NFC change myself in a few minutes.

[kamaub]

I'm sorry I took so long to notify you but I need to bring these bots back to green as soon as possible, they have been broken too long.

[xbolva00]

In D122271#3408060, @kamaub wrote:

This option appears to incorrectly warn warning that unsigned NumEntries = getNumEntries(); is "set but not used" in llvm/include/llvm/ADT/DenseMap.h:129 and so it is breaking the ppc64le-lld-multistage-test bot.
This could be because the only use is in an assert which is in a corner case maybe?

const KeyT EmptyKey = getEmptyKey(), TombstoneKey = getTombstoneKey();
if (std::is_trivially_destructible<ValueT>::value) {
  // Use a simpler loop when values don't need destruction.
  for (BucketT *P = getBuckets(), *E = getBucketsEnd(); P != E; ++P)
    P->getFirst() = EmptyKey;
} else {
  unsigned NumEntries = getNumEntries();
  for (BucketT *P = getBuckets(), *E = getBucketsEnd(); P != E; ++P) {
    if (!KeyInfoT::isEqual(P->getFirst(), EmptyKey)) {
      if (!KeyInfoT::isEqual(P->getFirst(), TombstoneKey)) {
        P->getSecond().~ValueT();
        --NumEntries;
      }
      P->getFirst() = EmptyKey;
    }
  }
  assert(NumEntries == 0 && "Node count imbalance!");
}

I noticed you were commit NFCI changes to makes sure you did not break any builds before reapplying but it appears you missed llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:3924 which is causing build breaks to both the sanitizer-ppc64be-linux and sanitizer-ppc64le-linux bots. Is there an NFCI planned for it by you? If not I'll submit an NFC change myself in a few minutes.

This option appears to incorrectly warn warning that

Warns correctly in configurations where assert is removed. (void) trick will solve that.
Sorry, I missed this one, as this was outside common code + X86. Will push fix soon.

Edit: fixes pushed.

[uabelho]

Hi,

I noticed these to warning when compiling libunwind/src/UnwindLevel1.c with this patch:

00:22:48 /repo/bbiswjenk/fem2s10-eiffel176/workspace/llvm/sdk_1_20_ki_dev_test/libunwind/src/UnwindLevel1.c:175:12: warning: variable 'framesWalked' set but not used [-Wunused-but-set-variable]
00:22:48   unsigned framesWalked = 1;
00:22:48            ^
00:22:48 /repo/bbiswjenk/fem2s10-eiffel176/workspace/llvm/sdk_1_20_ki_dev_test/libunwind/src/UnwindLevel1.c:293:12: warning: variable 'framesWalked' set but not used [-Wunused-but-set-variable]
00:22:48   unsigned framesWalked = 1;
00:22:48            ^
00:22:48 2 warnings generated.

[xbolva00]

In D122271#3413193, @uabelho wrote:

Hi,

I noticed these to warning when compiling libunwind/src/UnwindLevel1.c with this patch:

00:22:48 /repo/bbiswjenk/fem2s10-eiffel176/workspace/llvm/sdk_1_20_ki_dev_test/libunwind/src/UnwindLevel1.c:175:12: warning: variable 'framesWalked' set but not used [-Wunused-but-set-variable]
00:22:48   unsigned framesWalked = 1;
00:22:48            ^
00:22:48 /repo/bbiswjenk/fem2s10-eiffel176/workspace/llvm/sdk_1_20_ki_dev_test/libunwind/src/UnwindLevel1.c:293:12: warning: variable 'framesWalked' set but not used [-Wunused-but-set-variable]
00:22:48   unsigned framesWalked = 1;
00:22:48            ^
00:22:48 2 warnings generated.

In D122271#3413193, @uabelho wrote:

Hi,

I noticed these to warning when compiling libunwind/src/UnwindLevel1.c with this patch:

00:22:48 /repo/bbiswjenk/fem2s10-eiffel176/workspace/llvm/sdk_1_20_ki_dev_test/libunwind/src/UnwindLevel1.c:175:12: warning: variable 'framesWalked' set but not used [-Wunused-but-set-variable]
00:22:48   unsigned framesWalked = 1;
00:22:48            ^
00:22:48 /repo/bbiswjenk/fem2s10-eiffel176/workspace/llvm/sdk_1_20_ki_dev_test/libunwind/src/UnwindLevel1.c:293:12: warning: variable 'framesWalked' set but not used [-Wunused-but-set-variable]
00:22:48   unsigned framesWalked = 1;
00:22:48            ^
00:22:48 2 warnings generated.

A warning was improved so fires more often.

Clearly, sometimes in __unw_phase2_resume, “fn” is ignored (so passed framesWalked is throw away as well). I see nothing wrong to warn here.

(void)framesWalked; will silence it.

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;
+  bool isIncrementDecrementUnaryOp = 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));
+  } else if (UnaryOperator *UO = dyn_cast<UnaryOperator>(E)) {
+    if (!UO->isIncrementDecrementOp())
+      return;
+    isIncrementDecrementUnaryOp = true;
+    LHS = dyn_cast<DeclRefExpr>(UO->getSubExpr());
   }
   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())
+  // assignment (+=, ...) or increment/decrement unary operator to avoid
+  // potential unused-but-set-variable warning.
+  if ((IsCompoundAssign || isIncrementDecrementUnaryOp) &&
+      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

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

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;
 }
+
+void f4(void) {
+  int x1 = 0; // expected-warning{{variable 'x1' set but not used}}
+  x1++;
+  int x2 = 0; // expected-warning{{variable 'x2' set but not used}}
+  x2--;
+  int x3 = 0; // expected-warning{{variable 'x3' set but not used}}
+  ++x3;
+  int x4 = 0; // expected-warning{{variable 'x4' set but not used}}
+  --x4;
+
+  volatile int v1 = 0;
+  ++v1;
+  typedef volatile int volint;
+  volint v2 = 0;
+  v2++;
+}

clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp

 // RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-variable -verify %s
 
 struct S {
   int i;
 };
 
 struct __attribute__((warn_unused)) SWarnUnused {
   int j;
   void operator +=(int);
+  void operator ++();
 };
 
 int f0() {
   int y; // expected-warning{{variable 'y' set but not used}}
   y = 0;
 
   int z __attribute__((unused));
   z = 0;

 }
 
 template<typename T> void f4(T n) {
   // Don't warn for (potentially) overloaded compound assignment operators in
   // template code.
   SWarnUnused swu;
   swu += n;
 }
+
+template <typename T> void f5() {
+  // Don't warn for overloaded pre/post operators in template code.
+  SWarnUnused swu;
+  ++swu;
+}