diff options
-rw-r--r-- | include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h | 19 | ||||
-rw-r--r-- | lib/StaticAnalyzer/Core/BugReporter.cpp | 73 | ||||
-rw-r--r-- | lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp | 5 | ||||
-rw-r--r-- | lib/StaticAnalyzer/Core/PathDiagnostic.cpp | 82 | ||||
-rw-r--r-- | test/Analysis/dtor.cpp | 69 |
5 files changed, 184 insertions, 64 deletions
diff --git a/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h b/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h index 730a558b74..73958a6ef4 100644 --- a/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h +++ b/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h @@ -148,6 +148,15 @@ public: assert(Range.isValid()); } + /// Create a location at an explicit offset in the source. + /// + /// This should only be used if there are no more appropriate constructors. + PathDiagnosticLocation(SourceLocation loc, const SourceManager &sm) + : K(SingleLocK), S(0), D(0), SM(&sm), Loc(loc, sm), Range(genRange()) { + assert(Loc.isValid()); + assert(Range.isValid()); + } + /// Create a location corresponding to the given declaration. static PathDiagnosticLocation create(const Decl *D, const SourceManager &SM) { @@ -163,6 +172,14 @@ public: const SourceManager &SM, const LocationOrAnalysisDeclContext LAC); + /// Create a location for the end of the statement. + /// + /// If the statement is a CompoundStatement, the location will point to the + /// closing brace instead of following it. + static PathDiagnosticLocation createEnd(const Stmt *S, + const SourceManager &SM, + const LocationOrAnalysisDeclContext LAC); + /// Create the location for the operator of the binary expression. /// Assumes the statement has a valid location. static PathDiagnosticLocation createOperatorLoc(const BinaryOperator *BO, @@ -637,6 +654,8 @@ public: void pushActivePath(PathPieces *p) { pathStack.push_back(p); } void popActivePath() { if (!pathStack.empty()) pathStack.pop_back(); } + + bool isWithinCall() const { return !pathStack.empty(); } // PathDiagnostic(); PathDiagnostic(const Decl *DeclWithIssue, diff --git a/lib/StaticAnalyzer/Core/BugReporter.cpp b/lib/StaticAnalyzer/Core/BugReporter.cpp index 08588f60ab..12479ca53a 100644 --- a/lib/StaticAnalyzer/Core/BugReporter.cpp +++ b/lib/StaticAnalyzer/Core/BugReporter.cpp @@ -441,21 +441,23 @@ static void GenerateMinimalPathDiagnostic(PathDiagnostic& PD, } if (const CallEnter *CE = dyn_cast<CallEnter>(&P)) { + // Flush all locations, and pop the active path. + bool VisitedEntireCall = PD.isWithinCall(); PD.popActivePath(); - // The current active path should never be empty. Either we - // just added a bunch of stuff to the top-level path, or - // we have a previous CallExitEnd. If the front of the active - // path is not a PathDiagnosticCallPiece, it means that the + + // Either we just added a bunch of stuff to the top-level path, or + // we have a previous CallExitEnd. If the former, it means that the // path terminated within a function call. We must then take the // current contents of the active path and place it within // a new PathDiagnosticCallPiece. - assert(!PD.getActivePath().empty()); - PathDiagnosticCallPiece *C = - dyn_cast<PathDiagnosticCallPiece>(PD.getActivePath().front()); - if (!C) { + PathDiagnosticCallPiece *C; + if (VisitedEntireCall) { + C = cast<PathDiagnosticCallPiece>(PD.getActivePath().front()); + } else { const Decl *Caller = CE->getLocationContext()->getDecl(); C = PathDiagnosticCallPiece::construct(PD.getActivePath(), Caller); } + C->setCallee(*CE, SMgr); if (!CallStack.empty()) { assert(CallStack.back().first == C); @@ -868,6 +870,7 @@ public: void rawAddEdge(PathDiagnosticLocation NewLoc); void addContext(const Stmt *S); + void addContext(const PathDiagnosticLocation &L); void addExtendedContext(const Stmt *S); }; } // end anonymous namespace @@ -1035,7 +1038,10 @@ void EdgeBuilder::addContext(const Stmt *S) { return; PathDiagnosticLocation L(S, PDB.getSourceManager(), PDB.LC); + addContext(L); +} +void EdgeBuilder::addContext(const PathDiagnosticLocation &L) { while (!CLocs.empty()) { const PathDiagnosticLocation &TopContextLoc = CLocs.back(); @@ -1151,25 +1157,22 @@ static void GenerateExtensivePathDiagnostic(PathDiagnostic& PD, } if (const CallExitEnd *CE = dyn_cast<CallExitEnd>(&P)) { - const StackFrameContext *LCtx = - CE->getLocationContext()->getCurrentStackFrame(); - // FIXME: This needs to handle implicit calls. - if (const Stmt *S = CE->getCalleeContext()->getCallSite()) { - if (const Expr *Ex = dyn_cast<Expr>(S)) + const Stmt *S = CE->getCalleeContext()->getCallSite(); + if (const Expr *Ex = dyn_cast_or_null<Expr>(S)) { reversePropagateIntererstingSymbols(*PDB.getBugReport(), IE, N->getState().getPtr(), Ex, N->getLocationContext()); - PathDiagnosticLocation Loc(S, - PDB.getSourceManager(), - LCtx); - EB.addEdge(Loc, true); - EB.flushLocations(); - PathDiagnosticCallPiece *C = - PathDiagnosticCallPiece::construct(N, *CE, SM); - PD.getActivePath().push_front(C); - PD.pushActivePath(&C->path); - CallStack.push_back(StackDiagPair(C, N)); } + + PathDiagnosticCallPiece *C = + PathDiagnosticCallPiece::construct(N, *CE, SM); + + EB.addEdge(C->callReturn, true); + EB.flushLocations(); + + PD.getActivePath().push_front(C); + PD.pushActivePath(&C->path); + CallStack.push_back(StackDiagPair(C, N)); break; } @@ -1183,30 +1186,26 @@ static void GenerateExtensivePathDiagnostic(PathDiagnostic& PD, EB.addEdge(pos); // Flush all locations, and pop the active path. + bool VisitedEntireCall = PD.isWithinCall(); EB.flushLocations(); PD.popActivePath(); - assert(!PD.getActivePath().empty()); PDB.LC = N->getLocationContext(); - // The current active path should never be empty. Either we - // just added a bunch of stuff to the top-level path, or - // we have a previous CallExitEnd. If the front of the active - // path is not a PathDiagnosticCallPiece, it means that the + // Either we just added a bunch of stuff to the top-level path, or + // we have a previous CallExitEnd. If the former, it means that the // path terminated within a function call. We must then take the // current contents of the active path and place it within // a new PathDiagnosticCallPiece. - PathDiagnosticCallPiece *C = - dyn_cast<PathDiagnosticCallPiece>(PD.getActivePath().front()); - if (!C) { - const Decl * Caller = CE->getLocationContext()->getDecl(); + PathDiagnosticCallPiece *C; + if (VisitedEntireCall) { + C = cast<PathDiagnosticCallPiece>(PD.getActivePath().front()); + } else { + const Decl *Caller = CE->getLocationContext()->getDecl(); C = PathDiagnosticCallPiece::construct(PD.getActivePath(), Caller); } - // FIXME: We still need a location for implicit calls. - if (CE->getCallExpr()) { - C->setCallee(*CE, SM); - EB.addContext(CE->getCallExpr()); - } + C->setCallee(*CE, SM); + EB.addContext(C->getLocation()); if (!CallStack.empty()) { assert(CallStack.back().first == C); diff --git a/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp b/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp index 4431c03d49..89d1e27c3d 100644 --- a/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp +++ b/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp @@ -293,6 +293,11 @@ bool ExprEngine::inlineCall(const CallEvent &Call, if (!ADC->getCFGBuildOptions().AddImplicitDtors || !ADC->getCFGBuildOptions().AddInitializers) return false; + // FIXME: This is a hack. We only process VarDecl destructors right now, + // so we should only inline VarDecl constructors. + if (const CXXConstructorCall *Ctor = dyn_cast<CXXConstructorCall>(&Call)) + if (!isa<VarRegion>(Ctor->getCXXThisVal().getAsRegion())) + return false; break; } case CE_CXXAllocator: diff --git a/lib/StaticAnalyzer/Core/PathDiagnostic.cpp b/lib/StaticAnalyzer/Core/PathDiagnostic.cpp index bfb8bf6432..5496df0f8f 100644 --- a/lib/StaticAnalyzer/Core/PathDiagnostic.cpp +++ b/lib/StaticAnalyzer/Core/PathDiagnostic.cpp @@ -16,6 +16,7 @@ #include "clang/Basic/SourceManager.h" #include "clang/AST/Expr.h" #include "clang/AST/Decl.h" +#include "clang/AST/DeclCXX.h" #include "clang/AST/DeclObjC.h" #include "clang/AST/ParentMap.h" #include "clang/AST/StmtCXX.h" @@ -242,13 +243,14 @@ PathDiagnosticConsumer::FlushDiagnostics(SmallVectorImpl<std::string> *Files) { //===----------------------------------------------------------------------===// static SourceLocation getValidSourceLocation(const Stmt* S, - LocationOrAnalysisDeclContext LAC) { - SourceLocation L = S->getLocStart(); + LocationOrAnalysisDeclContext LAC, + bool UseEnd = false) { + SourceLocation L = UseEnd ? S->getLocEnd() : S->getLocStart(); assert(!LAC.isNull() && "A valid LocationContext or AnalysisDeclContext should " "be passed to PathDiagnosticLocation upon creation."); // S might be a temporary statement that does not have a location in the - // source code, so find an enclosing statement and use it's location. + // source code, so find an enclosing statement and use its location. if (!L.isValid()) { ParentMap *PM = 0; @@ -259,7 +261,7 @@ static SourceLocation getValidSourceLocation(const Stmt* S, while (!L.isValid()) { S = PM->getParent(S); - L = S->getLocStart(); + L = UseEnd ? S->getLocEnd() : S->getLocStart(); } } @@ -280,6 +282,17 @@ PathDiagnosticLocation SM, SingleLocK); } + +PathDiagnosticLocation +PathDiagnosticLocation::createEnd(const Stmt *S, + const SourceManager &SM, + LocationOrAnalysisDeclContext LAC) { + if (const CompoundStmt *CS = dyn_cast<CompoundStmt>(S)) + return createEndBrace(CS, SM); + return PathDiagnosticLocation(getValidSourceLocation(S, LAC, /*End=*/true), + SM, SingleLocK); +} + PathDiagnosticLocation PathDiagnosticLocation::createOperatorLoc(const BinaryOperator *BO, const SourceManager &SM) { @@ -339,9 +352,6 @@ PathDiagnosticLocation else if (const PostStmt *PS = dyn_cast<PostStmt>(&P)) { S = PS->getStmt(); } - else if (const CallExitEnd *CEE = dyn_cast<CallExitEnd>(&P)) { - S = CEE->getCalleeContext()->getCallSite(); - } return PathDiagnosticLocation(S, SMng, P.getLocationContext()); } @@ -498,21 +508,36 @@ PathDiagnosticLocation PathDiagnostic::getLocation() const { // Manipulation of PathDiagnosticCallPieces. //===----------------------------------------------------------------------===// -static PathDiagnosticLocation getLastStmtLoc(const ExplodedNode *N, - const SourceManager &SM) { - while (N) { - ProgramPoint PP = N->getLocation(); - if (const StmtPoint *SP = dyn_cast<StmtPoint>(&PP)) - return PathDiagnosticLocation(SP->getStmt(), SM, PP.getLocationContext()); - // FIXME: Handle implicit calls. - if (const CallExitEnd *CEE = dyn_cast<CallExitEnd>(&PP)) - if (const Stmt *CallSite = CEE->getCalleeContext()->getCallSite()) - return PathDiagnosticLocation(CallSite, SM, PP.getLocationContext()); - if (N->pred_empty()) - break; - N = *N->pred_begin(); +static PathDiagnosticLocation +getLocationForCaller(const StackFrameContext *SFC, + const LocationContext *CallerCtx, + const SourceManager &SM) { + const CFGBlock &Block = *SFC->getCallSiteBlock(); + CFGElement Source = Block[SFC->getIndex()]; + + switch (Source.getKind()) { + case CFGElement::Invalid: + llvm_unreachable("Invalid CFGElement"); + case CFGElement::Statement: + return PathDiagnosticLocation(cast<CFGStmt>(Source).getStmt(), + SM, CallerCtx); + case CFGElement::Initializer: { + const CFGInitializer &Init = cast<CFGInitializer>(Source); + return PathDiagnosticLocation(Init.getInitializer()->getInit(), + SM, CallerCtx); } - return PathDiagnosticLocation(); + case CFGElement::AutomaticObjectDtor: { + const CFGAutomaticObjDtor &Dtor = cast<CFGAutomaticObjDtor>(Source); + return PathDiagnosticLocation::createEnd(Dtor.getTriggerStmt(), + SM, CallerCtx); + } + case CFGElement::BaseDtor: + case CFGElement::MemberDtor: + case CFGElement::TemporaryDtor: + llvm_unreachable("not yet implemented!"); + } + + llvm_unreachable("Unknown CFGElement kind"); } PathDiagnosticCallPiece * @@ -520,7 +545,9 @@ PathDiagnosticCallPiece::construct(const ExplodedNode *N, const CallExitEnd &CE, const SourceManager &SM) { const Decl *caller = CE.getLocationContext()->getDecl(); - PathDiagnosticLocation pos = getLastStmtLoc(N, SM); + PathDiagnosticLocation pos = getLocationForCaller(CE.getCalleeContext(), + CE.getLocationContext(), + SM); return new PathDiagnosticCallPiece(caller, pos); } @@ -535,11 +562,11 @@ PathDiagnosticCallPiece::construct(PathPieces &path, void PathDiagnosticCallPiece::setCallee(const CallEnter &CE, const SourceManager &SM) { - const Decl *D = CE.getCalleeContext()->getDecl(); - Callee = D; - callEnter = PathDiagnosticLocation(CE.getCallExpr(), SM, - CE.getLocationContext()); - callEnterWithin = PathDiagnosticLocation::createBegin(D, SM); + const StackFrameContext *CalleeCtx = CE.getCalleeContext(); + Callee = CalleeCtx->getDecl(); + + callEnterWithin = PathDiagnosticLocation::createBegin(Callee, SM); + callEnter = getLocationForCaller(CalleeCtx, CE.getLocationContext(), SM); } IntrusiveRefCntPtr<PathDiagnosticEventPiece> @@ -677,6 +704,7 @@ std::string StackHintGeneratorForSymbol::getMessage(const ExplodedNode *N){ const CallExitEnd *CExit = dyn_cast<CallExitEnd>(&P); assert(CExit && "Stack Hints should be constructed at CallExitEnd points."); + // FIXME: Use CallEvent to abstract this over all calls. const Stmt *CallSite = CExit->getCalleeContext()->getCallSite(); const CallExpr *CE = dyn_cast_or_null<CallExpr>(CallSite); if (!CE) diff --git a/test/Analysis/dtor.cpp b/test/Analysis/dtor.cpp index 5a14f3025d..8d67f78a4a 100644 --- a/test/Analysis/dtor.cpp +++ b/test/Analysis/dtor.cpp @@ -34,3 +34,72 @@ void testSmartPointer() { } *mem = 0; // expected-warning{{Use of memory after it is freed}} } + + +void doSomething(); +void testSmartPointer2() { + char *mem = (char*)malloc(4); + { + SmartPointer Deleter(mem); + // Remove dead bindings... + doSomething(); + // destructor called here + } + *mem = 0; // expected-warning{{Use of memory after it is freed}} +} + + +class Subclass : public SmartPointer { +public: + Subclass(void *x) : SmartPointer(x) {} +}; + +void testSubclassSmartPointer() { + char *mem = (char*)malloc(4); + { + Subclass Deleter(mem); + // Remove dead bindings... + doSomething(); + // destructor called here + } + *mem = 0; // expected-warning{{Use of memory after it is freed}} +} + + +class MultipleInheritance : public Subclass, public SmartPointer { +public: + MultipleInheritance(void *a, void *b) : Subclass(a), SmartPointer(b) {} +}; + +void testMultipleInheritance1() { + char *mem = (char*)malloc(4); + { + MultipleInheritance Deleter(mem, 0); + // Remove dead bindings... + doSomething(); + // destructor called here + } + *mem = 0; // expected-warning{{Use of memory after it is freed}} +} + +void testMultipleInheritance2() { + char *mem = (char*)malloc(4); + { + MultipleInheritance Deleter(0, mem); + // Remove dead bindings... + doSomething(); + // destructor called here + } + *mem = 0; // expected-warning{{Use of memory after it is freed}} +} + +void testMultipleInheritance3() { + char *mem = (char*)malloc(4); + { + MultipleInheritance Deleter(mem, mem); + // Remove dead bindings... + doSomething(); + // destructor called here + // expected-warning@25 {{Attempt to free released memory}} + } +} |