On Oct 25, 2012, at 15:07 , Ted Kremenek <[email protected]> wrote:
> Author: kremenek > Date: Thu Oct 25 17:07:10 2012 > New Revision: 166728 > > URL: http://llvm.org/viewvc/llvm-project?rev=166728&view=rev > Log: > TrackConstraintBRVisitor and ConditionBRVisitor can emit similar > path notes for cases where a value may be assumed to be null, etc. > Instead of having redundant diagnostics, do a pass over the generated > PathDiagnostic pieces and remove notes from TrackConstraintBRVisitor > that are already covered by ConditionBRVisitor, whose notes tend > to be better. > > Fixes <rdar://problem/12252783> > > Modified: > > cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h > cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h > cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp > cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp > cfe/trunk/test/Analysis/conditional-operator-path-notes.c > cfe/trunk/test/Analysis/diagnostics/deref-track-symbolic-region.cpp > cfe/trunk/test/Analysis/inline-plist.c > cfe/trunk/test/Analysis/inlining/path-notes.c > cfe/trunk/test/Analysis/method-call-path-notes.cpp > cfe/trunk/test/Analysis/null-deref-path-notes.m > cfe/trunk/test/Analysis/plist-output-alternate.m > cfe/trunk/test/Analysis/plist-output.m > cfe/trunk/test/Analysis/retain-release-path-notes.m > > Modified: > cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h?rev=166728&r1=166727&r2=166728&view=diff > ============================================================================== > --- > cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h > (original) > +++ > cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h > Thu Oct 25 17:07:10 2012 > @@ -141,6 +141,10 @@ > > void Profile(llvm::FoldingSetNodeID &ID) const; > > + /// Return the tag associated with this visitor. This tag will be used > + /// to make all PathDiagnosticPieces created by this visitor. > + static const char *getTag(); > + > PathDiagnosticPiece *VisitNode(const ExplodedNode *N, > const ExplodedNode *PrevN, > BugReporterContext &BRC, > @@ -170,6 +174,9 @@ > ID.AddPointer(&x); > } > > + /// Return the tag associated with this visitor. This tag will be used > + /// to make all PathDiagnosticPieces created by this visitor. > + static const char *getTag(); > > virtual PathDiagnosticPiece *VisitNode(const ExplodedNode *N, > const ExplodedNode *Prev, > > Modified: > cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h?rev=166728&r1=166727&r2=166728&view=diff > ============================================================================== > --- cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h > (original) > +++ cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h > Thu Oct 25 17:07:10 2012 > @@ -320,6 +320,13 @@ > const std::string str; > const Kind kind; > const DisplayHint Hint; > + > + /// A constant string that can be used to tag the PathDiagnosticPiece, > + /// typically with the identification of the creator. The actual pointer > + /// value is meant to be an identifier; the string itself is useful for > + /// debugging. > + StringRef Tag; > + > std::vector<SourceRange> ranges; > > PathDiagnosticPiece() LLVM_DELETED_FUNCTION; > @@ -336,6 +343,16 @@ > > llvm::StringRef getString() const { return str; } > > + /// Tag this PathDiagnosticPiece with the given C-string. > + void setTag(const char *tag) { Tag = tag; } > + > + /// Return the opaque tag (if any) on the PathDiagnosticPiece. > + const void *getTag() const { return Tag.data(); } > + > + /// Return the string representation of the tag. This is useful > + /// for debugging. > + StringRef getTagStr() const { return Tag; } > + > /// getDisplayHint - Return a hint indicating where the diagnostic should > /// be displayed by the PathDiagnosticConsumer. > DisplayHint getDisplayHint() const { return Hint; } > > Modified: cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp?rev=166728&r1=166727&r2=166728&view=diff > ============================================================================== > --- cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp (original) > +++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp Thu Oct 25 17:07:10 2012 > @@ -118,6 +118,68 @@ > // Diagnostic cleanup. > //===----------------------------------------------------------------------===// > > +static PathDiagnosticEventPiece * > +eventsDescribeSameCondition(PathDiagnosticEventPiece *X, > + PathDiagnosticEventPiece *Y) { > + // Prefer diagnostics that come from ConditionBRVisitor over > + // those that came from TrackConstraintBRVisitor. > + const void *tagPreferred = ConditionBRVisitor::getTag(); > + const void *tagLesser = TrackConstraintBRVisitor::getTag(); > + > + if (X->getLocation() != Y->getLocation()) > + return 0; > + > + if (X->getTag() == tagPreferred && Y->getTag() == tagLesser) > + return X; > + > + if (Y->getTag() == tagPreferred && X->getTag() == tagLesser) > + return Y; > + > + return 0; > +} > + > +static void RemoveRedundantMsgs(PathPieces &path) { New LLVM coding conventions would have this be lowerCamelCase. > + unsigned N = path.size(); > + if (N < 2) > + return; > + for (unsigned i = 0; i < N; ++i) { I would appreciate a comment reminding maintainers NOT to switch this to an iterator-based loop, since not only is PathPieces modified during the loop, but the path never actually "reaches the end" (since things get pushed on the back). > + IntrusiveRefCntPtr<PathDiagnosticPiece> piece(path.front()); > + path.pop_front(); > + > + switch (piece->getKind()) { > + case clang::ento::PathDiagnosticPiece::Call: > + RemoveRedundantMsgs(cast<PathDiagnosticCallPiece>(piece)->path); > + break; > + case clang::ento::PathDiagnosticPiece::Macro: > + > RemoveRedundantMsgs(cast<PathDiagnosticMacroPiece>(piece)->subPieces); > + break; > + case clang::ento::PathDiagnosticPiece::ControlFlow: > + break; > + case clang::ento::PathDiagnosticPiece::Event: { > + if (i == N-1) > + break; > + > + if (PathDiagnosticEventPiece *nextEvent = > + dyn_cast<PathDiagnosticEventPiece>(path.front().getPtr())) { > + PathDiagnosticEventPiece *event = > + cast<PathDiagnosticEventPiece>(piece); > + // Check to see if we should keep one of the two pieces. If we > + // come up with a preference, record which piece to keep, and > consume > + // another piece from the path. > + if (PathDiagnosticEventPiece *pieceToKeep = > + eventsDescribeSameCondition(event, nextEvent)) { > + piece = pieceToKeep; > + path.pop_front(); > + ++i; > + } > + } > + break; > + } > + } > + path.push_back(piece); > + } > +} > + > /// Recursively scan through a path and prune out calls and macros pieces > /// that aren't needed. Return true if afterwards the path contains > /// "interesting stuff" which means it should be pruned from the parent path. > @@ -2016,10 +2078,16 @@ > } while(finalReportConfigToken != originalReportConfigToken); > > // Finally, prune the diagnostic path of uninteresting stuff. > - if (!PD.path.empty() && R->shouldPrunePath()) { > - bool hasSomethingInteresting = RemoveUneededCalls(PD.getMutablePieces(), > R); > - assert(hasSomethingInteresting); > - (void) hasSomethingInteresting; > + if (!PD.path.empty()) { > + // Remove messages that are basically the same. > + RemoveRedundantMsgs(PD.getMutablePieces()); > + > + if (R->shouldPrunePath()) { > + bool hasSomethingInteresting = > RemoveUneededCalls(PD.getMutablePieces(), > + R); > + assert(hasSomethingInteresting); > + (void) hasSomethingInteresting; > + } It's probably cheaper to do this in the other order. > } > > return true; _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
