Add new tautological compare warning for bitwise-or with a non-zero constant

[rtrieu]

Summary

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.

https://bugs.llvm.org/show_bug.cgi?id=42666

[jfb]

include/clang/Basic/DiagnosticSemaKinds.td

8161 ↗

(On Diff #214484)

Why default ignore?

[rtrieu]

include/clang/Basic/DiagnosticSemaKinds.td

8161 ↗

(On Diff #214484)

This warning, like the tautological overlap warning, uses the CFG. CFG-based analysis are typically excluded from being default warnings due to the extra work of construction the CFG.

[jfb]

include/clang/Basic/DiagnosticSemaKinds.td

8161 ↗

(On Diff #214484)

Can you say so in the commit message?

[NoQ]

lib/Analysis/CFG.cpp

1118 ↗

(On Diff #214484)

It's kinda strange to me that we first confirm that it's a constant by doing tryTransformToIntOrEnumConstant, but then fire up the heavy machinery of EvaluateAsInt anyway. Did you consider using only EvaluateAsInt() to begin with? I guess you're trying to make sure that "the user's intent is clear" as other similar warnings do, right? Could you comment on that?

[rtrieu]

Create new function isIntOrEnumConstantZero to use instead of EvaluateAsInt

[rtrieu]

include/clang/Basic/DiagnosticSemaKinds.td

8161 ↗

(On Diff #214484)

Updated patch description.

lib/Analysis/CFG.cpp

1118 ↗

(On Diff #214484)

A new function has been created to check the zero-ness of the Expr. Since we know it the Expr comes from tryTransformToIntOrEnumConstant, this function doesn't have to handle the full Expr sub-classes.

[NoQ]

Looks great, thank you!

lib/Analysis/CFG.cpp

1118 ↗

(On Diff #214484)

Whoa!

[sylvestre.ledru]

You should also probably add a note in the release notes (maybe for the others changes too)
thanks for the work btw!

[xbolva00]

In D66046#1634055, @sylvestre.ledru wrote:

You should also probably add a note in the release notes (maybe for the others changes too)
thanks for the work btw!

+1

[xbolva00]

Could this patch solve https://bugs.llvm.org/show_bug.cgi?id=43573?

[rtrieu]

In D66046#1696785, @xbolva00 wrote:

Could this patch solve https://bugs.llvm.org/show_bug.cgi?id=43573?

No, but I left some notes on the bug on why negative values are hard and where to fix it.

[thakis]

Any reason why this is off by default?

[xbolva00]

Probably it is not very cheap in terms of compile time.

[thakis]

Ah, that's a good reason. Are they in -Wall? We've sometimes reconned "-Wall == needs CFG" :)

[xbolva00]

This needs explicit -Wtautological-compare - which makes me sad (useful warning but hidden for users).

cc @aaron.ballman maybe atleast put it under -Wextra?

[thakis]

Mr Trieu, what do you think about adding some or all of the Wtautological-compare warnings to Wall

[jfb]

In D66046#1717229, @thakis wrote:

Mr Trieu, what do you think about adding some or all of the Wtautological-compare warnings to Wall

It's addressed in the patch description: https://reviews.llvm.org/D66046?id=214484#inline-591988

[xbolva00]

That comment explains why this is not on by default (= without any -W flag) but does not explain why this is not part of -Wall (if user enables -Wall, he/she cares about warnings) [you asked about default ignore].

We have many warnings flagged as DefaultIgnore but they are part of -Wall.

[thakis]

include/clang/Basic/DiagnosticSemaKinds.td

8161 ↗

(On Diff #214484)

It addresses why it's not on-by-default, but not why it's not in -Wall. As I said 2 comments ago, we've kind of retconned -Wall to mean "needs CFG" at times.

[rtrieu]

In D66046#1717229, @thakis wrote:

Mr Trieu, what do you think about adding some or all of the Wtautological-compare warnings to Wall

I thought -Wtautological-compare was already part of -Wall, so I'm all for adding it. I made a patch https://reviews.llvm.org/D69292 with the proposed move to -Wall so it will have some more visibility. I'll be out for the conference and can commit it afterwards if no issues are brought up.

clang/docs/ReleaseNotes.rst

 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 - -Wtautological-overlap-compare will warn on negative numbers and non-int
   types.
 - -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
 -------------------------------------------------
 
 * In both C and C++ (C17 ``6.5.6p8``, C++ ``[expr.add]``), pointer arithmetic is
   only permitted within arrays. In particular, the behavior of a program is not
   defined if it adds a non-zero offset (or in C, any offset) to a null pointer,
   or if it forms a null pointer by subtracting an integer from a non-null

clang/include/clang/Analysis/CFG.h

 class CFGCallback {
 public:
   CFGCallback() = default;
   virtual ~CFGCallback() = default;
 
   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
 ///  control-flow of a Stmt.  The Stmt can represent an entire function body,
 ///  or a single expression.  A CFG will always contain one empty block that
 ///  represents the Exit point of the CFG.  A CFG will also contain a designated
 ///  Entry block.  The CFG solely represents control-flow; it consists of
 ///  CFGBlocks which are simply containers of Stmt*'s in the AST the CFG

clang/include/clang/Basic/DiagnosticGroups.td

def TautologicalInRangeCompare : DiagGroup<"tautological-constant-in-range-compare",
                                             TautologicalUnsignedEnumZeroCompare]>;
 // For compatibility with GCC; -Wtype-limits = -Wtautological-constant-in-range-compare
 def TypeLimits : DiagGroup<"type-limits", [TautologicalInRangeCompare]>;
 def TautologicalOutOfRangeCompare : DiagGroup<"tautological-constant-out-of-range-compare">;
 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">;
 def DuplicateDeclSpecifier : DiagGroup<"duplicate-decl-specifier">;
 def CompareDistinctPointerType : DiagGroup<"compare-distinct-pointer-types">;
 def GNUUnionCast : DiagGroup<"gnu-union-cast">;
 def GNUVariableSizedTypeNotAtEnd : DiagGroup<"gnu-variable-sized-type-not-at-end">;
 def Varargs : DiagGroup<"varargs">;

clang/include/clang/Basic/DiagnosticSemaKinds.td

 // should result in a warning, since these always evaluate to a constant.
 // Array comparisons have similar warnings
 def warn_comparison_always : Warning<
   "%select{self-|array }0comparison always evaluates to "
   "%select{a constant|true|false|'std::strong_ordering::equal'}1">,
   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;
 
 def warn_stringcompare : Warning<
   "result of comparison against %select{a string literal|@encode}0 is "
   "unspecified (use strncmp instead)">,
   InGroup<StringCompare>;

clang/lib/Analysis/CFG.cpp

if (AlwaysTrue || AlwaysFalse) {
       if (!LHSAlwaysTrue && !LHSAlwaysFalse && !RHSAlwaysTrue &&
           !RHSAlwaysFalse && BuildOpts.Observer)
         BuildOpts.Observer->compareAlwaysTrue(B, AlwaysTrue);
       return TryResult(AlwaysTrue);
     }
     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)
       return false;
     return !S->isTypeDependent() &&
            !S->isValueDependent() &&
            S->EvaluateAsRValue(outResult, *Context);
   }
 
   /// tryEvaluateBool - Try and evaluate the Stmt and return 0 or 1
   /// if we can evaluate to a known value, otherwise return -1.
   TryResult tryEvaluateBool(Expr *S) {
     if (!BuildOpts.PruneTriviallyFalseEdges ||
         S->isTypeDependent() || S->isValueDependent())
       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())
           return I->second; // already in map;
 
         // Retrieve result at first, or the map might be updated.
         TryResult Result = evaluateAsBooleanConditionNoCache(S);
         CachedBoolEvals[S] = Result; // update or insert
if (BinaryOperator *Bop = dyn_cast<BinaryOperator>(E)) {
       } else if (Bop->isEqualityOp()) {
           TryResult BopRes = checkIncorrectEqualityOperator(Bop);
           if (BopRes.isKnown())
             return BopRes.isTrue();
       } else if (Bop->isRelationalOp()) {
         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();
       }
     }
 
     bool Result;
     if (E->EvaluateAsBooleanCondition(Result, *Context))
       return Result;
 
     return {};

 
 CFGBlock *CFGBuilder::VisitUnaryOperator(UnaryOperator *U,
            AddStmtChoice asc) {
   if (asc.alwaysAdd(*this, U)) {
     autoCreateBlock();
     appendStmt(Block, U);
   }
 
+  if (U->getOpcode() == UO_LNot)
+    tryEvaluateBool(U->getSubExpr()->IgnoreParens());
+
   return Visit(U->getSubExpr(), AddStmtChoice());
 }
 
 CFGBlock *CFGBuilder::VisitLogicalOperator(BinaryOperator *B) {
   CFGBlock *ConfluenceBlock = Block ? Block : createBlock();
   appendStmt(ConfluenceBlock, B);
 
   if (badCFG)
if (B->isAssignmentOp()) {
     return Visit(B->getRHS());
   }
 
   if (asc.alwaysAdd(*this, B)) {
     autoCreateBlock();
     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
   // containing a DoStmt, and the LHS doesn't create a new block, then we should
   // return RBlock.  Otherwise we'll incorrectly return NULL.
   return (LBlock ? LBlock : RBlock);
 }
 

 }
 
 CFGBlock *CFGBuilder::VisitImplicitCastExpr(ImplicitCastExpr *E,
                                             AddStmtChoice asc) {
   if (asc.alwaysAdd(*this, E)) {
     autoCreateBlock();
     appendStmt(Block, E);
   }
+
+  if (E->getCastKind() == CK_IntegralToBoolean)
+    tryEvaluateBool(E->getSubExpr()->IgnoreParens());
+
   return Visit(E->getSubExpr(), AddStmtChoice());
 }
 
 CFGBlock *CFGBuilder::VisitConstantExpr(ConstantExpr *E, AddStmtChoice asc) {
   return Visit(E->getSubExpr(), AddStmtChoice());
 }
 
 CFGBlock *CFGBuilder::VisitIndirectGotoStmt(IndirectGotoStmt *I) {

clang/lib/Sema/AnalysisBasedWarnings.cpp

void compareBitwiseEquality(const BinaryOperator *B,
                               bool isAlwaysTrue) override {
     if (HasMacroID(B))
       return;
 
     SourceRange DiagRange = B->getSourceRange();
     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
 
 //===----------------------------------------------------------------------===//
 // Check for infinite self-recursion in functions
 //===----------------------------------------------------------------------===//
 
 // Returns true if the function is called anywhere within the CFGBlock.
AC.getCFGBuildOptions()
       .setAlwaysAdd(Stmt::BlockExprClass)
       .setAlwaysAdd(Stmt::CStyleCastExprClass)
       .setAlwaysAdd(Stmt::DeclRefExprClass)
       .setAlwaysAdd(Stmt::ImplicitCastExprClass)
       .setAlwaysAdd(Stmt::UnaryOperatorClass)
       .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;
   }
 
   // Emit delayed diagnostics.
   if (!fscope->PossiblyUnreachableDiags.empty()) {
     bool analyzed = false;
 
AnalysisBasedWarnings::IssueWarnings(sema::AnalysisBasedWarnings::Policy P,
 
   // Check for throw out of non-throwing function.
   if (!Diags.isIgnored(diag::warn_throw_in_noexcept_func, D->getBeginLoc()))
     if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D))
       if (S.getLangOpts().CPlusPlus && isNoexcept(FD))
         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();
   }
 
   // Collect statistics about the CFG if it was built.
   if (S.CollectStats && AC.isCFGBuilt()) {
     ++NumFunctionsAnalyzed;
     if (CFG *cfg = AC.getCFG()) {
       // If we successfully built a CFG for this context, record some more

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

-// 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}}
   if ((x & 8) != 4) {}  // expected-warning {{bitwise comparison always evaluates to true}}
   if ((2 & x) != 4) {}  // expected-warning {{bitwise comparison always evaluates to true}}
   if ((x | 4) == 3) {}  // expected-warning {{bitwise comparison always evaluates to false}}
   if ((x | 3) != 4) {}  // expected-warning {{bitwise comparison always evaluates to true}}
   if ((5 | x) != 3) {}  // expected-warning {{bitwise comparison always evaluates to true}}
   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) {}
   if ((x | 4) != 4) {}
 
   if ((x & 9) == 8) {}
   if ((x & 9) != 8) {}
   if ((x | 4) == 5) {}
   if ((x | 4) != 5) {}
 
   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

+// 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}}
+}