Skip to content

Commit 70e7e87

Browse files
jrose-applejrose-apple
committedFeb 19, 2014
[analyzer] Extend IdenticalExprChecker to check the two branches of an if.
This extends the checks for identical expressions to handle identical statements, and compares the consequent and alternative ("then" and "else") branches of an if-statement to see if they are identical, treating a single statement surrounded by braces as equivalent to one without braces. This does /not/ check subsequent branches in an if/else chain, let alone branches that are not consecutive. This may improve in a future patch, but it would certainly take more work. Patch by Daniel Fahlgren! llvm-svn: 201701
·
llvmorg-21-initllvmorg-3.5.0-rc1
1 parent daeafb4 commit 70e7e87

File tree

3 files changed

+340
-51
lines changed

3 files changed

+340
-51
lines changed
 

‎clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp

+188-51
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,8 @@
2626
using namespace clang;
2727
using namespace ento;
2828

29-
static bool isIdenticalExpr(const ASTContext &Ctx, const Expr *Expr1,
30-
const Expr *Expr2, bool IgnoreSideEffects = false);
29+
static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
30+
const Stmt *Stmt2, bool IgnoreSideEffects = false);
3131
//===----------------------------------------------------------------------===//
3232
// FindIdenticalExprVisitor - Identify nodes using identical expressions.
3333
//===----------------------------------------------------------------------===//
@@ -41,8 +41,10 @@ class FindIdenticalExprVisitor
4141
AnalysisDeclContext *A)
4242
: BR(B), Checker(Checker), AC(A) {}
4343
// FindIdenticalExprVisitor only visits nodes
44-
// that are binary operators or conditional operators.
44+
// that are binary operators, if statements or
45+
// conditional operators.
4546
bool VisitBinaryOperator(const BinaryOperator *B);
47+
bool VisitIfStmt(const IfStmt *I);
4648
bool VisitConditionalOperator(const ConditionalOperator *C);
4749

4850
private:
@@ -52,6 +54,39 @@ class FindIdenticalExprVisitor
5254
};
5355
} // end anonymous namespace
5456

57+
bool FindIdenticalExprVisitor::VisitIfStmt(const IfStmt *I) {
58+
const Stmt *Stmt1 = I->getThen();
59+
const Stmt *Stmt2 = I->getElse();
60+
61+
if (!Stmt1 || !Stmt2)
62+
return true;
63+
64+
// Special handling for code like:
65+
//
66+
// if (b) {
67+
// i = 1;
68+
// } else
69+
// i = 1;
70+
if (const CompoundStmt *CompStmt = dyn_cast<CompoundStmt>(Stmt1)) {
71+
if (CompStmt->size() == 1)
72+
Stmt1 = CompStmt->body_back();
73+
}
74+
if (const CompoundStmt *CompStmt = dyn_cast<CompoundStmt>(Stmt2)) {
75+
if (CompStmt->size() == 1)
76+
Stmt2 = CompStmt->body_back();
77+
}
78+
79+
if (isIdenticalStmt(AC->getASTContext(), Stmt1, Stmt2, true)) {
80+
PathDiagnosticLocation ELoc =
81+
PathDiagnosticLocation::createBegin(I, BR.getSourceManager(), AC);
82+
BR.EmitBasicReport(AC->getDecl(), Checker,
83+
"Identical branches",
84+
categories::LogicError,
85+
"true and false branches are identical", ELoc);
86+
}
87+
return true;
88+
}
89+
5590
bool FindIdenticalExprVisitor::VisitBinaryOperator(const BinaryOperator *B) {
5691
BinaryOperator::Opcode Op = B->getOpcode();
5792
if (!BinaryOperator::isComparisonOp(Op))
@@ -107,7 +142,7 @@ bool FindIdenticalExprVisitor::VisitBinaryOperator(const BinaryOperator *B) {
107142
// No special case with floating-point representation, report as usual.
108143
}
109144

110-
if (isIdenticalExpr(AC->getASTContext(), B->getLHS(), B->getRHS())) {
145+
if (isIdenticalStmt(AC->getASTContext(), B->getLHS(), B->getRHS())) {
111146
PathDiagnosticLocation ELoc =
112147
PathDiagnosticLocation::createOperatorLoc(B, BR.getSourceManager());
113148
StringRef Message;
@@ -131,7 +166,7 @@ bool FindIdenticalExprVisitor::VisitConditionalOperator(
131166
// Check if expressions in conditional expression are identical
132167
// from a symbolic point of view.
133168

134-
if (isIdenticalExpr(AC->getASTContext(), C->getTrueExpr(),
169+
if (isIdenticalStmt(AC->getASTContext(), C->getTrueExpr(),
135170
C->getFalseExpr(), true)) {
136171
PathDiagnosticLocation ELoc =
137172
PathDiagnosticLocation::createConditionalColonLoc(
@@ -153,89 +188,191 @@ bool FindIdenticalExprVisitor::VisitConditionalOperator(
153188
return true;
154189
}
155190

156-
/// \brief Determines whether two expression trees are identical regarding
191+
/// \brief Determines whether two statement trees are identical regarding
157192
/// operators and symbols.
158193
///
159194
/// Exceptions: expressions containing macros or functions with possible side
160195
/// effects are never considered identical.
161196
/// Limitations: (t + u) and (u + t) are not considered identical.
162197
/// t*(u + t) and t*u + t*t are not considered identical.
163198
///
164-
static bool isIdenticalExpr(const ASTContext &Ctx, const Expr *Expr1,
165-
const Expr *Expr2, bool IgnoreSideEffects) {
166-
// If Expr1 & Expr2 are of different class then they are not
167-
// identical expression.
168-
if (Expr1->getStmtClass() != Expr2->getStmtClass())
169-
return false;
170-
// If Expr1 has side effects then don't warn even if expressions
171-
// are identical.
172-
if (!IgnoreSideEffects && Expr1->HasSideEffects(Ctx))
199+
static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
200+
const Stmt *Stmt2, bool IgnoreSideEffects) {
201+
202+
if (!Stmt1 || !Stmt2) {
203+
if (!Stmt1 && !Stmt2)
204+
return true;
173205
return false;
174-
// If either expression comes from a macro then don't warn even if
175-
// the expressions are identical.
176-
if ((Expr1->getExprLoc().isMacroID()) || (Expr2->getExprLoc().isMacroID()))
206+
}
207+
208+
// If Stmt1 & Stmt2 are of different class then they are not
209+
// identical statements.
210+
if (Stmt1->getStmtClass() != Stmt2->getStmtClass())
177211
return false;
178-
// If all children of two expressions are identical, return true.
179-
Expr::const_child_iterator I1 = Expr1->child_begin();
180-
Expr::const_child_iterator I2 = Expr2->child_begin();
181-
while (I1 != Expr1->child_end() && I2 != Expr2->child_end()) {
182-
const Expr *Child1 = dyn_cast<Expr>(*I1);
183-
const Expr *Child2 = dyn_cast<Expr>(*I2);
184-
if (!Child1 || !Child2 || !isIdenticalExpr(Ctx, Child1, Child2,
185-
IgnoreSideEffects))
212+
213+
const Expr *Expr1 = dyn_cast<Expr>(Stmt1);
214+
const Expr *Expr2 = dyn_cast<Expr>(Stmt2);
215+
216+
if (Expr1 && Expr2) {
217+
// If Stmt1 has side effects then don't warn even if expressions
218+
// are identical.
219+
if (!IgnoreSideEffects && Expr1->HasSideEffects(Ctx))
220+
return false;
221+
// If either expression comes from a macro then don't warn even if
222+
// the expressions are identical.
223+
if ((Expr1->getExprLoc().isMacroID()) || (Expr2->getExprLoc().isMacroID()))
224+
return false;
225+
226+
// If all children of two expressions are identical, return true.
227+
Expr::const_child_iterator I1 = Expr1->child_begin();
228+
Expr::const_child_iterator I2 = Expr2->child_begin();
229+
while (I1 != Expr1->child_end() && I2 != Expr2->child_end()) {
230+
if (!*I1 || !*I2 || !isIdenticalStmt(Ctx, *I1, *I2, IgnoreSideEffects))
231+
return false;
232+
++I1;
233+
++I2;
234+
}
235+
// If there are different number of children in the statements, return
236+
// false.
237+
if (I1 != Expr1->child_end())
238+
return false;
239+
if (I2 != Expr2->child_end())
186240
return false;
187-
++I1;
188-
++I2;
189241
}
190-
// If there are different number of children in the expressions, return false.
191-
// (TODO: check if this is a redundant condition.)
192-
if (I1 != Expr1->child_end())
193-
return false;
194-
if (I2 != Expr2->child_end())
195-
return false;
196242

197-
switch (Expr1->getStmtClass()) {
243+
switch (Stmt1->getStmtClass()) {
198244
default:
199245
return false;
200246
case Stmt::CallExprClass:
201247
case Stmt::ArraySubscriptExprClass:
202-
case Stmt::CStyleCastExprClass:
203248
case Stmt::ImplicitCastExprClass:
204249
case Stmt::ParenExprClass:
250+
case Stmt::BreakStmtClass:
251+
case Stmt::ContinueStmtClass:
252+
case Stmt::NullStmtClass:
253+
return true;
254+
case Stmt::CStyleCastExprClass: {
255+
const CStyleCastExpr* CastExpr1 = cast<CStyleCastExpr>(Stmt1);
256+
const CStyleCastExpr* CastExpr2 = cast<CStyleCastExpr>(Stmt2);
257+
258+
return CastExpr1->getTypeAsWritten() == CastExpr2->getTypeAsWritten();
259+
}
260+
case Stmt::ReturnStmtClass: {
261+
const ReturnStmt *ReturnStmt1 = cast<ReturnStmt>(Stmt1);
262+
const ReturnStmt *ReturnStmt2 = cast<ReturnStmt>(Stmt2);
263+
264+
return isIdenticalStmt(Ctx, ReturnStmt1->getRetValue(),
265+
ReturnStmt2->getRetValue(), IgnoreSideEffects);
266+
}
267+
case Stmt::ForStmtClass: {
268+
const ForStmt *ForStmt1 = cast<ForStmt>(Stmt1);
269+
const ForStmt *ForStmt2 = cast<ForStmt>(Stmt2);
270+
271+
if (!isIdenticalStmt(Ctx, ForStmt1->getInit(), ForStmt2->getInit(),
272+
IgnoreSideEffects))
273+
return false;
274+
if (!isIdenticalStmt(Ctx, ForStmt1->getCond(), ForStmt2->getCond(),
275+
IgnoreSideEffects))
276+
return false;
277+
if (!isIdenticalStmt(Ctx, ForStmt1->getInc(), ForStmt2->getInc(),
278+
IgnoreSideEffects))
279+
return false;
280+
if (!isIdenticalStmt(Ctx, ForStmt1->getBody(), ForStmt2->getBody(),
281+
IgnoreSideEffects))
282+
return false;
283+
return true;
284+
}
285+
case Stmt::DoStmtClass: {
286+
const DoStmt *DStmt1 = cast<DoStmt>(Stmt1);
287+
const DoStmt *DStmt2 = cast<DoStmt>(Stmt2);
288+
289+
if (!isIdenticalStmt(Ctx, DStmt1->getCond(), DStmt2->getCond(),
290+
IgnoreSideEffects))
291+
return false;
292+
if (!isIdenticalStmt(Ctx, DStmt1->getBody(), DStmt2->getBody(),
293+
IgnoreSideEffects))
294+
return false;
295+
return true;
296+
}
297+
case Stmt::WhileStmtClass: {
298+
const WhileStmt *WStmt1 = cast<WhileStmt>(Stmt1);
299+
const WhileStmt *WStmt2 = cast<WhileStmt>(Stmt2);
300+
301+
return isIdenticalStmt(Ctx, WStmt1->getCond(), WStmt2->getCond(),
302+
IgnoreSideEffects);
303+
}
304+
case Stmt::IfStmtClass: {
305+
const IfStmt *IStmt1 = cast<IfStmt>(Stmt1);
306+
const IfStmt *IStmt2 = cast<IfStmt>(Stmt2);
307+
308+
if (!isIdenticalStmt(Ctx, IStmt1->getCond(), IStmt2->getCond(),
309+
IgnoreSideEffects))
310+
return false;
311+
if (!isIdenticalStmt(Ctx, IStmt1->getThen(), IStmt2->getThen(),
312+
IgnoreSideEffects))
313+
return false;
314+
if (!isIdenticalStmt(Ctx, IStmt1->getElse(), IStmt2->getElse(),
315+
IgnoreSideEffects))
316+
return false;
317+
return true;
318+
}
319+
case Stmt::CompoundStmtClass: {
320+
const CompoundStmt *CompStmt1 = cast<CompoundStmt>(Stmt1);
321+
const CompoundStmt *CompStmt2 = cast<CompoundStmt>(Stmt2);
322+
323+
if (CompStmt1->size() != CompStmt2->size())
324+
return false;
325+
326+
CompoundStmt::const_body_iterator I1 = CompStmt1->body_begin();
327+
CompoundStmt::const_body_iterator I2 = CompStmt2->body_begin();
328+
while (I1 != CompStmt1->body_end() && I2 != CompStmt2->body_end()) {
329+
if (!isIdenticalStmt(Ctx, *I1, *I2, IgnoreSideEffects))
330+
return false;
331+
++I1;
332+
++I2;
333+
}
334+
205335
return true;
336+
}
337+
case Stmt::CompoundAssignOperatorClass:
206338
case Stmt::BinaryOperatorClass: {
207-
const BinaryOperator *BinOp1 = cast<BinaryOperator>(Expr1);
208-
const BinaryOperator *BinOp2 = cast<BinaryOperator>(Expr2);
339+
const BinaryOperator *BinOp1 = cast<BinaryOperator>(Stmt1);
340+
const BinaryOperator *BinOp2 = cast<BinaryOperator>(Stmt2);
209341
return BinOp1->getOpcode() == BinOp2->getOpcode();
210342
}
211343
case Stmt::CharacterLiteralClass: {
212-
const CharacterLiteral *CharLit1 = cast<CharacterLiteral>(Expr1);
213-
const CharacterLiteral *CharLit2 = cast<CharacterLiteral>(Expr2);
344+
const CharacterLiteral *CharLit1 = cast<CharacterLiteral>(Stmt1);
345+
const CharacterLiteral *CharLit2 = cast<CharacterLiteral>(Stmt2);
214346
return CharLit1->getValue() == CharLit2->getValue();
215347
}
216348
case Stmt::DeclRefExprClass: {
217-
const DeclRefExpr *DeclRef1 = cast<DeclRefExpr>(Expr1);
218-
const DeclRefExpr *DeclRef2 = cast<DeclRefExpr>(Expr2);
349+
const DeclRefExpr *DeclRef1 = cast<DeclRefExpr>(Stmt1);
350+
const DeclRefExpr *DeclRef2 = cast<DeclRefExpr>(Stmt2);
219351
return DeclRef1->getDecl() == DeclRef2->getDecl();
220352
}
221353
case Stmt::IntegerLiteralClass: {
222-
const IntegerLiteral *IntLit1 = cast<IntegerLiteral>(Expr1);
223-
const IntegerLiteral *IntLit2 = cast<IntegerLiteral>(Expr2);
354+
const IntegerLiteral *IntLit1 = cast<IntegerLiteral>(Stmt1);
355+
const IntegerLiteral *IntLit2 = cast<IntegerLiteral>(Stmt2);
224356
return IntLit1->getValue() == IntLit2->getValue();
225357
}
226358
case Stmt::FloatingLiteralClass: {
227-
const FloatingLiteral *FloatLit1 = cast<FloatingLiteral>(Expr1);
228-
const FloatingLiteral *FloatLit2 = cast<FloatingLiteral>(Expr2);
359+
const FloatingLiteral *FloatLit1 = cast<FloatingLiteral>(Stmt1);
360+
const FloatingLiteral *FloatLit2 = cast<FloatingLiteral>(Stmt2);
229361
return FloatLit1->getValue().bitwiseIsEqual(FloatLit2->getValue());
230362
}
363+
case Stmt::StringLiteralClass: {
364+
const StringLiteral *StringLit1 = cast<StringLiteral>(Stmt1);
365+
const StringLiteral *StringLit2 = cast<StringLiteral>(Stmt2);
366+
return StringLit1->getString() == StringLit2->getString();
367+
}
231368
case Stmt::MemberExprClass: {
232-
const MemberExpr *MemberExpr1 = cast<MemberExpr>(Expr1);
233-
const MemberExpr *MemberExpr2 = cast<MemberExpr>(Expr2);
234-
return MemberExpr1->getMemberDecl() == MemberExpr2->getMemberDecl();
369+
const MemberExpr *MemberStmt1 = cast<MemberExpr>(Stmt1);
370+
const MemberExpr *MemberStmt2 = cast<MemberExpr>(Stmt2);
371+
return MemberStmt1->getMemberDecl() == MemberStmt2->getMemberDecl();
235372
}
236373
case Stmt::UnaryOperatorClass: {
237-
const UnaryOperator *UnaryOp1 = cast<UnaryOperator>(Expr1);
238-
const UnaryOperator *UnaryOp2 = cast<UnaryOperator>(Expr2);
374+
const UnaryOperator *UnaryOp1 = cast<UnaryOperator>(Stmt1);
375+
const UnaryOperator *UnaryOp2 = cast<UnaryOperator>(Stmt2);
239376
return UnaryOp1->getOpcode() == UnaryOp2->getOpcode();
240377
}
241378
}

‎clang/test/Analysis/identical-expressions.cpp

+151
Original file line numberDiff line numberDiff line change
@@ -1043,6 +1043,11 @@ void test_float() {
10431043
a = a > 5 ? a : a; // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
10441044
}
10451045

1046+
const char *test_string() {
1047+
float a = 0;
1048+
return a > 5 ? "abc" : "abc"; // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
1049+
}
1050+
10461051
void test_unsigned_expr() {
10471052
unsigned a = 0;
10481053
unsigned b = 0;
@@ -1158,3 +1163,149 @@ void test_signed_nested_cond_expr() {
11581163
int c = 3;
11591164
a = a > 5 ? (b > 5 ? 1 : 4) : (b > 5 ? 4 : 4); // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
11601165
}
1166+
1167+
void test_identical_branches1(bool b) {
1168+
int i = 0;
1169+
if (b) { // expected-warning {{true and false branches are identical}}
1170+
++i;
1171+
} else {
1172+
++i;
1173+
}
1174+
}
1175+
1176+
void test_identical_branches2(bool b) {
1177+
int i = 0;
1178+
if (b) { // expected-warning {{true and false branches are identical}}
1179+
++i;
1180+
} else
1181+
++i;
1182+
}
1183+
1184+
void test_identical_branches3(bool b) {
1185+
int i = 0;
1186+
if (b) { // no warning
1187+
++i;
1188+
} else {
1189+
i++;
1190+
}
1191+
}
1192+
1193+
void test_identical_branches4(bool b) {
1194+
int i = 0;
1195+
if (b) { // expected-warning {{true and false branches are identical}}
1196+
} else {
1197+
}
1198+
}
1199+
1200+
void test_identical_branches_break(bool b) {
1201+
while (true) {
1202+
if (b) // expected-warning {{true and false branches are identical}}
1203+
break;
1204+
else
1205+
break;
1206+
}
1207+
}
1208+
1209+
void test_identical_branches_continue(bool b) {
1210+
while (true) {
1211+
if (b) // expected-warning {{true and false branches are identical}}
1212+
continue;
1213+
else
1214+
continue;
1215+
}
1216+
}
1217+
1218+
void test_identical_branches_func(bool b) {
1219+
if (b) // expected-warning {{true and false branches are identical}}
1220+
func();
1221+
else
1222+
func();
1223+
}
1224+
1225+
void test_identical_branches_func_arguments(bool b) {
1226+
if (b) // no-warning
1227+
funcParam(1);
1228+
else
1229+
funcParam(2);
1230+
}
1231+
1232+
void test_identical_branches_cast1(bool b) {
1233+
long v = -7;
1234+
if (b) // no-warning
1235+
v = (signed int) v;
1236+
else
1237+
v = (unsigned int) v;
1238+
}
1239+
1240+
void test_identical_branches_cast2(bool b) {
1241+
long v = -7;
1242+
if (b) // expected-warning {{true and false branches are identical}}
1243+
v = (signed int) v;
1244+
else
1245+
v = (signed int) v;
1246+
}
1247+
1248+
int test_identical_branches_return_int(bool b) {
1249+
int i = 0;
1250+
if (b) { // expected-warning {{true and false branches are identical}}
1251+
i++;
1252+
return i;
1253+
} else {
1254+
i++;
1255+
return i;
1256+
}
1257+
}
1258+
1259+
int test_identical_branches_return_func(bool b) {
1260+
if (b) { // expected-warning {{true and false branches are identical}}
1261+
return func();
1262+
} else {
1263+
return func();
1264+
}
1265+
}
1266+
1267+
void test_identical_branches_for(bool b) {
1268+
int i;
1269+
int j;
1270+
if (b) { // expected-warning {{true and false branches are identical}}
1271+
for (i = 0, j = 0; i < 10; i++)
1272+
j += 4;
1273+
} else {
1274+
for (i = 0, j = 0; i < 10; i++)
1275+
j += 4;
1276+
}
1277+
}
1278+
1279+
void test_identical_branches_while(bool b) {
1280+
int i = 10;
1281+
if (b) { // expected-warning {{true and false branches are identical}}
1282+
while (func())
1283+
i--;
1284+
} else {
1285+
while (func())
1286+
i--;
1287+
}
1288+
}
1289+
1290+
void test_identical_branches_do_while(bool b) {
1291+
int i = 10;
1292+
if (b) { // expected-warning {{true and false branches are identical}}
1293+
do {
1294+
i--;
1295+
} while (func());
1296+
} else {
1297+
do {
1298+
i--;
1299+
} while (func());
1300+
}
1301+
}
1302+
1303+
void test_identical_branches_if(bool b, int i) {
1304+
if (b) { // expected-warning {{true and false branches are identical}}
1305+
if (i < 5)
1306+
i += 10;
1307+
} else {
1308+
if (i < 5)
1309+
i += 10;
1310+
}
1311+
}

‎clang/test/Analysis/misc-ps-region-store.cpp

+1
Original file line numberDiff line numberDiff line change
@@ -588,6 +588,7 @@ void rdar11401827() {
588588
int x = int();
589589
if (!x) {
590590
clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
591+
; // Suppress warning that both branches are identical
591592
}
592593
else {
593594
clang_analyzer_warnIfReached(); // no-warning

0 commit comments

Comments
 (0)
Please sign in to comment.