From 2eae2903cfa0c5bf185b0ff8b90b5c95a0bc2f9e Mon Sep 17 00:00:00 2001 From: Jordan Rose Date: Wed, 19 Feb 2014 17:44:16 +0000 Subject: [PATCH] [analyzer] Extend IdenticalExprChecker to check logical and bitwise expressions. IdenticalExprChecker now warns if any expressions in a logical or bitwise chain (&&, ||, &, |, or ^) are the same. Unlike the previous patch, this actually checks all subexpressions against each other (an O(N^2) operation, but N is likely to be small). Patch by Daniel Fahlgren! git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@201702 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../Checkers/IdenticalExprChecker.cpp | 87 ++++++++++++++++--- test/Analysis/identical-expressions.cpp | 86 ++++++++++++++++++ 2 files changed, 161 insertions(+), 12 deletions(-) diff --git a/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp b/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp index 6c6f472f3f..a76dff120b 100644 --- a/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp @@ -35,6 +35,9 @@ static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1, namespace { class FindIdenticalExprVisitor : public RecursiveASTVisitor { + BugReporter &BR; + const CheckerBase *Checker; + AnalysisDeclContext *AC; public: explicit FindIdenticalExprVisitor(BugReporter &B, const CheckerBase *Checker, @@ -48,12 +51,59 @@ public: bool VisitConditionalOperator(const ConditionalOperator *C); private: - BugReporter &BR; - const CheckerBase *Checker; - AnalysisDeclContext *AC; + void reportIdenticalExpr(const BinaryOperator *B, bool CheckBitwise, + ArrayRef Sr); + void checkBitwiseOrLogicalOp(const BinaryOperator *B, bool CheckBitwise); + void checkComparisonOp(const BinaryOperator *B); }; } // end anonymous namespace +void FindIdenticalExprVisitor::reportIdenticalExpr(const BinaryOperator *B, + bool CheckBitwise, + ArrayRef Sr) { + StringRef Message; + if (CheckBitwise) + Message = "identical expressions on both sides of bitwise operator"; + else + Message = "identical expressions on both sides of logical operator"; + + PathDiagnosticLocation ELoc = + PathDiagnosticLocation::createOperatorLoc(B, BR.getSourceManager()); + BR.EmitBasicReport(AC->getDecl(), Checker, + "Use of identical expressions", + categories::LogicError, + Message, ELoc, Sr); +} + +void FindIdenticalExprVisitor::checkBitwiseOrLogicalOp(const BinaryOperator *B, + bool CheckBitwise) { + SourceRange Sr[2]; + + const Expr *LHS = B->getLHS(); + const Expr *RHS = B->getRHS(); + + // Split operators as long as we still have operators to split on. We will + // get called for every binary operator in an expression so there is no need + // to check every one against each other here, just the right most one with + // the others. + while (const BinaryOperator *B2 = dyn_cast(LHS)) { + if (B->getOpcode() != B2->getOpcode()) + break; + if (isIdenticalStmt(AC->getASTContext(), RHS, B2->getRHS())) { + Sr[0] = RHS->getSourceRange(); + Sr[1] = B2->getRHS()->getSourceRange(); + reportIdenticalExpr(B, CheckBitwise, Sr); + } + LHS = B2->getLHS(); + } + + if (isIdenticalStmt(AC->getASTContext(), RHS, LHS)) { + Sr[0] = RHS->getSourceRange(); + Sr[1] = LHS->getSourceRange(); + reportIdenticalExpr(B, CheckBitwise, Sr); + } +} + bool FindIdenticalExprVisitor::VisitIfStmt(const IfStmt *I) { const Stmt *Stmt1 = I->getThen(); const Stmt *Stmt2 = I->getElse(); @@ -89,8 +139,25 @@ bool FindIdenticalExprVisitor::VisitIfStmt(const IfStmt *I) { bool FindIdenticalExprVisitor::VisitBinaryOperator(const BinaryOperator *B) { BinaryOperator::Opcode Op = B->getOpcode(); - if (!BinaryOperator::isComparisonOp(Op)) - return true; + + if (BinaryOperator::isBitwiseOp(Op)) + checkBitwiseOrLogicalOp(B, true); + + if (BinaryOperator::isLogicalOp(Op)) + checkBitwiseOrLogicalOp(B, false); + + if (BinaryOperator::isComparisonOp(Op)) + checkComparisonOp(B); + + // We want to visit ALL nodes (subexpressions of binary comparison + // expressions too) that contains comparison operators. + // True is always returned to traverse ALL nodes. + return true; +} + +void FindIdenticalExprVisitor::checkComparisonOp(const BinaryOperator *B) { + BinaryOperator::Opcode Op = B->getOpcode(); + // // Special case for floating-point representation. // @@ -123,21 +190,21 @@ bool FindIdenticalExprVisitor::VisitBinaryOperator(const BinaryOperator *B) { (DeclRef2->getType()->hasFloatingRepresentation())) { if (DeclRef1->getDecl() == DeclRef2->getDecl()) { if ((Op == BO_EQ) || (Op == BO_NE)) { - return true; + return; } } } } else if ((FloatLit1) && (FloatLit2)) { if (FloatLit1->getValue().bitwiseIsEqual(FloatLit2->getValue())) { if ((Op == BO_EQ) || (Op == BO_NE)) { - return true; + return; } } } else if (LHS->getType()->hasFloatingRepresentation()) { // If any side of comparison operator still has floating-point // representation, then it's an expression. Don't warn. // Here only LHS is checked since RHS will be implicit casted to float. - return true; + return; } else { // No special case with floating-point representation, report as usual. } @@ -154,10 +221,6 @@ bool FindIdenticalExprVisitor::VisitBinaryOperator(const BinaryOperator *B) { "Compare of identical expressions", categories::LogicError, Message, ELoc); } - // We want to visit ALL nodes (subexpressions of binary comparison - // expressions too) that contains comparison operators. - // True is always returned to traverse ALL nodes. - return true; } bool FindIdenticalExprVisitor::VisitConditionalOperator( diff --git a/test/Analysis/identical-expressions.cpp b/test/Analysis/identical-expressions.cpp index acb7c8c57b..950cdd93ff 100644 --- a/test/Analysis/identical-expressions.cpp +++ b/test/Analysis/identical-expressions.cpp @@ -1309,3 +1309,89 @@ void test_identical_branches_if(bool b, int i) { i += 10; } } + +void test_identical_bitwise1() { + int a = 5 | 5; // expected-warning {{identical expressions on both sides of bitwise operator}} +} + +void test_identical_bitwise2() { + int a = 5; + int b = a | a; // expected-warning {{identical expressions on both sides of bitwise operator}} +} + +void test_identical_bitwise3() { + int a = 5; + int b = (a | a); // expected-warning {{identical expressions on both sides of bitwise operator}} +} + +void test_identical_bitwise4() { + int a = 4; + int b = a | 4; // no-warning +} + +void test_identical_bitwise5() { + int a = 4; + int b = 4; + int c = a | b; // no-warning +} + +void test_identical_bitwise6() { + int a = 5; + int b = a | 4 | a; // expected-warning {{identical expressions on both sides of bitwise operator}} +} + +void test_identical_bitwise7() { + int a = 5; + int b = func() | func(); // no-warning +} + +void test_identical_logical1(int a) { + if (a == 4 && a == 4) // expected-warning {{identical expressions on both sides of logical operator}} + ; +} + +void test_identical_logical2(int a) { + if (a == 4 || a == 5 || a == 4) // expected-warning {{identical expressions on both sides of logical operator}} + ; +} + +void test_identical_logical3(int a) { + if (a == 4 || a == 5 || a == 6) // no-warning + ; +} + +void test_identical_logical4(int a) { + if (a == func() || a == func()) // no-warning + ; +} + +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wlogical-op-parentheses" +void test_identical_logical5(int x, int y) { + if (x == 4 && y == 5 || x == 4 && y == 6) // no-warning + ; +} + +void test_identical_logical6(int x, int y) { + if (x == 4 && y == 5 || x == 4 && y == 5) // expected-warning {{identical expressions on both sides of logical operator}} + ; +} + +void test_identical_logical7(int x, int y) { + // FIXME: We should warn here + if (x == 4 && y == 5 || x == 4) + ; +} + +void test_identical_logical8(int x, int y) { + // FIXME: We should warn here + if (x == 4 || y == 5 && x == 4) + ; +} + +void test_identical_logical9(int x, int y) { + // FIXME: We should warn here + if (x == 4 || x == 4 && y == 5) + ; +} +#pragma clang diagnostic pop -- 2.49.0