New tautological warning for bitwise-or with non-zero constant always…

… true.

Taking a value and the bitwise-or it with a non-zero constant will always
result in a non-zero value. In a boolean context, this is always true.

if (x | 0x4) {}  // always true, intended '&'

This patch creates a new warning group -Wtautological-bitwise-compare for this
warning. It also moves in the existing tautological bitwise comparisons into
this group. A few other changes were needed to the CFGBuilder so that all bool
contexts would be checked. The warnings in -Wtautological-bitwise-compare will
be off by default due to using the CFG.

Fixes: https://bugs.llvm.org/show_bug.cgi?id=42666
Differential Revision: https://reviews.llvm.org/D66046

llvm-svn: 375318

clang/docs/ReleaseNotes.rst

@@ -56,6 +56,11 @@ Improvements to Clang's diagnostics
 - -Wtautological-compare for self comparisons and
   -Wtautological-overlap-compare will now look through member and array
   access to determine if two operand expressions are the same.
+- -Wtautological-bitwise-compare is a new warning group.  This group has the
+  current warning which diagnoses the tautological comparison of a bitwise
+  operation and a constant.  The group also has the new warning which diagnoses
+  when a bitwise-or with a non-negative value is converted to a bool, since
+  that bool will always be true.
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------

clang/include/clang/Analysis/CFG.h

@@ -1213,6 +1213,7 @@ class CFGCallback {
   virtual void compareAlwaysTrue(const BinaryOperator *B, bool isAlwaysTrue) {}
   virtual void compareBitwiseEquality(const BinaryOperator *B,
                                       bool isAlwaysTrue) {}
+  virtual void compareBitwiseOr(const BinaryOperator *B) {}
 };
 
 /// Represents a source-level, intra-procedural CFG that represents the

clang/include/clang/Basic/DiagnosticGroups.td

@@ -516,12 +516,14 @@ def TautologicalConstantCompare : DiagGroup<"tautological-constant-compare",
                                             [TautologicalOutOfRangeCompare]>;
 def TautologicalPointerCompare : DiagGroup<"tautological-pointer-compare">;
 def TautologicalOverlapCompare : DiagGroup<"tautological-overlap-compare">;
+def TautologicalBitwiseCompare : DiagGroup<"tautological-bitwise-compare">;
 def TautologicalUndefinedCompare : DiagGroup<"tautological-undefined-compare">;
 def TautologicalObjCBoolCompare : DiagGroup<"tautological-objc-bool-compare">;
 def TautologicalCompare : DiagGroup<"tautological-compare",
                                     [TautologicalConstantCompare,
                                      TautologicalPointerCompare,
                                      TautologicalOverlapCompare,
+                                     TautologicalBitwiseCompare,
                                      TautologicalUndefinedCompare,
                                      TautologicalObjCBoolCompare]>;
 def HeaderHygiene : DiagGroup<"header-hygiene">;

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -8332,7 +8332,10 @@ def warn_comparison_always : Warning<
   InGroup<TautologicalCompare>;
 def warn_comparison_bitwise_always : Warning<
   "bitwise comparison always evaluates to %select{false|true}0">,
-  InGroup<TautologicalCompare>;
+  InGroup<TautologicalBitwiseCompare>, DefaultIgnore;
+def warn_comparison_bitwise_or : Warning<
+  "bitwise or with non-zero value always evaluates to true">,
+  InGroup<TautologicalBitwiseCompare>, DefaultIgnore;
 def warn_tautological_overlap_comparison : Warning<
   "overlapping comparisons always evaluate to %select{false|true}0">,
   InGroup<TautologicalOverlapCompare>, DefaultIgnore;

clang/lib/Analysis/CFG.cpp

@@ -1139,6 +1139,31 @@ class CFGBuilder {
     return {};
   }
 
+  /// A bitwise-or with a non-zero constant always evaluates to true.
+  TryResult checkIncorrectBitwiseOrOperator(const BinaryOperator *B) {
+    const Expr *LHSConstant =
+        tryTransformToIntOrEnumConstant(B->getLHS()->IgnoreParenImpCasts());
+    const Expr *RHSConstant =
+        tryTransformToIntOrEnumConstant(B->getRHS()->IgnoreParenImpCasts());
+
+    if ((LHSConstant && RHSConstant) || (!LHSConstant && !RHSConstant))
+      return {};
+
+    const Expr *Constant = LHSConstant ? LHSConstant : RHSConstant;
+
+    Expr::EvalResult Result;
+    if (!Constant->EvaluateAsInt(Result, *Context))
+      return {};
+
+    if (Result.Val.getInt() == 0)
+      return {};
+
+    if (BuildOpts.Observer)
+      BuildOpts.Observer->compareBitwiseOr(B);
+
+    return TryResult(true);
+  }
+
   /// Try and evaluate an expression to an integer constant.
   bool tryEvaluate(Expr *S, Expr::EvalResult &outResult) {
     if (!BuildOpts.PruneTriviallyFalseEdges)
@@ -1156,7 +1181,7 @@ class CFGBuilder {
       return {};
 
     if (BinaryOperator *Bop = dyn_cast<BinaryOperator>(S)) {
-      if (Bop->isLogicalOp()) {
+      if (Bop->isLogicalOp() || Bop->isEqualityOp()) {
         // Check the cache first.
         CachedBoolEvalsTy::iterator I = CachedBoolEvals.find(S);
         if (I != CachedBoolEvals.end())
@@ -1240,6 +1265,10 @@ class CFGBuilder {
         TryResult BopRes = checkIncorrectRelationalOperator(Bop);
         if (BopRes.isKnown())
           return BopRes.isTrue();
+      } else if (Bop->getOpcode() == BO_Or) {
+        TryResult BopRes = checkIncorrectBitwiseOrOperator(Bop);
+        if (BopRes.isKnown())
+          return BopRes.isTrue();
       }
     }
 
@@ -2340,6 +2369,9 @@ CFGBlock *CFGBuilder::VisitUnaryOperator(UnaryOperator *U,
     appendStmt(Block, U);
   }
 
+  if (U->getOpcode() == UO_LNot)
+    tryEvaluateBool(U->getSubExpr()->IgnoreParens());
+
   return Visit(U->getSubExpr(), AddStmtChoice());
 }
 
@@ -2474,6 +2506,9 @@ CFGBlock *CFGBuilder::VisitBinaryOperator(BinaryOperator *B,
     appendStmt(Block, B);
   }
 
+  if (B->isEqualityOp() || B->isRelationalOp())
+    tryEvaluateBool(B);
+
   CFGBlock *RBlock = Visit(B->getRHS());
   CFGBlock *LBlock = Visit(B->getLHS());
   // If visiting RHS causes us to finish 'Block', e.g. the RHS is a StmtExpr
@@ -4527,6 +4562,10 @@ CFGBlock *CFGBuilder::VisitImplicitCastExpr(ImplicitCastExpr *E,
     autoCreateBlock();
     appendStmt(Block, E);
   }
+
+  if (E->getCastKind() == CK_IntegralToBoolean)
+    tryEvaluateBool(E->getSubExpr()->IgnoreParens());
+
   return Visit(E->getSubExpr(), AddStmtChoice());
 }
 

clang/lib/Sema/AnalysisBasedWarnings.cpp

@@ -159,6 +159,20 @@ class LogicalErrorHandler : public CFGCallback {
     S.Diag(B->getExprLoc(), diag::warn_comparison_bitwise_always)
         << DiagRange << isAlwaysTrue;
   }
+
+  void compareBitwiseOr(const BinaryOperator *B) override {
+    if (HasMacroID(B))
+      return;
+
+    SourceRange DiagRange = B->getSourceRange();
+    S.Diag(B->getExprLoc(), diag::warn_comparison_bitwise_or) << DiagRange;
+  }
+
+  static bool hasActiveDiagnostics(DiagnosticsEngine &Diags,
+                                   SourceLocation Loc) {
+    return !Diags.isIgnored(diag::warn_tautological_overlap_comparison, Loc) ||
+           !Diags.isIgnored(diag::warn_comparison_bitwise_or, Loc);
+  }
 };
 } // anonymous namespace
 
@@ -2070,10 +2084,9 @@ AnalysisBasedWarnings::IssueWarnings(sema::AnalysisBasedWarnings::Policy P,
       .setAlwaysAdd(Stmt::AttributedStmtClass);
   }
 
-  // Install the logical handler for -Wtautological-overlap-compare
+  // Install the logical handler.
   llvm::Optional<LogicalErrorHandler> LEH;
-  if (!Diags.isIgnored(diag::warn_tautological_overlap_comparison,
-                       D->getBeginLoc())) {
+  if (LogicalErrorHandler::hasActiveDiagnostics(Diags, D->getBeginLoc())) {
     LEH.emplace(S);
     AC.getCFGBuildOptions().Observer = &*LEH;
   }
@@ -2222,9 +2235,8 @@ AnalysisBasedWarnings::IssueWarnings(sema::AnalysisBasedWarnings::Policy P,
         checkThrowInNonThrowingFunc(S, FD, AC);
 
   // If none of the previous checks caused a CFG build, trigger one here
-  // for -Wtautological-overlap-compare
-  if (!Diags.isIgnored(diag::warn_tautological_overlap_comparison,
-                       D->getBeginLoc())) {
+  // for the logical error handler.
+  if (LogicalErrorHandler::hasActiveDiagnostics(Diags, D->getBeginLoc())) {
     AC.getCFG();
   }
 

clang/test/Sema/warn-bitwise-compare.c

@@ -1,7 +1,12 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-compare %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-bitwise-compare %s
 
 #define mydefine 2
 
+enum {
+  ZERO,
+  ONE,
+};
+
 void f(int x) {
   if ((8 & x) == 3) {}  // expected-warning {{bitwise comparison always evaluates to false}}
   if ((x & 8) == 4) {}  // expected-warning {{bitwise comparison always evaluates to false}}
@@ -13,6 +18,9 @@ void f(int x) {
   if ((x & 0x15) == 0x13) {} // expected-warning {{bitwise comparison always evaluates to false}}
   if ((0x23 | x) == 0x155){} // expected-warning {{bitwise comparison always evaluates to false}}
 
+  if (!!((8 & x) == 3)) {}  // expected-warning {{bitwise comparison always evaluates to false}}
+  int y = ((8 & x) == 3) ? 1 : 2;  // expected-warning {{bitwise comparison always evaluates to false}}
+
   if ((x & 8) == 8) {}
   if ((x & 8) != 8) {}
   if ((x | 4) == 4) {}
@@ -26,3 +34,14 @@ void f(int x) {
   if ((x & mydefine) == 8) {}
   if ((x | mydefine) == 4) {}
 }
+
+void g(int x) {
+  if (x | 5) {}  // expected-warning {{bitwise or with non-zero value always evaluates to true}}
+  if (5 | x) {}  // expected-warning {{bitwise or with non-zero value always evaluates to true}}
+  if (!((x | 5))) {}  // expected-warning {{bitwise or with non-zero value always evaluates to true}}
+
+  if (x | -1) {}  // expected-warning {{bitwise or with non-zero value always evaluates to true}}
+  if (x | ONE) {}  // expected-warning {{bitwise or with non-zero value always evaluates to true}}
+
+  if (x | ZERO) {}
+}

clang/test/SemaCXX/warn-bitwise-compare.cpp

@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-bitwise-compare %s
+
+void test(int x) {
+  bool b1 = (8 & x) == 3;
+  // expected-warning@-1 {{bitwise comparison always evaluates to false}}
+  bool b2 = x | 5;
+  // expected-warning@-1 {{bitwise or with non-zero value always evaluates to true}}
+  bool b3 = (x | 5);
+  // expected-warning@-1 {{bitwise or with non-zero value always evaluates to true}}
+  bool b4 = !!(x | 5);
+  // expected-warning@-1 {{bitwise or with non-zero value always evaluates to true}}
+}