From: Jordan Rose Date: Tue, 10 Dec 2013 18:18:06 +0000 (+0000) Subject: [analyzer] Extend IdenticalExprChecker to check ternary operator results. X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=1dff6422f8e8cdc6268b444af797f16e0c45b2d9;p=clang [analyzer] Extend IdenticalExprChecker to check ternary operator results. Warn if both result expressions of a ternary operator (? :) are the same. Because only one of them will be executed, this warning will fire even if the expressions have side effects. Patch by Anders Rönnholm and Per Viberg! git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@196937 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h b/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h index b0670dad24..87e850f4b1 100644 --- a/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h +++ b/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h @@ -27,7 +27,7 @@ #include namespace clang { - +class ConditionalOperator; class AnalysisDeclContext; class BinaryOperator; class CompoundStmt; @@ -211,6 +211,9 @@ public: /// Assumes the statement has a valid location. static PathDiagnosticLocation createOperatorLoc(const BinaryOperator *BO, const SourceManager &SM); + static PathDiagnosticLocation createConditionalColonLoc( + const ConditionalOperator *CO, + const SourceManager &SM); /// For member expressions, return the location of the '.' or '->'. /// Assumes the statement has a valid location. diff --git a/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp b/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp index e696e38597..b17d799fc1 100644 --- a/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp @@ -11,7 +11,8 @@ /// \brief This defines IdenticalExprChecker, a check that warns about /// unintended use of identical expressions. /// -/// It checks for use of identical expressions with comparison operators. +/// It checks for use of identical expressions with comparison operators and +/// inside conditional expressions. /// //===----------------------------------------------------------------------===// @@ -26,7 +27,7 @@ using namespace clang; using namespace ento; static bool isIdenticalExpr(const ASTContext &Ctx, const Expr *Expr1, - const Expr *Expr2); + const Expr *Expr2, bool IgnoreSideEffects = false); //===----------------------------------------------------------------------===// // FindIdenticalExprVisitor - Identify nodes using identical expressions. //===----------------------------------------------------------------------===// @@ -38,8 +39,9 @@ public: explicit FindIdenticalExprVisitor(BugReporter &B, AnalysisDeclContext *A) : BR(B), AC(A) {} // FindIdenticalExprVisitor only visits nodes - // that are binary operators. + // that are binary operators or conditional operators. bool VisitBinaryOperator(const BinaryOperator *B); + bool VisitConditionalOperator(const ConditionalOperator *C); private: BugReporter &BR; @@ -118,6 +120,34 @@ bool FindIdenticalExprVisitor::VisitBinaryOperator(const BinaryOperator *B) { // True is always returned to traverse ALL nodes. return true; } + +bool FindIdenticalExprVisitor::VisitConditionalOperator( + const ConditionalOperator *C) { + + // Check if expressions in conditional expression are identical + // from a symbolic point of view. + + if (isIdenticalExpr(AC->getASTContext(), C->getTrueExpr(), + C->getFalseExpr(), true)) { + PathDiagnosticLocation ELoc = + PathDiagnosticLocation::createConditionalColonLoc( + C, BR.getSourceManager()); + + SourceRange Sr[2]; + Sr[0] = C->getTrueExpr()->getSourceRange(); + Sr[1] = C->getFalseExpr()->getSourceRange(); + BR.EmitBasicReport( + AC->getDecl(), "Identical expressions in conditional expression", + categories::LogicError, + "identical expressions on both sides of ':' in conditional expression", + ELoc, Sr); + } + // We want to visit ALL nodes (expressions in conditional + // expressions too) that contains conditional operators, + // thus always return true to traverse ALL nodes. + return true; +} + /// \brief Determines whether two expression trees are identical regarding /// operators and symbols. /// @@ -127,14 +157,14 @@ bool FindIdenticalExprVisitor::VisitBinaryOperator(const BinaryOperator *B) { /// t*(u + t) and t*u + t*t are not considered identical. /// static bool isIdenticalExpr(const ASTContext &Ctx, const Expr *Expr1, - const Expr *Expr2) { + const Expr *Expr2, bool IgnoreSideEffects) { // If Expr1 & Expr2 are of different class then they are not // identical expression. if (Expr1->getStmtClass() != Expr2->getStmtClass()) return false; // If Expr1 has side effects then don't warn even if expressions // are identical. - if (Expr1->HasSideEffects(Ctx)) + if (!IgnoreSideEffects && Expr1->HasSideEffects(Ctx)) return false; // Is expression is based on macro then don't warn even if // the expressions are identical. @@ -146,7 +176,8 @@ static bool isIdenticalExpr(const ASTContext &Ctx, const Expr *Expr1, while (I1 != Expr1->child_end() && I2 != Expr2->child_end()) { const Expr *Child1 = dyn_cast(*I1); const Expr *Child2 = dyn_cast(*I2); - if (!Child1 || !Child2 || !isIdenticalExpr(Ctx, Child1, Child2)) + if (!Child1 || !Child2 || !isIdenticalExpr(Ctx, Child1, Child2, + IgnoreSideEffects)) return false; ++I1; ++I2; @@ -161,6 +192,7 @@ static bool isIdenticalExpr(const ASTContext &Ctx, const Expr *Expr1, switch (Expr1->getStmtClass()) { default: return false; + case Stmt::CallExprClass: case Stmt::ArraySubscriptExprClass: case Stmt::CStyleCastExprClass: case Stmt::ImplicitCastExprClass: @@ -201,7 +233,7 @@ static bool isIdenticalExpr(const ASTContext &Ctx, const Expr *Expr1, const UnaryOperator *UnaryOp2 = dyn_cast(Expr2); if (UnaryOp1->getOpcode() != UnaryOp2->getOpcode()) return false; - return !UnaryOp1->isIncrementDecrementOp(); + return IgnoreSideEffects || !UnaryOp1->isIncrementDecrementOp(); } } } diff --git a/lib/StaticAnalyzer/Core/PathDiagnostic.cpp b/lib/StaticAnalyzer/Core/PathDiagnostic.cpp index b504db6349..054c93079d 100644 --- a/lib/StaticAnalyzer/Core/PathDiagnostic.cpp +++ b/lib/StaticAnalyzer/Core/PathDiagnostic.cpp @@ -609,6 +609,14 @@ PathDiagnosticLocation return PathDiagnosticLocation(BO->getOperatorLoc(), SM, SingleLocK); } +PathDiagnosticLocation + PathDiagnosticLocation::createConditionalColonLoc( + const ConditionalOperator *CO, + const SourceManager &SM) { + return PathDiagnosticLocation(CO->getColonLoc(), SM, SingleLocK); +} + + PathDiagnosticLocation PathDiagnosticLocation::createMemberLoc(const MemberExpr *ME, const SourceManager &SM) { diff --git a/test/Analysis/identical-expressions.cpp b/test/Analysis/identical-expressions.cpp index 50f341d393..3a331fb8f2 100644 --- a/test/Analysis/identical-expressions.cpp +++ b/test/Analysis/identical-expressions.cpp @@ -2,6 +2,21 @@ /* Only one expected warning per function allowed at the very end. */ +int func(void) +{ + return 0; +} + +int func2(void) +{ + return 0; +} + +int funcParam(int a) +{ + return 0; +} + /* '!=' operator*/ /* '!=' with float */ @@ -295,6 +310,38 @@ int checkNotEqualNestedBinaryOpIntPointerCompare2(void) { } /* end '!=' int* */ +/* '!=' with function*/ + +int checkNotEqualSameFunction() { + unsigned a = 0; + unsigned b = 1; + int res = (a+func() != a+func()); // no warning + return (0); +} + +int checkNotEqualDifferentFunction() { + unsigned a = 0; + unsigned b = 1; + int res = (a+func() != a+func2()); // no warning + return (0); +} + +int checkNotEqualSameFunctionSameParam() { + unsigned a = 0; + unsigned b = 1; + int res = (a+funcParam(a) != a+funcParam(a)); // no warning + return (0); +} + +int checkNotEqualSameFunctionDifferentParam() { + unsigned a = 0; + unsigned b = 1; + int res = (a+funcParam(a) != a+funcParam(b)); // no warning + return (0); +} + +/* end '!=' with function*/ + /* end '!=' */ @@ -526,6 +573,37 @@ int checkEqualNestedBinaryOpIntCompare3(void) { return (0); } +/* '==' with function*/ + +int checkEqualSameFunction() { + unsigned a = 0; + unsigned b = 1; + int res = (a+func() == a+func()); // no warning + return (0); +} + +int checkEqualDifferentFunction() { + unsigned a = 0; + unsigned b = 1; + int res = (a+func() == a+func2()); // no warning + return (0); +} + +int checkEqualSameFunctionSameParam() { + unsigned a = 0; + unsigned b = 1; + int res = (a+funcParam(a) == a+funcParam(a)); // no warning + return (0); +} + +int checkEqualSameFunctionDifferentParam() { + unsigned a = 0; + unsigned b = 1; + int res = (a+funcParam(a) == a+funcParam(b)); // no warning + return (0); +} + +/* end '==' with function*/ /* end EQ int */ @@ -940,3 +1018,143 @@ int checkGreaterThanNestedBinaryOpIntCompare3(void) { /* end GT with int */ /* end GT */ + + +/* Checking use of identical expressions in conditional operator*/ + +unsigned test_unsigned(unsigned a) { + unsigned b = 1; + a = a > 5 ? b : b; // expected-warning {{identical expressions on both sides of ':' in conditional expression}} + return a; +} + +void test_signed() { + int a = 0; + a = a > 5 ? a : a; // expected-warning {{identical expressions on both sides of ':' in conditional expression}} +} + +void test_bool(bool a) { + a = a > 0 ? a : a; // expected-warning {{identical expressions on both sides of ':' in conditional expression}} +} + +void test_float() { + float a = 0; + float b = 0; + a = a > 5 ? a : a; // expected-warning {{identical expressions on both sides of ':' in conditional expression}} +} + +void test_unsigned_expr() { + unsigned a = 0; + unsigned b = 0; + a = a > 5 ? a+b : a+b; // expected-warning {{identical expressions on both sides of ':' in conditional expression}} +} + +void test_signed_expr() { + int a = 0; + int b = 1; + a = a > 5 ? a+b : a+b; // expected-warning {{identical expressions on both sides of ':' in conditional expression}} +} + +void test_bool_expr(bool a) { + bool b = 0; + a = a > 0 ? a&&b : a&&b; // expected-warning {{identical expressions on both sides of ':' in conditional expression}} +} + +void test_unsigned_expr_negative() { + unsigned a = 0; + unsigned b = 0; + a = a > 5 ? a+b : b+a; // no warning +} + +void test_signed_expr_negative() { + int a = 0; + int b = 1; + a = a > 5 ? b+a : a+b; // no warning +} + +void test_bool_expr_negative(bool a) { + bool b = 0; + a = a > 0 ? a&&b : b&&a; // no warning +} + +void test_float_expr_positive() { + float a = 0; + float b = 0; + a = a > 5 ? a+b : a+b; // expected-warning {{identical expressions on both sides of ':' in conditional expression}} +} + +void test_expr_positive_func() { + unsigned a = 0; + unsigned b = 1; + a = a > 5 ? a+func() : a+func(); // expected-warning {{identical expressions on both sides of ':' in conditional expression}} +} + +void test_expr_negative_func() { + unsigned a = 0; + unsigned b = 1; + a = a > 5 ? a+func() : a+func2(); // no warning +} + +void test_expr_positive_funcParam() { + unsigned a = 0; + unsigned b = 1; + a = a > 5 ? a+funcParam(b) : a+funcParam(b); // expected-warning {{identical expressions on both sides of ':' in conditional expression}} +} + +void test_expr_negative_funcParam() { + unsigned a = 0; + unsigned b = 1; + a = a > 5 ? a+funcParam(a) : a+funcParam(b); // no warning +} + +void test_expr_positive_inc() { + unsigned a = 0; + unsigned b = 1; + a = a > 5 ? a++ : a++; // expected-warning {{identical expressions on both sides of ':' in conditional expression}} +} + +void test_expr_negative_inc() { + unsigned a = 0; + unsigned b = 1; + a = a > 5 ? a++ : b++; // no warning +} + +void test_expr_positive_assign() { + unsigned a = 0; + unsigned b = 1; + a = a > 5 ? a=1 : a=1; // expected-warning {{identical expressions on both sides of ':' in conditional expression}} +} + +void test_expr_negative_assign() { + unsigned a = 0; + unsigned b = 1; + a = a > 5 ? a=1 : a=2; // no warning +} + +void test_signed_nested_expr() { + int a = 0; + int b = 1; + int c = 3; + a = a > 5 ? a+b+(c+a)*(a + b*(c+a)) : a+b+(c+a)*(a + b*(c+a)); // expected-warning {{identical expressions on both sides of ':' in conditional expression}} +} + +void test_signed_nested_expr_negative() { + int a = 0; + int b = 1; + int c = 3; + a = a > 5 ? a+b+(c+a)*(a + b*(c+a)) : a+b+(c+a)*(a + b*(a+c)); // no warning +} + +void test_signed_nested_cond_expr_negative() { + int a = 0; + int b = 1; + int c = 3; + a = a > 5 ? (b > 5 ? 1 : 4) : (b > 5 ? 2 : 4); // no warning +} + +void test_signed_nested_cond_expr() { + int a = 0; + int b = 1; + int c = 3; + a = a > 5 ? (b > 5 ? 1 : 4) : (b > 5 ? 4 : 4); // expected-warning {{identical expressions on both sides of ':' in conditional expression}} +}