[analyzer] Extend bug reports with extra notes

These diagnostics are separate from the path-sensitive engine's path notes,
and can be added manually on top of path-sensitive or path-insensitive reports.

The new note diagnostics would appear as note:-diagnostic on console and
as blue bubbles in scan-build. In plist files they currently do not appear,
because format needs to be discussed with plist file users.

The analyzer option "-analyzer-config notes-as-events=true" would convert
notes to normal path notes, and put them at the beginning of the path.
This is a temporary hack to show the new notes in plist files.

A few checkers would be updated in subsequent commits,
including tests for this new feature.

Differential Revision: https://reviews.llvm.org/D24278

llvm-svn: 283092

Author: haoNoQ

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h

@@ -266,6 +266,9 @@ class AnalyzerOptions : public RefCountedBase<AnalyzerOptions> {
   /// \sa shouldWidenLoops
   Optional<bool> WidenLoops;
 
+  /// \sa shouldDisplayNotesAsEvents
+  Optional<bool> DisplayNotesAsEvents;
+
   /// A helper function that retrieves option for a given full-qualified
   /// checker name.
   /// Options for checkers can be specified via 'analyzer-config' command-line
@@ -534,6 +537,14 @@ class AnalyzerOptions : public RefCountedBase<AnalyzerOptions> {
   /// This is controlled by the 'widen-loops' config option.
   bool shouldWidenLoops();
 
+  /// Returns true if the bug reporter should transparently treat extra note
+  /// diagnostic pieces as event diagnostic pieces. Useful when the diagnostic
+  /// consumer doesn't support the extra note pieces.
+  ///
+  /// This is controlled by the 'notes-as-events' option, which defaults
+  /// to false when unset.
+  bool shouldDisplayNotesAsEvents();
+
 public:
   AnalyzerOptions() :
     AnalysisStoreOpt(RegionStoreModel),

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h

@@ -66,6 +66,8 @@ class BugReport : public llvm::ilist_node<BugReport> {
   typedef SmallVector<std::unique_ptr<BugReporterVisitor>, 8> VisitorList;
   typedef VisitorList::iterator visitor_iterator;
   typedef SmallVector<StringRef, 2> ExtraTextList;
+  typedef SmallVector<llvm::IntrusiveRefCntPtr<PathDiagnosticNotePiece>, 4>
+      NoteList;
 
 protected:
   friend class BugReporter;
@@ -82,7 +84,8 @@ class BugReport : public llvm::ilist_node<BugReport> {
   const ExplodedNode *ErrorNode;
   SmallVector<SourceRange, 4> Ranges;
   ExtraTextList ExtraText;
-  
+  NoteList Notes;
+
   typedef llvm::DenseSet<SymbolRef> Symbols;
   typedef llvm::DenseSet<const MemRegion *> Regions;
 
@@ -177,6 +180,18 @@ class BugReport : public llvm::ilist_node<BugReport> {
   const BugType& getBugType() const { return BT; }
   BugType& getBugType() { return BT; }
 
+  /// \brief True when the report has an execution path associated with it.
+  ///
+  /// A report is said to be path-sensitive if it was thrown against a
+  /// particular exploded node in the path-sensitive analysis graph.
+  /// Path-sensitive reports have their intermediate path diagnostics
+  /// auto-generated, perhaps with the help of checker-defined visitors,
+  /// and may contain extra notes.
+  /// Path-insensitive reports consist only of a single warning message
+  /// in a specific location, and perhaps extra notes.
+  /// Path-sensitive checkers are allowed to throw path-insensitive reports.
+  bool isPathSensitive() const { return ErrorNode != nullptr; }
+
   const ExplodedNode *getErrorNode() const { return ErrorNode; }
 
   StringRef getDescription() const { return Description; }
@@ -245,7 +260,27 @@ class BugReport : public llvm::ilist_node<BugReport> {
   void setDeclWithIssue(const Decl *declWithIssue) {
     DeclWithIssue = declWithIssue;
   }
-  
+
+  /// Add new item to the list of additional notes that need to be attached to
+  /// this path-insensitive report. If you want to add extra notes to a
+  /// path-sensitive report, you need to use a BugReporterVisitor because it
+  /// allows you to specify where exactly in the auto-generated path diagnostic
+  /// the extra note should appear.
+  void addNote(StringRef Msg, const PathDiagnosticLocation &Pos,
+                    ArrayRef<SourceRange> Ranges = {}) {
+    PathDiagnosticNotePiece *P =
+        new PathDiagnosticNotePiece(Pos, Msg);
+
+    for (const auto &R : Ranges)
+      P->addRange(R);
+
+    Notes.push_back(P);
+  }
+
+  virtual const NoteList &getNotes() {
+    return Notes;
+  }
+
   /// \brief This allows for addition of meta data to the diagnostic.
   ///
   /// Currently, only the HTMLDiagnosticClient knows how to display it. 

clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h

@@ -336,7 +336,7 @@ class PathDiagnosticLocationPair {
 
 class PathDiagnosticPiece : public RefCountedBaseVPTR {
 public:
-  enum Kind { ControlFlow, Event, Macro, Call };
+  enum Kind { ControlFlow, Event, Macro, Call, Note };
   enum DisplayHint { Above, Below };
 
 private:
@@ -452,7 +452,8 @@ class PathDiagnosticSpotPiece : public PathDiagnosticPiece {
   void Profile(llvm::FoldingSetNodeID &ID) const override;
 
   static bool classof(const PathDiagnosticPiece *P) {
-    return P->getKind() == Event || P->getKind() == Macro;
+    return P->getKind() == Event || P->getKind() == Macro ||
+           P->getKind() == Note;
   }
 };
 
@@ -710,6 +711,23 @@ class PathDiagnosticMacroPiece : public PathDiagnosticSpotPiece {
   void Profile(llvm::FoldingSetNodeID &ID) const override;
 };
 
+class PathDiagnosticNotePiece: public PathDiagnosticSpotPiece {
+public:
+  PathDiagnosticNotePiece(const PathDiagnosticLocation &Pos, StringRef S,
+                               bool AddPosRange = true)
+      : PathDiagnosticSpotPiece(Pos, S, Note, AddPosRange) {}
+
+  ~PathDiagnosticNotePiece() override;
+
+  static inline bool classof(const PathDiagnosticPiece *P) {
+    return P->getKind() == Note;
+  }
+
+  void dump() const override;
+
+  void Profile(llvm::FoldingSetNodeID &ID) const override;
+};
+
 /// PathDiagnostic - PathDiagnostic objects represent a single path-sensitive
 ///  diagnostic.  It represents an ordered-collection of PathDiagnosticPieces,
 ///  each which represent the pieces of the path.

clang/lib/Rewrite/HTMLRewrite.cpp

@@ -324,6 +324,7 @@ void html::AddHeaderFooterInternalBuiltinCSS(Rewriter &R, FileID FID,
       " .msgT { padding:0x; spacing:0x }\n"
       " .msgEvent { background-color:#fff8b4; color:#000000 }\n"
       " .msgControl { background-color:#bbbbbb; color:#000000 }\n"
+      " .msgNote { background-color:#ddeeff; color:#000000 }\n"
       " .mrange { background-color:#dfddf3 }\n"
       " .mrange { border-bottom:1px solid #6F9DBE }\n"
       " .PathIndex { font-weight: bold; padding:0px 5px; "
@@ -343,8 +344,12 @@ void html::AddHeaderFooterInternalBuiltinCSS(Rewriter &R, FileID FID,
       "   border-collapse: collapse; border-spacing: 0px;\n"
       " }\n"
       " td.rowname {\n"
-      "   text-align:right; font-weight:bold; color:#444444;\n"
-      "   padding-right:2ex; }\n"
+      "   text-align: right;\n"
+      "   vertical-align: top;\n"
+      "   font-weight: bold;\n"
+      "   color:#444444;\n"
+      "   padding-right:2ex;\n"
+      " }\n"
       "</style>\n</head>\n<body>";
 
   // Generate header

clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp

@@ -344,3 +344,10 @@ bool AnalyzerOptions::shouldWidenLoops() {
     WidenLoops = getBooleanOption("widen-loops", /*Default=*/false);
   return WidenLoops.getValue();
 }
+
+bool AnalyzerOptions::shouldDisplayNotesAsEvents() {
+  if (!DisplayNotesAsEvents.hasValue())
+    DisplayNotesAsEvents =
+        getBooleanOption("notes-as-events", /*Default=*/false);
+  return DisplayNotesAsEvents.getValue();
+}

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

@@ -112,15 +112,15 @@ static void removeRedundantMsgs(PathPieces &path) {
     path.pop_front();
 
     switch (piece->getKind()) {
-      case clang::ento::PathDiagnosticPiece::Call:
+      case PathDiagnosticPiece::Call:
         removeRedundantMsgs(cast<PathDiagnosticCallPiece>(piece)->path);
         break;
-      case clang::ento::PathDiagnosticPiece::Macro:
+      case PathDiagnosticPiece::Macro:
         removeRedundantMsgs(cast<PathDiagnosticMacroPiece>(piece)->subPieces);
         break;
-      case clang::ento::PathDiagnosticPiece::ControlFlow:
+      case PathDiagnosticPiece::ControlFlow:
         break;
-      case clang::ento::PathDiagnosticPiece::Event: {
+      case PathDiagnosticPiece::Event: {
         if (i == N-1)
           break;
 
@@ -140,6 +140,8 @@ static void removeRedundantMsgs(PathPieces &path) {
         }
         break;
       }
+      case PathDiagnosticPiece::Note:
+        break;
     }
     path.push_back(piece);
   }
@@ -197,6 +199,9 @@ static bool removeUnneededCalls(PathPieces &pieces, BugReport *R,
       }
       case PathDiagnosticPiece::ControlFlow:
         break;
+
+      case PathDiagnosticPiece::Note:
+        break;
     }
 
     pieces.push_back(piece);
@@ -3403,25 +3408,28 @@ void BugReporter::FlushReport(BugReport *exampleReport,
       exampleReport->getUniqueingLocation(),
       exampleReport->getUniqueingDecl()));
 
-  MaxBugClassSize = std::max(bugReports.size(),
-                             static_cast<size_t>(MaxBugClassSize));
+  if (exampleReport->isPathSensitive()) {
+    // Generate the full path diagnostic, using the generation scheme
+    // specified by the PathDiagnosticConsumer. Note that we have to generate
+    // path diagnostics even for consumers which do not support paths, because
+    // the BugReporterVisitors may mark this bug as a false positive.
+    assert(!bugReports.empty());
+
+    MaxBugClassSize =
+        std::max(bugReports.size(), static_cast<size_t>(MaxBugClassSize));
 
-  // Generate the full path diagnostic, using the generation scheme
-  // specified by the PathDiagnosticConsumer. Note that we have to generate
-  // path diagnostics even for consumers which do not support paths, because
-  // the BugReporterVisitors may mark this bug as a false positive.
-  if (!bugReports.empty())
     if (!generatePathDiagnostic(*D.get(), PD, bugReports))
       return;
 
-  MaxValidBugClassSize = std::max(bugReports.size(),
-                                  static_cast<size_t>(MaxValidBugClassSize));
+    MaxValidBugClassSize =
+        std::max(bugReports.size(), static_cast<size_t>(MaxValidBugClassSize));
 
-  // Examine the report and see if the last piece is in a header. Reset the
-  // report location to the last piece in the main source file.
-  AnalyzerOptions& Opts = getAnalyzerOptions();
-  if (Opts.shouldReportIssuesInMainSourceFile() && !Opts.AnalyzeAll)
-    D->resetDiagnosticLocationToMainFile();
+    // Examine the report and see if the last piece is in a header. Reset the
+    // report location to the last piece in the main source file.
+    AnalyzerOptions &Opts = getAnalyzerOptions();
+    if (Opts.shouldReportIssuesInMainSourceFile() && !Opts.AnalyzeAll)
+      D->resetDiagnosticLocationToMainFile();
+  }
 
   // If the path is empty, generate a single step path with the location
   // of the issue.
@@ -3434,6 +3442,27 @@ void BugReporter::FlushReport(BugReport *exampleReport,
     D->setEndOfPath(std::move(piece));
   }
 
+  PathPieces &Pieces = D->getMutablePieces();
+  if (getAnalyzerOptions().shouldDisplayNotesAsEvents()) {
+    // For path diagnostic consumers that don't support extra notes,
+    // we may optionally convert those to path notes.
+    for (auto I = exampleReport->getNotes().rbegin(),
+              E = exampleReport->getNotes().rend(); I != E; ++I) {
+      PathDiagnosticNotePiece *Piece = I->get();
+      PathDiagnosticEventPiece *ConvertedPiece =
+          new PathDiagnosticEventPiece(Piece->getLocation(),
+                                       Piece->getString());
+      for (const auto &R: Piece->getRanges())
+        ConvertedPiece->addRange(R);
+
+      Pieces.push_front(ConvertedPiece);
+    }
+  } else {
+    for (auto I = exampleReport->getNotes().rbegin(),
+              E = exampleReport->getNotes().rend(); I != E; ++I)
+      Pieces.push_front(*I);
+  }
+
   // Get the meta data.
   const BugReport::ExtraTextList &Meta = exampleReport->getExtraText();
   for (BugReport::ExtraTextList::const_iterator i = Meta.begin(),
@@ -3518,6 +3547,13 @@ LLVM_DUMP_METHOD void PathDiagnosticMacroPiece::dump() const {
   // FIXME: Print which macro is being invoked.
 }
 
+LLVM_DUMP_METHOD void PathDiagnosticNotePiece::dump() const {
+  llvm::errs() << "NOTE\n--------------\n";
+  llvm::errs() << getString() << "\n";
+  llvm::errs() << " ---- at ----\n";
+  getLocation().dump();
+}
+
 LLVM_DUMP_METHOD void PathDiagnosticLocation::dump() const {
   if (!isValid()) {
     llvm::errs() << "<INVALID>\n";