From: Anna Zaks Date: Tue, 8 Jan 2013 00:25:29 +0000 (+0000) Subject: [analyzer] Include the bug uniqueing location in the issue_hash. X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=97bfb558f69c09b01a5c1510f08dc91eb62329a7;p=clang [analyzer] Include the bug uniqueing location in the issue_hash. The issue here is that if we have 2 leaks reported at the same line for which we cannot print the corresponding region info, they will get treated as the same by issue_hash+description. We need to AUGMENT the issue_hash with the allocation info to differentiate the two issues. Add the "hash" (offset from the beginning of a function) representing allocation site to solve the issue. We might want to generalize solution in the future when we decide to track more than just the 2 locations from the diagnostics. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@171825 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h index 3ee7925304..deb2ef6dcc 100644 --- a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h +++ b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h @@ -75,6 +75,8 @@ protected: std::string Description; PathDiagnosticLocation Location; PathDiagnosticLocation UniqueingLocation; + const Decl *UniqueingDecl; + const ExplodedNode *ErrorNode; SmallVector Ranges; ExtraTextList ExtraText; @@ -162,9 +164,10 @@ public: /// for uniquing reports. For example, memory leaks checker, could set this to /// the allocation site, rather then the location where the bug is reported. BugReport(BugType& bt, StringRef desc, const ExplodedNode *errornode, - PathDiagnosticLocation LocationToUnique) + PathDiagnosticLocation LocationToUnique, const Decl *DeclToUnique) : BT(bt), DeclWithIssue(0), Description(desc), UniqueingLocation(LocationToUnique), + UniqueingDecl(DeclToUnique), ErrorNode(errornode), ConfigurationChangeToken(0), DoNotPrunePath(false) {} @@ -260,6 +263,16 @@ public: /// This location is used by clients rendering diagnostics. virtual PathDiagnosticLocation getLocation(const SourceManager &SM) const; + /// \brief Get the location on which the report should be uniqued. + PathDiagnosticLocation getUniqueingLocation() const { + return UniqueingLocation; + } + + /// \brief Get the declaration containing the uniqueing location. + const Decl *getUniqueingDecl() const { + return UniqueingDecl; + } + const Stmt *getStmt() const; /// \brief Add a range to a bug report. diff --git a/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h b/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h index 8eb48a6c17..01d7f48275 100644 --- a/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h +++ b/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h @@ -672,11 +672,17 @@ class PathDiagnostic : public llvm::FoldingSetNode { PathPieces pathImpl; llvm::SmallVector pathStack; + /// \brief Important bug uniqueing location. + /// The location info is useful to differentiate between bugs. + PathDiagnosticLocation UniqueingLoc; + const Decl *UniqueingDecl; + PathDiagnostic(); // Do not implement. public: PathDiagnostic(const Decl *DeclWithIssue, StringRef bugtype, StringRef verboseDesc, StringRef shortDesc, - StringRef category); + StringRef category, PathDiagnosticLocation LocationToUnique, + const Decl *DeclToUnique); ~PathDiagnostic(); @@ -738,6 +744,16 @@ public: return Loc; } + /// \brief Get the location on which the report should be uniqued. + PathDiagnosticLocation getUniqueingLoc() const { + return UniqueingLoc; + } + + /// \brief Get the declaration containing the uniqueing location. + const Decl *getUniqueingDecl() const { + return UniqueingDecl; + } + void flattenLocations() { Loc.flatten(); for (PathPieces::iterator I = pathImpl.begin(), E = pathImpl.end(); diff --git a/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp b/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp index b899b6f9b7..a7c73b87c1 100644 --- a/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp @@ -99,8 +99,8 @@ private: CheckerContext &C) const; /// Find the allocation site for Sym on the path leading to the node N. - const Stmt *getAllocationSite(const ExplodedNode *N, SymbolRef Sym, - CheckerContext &C) const; + const ExplodedNode *getAllocationNode(const ExplodedNode *N, SymbolRef Sym, + CheckerContext &C) const; BugReport *generateAllocatedDataNotReleasedReport(const AllocationPair &AP, ExplodedNode *N, @@ -486,8 +486,8 @@ void MacOSKeychainAPIChecker::checkPostStmt(const CallExpr *CE, } // TODO: This logic is the same as in Malloc checker. -const Stmt * -MacOSKeychainAPIChecker::getAllocationSite(const ExplodedNode *N, +const ExplodedNode * +MacOSKeychainAPIChecker::getAllocationNode(const ExplodedNode *N, SymbolRef Sym, CheckerContext &C) const { const LocationContext *LeakContext = N->getLocationContext(); @@ -505,12 +505,7 @@ MacOSKeychainAPIChecker::getAllocationSite(const ExplodedNode *N, N = N->pred_empty() ? NULL : *(N->pred_begin()); } - ProgramPoint P = AllocNode->getLocation(); - if (CallExitEnd *Exit = dyn_cast(&P)) - return Exit->getCalleeContext()->getCallSite(); - if (clang::PostStmt *PS = dyn_cast(&P)) - return PS->getStmt(); - return 0; + return AllocNode; } BugReport *MacOSKeychainAPIChecker:: @@ -528,11 +523,22 @@ BugReport *MacOSKeychainAPIChecker:: // With leaks, we want to unique them by the location where they were // allocated, and only report a single path. PathDiagnosticLocation LocUsedForUniqueing; - if (const Stmt *AllocStmt = getAllocationSite(N, AP.first, C)) + const ExplodedNode *AllocNode = getAllocationNode(N, AP.first, C); + const Stmt *AllocStmt = 0; + ProgramPoint P = AllocNode->getLocation(); + if (CallExitEnd *Exit = dyn_cast(&P)) + AllocStmt = Exit->getCalleeContext()->getCallSite(); + else if (clang::PostStmt *PS = dyn_cast(&P)) + AllocStmt = PS->getStmt(); + + if (AllocStmt) LocUsedForUniqueing = PathDiagnosticLocation::createBegin(AllocStmt, - C.getSourceManager(), N->getLocationContext()); + C.getSourceManager(), + AllocNode->getLocationContext()); + + BugReport *Report = new BugReport(*BT, os.str(), N, LocUsedForUniqueing, + AllocNode->getLocationContext()->getDecl()); - BugReport *Report = new BugReport(*BT, os.str(), N, LocUsedForUniqueing); Report->addVisitor(new SecKeychainBugVisitor(AP.first)); markInteresting(Report, AP); return Report; diff --git a/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/lib/StaticAnalyzer/Checkers/MallocChecker.cpp index 56b338fd4c..70f426df20 100644 --- a/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -113,7 +113,7 @@ struct ReallocPair { } }; -typedef std::pair LeakInfo; +typedef std::pair LeakInfo; class MallocChecker : public Checkerpred_empty() ? NULL : *(N->pred_begin()); } - ProgramPoint P = AllocNode->getLocation(); - const Stmt *AllocationStmt = 0; - if (CallExitEnd *Exit = dyn_cast(&P)) - AllocationStmt = Exit->getCalleeContext()->getCallSite(); - else if (StmtPoint *SP = dyn_cast(&P)) - AllocationStmt = SP->getStmt(); - - return LeakInfo(AllocationStmt, ReferenceRegion); + return LeakInfo(AllocNode, ReferenceRegion); } void MallocChecker::reportLeak(SymbolRef Sym, ExplodedNode *N, @@ -1066,12 +1059,20 @@ void MallocChecker::reportLeak(SymbolRef Sym, ExplodedNode *N, // With leaks, we want to unique them by the location where they were // allocated, and only report a single path. PathDiagnosticLocation LocUsedForUniqueing; - const Stmt *AllocStmt = 0; + const ExplodedNode *AllocNode = 0; const MemRegion *Region = 0; - llvm::tie(AllocStmt, Region) = getAllocationSite(N, Sym, C); - if (AllocStmt) - LocUsedForUniqueing = PathDiagnosticLocation::createBegin(AllocStmt, - C.getSourceManager(), N->getLocationContext()); + llvm::tie(AllocNode, Region) = getAllocationSite(N, Sym, C); + + ProgramPoint P = AllocNode->getLocation(); + const Stmt *AllocationStmt = 0; + if (CallExitEnd *Exit = dyn_cast(&P)) + AllocationStmt = Exit->getCalleeContext()->getCallSite(); + else if (StmtPoint *SP = dyn_cast(&P)) + AllocationStmt = SP->getStmt(); + if (AllocationStmt) + LocUsedForUniqueing = PathDiagnosticLocation::createBegin(AllocationStmt, + C.getSourceManager(), + AllocNode->getLocationContext()); SmallString<200> buf; llvm::raw_svector_ostream os(buf); @@ -1082,7 +1083,9 @@ void MallocChecker::reportLeak(SymbolRef Sym, ExplodedNode *N, os << '\''; } - BugReport *R = new BugReport(*BT_Leak, os.str(), N, LocUsedForUniqueing); + BugReport *R = new BugReport(*BT_Leak, os.str(), N, + LocUsedForUniqueing, + AllocNode->getLocationContext()->getDecl()); R->markInteresting(Sym); R->addVisitor(new MallocBugVisitor(Sym, true)); C.emitReport(R); diff --git a/lib/StaticAnalyzer/Core/BugReporter.cpp b/lib/StaticAnalyzer/Core/BugReporter.cpp index d5e5f05c48..8e6bc69cc4 100644 --- a/lib/StaticAnalyzer/Core/BugReporter.cpp +++ b/lib/StaticAnalyzer/Core/BugReporter.cpp @@ -1527,8 +1527,9 @@ const Decl *BugReport::getDeclWithIssue() const { void BugReport::Profile(llvm::FoldingSetNodeID& hash) const { hash.AddPointer(&BT); hash.AddString(Description); - if (UniqueingLocation.isValid()) { - UniqueingLocation.Profile(hash); + PathDiagnosticLocation UL = getUniqueingLocation(); + if (UL.isValid()) { + UL.Profile(hash); } else if (Location.isValid()) { Location.Profile(hash); } else { @@ -2295,7 +2296,9 @@ void BugReporter::FlushReport(BugReport *exampleReport, exampleReport->getBugType().getName(), exampleReport->getDescription(), exampleReport->getShortDescription(/*Fallback=*/false), - BT.getCategory())); + BT.getCategory(), + exampleReport->getUniqueingLocation(), + exampleReport->getUniqueingDecl())); // Generate the full path diagnostic, using the generation scheme // specified by the PathDiagnosticConsumer. Note that we have to generate diff --git a/lib/StaticAnalyzer/Core/PathDiagnostic.cpp b/lib/StaticAnalyzer/Core/PathDiagnostic.cpp index 614a5a5679..ec2e1885f5 100644 --- a/lib/StaticAnalyzer/Core/PathDiagnostic.cpp +++ b/lib/StaticAnalyzer/Core/PathDiagnostic.cpp @@ -107,12 +107,16 @@ PathDiagnostic::~PathDiagnostic() {} PathDiagnostic::PathDiagnostic(const Decl *declWithIssue, StringRef bugtype, StringRef verboseDesc, - StringRef shortDesc, StringRef category) + StringRef shortDesc, StringRef category, + PathDiagnosticLocation LocationToUnique, + const Decl *DeclToUnique) : DeclWithIssue(declWithIssue), BugType(StripTrailingDots(bugtype)), VerboseDesc(StripTrailingDots(verboseDesc)), ShortDesc(StripTrailingDots(shortDesc)), Category(StripTrailingDots(category)), + UniqueingLoc(LocationToUnique), + UniqueingDecl(DeclToUnique), path(pathImpl) {} void PathDiagnosticConsumer::anchor() { } diff --git a/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp b/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp index f340509268..4d0f1d8d44 100644 --- a/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp +++ b/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp @@ -499,8 +499,21 @@ void PlistDiagnostics::FlushDiagnosticsImpl( *SM); FullSourceLoc FunLoc(SM->getExpansionLoc(Body->getLocStart()), *SM); o << " issue_hash" - << Loc.getExpansionLineNumber() - FunLoc.getExpansionLineNumber() - << "\n"; + << Loc.getExpansionLineNumber() - FunLoc.getExpansionLineNumber(); + + // Augment the hash with the bug uniqueing location. For example, + // this ensures that two leaks reported on the same line will have + // different issue_hashes. + PathDiagnosticLocation UPDLoc = D->getUniqueingLoc(); + if (UPDLoc.isValid()) { + FullSourceLoc UL(SM->getExpansionLoc(UPDLoc.asLocation()), + *SM); + FullSourceLoc UFunL(SM->getExpansionLoc( + D->getUniqueingDecl()->getBody()->getLocStart()), *SM); + o << "_" + << UL.getExpansionLineNumber() - UFunL.getExpansionLineNumber(); + } + o << "\n"; } } } diff --git a/test/Analysis/malloc-plist.c b/test/Analysis/malloc-plist.c index f595ce4c9c..a48b94d03d 100644 --- a/test/Analysis/malloc-plist.c +++ b/test/Analysis/malloc-plist.c @@ -388,7 +388,7 @@ void use_function_with_leak7() { // CHECK-NEXT: typeMemory leak // CHECK-NEXT: issue_context_kindfunction // CHECK-NEXT: issue_contextdiagnosticTest -// CHECK-NEXT: issue_hash5 +// CHECK-NEXT: issue_hash5_2 // CHECK-NEXT: location // CHECK-NEXT: // CHECK-NEXT: line15 @@ -550,7 +550,7 @@ void use_function_with_leak7() { // CHECK-NEXT: typeMemory leak // CHECK-NEXT: issue_context_kindfunction // CHECK-NEXT: issue_contextmyArrayAllocation -// CHECK-NEXT: issue_hash4 +// CHECK-NEXT: issue_hash4_2 // CHECK-NEXT: location // CHECK-NEXT: // CHECK-NEXT: line22 @@ -935,7 +935,7 @@ void use_function_with_leak7() { // CHECK-NEXT: typeMemory leak // CHECK-NEXT: issue_context_kindfunction // CHECK-NEXT: issue_contextreallocDiagnostics -// CHECK-NEXT: issue_hash5 +// CHECK-NEXT: issue_hash5_1 // CHECK-NEXT: location // CHECK-NEXT: // CHECK-NEXT: line29 @@ -1334,7 +1334,7 @@ void use_function_with_leak7() { // CHECK-NEXT: typeMemory leak // CHECK-NEXT: issue_context_kindfunction // CHECK-NEXT: issue_contexttest_wrapper -// CHECK-NEXT: issue_hash3 +// CHECK-NEXT: issue_hash3_1 // CHECK-NEXT: location // CHECK-NEXT: // CHECK-NEXT: line46 @@ -2413,7 +2413,7 @@ void use_function_with_leak7() { // CHECK-NEXT: typeMemory leak // CHECK-NEXT: issue_context_kindfunction // CHECK-NEXT: issue_contextreallocIntra -// CHECK-NEXT: issue_hash3 +// CHECK-NEXT: issue_hash3_1 // CHECK-NEXT: location // CHECK-NEXT: // CHECK-NEXT: line77 @@ -2681,7 +2681,7 @@ void use_function_with_leak7() { // CHECK-NEXT: typeMemory leak // CHECK-NEXT: issue_context_kindfunction // CHECK-NEXT: issue_contextuse_ret -// CHECK-NEXT: issue_hash3 +// CHECK-NEXT: issue_hash3_2 // CHECK-NEXT: location // CHECK-NEXT: // CHECK-NEXT: line87 @@ -2843,7 +2843,7 @@ void use_function_with_leak7() { // CHECK-NEXT: typeMemory leak // CHECK-NEXT: issue_context_kindfunction // CHECK-NEXT: issue_contextLeakedSymbol -// CHECK-NEXT: issue_hash8 +// CHECK-NEXT: issue_hash8_3 // CHECK-NEXT: location // CHECK-NEXT: // CHECK-NEXT: line98 @@ -3048,7 +3048,7 @@ void use_function_with_leak7() { // CHECK-NEXT: typeMemory leak // CHECK-NEXT: issue_context_kindfunction // CHECK-NEXT: issue_contextfunction_with_leak1 -// CHECK-NEXT: issue_hash2 +// CHECK-NEXT: issue_hash2_1 // CHECK-NEXT: location // CHECK-NEXT: // CHECK-NEXT: line104 @@ -3253,7 +3253,7 @@ void use_function_with_leak7() { // CHECK-NEXT: typeMemory leak // CHECK-NEXT: issue_context_kindfunction // CHECK-NEXT: issue_contextfunction_with_leak2 -// CHECK-NEXT: issue_hash2 +// CHECK-NEXT: issue_hash2_1 // CHECK-NEXT: location // CHECK-NEXT: // CHECK-NEXT: line112 @@ -3555,7 +3555,7 @@ void use_function_with_leak7() { // CHECK-NEXT: typeMemory leak // CHECK-NEXT: issue_context_kindfunction // CHECK-NEXT: issue_contextfunction_with_leak3 -// CHECK-NEXT: issue_hash3 +// CHECK-NEXT: issue_hash3_1 // CHECK-NEXT: location // CHECK-NEXT: // CHECK-NEXT: line121 @@ -3857,7 +3857,7 @@ void use_function_with_leak7() { // CHECK-NEXT: typeMemory leak // CHECK-NEXT: issue_context_kindfunction // CHECK-NEXT: issue_contextfunction_with_leak4 -// CHECK-NEXT: issue_hash5 +// CHECK-NEXT: issue_hash5_1 // CHECK-NEXT: location // CHECK-NEXT: // CHECK-NEXT: line132 @@ -4062,7 +4062,7 @@ void use_function_with_leak7() { // CHECK-NEXT: typeMemory leak // CHECK-NEXT: issue_context_kindfunction // CHECK-NEXT: issue_contextfunction_with_leak5 -// CHECK-NEXT: issue_hash2 +// CHECK-NEXT: issue_hash2_1 // CHECK-NEXT: location // CHECK-NEXT: // CHECK-NEXT: line143 @@ -4267,7 +4267,7 @@ void use_function_with_leak7() { // CHECK-NEXT: typeMemory leak // CHECK-NEXT: issue_context_kindfunction // CHECK-NEXT: issue_contextfunction_with_leak6 -// CHECK-NEXT: issue_hash2 +// CHECK-NEXT: issue_hash2_1 // CHECK-NEXT: location // CHECK-NEXT: // CHECK-NEXT: line154 @@ -4501,7 +4501,7 @@ void use_function_with_leak7() { // CHECK-NEXT: typeMemory leak // CHECK-NEXT: issue_context_kindfunction // CHECK-NEXT: issue_contextuse_function_with_leak7 -// CHECK-NEXT: issue_hash2 +// CHECK-NEXT: issue_hash2_1 // CHECK-NEXT: location // CHECK-NEXT: // CHECK-NEXT: line170 @@ -4510,3 +4510,5 @@ void use_function_with_leak7() { // CHECK-NEXT: // CHECK-NEXT: // CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: