Skip to content

Commit fd874e5

Browse files
CodesbyusmanAaronBallman
Codesbyusman
authored and
AaronBallman
committed
Missing tautological compare warnings due to unary operators
The patch mainly focuses on the no warnings for -Wtautological-compare. It work fine for the positive numbers but doesn't for the 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. Fixes #42918 Differential Revision: https://reviews.llvm.org/D130510
1 parent d53e245 commit fd874e5

File tree

5 files changed

+136
-18
lines changed

5 files changed

+136
-18
lines changed

‎clang/docs/ReleaseNotes.rst

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,9 @@ Bug Fixes
7575
This fixes `Issue 56094 <https://github.com/llvm/llvm-project/issues/56094>`_.
7676
- Fix `#57151 <https://github.com/llvm/llvm-project/issues/57151>`_.
7777
``-Wcomma`` is emitted for void returning functions.
78+
- ``-Wtautological-compare`` missed warnings for tautological comparisons
79+
involving a negative integer literal. This fixes
80+
`Issue 42918 <https://github.com/llvm/llvm-project/issues/42918>`_.
7881

7982
Improvements to Clang's diagnostics
8083
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

‎clang/lib/Analysis/CFG.cpp

Lines changed: 57 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,10 @@ static SourceLocation GetEndLoc(Decl *D) {
7272

7373
/// Returns true on constant values based around a single IntegerLiteral.
7474
/// Allow for use of parentheses, integer casts, and negative signs.
75+
/// FIXME: it would be good to unify this function with
76+
/// getIntegerLiteralSubexpressionValue at some point given the similarity
77+
/// between the functions.
78+
7579
static bool IsIntegerLiteralConstantExpr(const Expr *E) {
7680
// Allow parentheses
7781
E = E->IgnoreParens();
@@ -964,15 +968,16 @@ class CFGBuilder {
964968
const Expr *LHSExpr = B->getLHS()->IgnoreParens();
965969
const Expr *RHSExpr = B->getRHS()->IgnoreParens();
966970

967-
const IntegerLiteral *IntLiteral = dyn_cast<IntegerLiteral>(LHSExpr);
971+
Optional<llvm::APInt> IntLiteral1 =
972+
getIntegerLiteralSubexpressionValue(LHSExpr);
968973
const Expr *BoolExpr = RHSExpr;
969974

970-
if (!IntLiteral) {
971-
IntLiteral = dyn_cast<IntegerLiteral>(RHSExpr);
975+
if (!IntLiteral1) {
976+
IntLiteral1 = getIntegerLiteralSubexpressionValue(RHSExpr);
972977
BoolExpr = LHSExpr;
973978
}
974979

975-
if (!IntLiteral)
980+
if (!IntLiteral1)
976981
return TryResult();
977982

978983
const BinaryOperator *BitOp = dyn_cast<BinaryOperator>(BoolExpr);
@@ -981,26 +986,26 @@ class CFGBuilder {
981986
const Expr *LHSExpr2 = BitOp->getLHS()->IgnoreParens();
982987
const Expr *RHSExpr2 = BitOp->getRHS()->IgnoreParens();
983988

984-
const IntegerLiteral *IntLiteral2 = dyn_cast<IntegerLiteral>(LHSExpr2);
989+
Optional<llvm::APInt> IntLiteral2 =
990+
getIntegerLiteralSubexpressionValue(LHSExpr2);
985991

986992
if (!IntLiteral2)
987-
IntLiteral2 = dyn_cast<IntegerLiteral>(RHSExpr2);
993+
IntLiteral2 = getIntegerLiteralSubexpressionValue(RHSExpr2);
988994

989995
if (!IntLiteral2)
990996
return TryResult();
991997

992-
llvm::APInt L1 = IntLiteral->getValue();
993-
llvm::APInt L2 = IntLiteral2->getValue();
994-
if ((BitOp->getOpcode() == BO_And && (L2 & L1) != L1) ||
995-
(BitOp->getOpcode() == BO_Or && (L2 | L1) != L1)) {
998+
if ((BitOp->getOpcode() == BO_And &&
999+
(*IntLiteral2 & *IntLiteral1) != *IntLiteral1) ||
1000+
(BitOp->getOpcode() == BO_Or &&
1001+
(*IntLiteral2 | *IntLiteral1) != *IntLiteral1)) {
9961002
if (BuildOpts.Observer)
9971003
BuildOpts.Observer->compareBitwiseEquality(B,
9981004
B->getOpcode() != BO_EQ);
9991005
TryResult(B->getOpcode() != BO_EQ);
10001006
}
10011007
} else if (BoolExpr->isKnownToHaveBooleanValue()) {
1002-
llvm::APInt IntValue = IntLiteral->getValue();
1003-
if ((IntValue == 1) || (IntValue == 0)) {
1008+
if ((*IntLiteral1 == 1) || (*IntLiteral1 == 0)) {
10041009
return TryResult();
10051010
}
10061011
return TryResult(B->getOpcode() != BO_EQ);
@@ -1009,6 +1014,46 @@ class CFGBuilder {
10091014
return TryResult();
10101015
}
10111016

1017+
// Helper function to get an APInt from an expression. Supports expressions
1018+
// which are an IntegerLiteral or a UnaryOperator and returns the value with
1019+
// all operations performed on it.
1020+
// FIXME: it would be good to unify this function with
1021+
// IsIntegerLiteralConstantExpr at some point given the similarity between the
1022+
// functions.
1023+
Optional<llvm::APInt> getIntegerLiteralSubexpressionValue(const Expr *E) {
1024+
1025+
// If unary.
1026+
if (const auto *UnOp = dyn_cast<UnaryOperator>(E->IgnoreParens())) {
1027+
// Get the sub expression of the unary expression and get the Integer
1028+
// Literal.
1029+
const Expr *SubExpr = UnOp->getSubExpr()->IgnoreParens();
1030+
1031+
if (const auto *IntLiteral = dyn_cast<IntegerLiteral>(SubExpr)) {
1032+
1033+
llvm::APInt Value = IntLiteral->getValue();
1034+
1035+
// Perform the operation manually.
1036+
switch (UnOp->getOpcode()) {
1037+
case UO_Plus:
1038+
return Value;
1039+
case UO_Minus:
1040+
return -Value;
1041+
case UO_Not:
1042+
return ~Value;
1043+
case UO_LNot:
1044+
return llvm::APInt(Value.getBitWidth(), !Value);
1045+
default:
1046+
assert(false && "Unexpected unary operator!");
1047+
return llvm::None;
1048+
}
1049+
}
1050+
} else if (const auto *IntLiteral =
1051+
dyn_cast<IntegerLiteral>(E->IgnoreParens()))
1052+
return IntLiteral->getValue();
1053+
1054+
return llvm::None;
1055+
}
1056+
10121057
TryResult analyzeLogicOperatorCondition(BinaryOperatorKind Relation,
10131058
const llvm::APSInt &Value1,
10141059
const llvm::APSInt &Value2) {

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

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// RUN: %clang_cc1 -fsyntax-only -verify -Wall -Wno-unused %s
33

44
#define mydefine 2
5+
#define mydefine2 -2
56

67
enum {
78
ZERO,
@@ -11,29 +12,85 @@ enum {
1112
void f(int x) {
1213
if ((8 & x) == 3) {} // expected-warning {{bitwise comparison always evaluates to false}}
1314
if ((x & 8) == 4) {} // expected-warning {{bitwise comparison always evaluates to false}}
15+
if ((-8 & x) == 3) {} // expected-warning {{bitwise comparison always evaluates to false}}
16+
if ((x & -8) == 4) {} // expected-warning {{bitwise comparison always evaluates to false}}
17+
1418
if ((x & 8) != 4) {} // expected-warning {{bitwise comparison always evaluates to true}}
1519
if ((2 & x) != 4) {} // expected-warning {{bitwise comparison always evaluates to true}}
20+
if ((x & -8) != 4) {} // expected-warning {{bitwise comparison always evaluates to true}}
21+
if ((-2 & x) != 3) {} // expected-warning {{bitwise comparison always evaluates to true}}
22+
1623
if ((x | 4) == 3) {} // expected-warning {{bitwise comparison always evaluates to false}}
1724
if ((x | 3) != 4) {} // expected-warning {{bitwise comparison always evaluates to true}}
1825
if ((5 | x) != 3) {} // expected-warning {{bitwise comparison always evaluates to true}}
26+
27+
if ((x | -4) == 3) {} // expected-warning {{bitwise comparison always evaluates to false}}
28+
if ((x | -3) != 4) {} // expected-warning {{bitwise comparison always evaluates to true}}
29+
if ((-5 | x) != 3) {} // expected-warning {{bitwise comparison always evaluates to true}}
30+
1931
if ((x & 0x15) == 0x13) {} // expected-warning {{bitwise comparison always evaluates to false}}
32+
if ((x & 0xFFEB) == 0x13) {} // expected-warning {{bitwise comparison always evaluates to false}}
33+
2034
if ((0x23 | x) == 0x155){} // expected-warning {{bitwise comparison always evaluates to false}}
35+
if ((0xFFDD | x) == 0x155){} // expected-warning {{bitwise comparison always evaluates to false}}
2136

2237
if (!!((8 & x) == 3)) {} // expected-warning {{bitwise comparison always evaluates to false}}
38+
if (!!((-8 & x) == 3)) {} // expected-warning {{bitwise comparison always evaluates to false}}
39+
2340
int y = ((8 & x) == 3) ? 1 : 2; // expected-warning {{bitwise comparison always evaluates to false}}
41+
y = ((-8 & x) == 3) ? 1 : 2; // expected-warning {{bitwise comparison always evaluates to false}}
42+
y = ((3 | x) != 5) ? 1 : 2; // expected-warning {{bitwise comparison always evaluates to true}}
43+
y = ((-3 | x) != 5) ? 1 : 2; // expected-warning {{bitwise comparison always evaluates to true}}
44+
45+
if ((x & 0) != !0) {} // expected-warning {{bitwise comparison always evaluates to true}}
46+
if ((x & 0) == !0) {} // expected-warning {{bitwise comparison always evaluates to false}}
47+
if ((x & 2) == !0) {} // expected-warning {{bitwise comparison always evaluates to false}}
48+
49+
if ((x | 4) == +3) {} // expected-warning {{bitwise comparison always evaluates to false}}
50+
if ((x | 3) != +4) {} // expected-warning {{bitwise comparison always evaluates to true}}
51+
52+
if ((x & 8) != ~4) {} // expected-warning {{bitwise comparison always evaluates to true}}
53+
if ((x & 0) == ~4) {} // expected-warning {{bitwise comparison always evaluates to false}}
2454

2555
if ((x & 8) == 8) {}
2656
if ((x & 8) != 8) {}
2757
if ((x | 4) == 4) {}
2858
if ((x | 4) != 4) {}
2959

60+
if ((x | 4) == +4) {}
61+
if ((x | 4) != +4) {}
62+
63+
if ((x & 1) == !12) {}
64+
if ((x | 0) == !0) {}
65+
66+
if ((x | 1) == ~4) {}
67+
if ((x | 1) != ~0) {}
68+
69+
if ((-2 & x) != 4) {}
70+
if ((x & -8) == -8) {}
71+
if ((x & -8) != -8) {}
72+
if ((x | -4) == -4) {}
73+
if ((x | -4) != -4) {}
74+
3075
if ((x & 9) == 8) {}
3176
if ((x & 9) != 8) {}
3277
if ((x | 4) == 5) {}
3378
if ((x | 4) != 5) {}
3479

80+
if ((x & -9) == -10) {}
81+
if ((x & -9) != -10) {}
82+
if ((x | -4) == -3) {}
83+
if ((x | -4) != -3) {}
84+
85+
if ((x^0) == 0) {}
86+
3587
if ((x & mydefine) == 8) {}
3688
if ((x | mydefine) == 4) {}
89+
90+
if ((x & mydefine2) == 8) {}
91+
if ((x | mydefine2) == 4) {}
92+
93+
if ((x & 1) == 1L) {}
3794
}
3895

3996
void g(int x) {

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,3 +11,14 @@ void test(int x) {
1111
bool b4 = !!(x | 5);
1212
// expected-warning@-1 {{bitwise or with non-zero value always evaluates to true}}
1313
}
14+
15+
template <int I, class T> // silence
16+
void foo(int x) {
17+
bool b1 = (x & sizeof(T)) == 8;
18+
bool b2 = (x & I) == 8;
19+
bool b3 = (x & 4) == 8; // expected-warning {{bitwise comparison always evaluates to false}}
20+
}
21+
22+
void run(int x) {
23+
foo<4, int>(8); // expected-note {{in instantiation of function template specialization 'foo<4, int>' requested here}}
24+
}

‎clang/test/SemaCXX/warn-unreachable.cpp

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -396,16 +396,18 @@ void tautological_compare(bool x, int y) {
396396
if (y == -1 && y != -1) // expected-note {{silence}}
397397
calledFun(); // expected-warning {{will never be executed}}
398398

399-
// TODO: Extend warning to the following code:
400-
if (x < -1)
401-
calledFun();
402-
if (x == -1)
403-
calledFun();
399+
if (x == -1) // expected-note {{silence}}
400+
calledFun(); // expected-warning {{will never be executed}}
404401

405-
if (x != -1)
402+
if (x != -1) // expected-note {{silence}}
406403
calledFun();
407404
else
405+
calledFun(); // expected-warning {{will never be executed}}
406+
407+
// TODO: Extend warning to the following code:
408+
if (x < -1)
408409
calledFun();
410+
409411
if (-1 > x)
410412
calledFun();
411413
else

0 commit comments

Comments
 (0)