Missing tautological compare warnings due to unary operators

The patch mainly focuses on the lack of warnings for
-Wtautological-compare. It works fine for positive numbers but doesn't
for negative numbers. This is because the warning explicitly checks for
an IntegerLiteral AST node, but -1 is represented by a UnaryOperator
with an IntegerLiteral sub-Expr.

For the below code we have warnings:

if (0 == (5 | x)) {}

but not for

if (0 == (-5 | x)) {}

This patch changes the analysis to not look at the AST node directly to
see if it is an IntegerLiteral, but instead attempts to evaluate the
expression to see if it is an integer constant expression. This handles
unary negation signs, but also handles all the other possible operators
as well.

Fixes #42918
Differential Revision: https://reviews.llvm.org/D130510

Author: Codesbyusman

clang/docs/ReleaseNotes.rst

@@ -49,6 +49,10 @@ Major New Features
 
 Bug Fixes
 ---------
+- ``-Wtautological-compare`` missed warnings for tautological comparisons
+  involving a negative integer literal. This fixes
+  `Issue 42918 <https://github.com/llvm/llvm-project/issues/42918>`_.
+
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

clang/lib/Analysis/CFG.cpp

@@ -964,43 +964,44 @@ class CFGBuilder {
     const Expr *LHSExpr = B->getLHS()->IgnoreParens();
     const Expr *RHSExpr = B->getRHS()->IgnoreParens();
 
-    const IntegerLiteral *IntLiteral = dyn_cast<IntegerLiteral>(LHSExpr);
-    const Expr *BoolExpr = RHSExpr;
+    const Expr *BoolExpr = nullptr; // To store the expression.
+    Expr::EvalResult IntExprResult; // If integer literal then will save value.
 
-    if (!IntLiteral) {
-      IntLiteral = dyn_cast<IntegerLiteral>(RHSExpr);
+    if (LHSExpr->EvaluateAsInt(IntExprResult, *Context))
+      BoolExpr = RHSExpr;
+    else if (RHSExpr->EvaluateAsInt(IntExprResult, *Context))
       BoolExpr = LHSExpr;
-    }
-
-    if (!IntLiteral)
+    else
       return TryResult();
 
+    llvm::APInt L1 = IntExprResult.Val.getInt();
+
     const BinaryOperator *BitOp = dyn_cast<BinaryOperator>(BoolExpr);
-    if (BitOp && (BitOp->getOpcode() == BO_And ||
-                  BitOp->getOpcode() == BO_Or)) {
-      const Expr *LHSExpr2 = BitOp->getLHS()->IgnoreParens();
-      const Expr *RHSExpr2 = BitOp->getRHS()->IgnoreParens();
+    if (BitOp &&
+        (BitOp->getOpcode() == BO_And || BitOp->getOpcode() == BO_Or)) {
 
-      const IntegerLiteral *IntLiteral2 = dyn_cast<IntegerLiteral>(LHSExpr2);
+      // If integer literal in expression identified then will save value.
+      Expr::EvalResult IntExprResult2;
 
-      if (!IntLiteral2)
-        IntLiteral2 = dyn_cast<IntegerLiteral>(RHSExpr2);
+      if (BitOp->getLHS()->EvaluateAsInt(IntExprResult2, *Context))
+        ; // LHS is a constant expression.
+      else if (BitOp->getRHS()->EvaluateAsInt(IntExprResult2, *Context))
+        ; // RHS is a constant expression.
+      else
+        return TryResult(); // Neither is a constant expression, bail out.
 
-      if (!IntLiteral2)
-        return TryResult();
+      llvm::APInt L2 = IntExprResult2.Val.getInt();
 
-      llvm::APInt L1 = IntLiteral->getValue();
-      llvm::APInt L2 = IntLiteral2->getValue();
       if ((BitOp->getOpcode() == BO_And && (L2 & L1) != L1) ||
-          (BitOp->getOpcode() == BO_Or  && (L2 | L1) != L1)) {
+          (BitOp->getOpcode() == BO_Or && (L2 | L1) != L1)) {
         if (BuildOpts.Observer)
           BuildOpts.Observer->compareBitwiseEquality(B,
                                                      B->getOpcode() != BO_EQ);
         TryResult(B->getOpcode() != BO_EQ);
       }
     } else if (BoolExpr->isKnownToHaveBooleanValue()) {
-      llvm::APInt IntValue = IntLiteral->getValue();
-      if ((IntValue == 1) || (IntValue == 0)) {
+      llvm::APInt IntValue = IntExprResult.Val.getInt(); // Getting the value.
+      if ((L1 == 1) || (L1 == 0)) {
         return TryResult();
       }
       return TryResult(B->getOpcode() != BO_EQ);

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

@@ -2,6 +2,7 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -Wall -Wno-unused %s
 
 #define mydefine 2
+#define mydefine2 -2
 
 enum {
   ZERO,
@@ -11,29 +12,67 @@ enum {
 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 ((-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 & -8) != 4) {}  // expected-warning {{bitwise comparison always evaluates to true}}
+  if ((-2 & x) != 3) {}  // 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 | -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 ((x & 0xFFEB) == 0x13) {} // expected-warning {{bitwise comparison always evaluates to false}}
+
   if ((0x23 | x) == 0x155){} // expected-warning {{bitwise comparison always evaluates to false}}
+  if ((0xFFDD | x) == 0x155){} // expected-warning {{bitwise comparison always evaluates to false}}
 
   if (!!((8 & x) == 3)) {}  // 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}}
+      y = ((-8 & x) == 3) ? 1 : 2;  // expected-warning {{bitwise comparison always evaluates to false}}
+      y = ((3 | x) != 5) ? 1 : 2;  // expected-warning {{bitwise comparison always evaluates to true}}
+      y = ((-3 | x) != 5) ? 1 : 2;  // expected-warning {{bitwise comparison always evaluates to true}}
 
   if ((x & 8) == 8) {}
   if ((x & 8) != 8) {}
   if ((x | 4) == 4) {}
   if ((x | 4) != 4) {}
 
+  if ((-2 & x) != 4) {}
+  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 & -9) == -10) {}
+  if ((x & -9) != -10) {}
+  if ((x | -4) == -3) {}
+  if ((x | -4) != -3) {}
+
+  if ((x^0) == 0) {}
+
   if ((x & mydefine) == 8) {}
   if ((x | mydefine) == 4) {}
+
+  if ((x & mydefine2) == 8) {}
+  if ((x | mydefine2) == 4) {}
+
 }
 
 void g(int x) {

clang/test/SemaCXX/warn-unreachable.cpp

@@ -396,15 +396,16 @@ void tautological_compare(bool x, int y) {
   if (y == -1 && y != -1)  // expected-note {{silence}}
     calledFun();        // expected-warning {{will never be executed}}
 
-  // TODO: Extend warning to the following code:
-  if (x < -1)
-    calledFun();
-  if (x == -1)
-    calledFun();
+  if (x == -1)   // expected-note {{silence}}
+    calledFun(); // expected-warning {{will never be executed}}
 
-  if (x != -1)
+  if (x != -1)   // expected-note {{silence}}
     calledFun();
   else
+    calledFun(); // expected-warning {{will never be executed}}
+
+  // TODO: Extend warning to the following code:
+  if (x < -1)
     calledFun();
   if (-1 > x)
     calledFun();