[cfe-commits] r166728 - in /cfe/trunk: include/clang/StaticAnalyzer/Core/BugReporter/ lib/StaticAnalyzer/Core/ test/Analysis/ test/Analysis/diagnostics/ test/Analysis/inlining/

[Jordan Rose]

On Oct 25, 2012, at 15:07 , Ted Kremenek <kremenek at apple.com> 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;