Extend -Wtautological-bitwise-compare "bitwise or with non-zero value" warnings

[sberg]

Summary

...to C++ constant expression operands.

(I had been puzzled why Clang had not found <https://git.libreoffice.org/core/+/
55f3a4595891a8cc22272225d1c82419f28d4ef9%5E!/> "cid#1465512 Wrong operator used"
in the LibreOffice source code, only found by Coverity Scan. At least building
LibreOffice with this change caused no false positives.)

In C, values of const-qualified variables are never constant expressions, so
nothing should change for C code.

To avoid potential further false positives, restrict this change only to the
"bitwise or with non-zero value" warnings while keeping all other
-Wtautological-bitwise-compare warnings as-is, at least for now.

[jordan_rose]

Nothing seems too mysterious here, but why limit to constexpr instead of anything where Clang can see a constant value, which would make it work in C as well? (If you're allowing constexpr, you're already allowing arbitrary work.)

[sberg]

My naive assumption was that this warning had initially been restricted to integer literals and enumerators to avoid false positives. Hence my conservative approach of extending merely to constant integer expressions for now. Maybe Richard as the original author of the warning (IIUC) can shed some light on that original restriction?

[rtrieu]

I looked back on the commits and while I did commit, I did it on the behalf of Anders Rönnholm, who did not have commit access at the time. I haven't seen activity from Anders in a while, but added to subscribers just in case.

Way back then, the warning only did operands of a DeclRefExpr and an IntegerLiteral. Over time, that has been extended, case by case, to include whatever new cases people can think up. I don't mind extending the warnings, but we need to be mindful of how the warnings appear. If the sub-expression becomes too large, it will be difficult for the user to understand where the problem is and which constants the compiler is talking about. We may already be at that point. The example could have a more complex initializer for the constant variables, and the warning would be harder to follow. Maybe we also look at the variable initializers and only allow for simple ones. I need to give this some more thought.

To avoid potential further false positives, restrict this change only to the
"bitwise or with non-zero value" warnings while keeping all other
-Wtautological-bitwise-compare warnings as-is, at least for now.

Are you planning to allow this change to other warnings that use the same helper functions?
Also, have you tried running warning over a codebase?

clang/lib/Analysis/CFG.cpp

96–97

The original comment specifies the allowed Expr's by the specific AST nodes they represent. Please use that. I think "IntegerLiteral constant expression, EnumConstantDecl, or constant value VarDecl" would work.

110

IntergerLiteral and EnumConstantDecl are known to have integer types. However, it is possible for a VarDecl to have other types. There should be a check for integer types here.

175

Need a comment here about how tryTransformToIntOrEnumConstant also allows a DeclRefExpr to have a constant VarDecl, but this case is currently excluded for this warning.

[sberg]

In D85287#2199463, @rtrieu wrote:

Are you planning to allow this change to other warnings that use the same helper functions?

No, I didn't plan to work on this further. Just scratching that itch of why Clang didn't emit that one warning it "obviously" should have emitted.

Also, have you tried running warning over a codebase?

As I wrote: "At least building LibreOffice with this change caused no false positives." (Or maybe I misunderstand your question.)

[rtrieu]

In D85287#2199490, @sberg wrote:

In D85287#2199463, @rtrieu wrote:

Also, have you tried running warning over a codebase?

As I wrote: "At least building LibreOffice with this change caused no false positives." (Or maybe I misunderstand your question.)

My bad. I read too fast and thought you said you ran the scan over LibreOffice instead of your warning.

[xbolva00]

Looks useful, +1

clang/lib/Analysis/CFG.cpp

if (const auto *UO = dyn_cast<UnaryOperator>(E)) {
     if (UO->getOpcode() != UO_Minus)
       return false;
     E = UO->getSubExpr();
   }
 
   return isa<IntegerLiteral>(E);
 }
 
-/// Helper for tryNormalizeBinaryOperator. Attempts to extract an IntegerLiteral
-/// constant expression or EnumConstantDecl from the given Expr. If it fails,
+/// Helper for tryNormalizeBinaryOperator. Attempts to extract a suitable
+/// integer or enum constant from the given Expr. If it fails,
 /// returns nullptr.
-static const Expr *tryTransformToIntOrEnumConstant(const Expr *E) {
+static const Expr *tryTransformToIntOrEnumConstant(const Expr *E,
+                                                   ASTContext &Ctx) {
   E = E->IgnoreParens();
   if (IsIntegerLiteralConstantExpr(E))
     return E;
-  if (auto *DR = dyn_cast<DeclRefExpr>(E->IgnoreParenImpCasts()))
-    return isa<EnumConstantDecl>(DR->getDecl()) ? DR : nullptr;
+  if (auto *DR = dyn_cast<DeclRefExpr>(E->IgnoreParenImpCasts())) {
+    auto *D = DR->getDecl();
+    if (isa<EnumConstantDecl>(D))
+      return DR;
+    if (auto *VD = dyn_cast<VarDecl>(D))
+      if (VD->isUsableInConstantExpressions(Ctx))
+        return DR;
+  }
   return nullptr;
 }
 
 /// Tries to interpret a binary operator into `Expr Op NumExpr` form, if
-/// NumExpr is an integer literal or an enum constant.
+/// NumExpr is a suitable integer or an enum constant.
 ///
 /// If this fails, at least one of the returned DeclRefExpr or Expr will be
 /// null.
 static std::tuple<const Expr *, BinaryOperatorKind, const Expr *>
-tryNormalizeBinaryOperator(const BinaryOperator *B) {
+tryNormalizeBinaryOperator(const BinaryOperator *B, ASTContext &Ctx) {
   BinaryOperatorKind Op = B->getOpcode();
 
   const Expr *MaybeDecl = B->getLHS();
-  const Expr *Constant = tryTransformToIntOrEnumConstant(B->getRHS());
+  const Expr *Constant = tryTransformToIntOrEnumConstant(B->getRHS(), Ctx);
   // Expr looked like `0 == Foo` instead of `Foo == 0`
   if (Constant == nullptr) {
     // Flip the operator
     if (Op == BO_GT)
       Op = BO_LT;
     else if (Op == BO_GE)
       Op = BO_LE;
     else if (Op == BO_LT)
       Op = BO_GT;
     else if (Op == BO_LE)
       Op = BO_GE;
 
     MaybeDecl = B->getRHS();
-    Constant = tryTransformToIntOrEnumConstant(B->getLHS());
+    Constant = tryTransformToIntOrEnumConstant(B->getLHS(), Ctx);
   }
 
   return std::make_tuple(MaybeDecl, Op, Constant);
 }
 
 /// For an expression `x == Foo && x == Bar`, this determines whether the
 /// `Foo` and `Bar` are either of the same enumeration type, or both integer
 /// literals.
 ///
 /// It's an error to pass this arguments that are not either IntegerLiterals
-/// or DeclRefExprs (that have decls of type EnumConstantDecl)
+/// or DeclRefExprs
 static bool areExprTypesCompatible(const Expr *E1, const Expr *E2) {
   // User intent isn't clear if they're mixing int literals with enum
   // constants.
   if (isa<DeclRefExpr>(E1) != isa<DeclRefExpr>(E2))
     return false;
 
   // Integer literal comparisons, regardless of literal type, are acceptable.
   if (!isa<DeclRefExpr>(E1))
     return true;
 
-  // IntegerLiterals are handled above and only EnumConstantDecls are expected
+  // IntegerLiterals are handled above and only certain Decls are expected
   // beyond this point
   assert(isa<DeclRefExpr>(E1) && isa<DeclRefExpr>(E2));
   auto *Decl1 = cast<DeclRefExpr>(E1)->getDecl();
   auto *Decl2 = cast<DeclRefExpr>(E2)->getDecl();
 
-  assert(isa<EnumConstantDecl>(Decl1) && isa<EnumConstantDecl>(Decl2));
+  if (isa<EnumConstantDecl>(Decl1) && isa<EnumConstantDecl>(Decl2)) {
     const DeclContext *DC1 = Decl1->getDeclContext();
     const DeclContext *DC2 = Decl2->getDeclContext();
 
     assert(isa<EnumDecl>(DC1) && isa<EnumDecl>(DC2));
     return DC1 == DC2;
   }
 
+  return false;
+}
+
 namespace {
 
 class CFGBuilder;
 
 /// The CFG builder uses a recursive algorithm to build the CFG.  When
 ///  we process an expression, sometimes we know that we must add the
 ///  subexpressions as block-level expressions.  For example:
 ///
if (!LHS || !RHS)
       return {};
 
     if (!LHS->isComparisonOp() || !RHS->isComparisonOp())
       return {};
 
     const Expr *DeclExpr1;
     const Expr *NumExpr1;
     BinaryOperatorKind BO1;
-    std::tie(DeclExpr1, BO1, NumExpr1) = tryNormalizeBinaryOperator(LHS);
+    std::tie(DeclExpr1, BO1, NumExpr1) =
+        tryNormalizeBinaryOperator(LHS, *Context);
 
     if (!DeclExpr1 || !NumExpr1)
       return {};
 
     const Expr *DeclExpr2;
     const Expr *NumExpr2;
     BinaryOperatorKind BO2;
-    std::tie(DeclExpr2, BO2, NumExpr2) = tryNormalizeBinaryOperator(RHS);
+    std::tie(DeclExpr2, BO2, NumExpr2) =
+        tryNormalizeBinaryOperator(RHS, *Context);
 
     if (!DeclExpr2 || !NumExpr2)
       return {};
 
     // Check that it is the same variable on both sides.
     if (!Expr::isSameComparisonOperand(DeclExpr1, DeclExpr2))
       return {};
 
if (AlwaysTrue || AlwaysFalse) {
         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());
+    const Expr *LHSConstant = tryTransformToIntOrEnumConstant(
+        B->getLHS()->IgnoreParenImpCasts(), *Context);
+    const Expr *RHSConstant = tryTransformToIntOrEnumConstant(
+        B->getRHS()->IgnoreParenImpCasts(), *Context);
 
     if ((LHSConstant && RHSConstant) || (!LHSConstant && !RHSConstant))
       return {};
 
     const Expr *Constant = LHSConstant ? LHSConstant : RHSConstant;
 
     Expr::EvalResult Result;
     if (!Constant->EvaluateAsInt(Result, *Context))

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

 // RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-bitwise-compare %s
 // RUN: %clang_cc1 -fsyntax-only -verify -Wall -Wno-unused %s
 
 #define mydefine 2
 
 enum {
   ZERO,
   ONE,
 };
 
+const int const1 = 1;
+
 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}}
void f(int x) {
 
   if ((x & 9) == 8) {}
   if ((x & 9) != 8) {}
   if ((x | 4) == 5) {}
   if ((x | 4) != 5) {}
 
   if ((x & mydefine) == 8) {}
   if ((x | mydefine) == 4) {}
+
+  if ((x & const1) == 8) {}
+  if ((x | const1) == 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) {}
+
+  if (x | const1) {}
 }

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

 // RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-bitwise-compare %s
 // RUN: %clang_cc1 -fsyntax-only -verify -Wall -Wno-unused %s
 
+int f() { return 3; }
+
+const int const1 = 1;
+constexpr int const2 = 2;
+const int const3 = f();
+
 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}}
+  bool b5 = (x | const1);
+  // expected-warning@-1 {{bitwise or with non-zero value always evaluates to true}}
+  bool b6 = (x | const2);
+  // expected-warning@-1 {{bitwise or with non-zero value always evaluates to true}}
+  bool b7 = (x | const3);
+
+  // No warnings at least for now:
+  bool b8 = (x & const1) == 8;
+  bool b9 = (x | const1) == 4;
+  bool b10 = (x & const2) == 8;
+  bool b11 = (x | const2) == 4;
 }