From efd182d9929cbe369a814a5bcd0d4f4b4ae64fc3 Mon Sep 17 00:00:00 2001 From: Anna Zaks Date: Fri, 16 Sep 2011 19:18:30 +0000 Subject: [PATCH] [analyzer] Refactor: make PathDiagnosticLocation responsible for validation of SourceLocations (commit 5 of ?): - Get rid of PathDiagnosticLocation(SourceRange r,..) constructor by providing a bunch of create methods. - The PathDiagnosticLocation(SourceLocation L,..), which is used by crate methods, will eventually become private. - Test difference is in the case when the report starts at the beginning of the function. We used to represent that point as a range of the very first token in the first statement. Now, it's just a single location representing the first character of the first statement. llvm-svn: 139932 --- .../Core/BugReporter/PathDiagnostic.h | 53 +++++++++-- clang/lib/StaticAnalyzer/Core/BugReporter.cpp | 36 ++++--- .../Core/BugReporterVisitors.cpp | 4 +- .../StaticAnalyzer/Core/PathDiagnostic.cpp | 95 +++++++++++++------ clang/test/Analysis/plist-output-alternate.m | 2 +- 5 files changed, 134 insertions(+), 56 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h index e24544a1a95f..0c21788723e4 100644 --- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h +++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h @@ -23,6 +23,8 @@ namespace clang { +class BinaryOperator; +class CompoundStmt; class Decl; class LocationContext; class ProgramPoint; @@ -98,21 +100,48 @@ public: PathDiagnosticLocation(FullSourceLoc L) : K(SingleLocK), R(L, L), S(0), D(0), SM(&L.getManager()), LC(0) {} - /// Constructs a location when no specific statement is available. - /// Defaults to end of brace for the enclosing function body. - PathDiagnosticLocation(const LocationContext *lc, const SourceManager &sm); - + PathDiagnosticLocation(SourceLocation L, const SourceManager &sm, + Kind kind = SingleLocK) + : K(kind), R(L, L), S(0), D(0), SM(&sm), LC(0) {} + PathDiagnosticLocation(const Stmt *s, const SourceManager &sm, const LocationContext *lc) : K(StmtK), S(s), D(0), SM(&sm), LC(lc) {} - PathDiagnosticLocation(SourceRange r, const SourceManager &sm) - : K(RangeK), R(r), S(0), D(0), SM(&sm), LC(0) {} - PathDiagnosticLocation(const Decl *d, const SourceManager &sm) : K(DeclK), S(0), D(d), SM(&sm), LC(0) {} + // Create a location for the beginning of the statement. + static PathDiagnosticLocation createBeginStmt(const Stmt *S, + const SourceManager &SM, + const LocationContext *LC); + + /// Create the location for the operator of the binary expression. + /// Assumes the statement has a valid location. + static PathDiagnosticLocation createOperatorLoc(const BinaryOperator *BO, + const SourceManager &SM); + + /// Create a location for the beginning of the compound statement. + /// Assumes the statement has a valid location. + static PathDiagnosticLocation createBeginBrace(const CompoundStmt *CS, + const SourceManager &SM); + + /// Create a location for the end of the compound statement. + /// Assumes the statement has a valid location. + static PathDiagnosticLocation createEndBrace(const CompoundStmt *CS, + const SourceManager &SM); + + /// Create a location for the beginning of the enclosing declaration body. + /// Defaults to the beginning of the first statement in the declaration body. + static PathDiagnosticLocation createDeclBegin(const LocationContext *LC, + const SourceManager &SM); + + /// Constructs a location for the end of the enclosing declaration body. + /// Defaults to the end of brace. + static PathDiagnosticLocation createDeclEnd(const LocationContext *LC, + const SourceManager &SM); + /// Create a location corresponding to the given valid ExplodedNode. PathDiagnosticLocation(const ProgramPoint& P, const SourceManager &SMng); @@ -144,6 +173,16 @@ public: *this = PathDiagnosticLocation(); } + /// Specify that the object represents a single location. + void setSingleLocKind() { + if (K == SingleLocK) + return; + + SourceLocation L = asLocation(); + K = SingleLocK; + R = SourceRange(L, L); + } + void flatten(); const SourceManager& getManager() const { assert(isValid()); return *SM; } diff --git a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp index 166b6fdd09e0..9022a1e6f9d8 100644 --- a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp +++ b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp @@ -175,8 +175,8 @@ PathDiagnosticBuilder::ExecutionContinues(const ExplodedNode *N) { if (const Stmt *S = GetNextStmt(N)) return PathDiagnosticLocation(S, getSourceManager(), getLocationContext()); - return FullSourceLoc(N->getLocationContext()->getDecl()->getBodyRBrace(), - getSourceManager()); + return PathDiagnosticLocation::createDeclEnd(N->getLocationContext(), + getSourceManager()); } PathDiagnosticLocation @@ -665,7 +665,8 @@ static void GenerateMinimalPathDiagnostic(PathDiagnostic& PD, if (*(Src->succ_begin()+1) == Dst) { os << "false"; PathDiagnosticLocation End(B->getLHS(), SMgr, LC); - PathDiagnosticLocation Start(B->getOperatorLoc(), SMgr); + PathDiagnosticLocation Start = + PathDiagnosticLocation::createOperatorLoc(B, SMgr); PD.push_front(new PathDiagnosticControlFlowPiece(Start, End, os.str())); } @@ -691,7 +692,8 @@ static void GenerateMinimalPathDiagnostic(PathDiagnostic& PD, else { os << "true"; PathDiagnosticLocation End(B->getLHS(), SMgr, LC); - PathDiagnosticLocation Start(B->getOperatorLoc(), SMgr); + PathDiagnosticLocation Start = + PathDiagnosticLocation::createOperatorLoc(B, SMgr); PD.push_front(new PathDiagnosticControlFlowPiece(Start, End, os.str())); } @@ -879,7 +881,7 @@ class EdgeBuilder { } if (firstCharOnly) - L = PathDiagnosticLocation(L.asLocation()); + L.setSingleLocKind(); return L; } @@ -908,17 +910,14 @@ public: ~EdgeBuilder() { while (!CLocs.empty()) popLocation(); - + // Finally, add an initial edge from the start location of the first // statement (if it doesn't already exist). - // FIXME: Should handle CXXTryStmt if analyser starts supporting C++. - if (const CompoundStmt *CS = - dyn_cast_or_null(PDB.getCodeDecl().getBody())) - if (!CS->body_empty()) { - SourceLocation Loc = (*CS->body_begin())->getLocStart(); - rawAddEdge(PathDiagnosticLocation(Loc, PDB.getSourceManager())); - } - + PathDiagnosticLocation L = PathDiagnosticLocation::createDeclBegin( + PDB.getLocationContext(), + PDB.getSourceManager()); + if (L.isValid()) + rawAddEdge(L); } void addEdge(PathDiagnosticLocation NewLoc, bool alwaysAdd = false); @@ -1117,6 +1116,7 @@ static void GenerateExtensivePathDiagnostic(PathDiagnostic& PD, PathDiagnosticBuilder &PDB, const ExplodedNode *N) { EdgeBuilder EB(PD, PDB); + const SourceManager& SM = PDB.getSourceManager(); const ExplodedNode *NextNode = N->pred_empty() ? NULL : *(N->pred_begin()); while (NextNode) { @@ -1132,8 +1132,7 @@ static void GenerateExtensivePathDiagnostic(PathDiagnostic& PD, // Are we jumping to the head of a loop? Add a special diagnostic. if (const Stmt *Loop = BE->getDst()->getLoopTarget()) { - PathDiagnosticLocation L(Loop, PDB.getSourceManager(), - PDB.getLocationContext()); + PathDiagnosticLocation L(Loop, SM, PDB.getLocationContext()); const CompoundStmt *CS = NULL; if (!Term) { @@ -1151,9 +1150,8 @@ static void GenerateExtensivePathDiagnostic(PathDiagnostic& PD, PD.push_front(p); if (CS) { - PathDiagnosticLocation BL(CS->getRBracLoc(), - PDB.getSourceManager()); - BL = PathDiagnosticLocation(BL.asLocation()); + PathDiagnosticLocation BL = + PathDiagnosticLocation::createEndBrace(CS, SM); EB.addEdge(BL); } } diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp index baef8cc9c042..3356ed4eac7d 100644 --- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -90,8 +90,8 @@ BugReporterVisitor::getDefaultEndPath(BugReporterContext &BRC, if (const BlockEntrance *BE = dyn_cast(&PP)) { const CFGBlock *block = BE->getBlock(); if (block->getBlockID() == 0) { - L = PathDiagnosticLocation(PP.getLocationContext(), - BRC.getSourceManager()); + L = PathDiagnosticLocation::createDeclEnd(PP.getLocationContext(), + BRC.getSourceManager()); } } diff --git a/clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp b/clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp index 495451d261e4..3c798e0facb2 100644 --- a/clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp +++ b/clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp @@ -130,11 +130,71 @@ void PathDiagnosticClient::HandlePathDiagnostic(const PathDiagnostic *D) { // PathDiagnosticLocation methods. //===----------------------------------------------------------------------===// -PathDiagnosticLocation::PathDiagnosticLocation(const LocationContext *lc, - const SourceManager &sm) - : K(SingleLocK), S(0), D(0), SM(&sm), LC(lc) { +static SourceLocation getValidSourceLocation(const Stmt* S, + const LocationContext *LC) { + assert(LC); + SourceLocation L = S->getLocStart(); + + // 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. + if (!L.isValid()) { + ParentMap & PM = LC->getParentMap(); + + while (!L.isValid()) { + S = PM.getParent(S); + L = S->getLocStart(); + } + } + + return L; +} + +PathDiagnosticLocation + PathDiagnosticLocation::createBeginStmt(const Stmt *S, + const SourceManager &SM, + const LocationContext *LC) { + return PathDiagnosticLocation(getValidSourceLocation(S, LC), SM, SingleLocK); +} + +PathDiagnosticLocation + PathDiagnosticLocation::createOperatorLoc(const BinaryOperator *BO, + const SourceManager &SM) { + return PathDiagnosticLocation(BO->getOperatorLoc(), SM, SingleLocK); +} + +PathDiagnosticLocation + PathDiagnosticLocation::createBeginBrace(const CompoundStmt *CS, + const SourceManager &SM) { + SourceLocation L = CS->getLBracLoc(); + return PathDiagnosticLocation(L, SM, SingleLocK); +} + +PathDiagnosticLocation + PathDiagnosticLocation::createEndBrace(const CompoundStmt *CS, + const SourceManager &SM) { + SourceLocation L = CS->getRBracLoc(); + return PathDiagnosticLocation(L, SM, SingleLocK); +} + +PathDiagnosticLocation + PathDiagnosticLocation::createDeclBegin(const LocationContext *LC, + const SourceManager &SM) { + // FIXME: Should handle CXXTryStmt if analyser starts supporting C++. + if (const CompoundStmt *CS = + dyn_cast_or_null(LC->getDecl()->getBody())) + if (!CS->body_empty()) { + SourceLocation Loc = (*CS->body_begin())->getLocStart(); + return PathDiagnosticLocation(Loc, SM, SingleLocK); + } + + return PathDiagnosticLocation(); +} + +PathDiagnosticLocation + PathDiagnosticLocation::createDeclEnd(const LocationContext *LC, + const SourceManager &SM) { SourceLocation L = LC->getDecl()->getBodyRBrace(); - R = SourceRange(L, L); + return PathDiagnosticLocation(L, SM, SingleLocK); } PathDiagnosticLocation::PathDiagnosticLocation(const ProgramPoint& P, @@ -153,9 +213,9 @@ PathDiagnosticLocation::PathDiagnosticLocation(const ProgramPoint& P, invalidate(); } -PathDiagnosticLocation PathDiagnosticLocation::createEndOfPath( - const ExplodedNode* N, - const SourceManager &SM) { +PathDiagnosticLocation + PathDiagnosticLocation::createEndOfPath(const ExplodedNode* N, + const SourceManager &SM) { assert(N && "Cannot create a location with a null node."); const ExplodedNode *NI = N; @@ -174,26 +234,7 @@ PathDiagnosticLocation PathDiagnosticLocation::createEndOfPath( NI = NI->succ_empty() ? 0 : *(NI->succ_begin()); } - return PathDiagnosticLocation(N->getLocationContext(), SM); -} - -static SourceLocation getValidSourceLocation(const Stmt* S, - const LocationContext *LC) { - assert(LC); - SourceLocation L = S->getLocStart(); - - // 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. - if (!L.isValid()) { - ParentMap & PM = LC->getParentMap(); - - while (!L.isValid()) { - S = PM.getParent(S); - L = S->getLocStart(); - } - } - - return L; + return createDeclEnd(N->getLocationContext(), SM); } FullSourceLoc PathDiagnosticLocation::asLocation() const { diff --git a/clang/test/Analysis/plist-output-alternate.m b/clang/test/Analysis/plist-output-alternate.m index 997201150012..b3a0564289f9 100644 --- a/clang/test/Analysis/plist-output-alternate.m +++ b/clang/test/Analysis/plist-output-alternate.m @@ -620,7 +620,7 @@ void rdar8331641(int x) { // CHECK: // CHECK: // CHECK: line35 -// CHECK: col8 +// CHECK: col3 // CHECK: file0 // CHECK: // CHECK: