From: Ted Kremenek Date: Wed, 25 Jan 2012 23:47:14 +0000 (+0000) Subject: Rework flushing of diagnostics to PathDiagnosticConsumer. Now all the reports are... X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=bac341346f3c8e713a8f165120fd54b500ee3189;p=clang Rework flushing of diagnostics to PathDiagnosticConsumer. Now all the reports are batched up before being flushed to the underlying consumer implementation. This allows us to unique reports across analyses to multiple functions (which shows up with inlining). git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@148997 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h b/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h index fba5692d05..427c47aa84 100644 --- a/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h +++ b/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h @@ -48,20 +48,18 @@ class PathDiagnostic; class PathDiagnosticConsumer { virtual void anchor(); public: - PathDiagnosticConsumer() {} + PathDiagnosticConsumer() : flushed(false) {} + virtual ~PathDiagnosticConsumer(); + + void FlushDiagnostics(SmallVectorImpl *FilesMade); + + virtual void FlushDiagnosticsImpl(std::vector &Diags, + SmallVectorImpl *FilesMade) + = 0; - virtual ~PathDiagnosticConsumer() {} - - virtual void - FlushDiagnostics(SmallVectorImpl *FilesMade = 0) = 0; - - void FlushDiagnostics(SmallVectorImpl &FilesMade) { - FlushDiagnostics(&FilesMade); - } - virtual StringRef getName() const = 0; - void HandlePathDiagnostic(const PathDiagnostic* D); + void HandlePathDiagnostic(PathDiagnostic *D); enum PathGenerationScheme { Minimal, Extensive }; virtual PathGenerationScheme getGenerationScheme() const { return Minimal; } @@ -70,10 +68,8 @@ public: virtual bool useVerboseDescription() const { return true; } protected: - /// The actual logic for handling path diagnostics, as implemented - /// by subclasses of PathDiagnosticConsumer. - virtual void HandlePathDiagnosticImpl(const PathDiagnostic* D) = 0; - + bool flushed; + llvm::FoldingSet Diags; }; //===----------------------------------------------------------------------===// @@ -597,6 +593,8 @@ public: } void Profile(llvm::FoldingSetNodeID &ID) const; + + void FullProfile(llvm::FoldingSetNodeID &ID) const; }; } // end GR namespace diff --git a/include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h b/include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h index d4408aef11..b7294abb52 100644 --- a/include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h +++ b/include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h @@ -138,7 +138,7 @@ public: void FlushDiagnostics() { if (PD.get()) - PD->FlushDiagnostics(); + PD->FlushDiagnostics(0); } unsigned getMaxNodes() const { return MaxNodes; } diff --git a/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp b/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp index 0c4e4277e4..933e15c0de 100644 --- a/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp +++ b/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp @@ -39,15 +39,13 @@ class HTMLDiagnostics : public PathDiagnosticConsumer { llvm::sys::Path Directory, FilePrefix; bool createdDir, noDir; const Preprocessor &PP; - std::vector BatchedDiags; public: HTMLDiagnostics(const std::string& prefix, const Preprocessor &pp); virtual ~HTMLDiagnostics() { FlushDiagnostics(NULL); } - virtual void FlushDiagnostics(SmallVectorImpl *FilesMade); - - virtual void HandlePathDiagnosticImpl(const PathDiagnostic* D); + virtual void FlushDiagnosticsImpl(std::vector &Diags, + SmallVectorImpl *FilesMade); virtual StringRef getName() const { return "HTMLDiagnostics"; @@ -88,30 +86,13 @@ ento::createHTMLDiagnosticConsumer(const std::string& prefix, // Report processing. //===----------------------------------------------------------------------===// -void HTMLDiagnostics::HandlePathDiagnosticImpl(const PathDiagnostic* D) { - if (!D) - return; - - if (D->empty()) { - delete D; - return; +void HTMLDiagnostics::FlushDiagnosticsImpl( + std::vector &Diags, + SmallVectorImpl *FilesMade) { + for (std::vector::iterator it = Diags.begin(), + et = Diags.end(); it != et; ++it) { + ReportDiag(**it, FilesMade); } - - const_cast(D)->flattenLocations(); - BatchedDiags.push_back(D); -} - -void -HTMLDiagnostics::FlushDiagnostics(SmallVectorImpl *FilesMade) -{ - while (!BatchedDiags.empty()) { - const PathDiagnostic* D = BatchedDiags.back(); - BatchedDiags.pop_back(); - ReportDiag(*D, FilesMade); - delete D; - } - - BatchedDiags.clear(); } void HTMLDiagnostics::ReportDiag(const PathDiagnostic& D, diff --git a/lib/StaticAnalyzer/Core/PathDiagnostic.cpp b/lib/StaticAnalyzer/Core/PathDiagnostic.cpp index e398bae60f..c0bb180c82 100644 --- a/lib/StaticAnalyzer/Core/PathDiagnostic.cpp +++ b/lib/StaticAnalyzer/Core/PathDiagnostic.cpp @@ -84,11 +84,122 @@ PathDiagnostic::PathDiagnostic(StringRef bugtype, StringRef desc, void PathDiagnosticConsumer::anchor() { } -void PathDiagnosticConsumer::HandlePathDiagnostic(const PathDiagnostic *D) { - // For now this simply forwards to HandlePathDiagnosticImpl. In the future - // we can use this indirection to control for multi-threaded access to - // the PathDiagnosticConsumer from multiple bug reporters. - HandlePathDiagnosticImpl(D); +PathDiagnosticConsumer::~PathDiagnosticConsumer() { + // Delete the contents of the FoldingSet if it isn't empty already. + for (llvm::FoldingSet::iterator it = + Diags.begin(), et = Diags.end() ; it != et ; ++it) { + delete &*it; + } +} + +void PathDiagnosticConsumer::HandlePathDiagnostic(PathDiagnostic *D) { + if (!D) + return; + + if (D->empty()) { + delete D; + return; + } + + // We need to flatten the locations (convert Stmt* to locations) because + // the referenced statements may be freed by the time the diagnostics + // are emitted. + D->flattenLocations(); + + // Profile the node to see if we already have something matching it + llvm::FoldingSetNodeID profile; + D->Profile(profile); + void *InsertPos = 0; + + if (PathDiagnostic *orig = Diags.FindNodeOrInsertPos(profile, InsertPos)) { + // Keep the PathDiagnostic with the shorter path. + if (orig->size() <= D->size()) { + bool shouldKeepOriginal = true; + if (orig->size() == D->size()) { + // Here we break ties in a fairly arbitrary, but deterministic, way. + llvm::FoldingSetNodeID fullProfile, fullProfileOrig; + D->FullProfile(fullProfile); + orig->FullProfile(fullProfileOrig); + if (fullProfile.ComputeHash() < fullProfileOrig.ComputeHash()) + shouldKeepOriginal = false; + } + + if (shouldKeepOriginal) { + delete D; + return; + } + } + Diags.RemoveNode(orig); + delete orig; + } + + Diags.InsertNode(D); +} + + +namespace { +struct CompareDiagnostics { + // Compare if 'X' is "<" than 'Y'. + bool operator()(const PathDiagnostic *X, const PathDiagnostic *Y) const { + // First compare by location + const FullSourceLoc &XLoc = X->getLocation().asLocation(); + const FullSourceLoc &YLoc = Y->getLocation().asLocation(); + if (XLoc < YLoc) + return true; + if (XLoc != YLoc) + return false; + + // Next, compare by bug type. + StringRef XBugType = X->getBugType(); + StringRef YBugType = Y->getBugType(); + if (XBugType < YBugType) + return true; + if (XBugType != YBugType) + return false; + + // Next, compare by bug description. + StringRef XDesc = X->getDescription(); + StringRef YDesc = Y->getDescription(); + if (XDesc < YDesc) + return true; + if (XDesc != YDesc) + return false; + + // FIXME: Further refine by comparing PathDiagnosticPieces? + return false; + } +}; +} + +void +PathDiagnosticConsumer::FlushDiagnostics(SmallVectorImpl *Files) { + if (flushed) + return; + + flushed = true; + + std::vector BatchDiags; + for (llvm::FoldingSet::iterator it = Diags.begin(), + et = Diags.end(); it != et; ++it) { + BatchDiags.push_back(&*it); + } + + // Clear out the FoldingSet. + Diags.clear(); + + // Sort the diagnostics so that they are always emitted in a deterministic + // order. + if (!BatchDiags.empty()) + std::sort(BatchDiags.begin(), BatchDiags.end(), CompareDiagnostics()); + + FlushDiagnosticsImpl(BatchDiags, Files); + + // Delete the flushed diagnostics. + for (std::vector::iterator it = BatchDiags.begin(), + et = BatchDiags.end(); it != et; ++it) { + const PathDiagnostic *D = *it; + delete D; + } } //===----------------------------------------------------------------------===// @@ -366,13 +477,17 @@ void PathDiagnosticMacroPiece::Profile(llvm::FoldingSetNodeID &ID) const { } void PathDiagnostic::Profile(llvm::FoldingSetNodeID &ID) const { - ID.AddInteger(Size); + if (Size) + getLocation().Profile(ID); ID.AddString(BugType); ID.AddString(Desc); ID.AddString(Category); +} + +void PathDiagnostic::FullProfile(llvm::FoldingSetNodeID &ID) const { + Profile(ID); for (const_iterator I = begin(), E = end(); I != E; ++I) ID.Add(*I); - for (meta_iterator I = meta_begin(), E = meta_end(); I != E; ++I) ID.AddString(*I); } diff --git a/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp b/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp index 22fabbf7ae..18eed8a0f9 100644 --- a/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp +++ b/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp @@ -25,43 +25,9 @@ using namespace ento; typedef llvm::DenseMap FIDMap; -namespace { -struct CompareDiagnostics { - // Compare if 'X' is "<" than 'Y'. - bool operator()(const PathDiagnostic *X, const PathDiagnostic *Y) const { - // First compare by location - const FullSourceLoc &XLoc = X->getLocation().asLocation(); - const FullSourceLoc &YLoc = Y->getLocation().asLocation(); - if (XLoc < YLoc) - return true; - if (XLoc != YLoc) - return false; - - // Next, compare by bug type. - StringRef XBugType = X->getBugType(); - StringRef YBugType = Y->getBugType(); - if (XBugType < YBugType) - return true; - if (XBugType != YBugType) - return false; - - // Next, compare by bug description. - StringRef XDesc = X->getDescription(); - StringRef YDesc = Y->getDescription(); - if (XDesc < YDesc) - return true; - if (XDesc != YDesc) - return false; - - // FIXME: Further refine by comparing PathDiagnosticPieces? - return false; - } -}; -} namespace { class PlistDiagnostics : public PathDiagnosticConsumer { - std::vector BatchedDiags; const std::string OutputFile; const LangOptions &LangOpts; llvm::OwningPtr SubPD; @@ -70,11 +36,10 @@ namespace { PlistDiagnostics(const std::string& prefix, const LangOptions &LangOpts, PathDiagnosticConsumer *subPD); - ~PlistDiagnostics() { FlushDiagnostics(NULL); } + virtual ~PlistDiagnostics() {} - void FlushDiagnostics(SmallVectorImpl *FilesMade); - - void HandlePathDiagnosticImpl(const PathDiagnostic* D); + void FlushDiagnosticsImpl(std::vector &Diags, + SmallVectorImpl *FilesMade); virtual StringRef getName() const { return "PlistDiagnostics"; @@ -321,46 +286,20 @@ static void ReportDiag(raw_ostream &o, const PathDiagnosticPiece& P, } } -void PlistDiagnostics::HandlePathDiagnosticImpl(const PathDiagnostic* D) { - if (!D) - return; - - if (D->empty()) { - delete D; - return; - } - - // We need to flatten the locations (convert Stmt* to locations) because - // the referenced statements may be freed by the time the diagnostics - // are emitted. - const_cast(D)->flattenLocations(); - BatchedDiags.push_back(D); -} - -void PlistDiagnostics::FlushDiagnostics(SmallVectorImpl - *FilesMade) { - - if (flushed) - return; - - flushed = true; - - // Sort the diagnostics so that they are always emitted in a deterministic - // order. - if (!BatchedDiags.empty()) - std::sort(BatchedDiags.begin(), BatchedDiags.end(), CompareDiagnostics()); - +void PlistDiagnostics::FlushDiagnosticsImpl( + std::vector &Diags, + SmallVectorImpl *FilesMade) { // Build up a set of FIDs that we use by scanning the locations and // ranges of the diagnostics. FIDMap FM; SmallVector Fids; const SourceManager* SM = 0; - if (!BatchedDiags.empty()) - SM = &(*BatchedDiags.begin())->begin()->getLocation().getManager(); + if (!Diags.empty()) + SM = &(*Diags.begin())->begin()->getLocation().getManager(); - for (std::vector::iterator DI = BatchedDiags.begin(), - DE = BatchedDiags.end(); DI != DE; ++DI) { + for (std::vector::iterator DI = Diags.begin(), + DE = Diags.end(); DI != DE; ++DI) { const PathDiagnostic *D = *DI; @@ -406,16 +345,13 @@ void PlistDiagnostics::FlushDiagnostics(SmallVectorImpl " diagnostics\n" " \n"; - for (std::vector::iterator DI=BatchedDiags.begin(), - DE = BatchedDiags.end(); DI!=DE; ++DI) { + for (std::vector::iterator DI=Diags.begin(), + DE = Diags.end(); DI!=DE; ++DI) { o << " \n" " path\n"; const PathDiagnostic *D = *DI; - // Create an owning smart pointer for 'D' just so that we auto-free it - // when we exit this method. - llvm::OwningPtr OwnedD(const_cast(D)); o << " \n"; @@ -438,9 +374,10 @@ void PlistDiagnostics::FlushDiagnostics(SmallVectorImpl // Output the diagnostic to the sub-diagnostic client, if any. if (SubPD) { - SubPD->HandlePathDiagnostic(OwnedD.take()); + std::vector SubDiags; + SubDiags.push_back(D); SmallVector SubFilesMade; - SubPD->FlushDiagnostics(SubFilesMade); + SubPD->FlushDiagnosticsImpl(SubDiags, &SubFilesMade); if (!SubFilesMade.empty()) { o << " " << SubPD->getName() << "_files\n"; @@ -462,6 +399,4 @@ void PlistDiagnostics::FlushDiagnostics(SmallVectorImpl if (FilesMade) FilesMade->push_back(OutputFile); - - BatchedDiags.clear(); } diff --git a/lib/StaticAnalyzer/Core/TextPathDiagnostics.cpp b/lib/StaticAnalyzer/Core/TextPathDiagnostics.cpp index 3543f7ff13..0c6b228274 100644 --- a/lib/StaticAnalyzer/Core/TextPathDiagnostics.cpp +++ b/lib/StaticAnalyzer/Core/TextPathDiagnostics.cpp @@ -31,9 +31,8 @@ public: TextPathDiagnostics(const std::string& output, DiagnosticsEngine &diag) : OutputFile(output), Diag(diag) {} - void HandlePathDiagnosticImpl(const PathDiagnostic* D); - - void FlushDiagnostics(SmallVectorImpl *FilesMade) { } + void FlushDiagnosticsImpl(std::vector &Diags, + SmallVectorImpl *FilesMade); virtual StringRef getName() const { return "TextPathDiagnostics"; @@ -53,18 +52,17 @@ ento::createTextPathDiagnosticConsumer(const std::string& out, return new TextPathDiagnostics(out, PP.getDiagnostics()); } -void TextPathDiagnostics::HandlePathDiagnosticImpl(const PathDiagnostic* D) { - if (!D) - return; - - if (D->empty()) { - delete D; - return; - } - - for (PathDiagnostic::const_iterator I=D->begin(), E=D->end(); I != E; ++I) { - unsigned diagID = Diag.getDiagnosticIDs()->getCustomDiagID( - DiagnosticIDs::Note, I->getString()); - Diag.Report(I->getLocation().asLocation(), diagID); +void TextPathDiagnostics::FlushDiagnosticsImpl( + std::vector &Diags, + SmallVectorImpl *FilesMade) { + for (std::vector::iterator it = Diags.begin(), + et = Diags.end(); it != et; ++it) { + const PathDiagnostic *D = *it; + for (PathDiagnostic::const_iterator I=D->begin(), E=D->end(); I != E; ++I) { + unsigned diagID = + Diag.getDiagnosticIDs()->getCustomDiagID(DiagnosticIDs::Note, + I->getString()); + Diag.Report(I->getLocation().asLocation(), diagID); + } } } diff --git a/test/Analysis/inline-unique-reports.c b/test/Analysis/inline-unique-reports.c new file mode 100644 index 0000000000..15b1e0b124 --- /dev/null +++ b/test/Analysis/inline-unique-reports.c @@ -0,0 +1,139 @@ +// RUN: %clang --analyze %s -Xclang -analyzer-inline-call -o %t > /dev/null 2>&1 +// RUN: FileCheck -input-file %t %s + +static inline bug(int *p) { + *p = 0xDEADBEEF; +} + +void test_bug_1() { + int *p = 0; + bug(p); +} + +void test_bug_2() { + int *p = 0; + bug(p); +} + +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: files +// CHECK: +// CHECK: +// CHECK: diagnostics +// CHECK: +// CHECK: +// CHECK: path +// CHECK: +// CHECK: +// CHECK: kindcontrol +// CHECK: edges +// CHECK: +// CHECK: +// CHECK: start +// CHECK: +// CHECK: +// CHECK: line5 +// CHECK: col3 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: line5 +// CHECK: col3 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: end +// CHECK: +// CHECK: +// CHECK: line9 +// CHECK: col12 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: line9 +// CHECK: col12 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: kindcontrol +// CHECK: edges +// CHECK: +// CHECK: +// CHECK: start +// CHECK: +// CHECK: +// CHECK: line9 +// CHECK: col12 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: line9 +// CHECK: col12 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: end +// CHECK: +// CHECK: +// CHECK: line5 +// CHECK: col3 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: line5 +// CHECK: col4 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: kindevent +// CHECK: location +// CHECK: +// CHECK: line5 +// CHECK: col3 +// CHECK: file0 +// CHECK: +// CHECK: ranges +// CHECK: +// CHECK: +// CHECK: +// CHECK: line5 +// CHECK: col4 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: line5 +// CHECK: col4 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: +// CHECK: extended_message +// CHECK: Dereference of null pointer (loaded from variable 'p') +// CHECK: message +// CHECK: Dereference of null pointer (loaded from variable 'p') +// CHECK: +// CHECK: +// CHECK: descriptionDereference of null pointer (loaded from variable 'p') +// CHECK: categoryLogic error +// CHECK: typeDereference of null pointer +// CHECK: location +// CHECK: +// CHECK: line5 +// CHECK: col3 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: