[analyzer] Extend bug reports with extra notes.

[NoQ]

Summary

This patch allows injecting extra note-like diagnostics into bug reports, which are separate from path diagnostics. Previously, we could only display single-piece warnings and auto-generated path diagnostics; the latter could have been extended with the means of bug reporter visitors, however diagnostics injected by the visitors were still tied to the execution paths.

The primary purpose of this patch is to get the Clone checker integrated into the clang static analyzer's bug reporter mechanism. It is absolutely vital to this checker to display a warning that refers to multiple source ranges (code clones). This patch properly integrates the Clone checker, so that its warnings were visible in the analyzer GUIs such as scan-view. Actual integration with CloneChecker moved to D24916.

Other checkers, even path-sensitive ones, could also benefit from the ability to add extra diagnostic pieces that are separate from path events. For example, if a bug report refers to a declaration of a class member, it is a good idea to highlight the relevant declaration, which may be far away from the execution path. This patch applies the new technique to the ObjCDealloc checker. Actual integration with ObjCDeallocChecker moved to D24915. Here we also see how the new extra notes interact with other path pieces.

HTML diagnostic consumer (-analyzer-output=html) is updated to use the new technique. Extra notes appear as blue planks (unlike the yellow path event planks and grey control flow planks), they are not numbered, and cannot be navigated through arrows, but they are included in the header. This represents the idea of how extra notes should ideally look like. The color is, of course, is chosen arbitrarily. Here are some examples of the newly generated HTML reports:

clone-checker-normal.html6 KBDownload

clone-checker-notes-as-events.html6 KBDownload

objc-dealloc-normal.html16 KBDownload

objc-dealloc-notes-as-events.html16 KBDownload

Null diagnostic consumer (with -analyzer-output unspecified) displays extra notes as "note:" diagnostics. It makes testing extra note pieces easier. Note that normal path notes are not displayed at this mode, so i'm not absolutely sure this is the right decision; however, this lets clone checker tests work without changes.

Text diagnostic consumer (with -analyzer-output=text) displays extra notes as "note:" diagnostics before the path diagnostics.

Plist diagnostic consumer was not updated yet(!) Currently, it ignores extra notes. This is because it is important to agree upon the format, so this decision will be delayed. Feedback on this matter is welcome :)

An option is added, -analyzer-config extra-notes-as-events=true (defaults to false), which converts extra notes to path events before sending to any diagnostic consumer. This allows to see the newly added notes in the Plist diagnostic consumer and display them without any changes to the viewer application (such as Xcode), however the looks would probably not be ideal or user-friendly (see also "HTML diagnostic consumer" above). The extra notes are put at the beginning, before any path events.

I slightly fixed the clone checker diagnostics. In particular, diagnostics cannot end with a period, and i capitalized the first letter. Also made slight changes to the suspicious clone structure, because path pieces require more than source ranges to operate (didn't look why though). See D24916 for more updates on the CloneChecker.

This patch includes a minor refactoring in FlushReport(), which introduces the BugReport::isPathSensitive() method and uses it. This should have no functional change, however it adds a stronger assertion, and, most importantly, this refactoring was useful for me to understand this piece of code and realize that i'm putting my code in the right place, so i decided that i should keep it.

[v.g.vassilev]

Thanks for working on this!

On my browser the note "Detected code clone" note appears slightly off the highlighted range which was a bit confusing to me.

Given my limited understanding of the SA bug reports, this looks good to me.

lib/StaticAnalyzer/Checkers/CloneChecker.cpp

83 ↗

(On Diff #70457)

Probably an indent here would make this look more consistent.

lib/StaticAnalyzer/Core/AnalyzerOptions.cpp

348

Extra new line?

[a.sidorin]

This definitely seems to be useful. However, this patch is pretty big. Some of its parts are not directly related with the feature being introduced (for example, changes for copypaste/sub-sequences.cpp). Is it possible to split this patch? Moreover, as I understand, you may not even need a review for refactoring-only changes. Or you can make a review for them which will be done much faster.
There is a temporary dump of some stylish comments after brief look.

lib/StaticAnalyzer/Core/BugReporter.cpp

3449

Reverse iteration on array and push the corresponding element to the front (always O(n)) seems a bit strange to me. I suggest using a combination of resize + move existing elements to the end + copy/transform from start. What do you think? Or am I missing something?

lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp

230

const auto &Piece : path?

lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

117

Range-for loop may look nicer here. What do you think?

119

if (isa<...>()) {

...
...

}
?

[bcraig]

Neat! I would have liked to have had this for the Excess Padding Checker. Currently, the padding checker has a very long diagnostic that recommends a new order for data members. I think a note (or fixit) would be more appropriate, but that support didn't exist previously.

[NoQ]

Thanks!

I could have split this up into three patches (one for the core and two patches for the checkers), however that'd mean that the first patch comes without tests; so i thought that the patch should be self-contained. Was it a bad idea after all?

lib/StaticAnalyzer/Core/BugReporter.cpp

3449

PathPieces is an std::list, so each push is O(1).

[zaks.anna]

Thanks!

Looks good overall. Several comments below.

lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp

160

What do you think about calling these PathDiagnosticNotePiece, specifically, looks like "Extra" does not add anything.

You'll probably need to change quite a few names to remove the "Extra", so up to you.

170

Why we need the second path? Please, add a comment as it's not obvious.

lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

119

@a.sidorin, LLVM coding guidelines prefer early exits http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code.

test/Analysis/copypaste/plist-diagnostics.cpp

4 ↗

(On Diff #70457)

We should call out that this is not working as expected right now. As far as I understand, this file is a test case for a FIXME, correct?

test/Analysis/copypaste/text-diagnostics.cpp

5 ↗

(On Diff #70457)

It's better to use full sentences for the warning messages: "Clone of this code is detected" (or "Clones of this code are detected" when there are more than one).

12 ↗

(On Diff #70457)

I would just say: "Code clone here". When I think about it, I assume the very first one is not a clone, but the ones we highlight with notes are clones.

[dcoughlin]

This is awesome! I have some minor comments on the dealloc notes. It is also fine to remove them entirely; we can add them in a later commit.

lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp

604 ↗

(On Diff #70457)

To be consistent with the rest of clang, I think it is better to remove the "The". That is: "Property is declared here" and "Property is synthesized here".

733 ↗

(On Diff #70457)

Is it possible to factor this out so we only have the hard-coded note text for each of "declared"/"synthesized" in one spot? This will make it easier to update the text if we decide to change it.

[NoQ]

This diff is a part of the original diff that contains core changes without checker changes (in particular, without tests). No changes were made, no comments addressed, just removed checker stuff. Uploading it separately so that it was easier to track changes in phabricator.

Actually improved diff coming soon.
Separate reviews for the checkers coming soon.

[NoQ]

Addressed inline comments. Renamed extra notes to notes.

[zaks.anna]

I have no further comments.

include/clang/StaticAnalyzer/Core/AnalyzerOptions.h

private:
   Optional<unsigned> MaxNodesPerTopLevelFunction;
 
   /// \sa shouldInlineLambdas
   Optional<bool> InlineLambdas;
 
   /// \sa shouldWidenLoops
   Optional<bool> WidenLoops;
 
+  /// \sa shouldDisplayExtraNotesAsEvents
+  Optional<bool> DisplayExtraNotesAsEvents;
+
   /// A helper function that retrieves option for a given full-qualified
   /// checker name.
   /// Options for checkers can be specified via 'analyzer-config' command-line
   /// option.
   /// Example:
   /// @code-analyzer-config unix.Malloc:OptionName=CheckerOptionValue @endcode
   /// or @code-analyzer-config unix:OptionName=GroupOptionValue @endcode
   /// for groups of checkers.
public:
   /// Returns true if lambdas should be inlined. Otherwise a sink node will be
   /// generated each time a LambdaExpr is visited.
   bool shouldInlineLambdas();
 
   /// Returns true if the analysis should try to widen loops.
   /// 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 'extra-notes-as-events' option, which defaults
+  /// to false when unset.
+  bool shouldDisplayExtraNotesAsEvents();
+
 public:
   AnalyzerOptions() :
     AnalysisStoreOpt(RegionStoreModel),
     AnalysisConstraintsOpt(RangeConstraintsModel),
     AnalysisDiagOpt(PD_HTML),
     AnalysisPurgeOpt(PurgeStmt),
     DisableAllChecks(0),
     ShowCheckerHelp(0),

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

public:
     virtual const ExplodedNode*
             getOriginalNode(const ExplodedNode *N) = 0;
   };
 
   typedef const SourceRange *ranges_iterator;
   typedef SmallVector<std::unique_ptr<BugReporterVisitor>, 8> VisitorList;
   typedef VisitorList::iterator visitor_iterator;
   typedef SmallVector<StringRef, 2> ExtraTextList;
+  typedef SmallVector<llvm::IntrusiveRefCntPtr<PathDiagnosticExtraNotePiece>, 4>
+      ExtraNoteList;
 
 protected:
   friend class BugReporter;
   friend class BugReportEquivClass;
 
   BugType& BT;
   const Decl *DeclWithIssue;
   std::string ShortDescription;
   std::string Description;
   PathDiagnosticLocation Location;
   PathDiagnosticLocation UniqueingLocation;
   const Decl *UniqueingDecl;
   
   const ExplodedNode *ErrorNode;
   SmallVector<SourceRange, 4> Ranges;
   ExtraTextList ExtraText;
-  
+  ExtraNoteList ExtraNotes;
   typedef llvm::DenseSet<SymbolRef> Symbols;
   typedef llvm::DenseSet<const MemRegion *> Regions;
 
   /// A (stack of) a set of symbols that are registered with this
   /// report as being "interesting", and thus used to help decide which
   /// diagnostics to include when constructing the final path diagnostic.
   /// The stack is largely used by BugReporter when generating PathDiagnostics
   /// for multiple PathDiagnosticConsumers.
: BT(bt), DeclWithIssue(nullptr), Description(desc),
       ErrorNode(errornode), ConfigurationChangeToken(0),
       DoNotPrunePath(false) {}
 
   virtual ~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; }
 
   StringRef getShortDescription(bool UseFallback = true) const {
     if (ShortDescription.empty() && UseFallback)
       return Description;
     return ShortDescription;
public:
   /// this issue semantically occurred.
   const Decl *getDeclWithIssue() const;
   
   /// Specifically set the Decl where an issue occurred.  This isn't necessary
   /// for BugReports that cover a path as it will be automatically inferred.
   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 addExtraNote(StringRef Msg, const PathDiagnosticLocation &Pos,
+                    ArrayRef<SourceRange> Ranges = {}) {
+    PathDiagnosticExtraNotePiece *P =
+        new PathDiagnosticExtraNotePiece(Pos, Msg);
+
+    for (const auto &R : Ranges)
+      P->addRange(R);
+
+    ExtraNotes.push_back(P);
+  }
+
+  virtual const ExtraNoteList &getExtraNotes() {
+    return ExtraNotes;
+  }
+
   /// \brief This allows for addition of meta data to the diagnostic.
   ///
   /// Currently, only the HTMLDiagnosticClient knows how to display it. 
   void addExtraText(StringRef S) {
     ExtraText.push_back(S);
   }
 
   virtual const ExtraTextList &getExtraText() {

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

 };
 
 //===----------------------------------------------------------------------===//
 // Path "pieces" for path-sensitive diagnostics.
 //===----------------------------------------------------------------------===//
 
 class PathDiagnosticPiece : public RefCountedBaseVPTR {
 public:
-  enum Kind { ControlFlow, Event, Macro, Call };
+  enum Kind { ControlFlow, Event, Macro, Call, ExtraNote };
   enum DisplayHint { Above, Below };
 
 private:
   const std::string str;
   const Kind kind;
   const DisplayHint Hint;
 
   /// \brief In the containing bug report, this piece is the last piece from
public:
   }
 
   PathDiagnosticLocation getLocation() const override { return Pos; }
   void flattenLocations() override { Pos.flatten(); }
 
   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() == ExtraNote;
   }
 };
 
 /// \brief Interface for classes constructing Stack hints.
 ///
 /// If a PathDiagnosticEvent occurs in a different frame than the final 
 /// diagnostic the hints can be used to summarize the effect of the call.
 class StackHintGenerator {
static inline bool classof(const PathDiagnosticPiece *P) {
     return P->getKind() == Macro;
   }
 
   void dump() const override;
 
   void Profile(llvm::FoldingSetNodeID &ID) const override;
 };
 
+class PathDiagnosticExtraNotePiece: public PathDiagnosticSpotPiece {
+public:
+  PathDiagnosticExtraNotePiece(const PathDiagnosticLocation &Pos, StringRef S,
+                               bool AddPosRange = true)
+      : PathDiagnosticSpotPiece(Pos, S, ExtraNote, AddPosRange) {}
+
+  ~PathDiagnosticExtraNotePiece() override;
+
+  static inline bool classof(const PathDiagnosticPiece *P) {
+    return P->getKind() == ExtraNote;
+  }
+
+  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.
 class PathDiagnostic : public llvm::FoldingSetNode {
   std::string CheckName;
   const Decl *DeclWithIssue;
   std::string BugType;
   std::string VerboseDesc;

lib/Rewrite/HTMLRewrite.cpp

os << "<style type=\"text/css\">\n"
       " .msg { float:left }\n"
       " .msg { padding:0.25em 1ex 0.25em 1ex }\n"
       " .msg { margin-top:10px; margin-bottom:10px }\n"
       " .msg { font-weight:bold }\n"
       " .msg { max-width:60em; word-wrap: break-word; white-space: pre-wrap }\n"
       " .msgT { padding:0x; spacing:0x }\n"
       " .msgEvent { background-color:#fff8b4; color:#000000 }\n"
       " .msgControl { background-color:#bbbbbb; color:#000000 }\n"
+      " .msgExtraNote { 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; "
         "margin-right:5px; }\n"
       " .PathIndex { -webkit-border-radius:8px }\n"
       " .PathIndexEvent { background-color:#bfba87 }\n"
       " .PathIndexControl { background-color:#8c8c8c }\n"
       " .PathNav a { text-decoration:none; font-size: larger }\n"
       " .CodeInsertionHint { font-weight: bold; background-color: #10dd10 }\n"
       " .CodeRemovalHint { background-color:#de1010 }\n"
       " .CodeRemovalHint { border-bottom:1px solid #6F9DBE }\n"
       " table.simpletable {\n"
       "   padding: 5px;\n"
       "   font-size:12pt;\n"
       "   margin:20px;\n"
       "   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
   R.InsertTextBefore(StartLoc, os.str());
   // Generate footer
 
   R.InsertTextAfter(EndLoc, "</body></html>\n");
 }

lib/StaticAnalyzer/Core/AnalyzerOptions.cpp

bool AnalyzerOptions::shouldInlineLambdas() {
   return InlineLambdas.getValue();
 }
 
 bool AnalyzerOptions::shouldWidenLoops() {
   if (!WidenLoops.hasValue())
     WidenLoops = getBooleanOption("widen-loops", /*Default=*/false);
   return WidenLoops.getValue();
 }
+
+
+bool AnalyzerOptions::shouldDisplayExtraNotesAsEvents() {
+  if (!DisplayExtraNotesAsEvents.hasValue())
+    DisplayExtraNotesAsEvents =
+        getBooleanOption("extra-notes-as-events", /*Default=*/false);
+  return DisplayExtraNotesAsEvents.getValue();
+}

lib/StaticAnalyzer/Core/BugReporter.cpp

static void removeRedundantMsgs(PathPieces &path) {
   // are streaming the path and modifying it in place.  This is done by
   // grabbing the front, processing it, and if we decide to keep it append
   // it to the end of the path.  The entire path is processed in this way.
   for (unsigned i = 0; i < N; ++i) {
     IntrusiveRefCntPtr<PathDiagnosticPiece> piece(path.front());
     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;
 
         if (PathDiagnosticEventPiece *nextEvent =
             dyn_cast<PathDiagnosticEventPiece>(path.front().get())) {
           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;
       }
+      case PathDiagnosticPiece::ExtraNote:
+        break;
     }
     path.push_back(piece);
   }
 }
 
 /// A map from PathDiagnosticPiece to the LocationContext of the inlined
 /// function call it represents.
 typedef llvm::DenseMap<const PathPieces *, const LocationContext *>
switch (piece->getKind()) {
 
         // We never throw away an event, but we do throw it away wholesale
         // as part of a path if we throw the entire path away.
         containsSomethingInteresting |= !event->isPrunable();
         break;
       }
       case PathDiagnosticPiece::ControlFlow:
         break;
+
+      case PathDiagnosticPiece::ExtraNote:
+        break;
     }
 
     pieces.push_back(piece);
   }
 
   return containsSomethingInteresting;
 }
 
void BugReporter::FlushReport(BugReport *exampleReport,
   std::unique_ptr<PathDiagnostic> D(new PathDiagnostic(
       exampleReport->getBugType().getCheckName(),
       exampleReport->getDeclWithIssue(), exampleReport->getBugType().getName(),
       exampleReport->getDescription(),
       exampleReport->getShortDescription(/*Fallback=*/false), BT.getCategory(),
       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.
-  if (!bugReports.empty())
+    assert(!bugReports.empty());
+
+    MaxBugClassSize =
+        std::max(bugReports.size(), static_cast<size_t>(MaxBugClassSize));
+
     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();
+    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.
   if (D->path.empty()) {
     PathDiagnosticLocation L = exampleReport->getLocation(getSourceManager());
     auto piece = llvm::make_unique<PathDiagnosticEventPiece>(
         L, exampleReport->getDescription());
     for (SourceRange Range : exampleReport->getRanges())
       piece->addRange(Range);
     D->setEndOfPath(std::move(piece));
   }
 
+  PathPieces &Pieces = D->getMutablePieces();
+  if (getAnalyzerOptions().shouldDisplayExtraNotesAsEvents()) {
+    // For path diagnostic consumers that don't support extra notes,
+    // we may optionally convert those to path notes.
+    for (auto I = exampleReport->getExtraNotes().rbegin(),
+              E = exampleReport->getExtraNotes().rend(); I != E; ++I) {
+      PathDiagnosticExtraNotePiece *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->getExtraNotes().rbegin(),
+              E = exampleReport->getExtraNotes().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(),
                                                 e = Meta.end(); i != e; ++i) {
     D->addMeta(*i);
   }
 
   PD.HandlePathDiagnostic(std::move(D));
LLVM_DUMP_METHOD void PathDiagnosticControlFlowPiece::dump() const {
   getEndLocation().dump();
 }
 
 LLVM_DUMP_METHOD void PathDiagnosticMacroPiece::dump() const {
   llvm::errs() << "MACRO\n--------------\n";
   // FIXME: Print which macro is being invoked.
 }
 
+LLVM_DUMP_METHOD void PathDiagnosticExtraNotePiece::dump() const {
+  llvm::errs() << "EXTRANOTE\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";
     return;
   }
 
   switch (K) {
   case RangeK:

lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp

if (const Decl *DeclWithIssue = D.getDeclWithIssue()) {
               SMgr.getExpansionLoc(path.back()->getLocation().asLocation()),
               SMgr);
           FullSourceLoc FunL(SMgr.getExpansionLoc(Body->getLocStart()), SMgr);
           offsetDecl = L.getExpansionLineNumber() - FunL.getExpansionLineNumber();
       }
   }
 
   // Process the path.
-  unsigned n = path.size();
-  unsigned max = n;
-
-  for (PathPieces::const_reverse_iterator I = path.rbegin(),
-       E = path.rend();
-        I != E; ++I, --n)
-    HandlePiece(R, FID, **I, n, max);
+  // Maintain the counts of extra note pieces separately.
+  unsigned TotalPieces = path.size();
+  unsigned TotalExtraPieces =
+      std::count_if(path.begin(), path.end(),
+                    [](const IntrusiveRefCntPtr<PathDiagnosticPiece> &p) {
+                      return isa<PathDiagnosticExtraNotePiece>(p.get());
+                    });
+
+  unsigned TotalRegularPieces = TotalPieces - TotalExtraPieces;
+  unsigned NumRegularPieces = TotalRegularPieces;
+  unsigned NumExtraPieces = TotalExtraPieces;
+
+  for (auto I = path.rbegin(), E = path.rend(); I != E; ++I) {
+    if (isa<PathDiagnosticExtraNotePiece>(I->get())) {
+      // Will also make a second pass through those later below,
+      // when the header text is ready.
+      HandlePiece(R, FID, **I, NumExtraPieces, TotalExtraPieces);
+      --NumExtraPieces;
+    } else {
+      HandlePiece(R, FID, **I, NumRegularPieces, TotalRegularPieces);
+      --NumRegularPieces;
+    }
+  }
 
   // Add line numbers, header, footer, etc.
 
   // unsigned FID = R.getSourceMgr().getMainFileID();
   html::EscapeText(R, FID);
   html::AddLineNumbers(R, FID);
 
   // If we have a preprocessor, relex the file and syntax highlight.
if (llvm::sys::path::is_relative(Entry->getName())) {
     llvm::sys::fs::current_path(DirName);
     DirName += '/';
   }
 
   int LineNumber = path.back()->getLocation().asLocation().getExpansionLineNumber();
   int ColumnNumber = path.back()->getLocation().asLocation().getExpansionColumnNumber();
 
   // Add the name of the file as an <h1> tag.
-
   {
     std::string s;
     llvm::raw_string_ostream os(s);
 
     os << "<!-- REPORTHEADER -->\n"
        << "<h3>Bug Summary</h3>\n<table class=\"simpletable\">\n"
           "<tr><td class=\"rowname\">File:</td><td>"
        << html::EscapeText(DirName)
        << html::EscapeText(Entry->getName())
-      << "</td></tr>\n<tr><td class=\"rowname\">Location:</td><td>"
+       << "</td></tr>\n<tr><td class=\"rowname\">Warning:</td><td>"
           "<a href=\"#EndPath\">line "
        << LineNumber
        << ", column "
        << ColumnNumber
-      << "</a></td></tr>\n"
-         "<tr><td class=\"rowname\">Description:</td><td>"
+       << "</a><br />"
        << D.getVerboseDescription() << "</td></tr>\n";
 
+    // The navigation across the extra notes pieces.
+    unsigned NumExtraPieces = 0;
+    for (auto I = path.begin(), E = path.end(); I != E; ++I) {
+      if (const auto *P = dyn_cast<PathDiagnosticExtraNotePiece>(I->get())) {
+        int LineNumber =
+            P->getLocation().asLocation().getExpansionLineNumber();
+        int ColumnNumber =
+            P->getLocation().asLocation().getExpansionColumnNumber();
+        os << "<tr><td class=\"rowname\">Note:</td><td>"
+           << "<a href=\"#ExtraNote" << NumExtraPieces << "\">line "
+           << LineNumber << ", column " << ColumnNumber << "</a><br />"
+           << P->getString() << "</td></tr>";
+        ++NumExtraPieces;
+      }
+    }
+
     // Output any other meta data.
 
     for (PathDiagnostic::meta_iterator I=D.meta_begin(), E=D.meta_end();
          I!=E; ++I) {
       os << "<tr><td></td><td>" << html::EscapeText(*I) << "</td></tr>\n";
     }
 
     os << "</table>\n<!-- REPORTSUMMARYEXTRA -->\n"
void HTMLDiagnostics::HandlePiece(Rewriter& R, FileID BugFileID,
   // Compute the margin offset by counting tabs and non-tabs.
   unsigned PosNo = 0;
   for (const char* c = LineStart; c != TokInstantiationPtr; ++c)
     PosNo += *c == '\t' ? 8 : 1;
 
   // Create the html for the message.
 
   const char *Kind = nullptr;
+  bool IsExtraNote = false;
+  bool SuppressIndex = (max == 1);
   switch (P.getKind()) {
   case PathDiagnosticPiece::Call:
-      llvm_unreachable("Calls should already be handled");
+      llvm_unreachable("Calls and extra notes should already be handled");
   case PathDiagnosticPiece::Event:  Kind = "Event"; break;
   case PathDiagnosticPiece::ControlFlow: Kind = "Control"; break;
     // Setting Kind to "Control" is intentional.
   case PathDiagnosticPiece::Macro: Kind = "Control"; break;
+  case PathDiagnosticPiece::ExtraNote:
+    Kind = "ExtraNote";
+    IsExtraNote = true;
+    SuppressIndex = true;
+    break;
   }
 
   std::string sbuf;
   llvm::raw_string_ostream os(sbuf);
 
   os << "\n<tr><td class=\"num\"></td><td class=\"line\"><div id=\"";
 
-  if (num == max)
+  if (IsExtraNote)
+    os << "ExtraNote" << num;
+  else if (num == max)
     os << "EndPath";
   else
     os << "Path" << num;
 
   os << "\" class=\"msg";
   if (Kind)
     os << " msg" << Kind;
   os << "\" style=\"margin-left:" << PosNo << "ex";
if (!isa<PathDiagnosticMacroPiece>(P)) {
     if (em < max_line/2)
       os << "; max-width:" << em << "em";
   }
   else
     os << "; max-width:100em";
 
   os << "\">";
 
-  if (max > 1) {
+  if (!SuppressIndex) {
     os << "<table class=\"msgT\"><tr><td valign=\"top\">";
     os << "<div class=\"PathIndex";
     if (Kind) os << " PathIndex" << Kind;
     os << "\">" << num << "</div>";
 
     if (num > 1) {
       os << "</td><td><div class=\"PathNav\"><a href=\"#Path"
          << (num - 1)
// Get the name of the macro by relexing it.
       Token TheTok;
       rawLexer.LexFromRawLexer(TheTok);
       for (unsigned i = 0, n = TheTok.getLength(); i < n; ++i)
         os << MacroName[i];
     }
 
     os << "':\n";
 
-    if (max > 1) {
+    if (!SuppressIndex) {
       os << "</td>";
       if (num < max) {
         os << "<td><div class=\"PathNav\"><a href=\"#";
         if (num == max - 1)
           os << "EndPath";
         else
           os << "Path" << (num + 1);
         os << "\" title=\"Next event ("
         << (num + 1)
         << ")\">&#x2192;</a></div></td>";
       }
 
       os << "</tr></table>";
     }
 
     // Within a macro piece.  Write out each event.
     ProcessMacroPiece(os, *MP, 0);
   }
   else {
     os << html::EscapeText(P.getString());
 
-    if (max > 1) {
+    if (!SuppressIndex) {
       os << "</td>";
       if (num < max) {
         os << "<td><div class=\"PathNav\"><a href=\"#";
         if (num == max - 1)
           os << "EndPath";
         else
           os << "Path" << (num + 1);
         os << "\" title=\"Next event ("

lib/StaticAnalyzer/Core/PathDiagnostic.cpp

 PathDiagnosticPiece::PathDiagnosticPiece(Kind k, DisplayHint hint)
   : kind(k), Hint(hint), LastInMainSourceFile(false) {}
 
 PathDiagnosticPiece::~PathDiagnosticPiece() {}
 PathDiagnosticEventPiece::~PathDiagnosticEventPiece() {}
 PathDiagnosticCallPiece::~PathDiagnosticCallPiece() {}
 PathDiagnosticControlFlowPiece::~PathDiagnosticControlFlowPiece() {}
 PathDiagnosticMacroPiece::~PathDiagnosticMacroPiece() {}
+PathDiagnosticExtraNotePiece::~PathDiagnosticExtraNotePiece() {}
 
 void PathPieces::flattenTo(PathPieces &Primary, PathPieces &Current,
                            bool ShouldFlattenMacros) const {
   for (PathPieces::const_iterator I = begin(), E = end(); I != E; ++I) {
     PathDiagnosticPiece *Piece = I->get();
 
     switch (Piece->getKind()) {
     case PathDiagnosticPiece::Call: {
case PathDiagnosticPiece::Macro: {
         Macro->subPieces.flattenTo(Primary, NewPath, ShouldFlattenMacros);
         // FIXME: This probably shouldn't mutate the original path piece.
         Macro->subPieces = NewPath;
       }
       break;
     }
     case PathDiagnosticPiece::Event:
     case PathDiagnosticPiece::ControlFlow:
+    case PathDiagnosticPiece::ExtraNote:
       Current.push_back(Piece);
       break;
     }
   }
 }
 
 PathDiagnostic::~PathDiagnostic() {}
 
for (unsigned i = 0, n = X.getRanges().size(); i < n; ++i) {
     if (XR != YR) {
       if (XR.getBegin() != YR.getBegin())
         return SM.isBeforeInTranslationUnit(XR.getBegin(), YR.getBegin());
       return SM.isBeforeInTranslationUnit(XR.getEnd(), YR.getEnd());
     }
   }
 
   switch (X.getKind()) {
-    case clang::ento::PathDiagnosticPiece::ControlFlow:
+    case PathDiagnosticPiece::ControlFlow:
       return compareControlFlow(cast<PathDiagnosticControlFlowPiece>(X),
                                 cast<PathDiagnosticControlFlowPiece>(Y));
-    case clang::ento::PathDiagnosticPiece::Event:
+    case PathDiagnosticPiece::Event:
+    case PathDiagnosticPiece::ExtraNote:
       return None;
-    case clang::ento::PathDiagnosticPiece::Macro:
+    case PathDiagnosticPiece::Macro:
       return compareMacro(cast<PathDiagnosticMacroPiece>(X),
                           cast<PathDiagnosticMacroPiece>(Y));
-    case clang::ento::PathDiagnosticPiece::Call:
+    case PathDiagnosticPiece::Call:
       return compareCall(cast<PathDiagnosticCallPiece>(X),
                          cast<PathDiagnosticCallPiece>(Y));
   }
   llvm_unreachable("all cases handled");
 }
 
 static Optional<bool> comparePath(const PathPieces &X, const PathPieces &Y) {
   if (X.size() != Y.size())

 
 void PathDiagnosticMacroPiece::Profile(llvm::FoldingSetNodeID &ID) const {
   PathDiagnosticSpotPiece::Profile(ID);
   for (PathPieces::const_iterator I = subPieces.begin(), E = subPieces.end();
        I != E; ++I)
     ID.Add(**I);
 }
 
+void PathDiagnosticExtraNotePiece::Profile(llvm::FoldingSetNodeID &ID) const {
+  PathDiagnosticSpotPiece::Profile(ID);
+}
+
 void PathDiagnostic::Profile(llvm::FoldingSetNodeID &ID) const {
   ID.Add(getLocation());
   ID.AddString(BugType);
   ID.AddString(VerboseDesc);
   ID.AddString(Category);
 }
 
 void PathDiagnostic::FullProfile(llvm::FoldingSetNodeID &ID) const {

lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

switch (P.getKind()) {
     case PathDiagnosticPiece::Event:
       ReportEvent(o, cast<PathDiagnosticSpotPiece>(P), FM, SM, LangOpts,
                   indent, depth, isKeyEvent);
       break;
     case PathDiagnosticPiece::Macro:
       ReportMacro(o, cast<PathDiagnosticMacroPiece>(P), FM, SM, LangOpts,
                   indent, depth);
       break;
+    case PathDiagnosticPiece::ExtraNote:
+      // FIXME: Extend the plist format to support those.
+      break;
   }
 }
 
 void PlistDiagnostics::FlushDiagnosticsImpl(
                                     std::vector<const PathDiagnostic *> &Diags,
                                     FilesMade *filesMade) {
   // Build up a set of FIDs that we use by scanning the locations and
   // ranges of the diagnostics.

lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

void FlushDiagnosticsImpl(std::vector<const PathDiagnostic *> &Diags,
 
     for (std::vector<const PathDiagnostic*>::iterator I = Diags.begin(),
          E = Diags.end(); I != E; ++I) {
       const PathDiagnostic *PD = *I;
       SourceLocation WarnLoc = PD->getLocation().asLocation();
       Diag.Report(WarnLoc, WarnID) << PD->getShortDescription()
                                    << PD->path.back()->getRanges();
 
+      // First, add extra notes, even if paths should not be included.
+      for (PathPieces::const_iterator PI = PD->path.begin(),
+                                      PE = PD->path.end(); PI != PE; ++PI) {
+        if (!isa<PathDiagnosticExtraNotePiece>(PI->get()))
+          continue;
+
+        SourceLocation NoteLoc = (*PI)->getLocation().asLocation();
+        Diag.Report(NoteLoc, NoteID) << (*PI)->getString()
+                                     << (*PI)->getRanges();
+      }
+
       if (!IncludePath)
         continue;
 
+      // Then, add the path notes if necessary.
       PathPieces FlatPath = PD->path.flatten(/*ShouldFlattenMacros=*/true);
       for (PathPieces::const_iterator PI = FlatPath.begin(),
-                                      PE = FlatPath.end();
-           PI != PE; ++PI) {
+                                      PE = FlatPath.end(); PI != PE; ++PI) {
+        if (isa<PathDiagnosticExtraNotePiece>(PI->get()))
+          continue;
+
         SourceLocation NoteLoc = (*PI)->getLocation().asLocation();
         Diag.Report(NoteLoc, NoteID) << (*PI)->getString()
                                      << (*PI)->getRanges();
       }
     }
   }
 };
 } // end anonymous namespace