[clang] 0cc3c18 - Missing tautological compare warnings due to unary operators
Aaron Ballman via cfe-commits
cfe-commits at lists.llvm.org
Thu Jul 28 04:49:16 PDT 2022
Author: Muhammad Usman Shahid
Date: 2022-07-28T07:45:28-04:00
New Revision: 0cc3c184c784d5f0d55de8ad0a9eeee876acd149
URL: https://github.com/llvm/llvm-project/commit/0cc3c184c784d5f0d55de8ad0a9eeee876acd149
DIFF: https://github.com/llvm/llvm-project/commit/0cc3c184c784d5f0d55de8ad0a9eeee876acd149.diff
LOG: 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
Added:
Modified:
clang/docs/ReleaseNotes.rst
clang/lib/Analysis/CFG.cpp
clang/test/Sema/warn-bitwise-compare.c
clang/test/SemaCXX/warn-unreachable.cpp
Removed:
################################################################################
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 4f5e71e0ba80..5d5e709ba4a1 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/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
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp
index 84178ff488a5..30617e48fde4 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/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);
diff --git a/clang/test/Sema/warn-bitwise-compare.c b/clang/test/Sema/warn-bitwise-compare.c
index 08a8b084fe79..b96881fe296e 100644
--- a/clang/test/Sema/warn-bitwise-compare.c
+++ b/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) {
diff --git a/clang/test/SemaCXX/warn-unreachable.cpp b/clang/test/SemaCXX/warn-unreachable.cpp
index c664c1912899..0d9468c51483 100644
--- a/clang/test/SemaCXX/warn-unreachable.cpp
+++ b/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();
More information about the cfe-commits
mailing list