From: Tom Care Date: Tue, 27 Jul 2010 23:30:21 +0000 (+0000) Subject: Added some false positive checking to UnreachableCodeChecker X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=7bce3a122296eba0e74f401c188e55c71935132f;p=clang Added some false positive checking to UnreachableCodeChecker - Allowed reporting of dead macros - Added path walking function to search for false positives in conditional statements - Updated some affected tests - Added some false positive test cases git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@109561 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/Checker/UnreachableCodeChecker.cpp b/lib/Checker/UnreachableCodeChecker.cpp index 9ee3d02d26..e95e6a832d 100644 --- a/lib/Checker/UnreachableCodeChecker.cpp +++ b/lib/Checker/UnreachableCodeChecker.cpp @@ -13,13 +13,14 @@ // A similar flow-sensitive only check exists in Analysis/ReachableCode.cpp //===----------------------------------------------------------------------===// +#include "clang/AST/ParentMap.h" +#include "clang/Basic/Builtins.h" #include "clang/Checker/PathSensitive/CheckerVisitor.h" #include "clang/Checker/PathSensitive/ExplodedGraph.h" #include "clang/Checker/PathSensitive/SVals.h" +#include "clang/Checker/PathSensitive/CheckerHelpers.h" #include "clang/Checker/BugReporter/BugReporter.h" #include "GRExprEngineExperimentalChecks.h" -#include "clang/AST/StmtCXX.h" -#include "clang/Basic/Builtins.h" #include "llvm/ADT/SmallPtrSet.h" // The number of CFGBlock pointers we want to reserve memory for. This is used @@ -35,9 +36,13 @@ public: void VisitEndAnalysis(ExplodedGraph &G, BugReporter &B, bool hasWorkRemaining); private: + typedef bool (*ExplodedNodeHeuristic)(const ExplodedNode &EN); + static SourceLocation GetUnreachableLoc(const CFGBlock &b, SourceRange &R); void FindUnreachableEntryPoints(const CFGBlock *CB); void MarkSuccessorsReachable(const CFGBlock *CB); + static const Expr *getConditon(const Stmt *S); + static bool isInvalidPath(const CFGBlock *CB, const ParentMap &PM); llvm::SmallPtrSet reachable; llvm::SmallPtrSet visited; @@ -61,14 +66,18 @@ void UnreachableCodeChecker::VisitEndAnalysis(ExplodedGraph &G, return; CFG *C = 0; + ParentMap *PM = 0; // Iterate over ExplodedGraph for (ExplodedGraph::node_iterator I = G.nodes_begin(); I != G.nodes_end(); ++I) { const ProgramPoint &P = I->getLocation(); + const LocationContext *LC = P.getLocationContext(); // Save the CFG if we don't have it already if (!C) - C = P.getLocationContext()->getCFG(); + C = LC->getCFG(); + if (!PM) + PM = &LC->getParentMap(); if (const BlockEntrance *BE = dyn_cast(&P)) { const CFGBlock *CB = BE->getBlock(); @@ -76,8 +85,8 @@ void UnreachableCodeChecker::VisitEndAnalysis(ExplodedGraph &G, } } - // Bail out if we didn't get the CFG - if (!C) + // Bail out if we didn't get the CFG or the ParentMap. + if (!C || !PM) return; ASTContext &Ctx = B.getContext(); @@ -86,34 +95,39 @@ void UnreachableCodeChecker::VisitEndAnalysis(ExplodedGraph &G, for (CFG::const_iterator I = C->begin(); I != C->end(); ++I) { const CFGBlock *CB = *I; // Check if the block is unreachable - if (!reachable.count(CB)) { - // Find the entry points for this block - FindUnreachableEntryPoints(CB); - // This block may have been pruned; check if we still want to report it - if (reachable.count(CB)) - continue; - - // We found a statement that wasn't covered - SourceRange S; - SourceLocation SL = GetUnreachableLoc(*CB, S); - if (S.getBegin().isMacroID() || S.getEnd().isMacroID() || S.isInvalid() - || SL.isInvalid()) - continue; - - // Special case for __builtin_unreachable. - // FIXME: This should be extended to include other unreachable markers, - // such as llvm_unreachable. - if (!CB->empty()) { - const Stmt *First = CB->front(); - if (const CallExpr *CE = dyn_cast(First)) { - if (CE->isBuiltinCall(Ctx) == Builtin::BI__builtin_unreachable) - continue; - } + if (reachable.count(CB)) + continue; + + // Find the entry points for this block + FindUnreachableEntryPoints(CB); + + // This block may have been pruned; check if we still want to report it + if (reachable.count(CB)) + continue; + + // Check for false positives + if (CB->size() > 0 && isInvalidPath(CB, *PM)) + continue; + + // We found a statement that wasn't covered + SourceRange S; + SourceLocation SL = GetUnreachableLoc(*CB, S); + if (S.isInvalid() || SL.isInvalid()) + continue; + + // Special case for __builtin_unreachable. + // FIXME: This should be extended to include other unreachable markers, + // such as llvm_unreachable. + if (!CB->empty()) { + const Stmt *First = CB->front(); + if (const CallExpr *CE = dyn_cast(First)) { + if (CE->isBuiltinCall(Ctx) == Builtin::BI__builtin_unreachable) + continue; } - - B.EmitBasicReport("Unreachable code", "This statement is never executed", - SL, S); } + + B.EmitBasicReport("Unreachable code", "This statement is never executed", + SL, S); } } @@ -156,3 +170,50 @@ SourceLocation UnreachableCodeChecker::GetUnreachableLoc(const CFGBlock &b, R = S->getSourceRange(); return S->getLocStart(); } + +// Returns the Expr* condition if this is a conditional statement, or 0 +// otherwise +const Expr *UnreachableCodeChecker::getConditon(const Stmt *S) { + if (const IfStmt *IS = dyn_cast(S)) { + return IS->getCond(); + } + else if (const SwitchStmt *SS = dyn_cast(S)) { + return SS->getCond(); + } + else if (const WhileStmt *WS = dyn_cast(S)) { + return WS->getCond(); + } + else if (const DoStmt *DS = dyn_cast(S)) { + return DS->getCond(); + } + else if (const ForStmt *FS = dyn_cast(S)) { + return FS->getCond(); + } + else if (const ConditionalOperator *CO = dyn_cast(S)) { + return CO->getCond(); + } + + return 0; +} + +// Traverse the predecessor Stmt*s from this CFGBlock to find any signs of a +// path that is a false positive. +bool UnreachableCodeChecker::isInvalidPath(const CFGBlock *CB, + const ParentMap &PM) { + + // Start at the end of the block and work up the path. + const Stmt *S = CB->back().getStmt(); + while (S) { + // Find any false positives in the conditions on this path. + if (const Expr *cond = getConditon(S)) { + if (containsMacro(cond) || containsEnum(cond) + || containsStaticLocal(cond) || containsBuiltinOffsetOf(cond) + || containsStmt(cond)) + return true; + } + // Get the previous statement. + S = PM.getParent(S); + } + + return false; +} diff --git a/test/Analysis/additive-folding.c b/test/Analysis/additive-folding.c index 6e81fcf8c2..e4a5651339 100644 --- a/test/Analysis/additive-folding.c +++ b/test/Analysis/additive-folding.c @@ -61,7 +61,7 @@ void eq_ne (unsigned a) { if (a+1 != 0) return; // no-warning if (a-1 != UINT_MAX-1) - return; // expected-warning{{never executed}} + return; // no-warning free(b); } @@ -72,7 +72,7 @@ void ne_eq (unsigned a) { if (a+1 == 0) return; // no-warning if (a-1 == UINT_MAX-1) - return; // expected-warning{{never executed}} + return; // no-warning free(b); } @@ -177,7 +177,7 @@ void adjustedLE (unsigned a) { void tautologyGT (unsigned a) { char* b = malloc(1); if (a > UINT_MAX) - return; // expected-warning{{never executed}} + return; // no-warning free(b); } diff --git a/test/Analysis/bstring.c b/test/Analysis/bstring.c index 3a5dfab33d..418b323300 100644 --- a/test/Analysis/bstring.c +++ b/test/Analysis/bstring.c @@ -53,7 +53,7 @@ void memcpy0 () { memcpy(dst, src, 4); // no-warning if (memcpy(dst, src, 4) != dst) { - (void)*(char*)0; // expected-warning{{never executed}} + (void)*(char*)0; // no-warning } } @@ -155,7 +155,7 @@ void memmove0 () { memmove(dst, src, 4); // no-warning if (memmove(dst, src, 4) != dst) { - (void)*(char*)0; // expected-warning{{never executed}} + (void)*(char*)0; // no-warning } } @@ -217,7 +217,7 @@ void memcmp3 () { char a[] = {1, 2, 3, 4}; if (memcmp(a, a, 4)) - (void)*(char*)0; // expected-warning{{never executed}} + (void)*(char*)0; // no-warning } void memcmp4 (char *input) { @@ -231,11 +231,11 @@ void memcmp5 (char *input) { char a[] = {1, 2, 3, 4}; if (memcmp(a, 0, 0)) // no-warning - (void)*(char*)0; // expected-warning{{never executed}} + (void)*(char*)0; // no-warning if (memcmp(0, a, 0)) // no-warning - (void)*(char*)0; // expected-warning{{never executed}} + (void)*(char*)0; // no-warning if (memcmp(a, input, 0)) // no-warning - (void)*(char*)0; // expected-warning{{never executed}} + (void)*(char*)0; // no-warning } void memcmp6 (char *a, char *b, size_t n) { diff --git a/test/Analysis/constant-folding.c b/test/Analysis/constant-folding.c index b1ca3eeec9..85e47c8dea 100644 --- a/test/Analysis/constant-folding.c +++ b/test/Analysis/constant-folding.c @@ -9,51 +9,51 @@ void testComparisons (int a) { // Sema can already catch the simple comparison a==a, // since that's usually a logic error (and not path-dependent). int b = a; - if (!(b==a)) WARN; - if (!(b>=a)) WARN; - if (!(b<=a)) WARN; - if (b!=a) WARN; - if (b>a) WARN; - if (b=a)) WARN; // expected-warning{{never executed}} + if (!(b<=a)) WARN; // expected-warning{{never executed}} + if (b!=a) WARN; // expected-warning{{never executed}} + if (b>a) WARN; // expected-warning{{never executed}} + if (b>0) != a) WARN; - if ((a^0) != a) WARN; - if ((a&(~0)) != a) WARN; - if ((a|0) != a) WARN; + if ((a*1) != a) WARN; // expected-warning{{never executed}} + if ((a/1) != a) WARN; // expected-warning{{never executed}} + if ((a+0) != a) WARN; // expected-warning{{never executed}} + if ((a-0) != a) WARN; // expected-warning{{never executed}} + if ((a<<0) != a) WARN; // expected-warning{{never executed}} + if ((a>>0) != a) WARN; // expected-warning{{never executed}} + if ((a^0) != a) WARN; // expected-warning{{never executed}} + if ((a&(~0)) != a) WARN; // expected-warning{{never executed}} + if ((a|0) != a) WARN; // expected-warning{{never executed}} } void testReductionToConstant (int a) { - if ((a*0) != 0) WARN; - if ((a&0) != 0) WARN; - if ((a|(~0)) != (~0)) WARN; + if ((a*0) != 0) WARN; // expected-warning{{never executed}} + if ((a&0) != 0) WARN; // expected-warning{{never executed}} + if ((a|(~0)) != (~0)) WARN; // expected-warning{{never executed}} } void testSymmetricIntSymOperations (int a) { - if ((2+a) != (a+2)) WARN; - if ((2*a) != (a*2)) WARN; - if ((2&a) != (a&2)) WARN; - if ((2^a) != (a^2)) WARN; - if ((2|a) != (a|2)) WARN; + if ((2+a) != (a+2)) WARN; // expected-warning{{never executed}} + if ((2*a) != (a*2)) WARN; // expected-warning{{never executed}} + if ((2&a) != (a&2)) WARN; // expected-warning{{never executed}} + if ((2^a) != (a^2)) WARN; // expected-warning{{never executed}} + if ((2|a) != (a|2)) WARN; // expected-warning{{never executed}} } void testAsymmetricIntSymOperations (int a) { - if (((~0) >> a) != (~0)) WARN; - if ((0 >> a) != 0) WARN; - if ((0 << a) != 0) WARN; + if (((~0) >> a) != (~0)) WARN; // expected-warning{{never executed}} + if ((0 >> a) != 0) WARN; // expected-warning{{never executed}} + if ((0 << a) != 0) WARN; // expected-warning{{never executed}} // Unsigned right shift shifts in zeroes. if ((((unsigned)(~0)) >> ((unsigned) a)) != ((unsigned)(~0))) @@ -62,11 +62,11 @@ void testAsymmetricIntSymOperations (int a) { void testLocations (char *a) { char *b = a; - if (!(b==a)) WARN; - if (!(b>=a)) WARN; - if (!(b<=a)) WARN; - if (b!=a) WARN; - if (b>a) WARN; - if (b=a)) WARN; // expected-warning{{never executed}} + if (!(b<=a)) WARN; // expected-warning{{never executed}} + if (b!=a) WARN; // expected-warning{{never executed}} + if (b>a) WARN; // expected-warning{{never executed}} + if (b 5) { // expected-warning{{never executed}} + return; + } +} + +// These next tests are path-sensitive + +void test4() { int a = 5; while (a > 1) @@ -36,13 +47,6 @@ void test3() { foo(a); } -void test4(unsigned a) { - while(1); - if (a > 5) { // expected-warning{{never executed}} - return; - } -} - extern void bar(char c); void test5(const char *c) { @@ -53,9 +57,34 @@ void test5(const char *c) { } } +// These next tests are false positives and should not generate warnings + void test6(const char *c) { if (c) return; if (!c) return; __builtin_unreachable(); // no-warning } +// Compile-time constant false positives +#define CONSTANT 0 +enum test_enum { Off, On }; +void test7() { + if (CONSTANT) + return; // no-warning + + if (sizeof(int)) + return; // no-warning + + if (Off) + return; // no-warning +} + +void test8() { + static unsigned a = 0; + + if (a) + a = 123; // no-warning + + a = 5; +} +