[analyzer] [NFC] PathDiagnostic: Create PathDiagnosticPopUpPiece

[Charusso]

Summary

This new piece is similar to our macro expansion printing in HTML reports:
On mouse-hover event it pops up on variables. Similar to note pieces it
supports plist diagnostics as well.

It is optional, on by default: add-pop-up-notes=true.

Extra: In HTML reports background-color: LemonChiffon was too light,
changed to PaleGoldenRod.

[NoQ]

In HTML reports background-color: LemonChiffon was too light, changed to PaleGoldenRod.

Both look good to me, though i'd easily believe that LemonChiffon is indeed indistinguishable from white on some monitors with certain brightness/contrast settings.

clang/lib/Rewrite/HTMLRewrite.cpp

333 ↗

(On Diff #195070)

Blue over yellow looks a bit acid, maybe use a... "bluer"?... background for our pop-ups? Like, both for the markup and for the actual popup.

431–433 ↗

(On Diff #195070)

Why is it necessary to re-lex everything in this case? I don't really know why this was necessary for macros, but in this case i'm more confused. The popup piece is supposed to point us to a specific source range; why can't we just highlight that source range directly?

[NoQ]

Let's decide whether we want to display the new notes in the text output. @alexfh: Given that clang-tidy is the primary consumer of the analyzer's text output mode, would you rather have or rather not have these new note:s in the text output? (scroll to the bottom of D53076 to see how they look). In my opinion, i'd rather have them in text output (because text output is hard to navigate, and having the necessary information exactly where you need it would probably be helpful), but rather not have them in tools that parse clang-tidy notes and stuff them into a source code view in some GUI (such tools do exist, right?), so im confused. I'd also prefer to have them be off in text output in tests, because such tests usually test checker-specific notes and having more checker-inspecific notes in the tests is more annoying than useful. But that's pretty minor; i'll be happy to write them down every time i write a test if they are believed to be a good thing in text output in general.

[alexfh]

In D60670#1469494, @NoQ wrote:

Let's decide whether we want to display the new notes in the text output. @alexfh: Given that clang-tidy is the primary consumer of the analyzer's text output mode, would you rather have or rather not have these new note:s in the text output? (scroll to the bottom of D53076 to see how they look). In my opinion, i'd rather have them in text output (because text output is hard to navigate, and having the necessary information exactly where you need it would probably be helpful), but rather not have them in tools that parse clang-tidy notes and stuff them into a source code view in some GUI (such tools do exist, right?), so im confused. I'd also prefer to have them be off in text output in tests, because such tests usually test checker-specific notes and having more checker-inspecific notes in the tests is more annoying than useful. But that's pretty minor; i'll be happy to write them down every time i write a test if they are believed to be a good thing in text output in general.

Can we add an option to enable or disable these notes similar to Clang's -fdiagnostics-show-option, -fdiagnostics-show-note-include-stack, etc?

[Szelethus]

• clang-format PlistDiagnostics.cpp

I wanted to do this for the longest time, but @george.karpenkov was very much against it. Is this desired? D52735#1251344

[Charusso]

Thanks!

@Szelethus, I will revert it.

In D60670#1470849, @alexfh wrote:

In D60670#1469494, @NoQ wrote:

Let's decide whether we want to display the new notes in the text output.

Can we add an option to enable or disable these notes similar to Clang's -fdiagnostics-show-option, -fdiagnostics-show-note-include-stack, etc?

@alexfh, add them as a Clang option with -f? I do not know how Clang Tidy would use that, so that I am not sure. I would create an AnalyzerOptions option as I see that far only.

clang/lib/Rewrite/HTMLRewrite.cpp

333 ↗

(On Diff #195070)

I am planning to change the entire CSS business as the current representation already too acid. Do you mean to change the PaleGoldenRod to blue on conditions as a new kind of highlight?

431–433 ↗

(On Diff #195070)

I just did not know about PathDiagnosticRange.

[alexfh]

In D60670#1474333, @Charusso wrote:

In D60670#1470849, @alexfh wrote:

Can we add an option to enable or disable these notes similar to Clang's -fdiagnostics-show-option, -fdiagnostics-show-note-include-stack, etc?

@alexfh, add them as a Clang option with -f? I do not know how Clang Tidy would use that, so that I am not sure. I would create an AnalyzerOptions option as I see that far only.

I gave Clang's -f options as an example only. An AnalyzerOptions option would work too. Just something we can change if needed.

[Charusso]

• add-pop-up-notes=true
• Revert clang-format.

[Charusso]

Feature request test is in D61060.

Now it is blue and the highlight is as wide as the condition:

blue.html1 MBDownload

Before:

before.html1 MBDownload

[NoQ]

Hmm, i just realized that there may be multiple pop-ups for the same expression, with different values. This may happen if the expression is used in a loop or the function is called multiple times. We'd have to include all of them somehow, maybe something like this:

   /------------------\
   | (42.a)  X is 5   |    /-----------------\
   | (69.a)  X is -2  |    | (42.b)  Y is 7  |
   \  /---------------/    \  /--------------/
    \/                      \/

if (X >= 0        &&        Y <= 10) {

  /-------------------------\
  | (43) Taking true branch |
  \-------------------------/
  /--------------------------\
  | (70) Taking false branch |
  \--------------------------/

}

[Charusso]

Fix multiple reports at the same range:

• Plain text became a table.
• Styling is the same as the bubbles.
• mrange (whatever it means) - the grey highlight cannot overlap pop-up ranges.

[Charusso]

Your indexing idea has a problem: zero index would be possible and usually you would compare the gray PathDiagnostic bubbles with these notes which index would get an offset by one.

At line 723 there would be a zero index, which is confusing, and at line 1052 you could see the new approach:

multiple.html236 KBDownload

[NoQ]

Wonderful!!

The reason why i don't want numbers on pop-up notes match numbers on regular notes is that the text on the pop-up note may no longer be true at the moment of time when we reach the regular note. Like, the value of the variable may change in between.

We can do one of the following:

• "42.a", "42.b", ... (still confusing as it if's part of 42);
• "42½", "42⅓", "42⅔", ..., just give up and drop them when there more than 10 popups between two events specific, because that's how many unicode vulgar fractions we have;
• Just "42 → 43" on all pop-ups (indicates that this is somewhere between 42 and 43).

If it turns out to be still confusing, i'd recommend committing it as off-by-default until we come up with a better UI. 'Cause the work is wonderful regardless.

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def

208 ↗

(On Diff #197000)

Let's specify "pop-up notes", because "extra notes" refer to the note pieces.

[Charusso]

index → index+1

[Charusso]

Thanks for the review!

I have not seen problematic indexing, that is why I was not sure about your idea. If someone ever find confusing stuff I would change it immediately to off by default.

Final look:

final.html236 KBDownload

[Charusso]

PS: That offset-by-one index + arrow confuse me more, so that I am not sure about that improvement being a useful one.

[NoQ]

Could you have a quick look at how does the vulgar fractions solution look?, i.e., 42½ etc., maybe you can modify the html directly if you don't want to code it, as it might be annoying to code. I don't immediately see any downsides.

[Charusso]

• Sub-state indexing.

[Charusso]

I have not got better idea to just index over each sub-state. Example at line 1052:

indices.html236 KBDownload

[Charusso]

PS: I do not like that fraction stuff as it is difficult to read, ugly and limited.

[Charusso]

@NoQ could you share your final thoughts here, please? I would like to move forward and never look back.

[NoQ]

Mmm, just had a fresh look and i kinda like it. I think in its current shape it's intuitive enough what this means.

Let's make sure that all popups have the extra .digit in them and commit.

[Charusso]

• Add extra state transition indices to every pop-up note.

[Charusso]

Thanks you! I hope this feature will be useful to see what is behind the scenes and create better reports.

Usage could be found at line 2041 with line 2129:

done.html915 KBDownload

[Charusso]

PS: I have no idea why the macro note is purple, but because the entire HTML printing is broken I hope you saw nothing.

[NoQ]

Go for it!

clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp

698–699 ↗

(On Diff #200109)

This comment looks outdated.

[Charusso]

Fix a nit in rL362632.

include/clang/StaticAnalyzer/Core/AnalyzerOptions.def

ANALYZER_OPTION(bool, MayInlineObjCMethod, "objc-inlining",
                 "Whether ObjectiveC inlining is enabled, false otherwise.",
                 true)
 
 ANALYZER_OPTION(bool, ShouldPrunePaths, "prune-paths",
                 "Whether irrelevant parts of a bug report path should "
                 "be pruned out of the final output.",
                 true)
 
+ANALYZER_OPTION(bool, ShouldAddPopUpNotes, "add-pop-up-notes",
+                "Whether pop-up notes should be added to the final output.",
+                true)
+
 ANALYZER_OPTION(
     bool, ShouldConditionalizeStaticInitializers,
     "cfg-conditional-static-initializers",
     "Whether 'static' initializers should be in conditional logic in the CFG.",
     true)
 
 ANALYZER_OPTION(bool, ShouldSynthesizeBodies, "faux-bodies",
                 "Whether the analyzer engine should synthesize fake "

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

 };
 
 //===----------------------------------------------------------------------===//
 // Path "pieces" for path-sensitive diagnostics.
 //===----------------------------------------------------------------------===//
 
 class PathDiagnosticPiece: public llvm::FoldingSetNode {
 public:
-  enum Kind { ControlFlow, Event, Macro, Call, Note };
+  enum Kind { ControlFlow, Event, Macro, Call, Note, PopUp };
   enum DisplayHint { Above, Below };
 
 private:
   const std::string str;
   const Kind kind;
   const DisplayHint Hint;
 
   /// 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 ||
-           P->getKind() == Note;
+           P->getKind() == Note || P->getKind() == PopUp;
   }
 };
 
 /// 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 {
public:
   void dump() const override;
 
   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 bool classof(const PathDiagnosticPiece *P) {
     return P->getKind() == Note;
   }
 
   void dump() const override;
 
   void Profile(llvm::FoldingSetNodeID &ID) const override;
 };
 
+class PathDiagnosticPopUpPiece: public PathDiagnosticSpotPiece {
+public:
+  PathDiagnosticPopUpPiece(const PathDiagnosticLocation &Pos, StringRef S,
+                           bool AddPosRange = true)
+      : PathDiagnosticSpotPiece(Pos, S, PopUp, AddPosRange) {}
+  ~PathDiagnosticPopUpPiece() override;
+
+  static bool classof(const PathDiagnosticPiece *P) {
+    return P->getKind() == PopUp;
+  }
+
+  void dump() const override;
+
+  void Profile(llvm::FoldingSetNodeID &ID) const override;
+};
+
 /// File IDs mapped to sets of line numbers.
 using FilesToLineNumsMap = std::map<FileID, std::set<unsigned>>;
 
 /// 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;

lib/Rewrite/HTMLRewrite.cpp

 .divider { background-color: gray; }
 .code { border-collapse:collapse; width:100%; }
 .code { font-family: "Monospace", monospace; font-size:10pt }
 .code { line-height: 1.2em }
 .comment { color: green; font-style: oblique }
 .keyword { color: blue }
 .string_literal { color: red }
 .directive { color: darkmagenta }
-/* Macro expansions. */
-.expansion { display: none; }
-.macro:hover .expansion {
+
+/* Macros and variables could have pop-up notes hidden by default.
+  - Macro pop-up:    expansion of the macro
+  - Variable pop-up: value (table) of the variable */
+.macro_popup, .variable_popup { display: none; }
+
+/* Pop-up appears on mouse-hover event. */
+.macro:hover .macro_popup, .variable:hover .variable_popup {
   display: block;
-  border: 2px solid #FF0000;
   padding: 2px;
-  background-color:#FFF0F0;
-  font-weight: normal;
   -webkit-border-radius:5px;
   -webkit-box-shadow:1px 1px 7px #000;
   border-radius:5px;
   box-shadow:1px 1px 7px #000;
   position: absolute;
   top: -1em;
   left:10em;
   z-index: 1
 }
 
+.macro_popup {
+  border: 2px solid red;
+  background-color:#FFF0F0;
+  font-weight: normal;
+}
+
+.variable_popup {
+  border: 2px solid blue;
+  background-color:#F0F0FF;
+  font-weight: bold;
+  font-family: Helvetica, sans-serif;
+  font-size: 9pt;
+}
+
+/* Pop-up notes needs a relative position as a base where they pops up. */
+.macro, .variable {
+  background-color: PaleGoldenRod;
+  position: relative;
+}
+.macro { color: DarkMagenta; }
+
 #tooltiphint {
   position: fixed;
   width: 50em;
   margin-left: -25em;
   left: 50%;
   padding: 10px;
   border: 1px solid #b0b0b0;
   border-radius: 2px;
   box-shadow: 1px 1px 7px black;
   background-color: #c0c0c0;
   z-index: 2;
 }
-.macro {
-  color: darkmagenta;
-  background-color:LemonChiffon;
-  /* Macros are position: relative to provide base for expansions. */
-  position: relative;
-}
 
 .num { width:2.5em; padding-right:2ex; background-color:#eeeeee }
 .num { text-align:right; font-size:8pt }
 .num { color:#444444 }
 .line { padding-left: 1ex; border-left: 3px solid #ccc }
 .line { white-space: pre }
 .msg { -webkit-box-shadow:1px 1px 7px #000 }
 .msg { box-shadow:1px 1px 7px #000 }

 .msgNote { background-color:#ddeeff; color:#000000 }
 .mrange { background-color:#dfddf3 }
 .mrange { border-bottom:1px solid #6F9DBE }
 .PathIndex { font-weight: bold; padding:0px 5px; margin-right:5px; }
 .PathIndex { -webkit-border-radius:8px }
 .PathIndex { border-radius:8px }
 .PathIndexEvent { background-color:#bfba87 }
 .PathIndexControl { background-color:#8c8c8c }
+.PathIndexPopUp { background-color: #879abc; }
 .PathNav a { text-decoration:none; font-size: larger }
 .CodeInsertionHint { font-weight: bold; background-color: #10dd10 }
 .CodeRemovalHint { background-color:#de1010 }
 .CodeRemovalHint { border-bottom:1px solid #6F9DBE }
 .selected{ background-color:orange !important; }
 
 table.simpletable {
   padding: 5px;
while (!Tok.is(tok::eof) &&
       Expansion += EscapeText(TmpPP.getSpelling(Tok));
       LineLen += Expansion.size();
 
       PrevPrevTok = PrevTok;
       PrevTok = Tok;
       TmpPP.Lex(Tok);
     }
 
-
-    // Insert the expansion as the end tag, so that multi-line macros all get
-    // highlighted.
-    Expansion = "<span class='expansion'>" + Expansion + "</span></span>";
+    // Insert the 'macro_popup' as the end tag, so that multi-line macros all
+    // get highlighted.
+    Expansion = "<span class='macro_popup'>" + Expansion + "</span></span>";
 
     HighlightRange(R, LLoc.getBegin(), LLoc.getEnd(), "<span class='macro'>",
                    Expansion.c_str(), LLoc.isTokenRange());
   }
 
   // Restore the preprocessor's old state.
   TmpPP.setDiagnostics(*OldDiags);
   TmpPP.setPragmasEnabled(PragmasPreviouslyEnabled);
 }

lib/StaticAnalyzer/Core/BugReporter.cpp

for (unsigned i = 0; i < N; ++i) {
 
     switch (piece->getKind()) {
       case PathDiagnosticPiece::Call:
         removeRedundantMsgs(cast<PathDiagnosticCallPiece>(*piece).path);
         break;
       case PathDiagnosticPiece::Macro:
         removeRedundantMsgs(cast<PathDiagnosticMacroPiece>(*piece).subPieces);
         break;
-      case PathDiagnosticPiece::ControlFlow:
-        break;
       case PathDiagnosticPiece::Event: {
         if (i == N-1)
           break;
 
         if (auto *nextEvent =
             dyn_cast<PathDiagnosticEventPiece>(path.front().get())) {
           auto *event = cast<PathDiagnosticEventPiece>(piece.get());
           // 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 (auto *pieceToKeep =
                   eventsDescribeSameCondition(event, nextEvent)) {
             piece = std::move(pieceToKeep == event ? piece : path.front());
             path.pop_front();
             ++i;
           }
         }
         break;
       }
+      case PathDiagnosticPiece::ControlFlow:
       case PathDiagnosticPiece::Note:
+      case PathDiagnosticPiece::PopUp:
         break;
     }
     path.push_back(std::move(piece));
   }
 }
 
 /// A map from PathDiagnosticPiece to the LocationContext of the inlined
 /// function call it represents.
switch (piece->getKind()) {
         auto &event = cast<PathDiagnosticEventPiece>(*piece);
 
         // 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::Note:
+      case PathDiagnosticPiece::PopUp:
         break;
     }
 
     pieces.push_back(std::move(piece));
   }
 
   return containsSomethingInteresting;
 }
 
+/// Same logic as above to remove extra pieces.
+static void removePopUpNotes(PathPieces &Path) {
+  for (unsigned int i = 0; i < Path.size(); ++i) {
+    auto Piece = std::move(Path.front());
+    Path.pop_front();
+    if (!isa<PathDiagnosticPopUpPiece>(*Piece))
+      Path.push_back(std::move(Piece));
+  }
+}
+
 /// Returns true if the given decl has been implicitly given a body, either by
 /// the analyzer or by the compiler proper.
 static bool hasImplicitBody(const Decl *D) {
   assert(D);
   return D->isImplicit() || !D->hasBody();
 }
 
 /// Recursively scan through a path and make sure that all call pieces have
static std::unique_ptr<PathDiagnostic> generatePathDiagnosticForConsumer(
   if (!PD->path.empty()) {
     if (R->shouldPrunePath() && Opts.ShouldPrunePaths) {
       bool stillHasNotes =
           removeUnneededCalls(PD->getMutablePieces(), R, LCM);
       assert(stillHasNotes);
       (void)stillHasNotes;
     }
 
+    // Remove pop-up notes if needed.
+    if (!Opts.ShouldAddPopUpNotes)
+      removePopUpNotes(PD->getMutablePieces());
+
     // Redirect all call pieces to have valid locations.
     adjustCallLocations(PD->getMutablePieces());
     removePiecesWithInvalidLocations(PD->getMutablePieces());
 
     if (AddPathEdges) {
 
       // Reduce the number of edges from a very conservative set
       // to an aesthetically pleasing subset that conveys the

lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp

public:
   bool supportsCrossFileDiagnostics() const override {
     return SupportsCrossFileDiagnostics;
   }
 
   unsigned ProcessMacroPiece(raw_ostream &os,
                              const PathDiagnosticMacroPiece& P,
                              unsigned num);
 
-  void HandlePiece(Rewriter& R, FileID BugFileID,
-                   const PathDiagnosticPiece& P, unsigned num, unsigned max);
+  void HandlePiece(Rewriter &R, FileID BugFileID, const PathDiagnosticPiece &P,
+                   const std::vector<SourceRange> &PopUpRanges, unsigned num,
+                   unsigned max);
 
   void HighlightRange(Rewriter& R, FileID BugFileID, SourceRange Range,
                       const char *HighlightStart = "<span class=\"mrange\">",
                       const char *HighlightEnd = "</span>");
 
   void ReportDiag(const PathDiagnostic& D,
                   FilesMade *filesMade);
 
if (event.key == "?") {
     return;
   }
   event.preventDefault();
 });
 </script>
 )<<<";
 }
 
+static void
+HandlePopUpPieceStartTag(Rewriter &R,
+                         const std::vector<SourceRange> &PopUpRanges) {
+  for (const auto &Range : PopUpRanges) {
+    html::HighlightRange(R, Range.getBegin(), Range.getEnd(), "",
+                         "<table class='variable_popup'><tbody>",
+                         /*IsTokenRange=*/true);
+  }
+}
+
+static void HandlePopUpPieceEndTag(Rewriter &R,
+                                   const PathDiagnosticPopUpPiece &Piece,
+                                   std::vector<SourceRange> &PopUpRanges,
+                                   unsigned int LastReportedPieceIndex,
+                                   unsigned int PopUpPieceIndex) {
+  SmallString<256> Buf;
+  llvm::raw_svector_ostream Out(Buf);
+
+  SourceRange Range(Piece.getLocation().asRange());
+
+  // Write out the path indices with a right arrow and the message as a row.
+  Out << "<tr><td valign='top'><div class='PathIndex PathIndexPopUp'>"
+      << LastReportedPieceIndex;
+
+  // Also annotate the state transition with extra indices.
+  Out << '.' << PopUpPieceIndex;
+
+  Out << "</div></td><td>" << Piece.getString() << "</td></tr>";
+
+  // If no report made at this range mark the variable and add the end tags.
+  if (std::find(PopUpRanges.begin(), PopUpRanges.end(), Range) ==
+      PopUpRanges.end()) {
+    // Store that we create a report at this range.
+    PopUpRanges.push_back(Range);
+
+    Out << "</tbody></table></span>";
+    html::HighlightRange(R, Range.getBegin(), Range.getEnd(),
+                         "<span class='variable'>", Buf.c_str(),
+                         /*IsTokenRange=*/true);
+
+  // Otherwise inject just the new row at the end of the range.
+  } else {
+    html::HighlightRange(R, Range.getBegin(), Range.getEnd(), "", Buf.c_str(),
+                         /*IsTokenRange=*/true);
+  }
+}
+
 void HTMLDiagnostics::RewriteFile(Rewriter &R,
                                   const PathPieces& path, FileID FID) {
   // Process the path.
   // Maintain the counts of extra note pieces separately.
   unsigned TotalPieces = path.size();
   unsigned TotalNotePieces =
       std::count_if(path.begin(), path.end(),
                     [](const std::shared_ptr<PathDiagnosticPiece> &p) {
                       return isa<PathDiagnosticNotePiece>(*p);
                     });
+  unsigned PopUpPieceCount =
+      std::count_if(path.begin(), path.end(),
+                    [](const std::shared_ptr<PathDiagnosticPiece> &p) {
+                      return isa<PathDiagnosticPopUpPiece>(*p);
+                    });
 
-  unsigned TotalRegularPieces = TotalPieces - TotalNotePieces;
+  unsigned TotalRegularPieces = TotalPieces - TotalNotePieces - PopUpPieceCount;
   unsigned NumRegularPieces = TotalRegularPieces;
   unsigned NumNotePieces = TotalNotePieces;
+  // Stores the count of the regular piece indices.
+  std::map<int, int> IndexMap;
 
+  // Stores the different ranges where we have reported something.
+  std::vector<SourceRange> PopUpRanges;
   for (auto I = path.rbegin(), E = path.rend(); I != E; ++I) {
-    if (isa<PathDiagnosticNotePiece>(I->get())) {
+    const auto &Piece = *I->get();
+
+    if (isa<PathDiagnosticPopUpPiece>(Piece)) {
+      ++IndexMap[NumRegularPieces];
+    } else if (isa<PathDiagnosticNotePiece>(Piece)) {
       // This adds diagnostic bubbles, but not navigation.
       // Navigation through note pieces would be added later,
       // as a separate pass through the piece list.
-      HandlePiece(R, FID, **I, NumNotePieces, TotalNotePieces);
+      HandlePiece(R, FID, Piece, PopUpRanges, NumNotePieces, TotalNotePieces);
       --NumNotePieces;
     } else {
-      HandlePiece(R, FID, **I, NumRegularPieces, TotalRegularPieces);
+      HandlePiece(R, FID, Piece, PopUpRanges, NumRegularPieces,
+                  TotalRegularPieces);
       --NumRegularPieces;
     }
   }
 
-  // Add line numbers, header, footer, etc.
+  // Secondary indexing if we are having multiple pop-ups between two notes.
+  // (e.g. [(13) 'a' is 'true'];  [(13.1) 'b' is 'false'];  [(13.2) 'c' is...)
+  NumRegularPieces = TotalRegularPieces;
+  for (auto I = path.rbegin(), E = path.rend(); I != E; ++I) {
+    const auto &Piece = *I->get();
+
+    if (const auto *PopUpP = dyn_cast<PathDiagnosticPopUpPiece>(&Piece)) {
+      int PopUpPieceIndex = IndexMap[NumRegularPieces];
+
+      // Pop-up pieces needs the index of the last reported piece and its count
+      // how many times we report to handle multiple reports on the same range.
+      // This marks the variable, adds the </table> end tag and the message
+      // (list element) as a row. The <table> start tag will be added after the
+      // rows has been written out. Note: It stores every different range.
+      HandlePopUpPieceEndTag(R, *PopUpP, PopUpRanges, NumRegularPieces,
+                             PopUpPieceIndex);
 
+      if (PopUpPieceIndex > 0)
+        --IndexMap[NumRegularPieces];
+
+    } else if (!isa<PathDiagnosticNotePiece>(Piece)) {
+      --NumRegularPieces;
+    }
+  }
+
+  // Add the <table> start tag of pop-up pieces based on the stored ranges.
+  HandlePopUpPieceStartTag(R, PopUpRanges);
+
+  // Add line numbers, header, footer, etc.
   html::EscapeText(R, FID);
   html::AddLineNumbers(R, FID);
 
   // If we have a preprocessor, relex the file and syntax highlight.
   // We might not have a preprocessor if we come from a deserialized AST file,
   // for example.
-
   html::SyntaxHighlight(R, FID, PP);
   html::HighlightMacros(R, FID, PP);
 }
 
-void HTMLDiagnostics::HandlePiece(Rewriter& R, FileID BugFileID,
-                                  const PathDiagnosticPiece& P,
+void HTMLDiagnostics::HandlePiece(Rewriter &R, FileID BugFileID,
+                                  const PathDiagnosticPiece &P,
+                                  const std::vector<SourceRange> &PopUpRanges,
                                   unsigned num, unsigned max) {
   // For now, just draw a box above the line in question, and emit the
   // warning.
   FullSourceLoc Pos = P.getLocation().asLocation();
 
   if (!Pos.isValid())
     return;
 
for (const char* c = LineStart; c != TokInstantiationPtr; ++c)
     PosNo += *c == '\t' ? 8 : 1;
 
   // Create the html for the message.
 
   const char *Kind = nullptr;
   bool IsNote = false;
   bool SuppressIndex = (max == 1);
   switch (P.getKind()) {
-  case PathDiagnosticPiece::Call:
-      llvm_unreachable("Calls and extra notes should already be handled");
-  case PathDiagnosticPiece::Event:  Kind = "Event"; break;
+  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::Note:
     Kind = "Note";
     IsNote = true;
     SuppressIndex = true;
     break;
+  case PathDiagnosticPiece::Call:
+  case PathDiagnosticPiece::PopUp:
+    llvm_unreachable("Calls and extra notes should already be handled");
   }
 
   std::string sbuf;
   llvm::raw_string_ostream os(sbuf);
 
   os << "\n<tr><td class=\"num\"></td><td class=\"line\"><div id=\"";
 
   if (IsNote)
void HTMLDiagnostics::HandlePiece(Rewriter &R, FileID BugFileID,
   unsigned DisplayPos = LineEnd - FileStart;
   SourceLocation Loc =
     SM.getLocForStartOfFile(LPosInfo.first).getLocWithOffset(DisplayPos);
 
   R.InsertTextBefore(Loc, os.str());
 
   // Now highlight the ranges.
   ArrayRef<SourceRange> Ranges = P.getRanges();
-  for (const auto &Range : Ranges)
+  for (const auto &Range : Ranges) {
+    // If we have already highlighted the range as a pop-up there is no work.
+    if (std::find(PopUpRanges.begin(), PopUpRanges.end(), Range) !=
+        PopUpRanges.end())
+      continue;
+
     HighlightRange(R, LPosInfo.first, Range);
   }
+}
 
 static void EmitAlphaCounter(raw_ostream &os, unsigned n) {
   unsigned x = n % ('z' - 'a');
   n /= 'z' - 'a';
 
   if (n > 0)
     EmitAlphaCounter(os, n);
 

lib/StaticAnalyzer/Core/PathDiagnostic.cpp

 PathDiagnosticCallPiece::~PathDiagnosticCallPiece() = default;
 
 PathDiagnosticControlFlowPiece::~PathDiagnosticControlFlowPiece() = default;
 
 PathDiagnosticMacroPiece::~PathDiagnosticMacroPiece() = default;
 
 PathDiagnosticNotePiece::~PathDiagnosticNotePiece() = default;
 
+PathDiagnosticPopUpPiece::~PathDiagnosticPopUpPiece() = default;
+
 void PathPieces::flattenTo(PathPieces &Primary, PathPieces &Current,
                            bool ShouldFlattenMacros) const {
   for (auto &Piece : *this) {
     switch (Piece->getKind()) {
     case PathDiagnosticPiece::Call: {
       auto &Call = cast<PathDiagnosticCallPiece>(*Piece);
       if (auto CallEnter = Call.getCallEnterEvent())
         Current.push_back(std::move(CallEnter));
case PathDiagnosticPiece::Macro: {
         // FIXME: This probably shouldn't mutate the original path piece.
         Macro.subPieces = NewPath;
       }
       break;
     }
     case PathDiagnosticPiece::Event:
     case PathDiagnosticPiece::ControlFlow:
     case PathDiagnosticPiece::Note:
+    case PathDiagnosticPiece::PopUp:
       Current.push_back(Piece);
       break;
     }
   }
 }
 
 PathDiagnostic::~PathDiagnostic() = default;
 
if (XR != YR) {
       return SM.isBeforeInTranslationUnit(XR.getEnd(), YR.getEnd());
     }
   }
 
   switch (X.getKind()) {
     case PathDiagnosticPiece::ControlFlow:
       return compareControlFlow(cast<PathDiagnosticControlFlowPiece>(X),
                                 cast<PathDiagnosticControlFlowPiece>(Y));
-    case PathDiagnosticPiece::Event:
-    case PathDiagnosticPiece::Note:
-      return None;
     case PathDiagnosticPiece::Macro:
       return compareMacro(cast<PathDiagnosticMacroPiece>(X),
                           cast<PathDiagnosticMacroPiece>(Y));
     case PathDiagnosticPiece::Call:
       return compareCall(cast<PathDiagnosticCallPiece>(X),
                          cast<PathDiagnosticCallPiece>(Y));
+    case PathDiagnosticPiece::Event:
+    case PathDiagnosticPiece::Note:
+    case PathDiagnosticPiece::PopUp:
+      return None;
   }
   llvm_unreachable("all cases handled");
 }
 
 static Optional<bool> comparePath(const PathPieces &X, const PathPieces &Y) {
   if (X.size() != Y.size())
     return X.size() < Y.size();
 
void PathDiagnosticMacroPiece::Profile(llvm::FoldingSetNodeID &ID) const {
   for (const auto &I : subPieces)
     ID.Add(*I);
 }
 
 void PathDiagnosticNotePiece::Profile(llvm::FoldingSetNodeID &ID) const {
   PathDiagnosticSpotPiece::Profile(ID);
 }
 
+void PathDiagnosticPopUpPiece::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 {

 
 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 PathDiagnosticPopUpPiece::dump() const {
+  llvm::errs() << "POP-UP\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/PlistDiagnostics.cpp

switch (P.getKind()) {
         break;
       case PathDiagnosticPiece::Macro:
         ReportMacroSubPieces(o, cast<PathDiagnosticMacroPiece>(P), indent,
                              depth);
         break;
       case PathDiagnosticPiece::Note:
         ReportNote(o, cast<PathDiagnosticNotePiece>(P), indent);
         break;
+      case PathDiagnosticPiece::PopUp:
+        ReportPopUp(o, cast<PathDiagnosticPopUpPiece>(P), indent);
+        break;
     }
   }
 
   void EmitRanges(raw_ostream &o, const ArrayRef<SourceRange> Ranges,
                   unsigned indent);
   void EmitMessage(raw_ostream &o, StringRef Message, unsigned indent);
 
   void ReportControlFlow(raw_ostream &o,
                          const PathDiagnosticControlFlowPiece& P,
                          unsigned indent);
   void ReportEvent(raw_ostream &o, const PathDiagnosticEventPiece& P,
                    unsigned indent, unsigned depth, bool isKeyEvent = false);
   void ReportCall(raw_ostream &o, const PathDiagnosticCallPiece &P,
                   unsigned indent, unsigned depth);
   void ReportMacroSubPieces(raw_ostream &o, const PathDiagnosticMacroPiece& P,
                             unsigned indent, unsigned depth);
   void ReportNote(raw_ostream &o, const PathDiagnosticNotePiece& P,
                   unsigned indent);
+
+  void ReportPopUp(raw_ostream &o, const PathDiagnosticPopUpPiece &P,
+                   unsigned indent);
 };
 
 } // end of anonymous namespace
 
 namespace {
 
 struct ExpansionInfo {
   std::string MacroName;
void PlistPrinter::ReportNote(raw_ostream &o, const PathDiagnosticNotePiece& P,
   // Output the text.
   EmitMessage(o, P.getString(), indent);
 
   // Finish up.
   --indent;
   Indent(o, indent); o << "</dict>\n";
 }
 
+void PlistPrinter::ReportPopUp(raw_ostream &o,
+                               const PathDiagnosticPopUpPiece &P,
+                               unsigned indent) {
+  const SourceManager &SM = PP.getSourceManager();
+
+  Indent(o, indent) << "<dict>\n";
+  ++indent;
+
+  Indent(o, indent) << "<key>kind</key><string>pop-up</string>\n";
+
+  // Output the location.
+  FullSourceLoc L = P.getLocation().asLocation();
+
+  Indent(o, indent) << "<key>location</key>\n";
+  EmitLocation(o, SM, L, FM, indent);
+
+  // Output the ranges (if any).
+  ArrayRef<SourceRange> Ranges = P.getRanges();
+  EmitRanges(o, Ranges, indent);
+
+  // Output the text.
+  EmitMessage(o, P.getString(), indent);
+
+  // Finish up.
+  --indent;
+  Indent(o, indent) << "</dict>\n";
+}
+
 //===----------------------------------------------------------------------===//
 // Static function definitions.
 //===----------------------------------------------------------------------===//
 
 /// Print coverage information to output stream {@code o}.
 /// May modify the used list of files {@code Fids} by inserting new ones.
 static void printCoverage(const PathDiagnostic *D,
                           unsigned InputIndentLevel,

lib/StaticAnalyzer/Core/SarifDiagnostics.cpp

static json::Object createLocation(json::Object &&PhysicalLocation,
   json::Object Ret{{"physicalLocation", std::move(PhysicalLocation)}};
   if (!Message.empty())
     Ret.insert({"message", createMessage(Message)});
   return Ret;
 }
 
 static Importance calculateImportance(const PathDiagnosticPiece &Piece) {
   switch (Piece.getKind()) {
-  case PathDiagnosticPiece::Kind::Call:
-  case PathDiagnosticPiece::Kind::Macro:
-  case PathDiagnosticPiece::Kind::Note:
+  case PathDiagnosticPiece::Call:
+  case PathDiagnosticPiece::Macro:
+  case PathDiagnosticPiece::Note:
+  case PathDiagnosticPiece::PopUp:
     // FIXME: What should be reported here?
     break;
-  case PathDiagnosticPiece::Kind::Event:
+  case PathDiagnosticPiece::Event:
     return Piece.getTagStr() == "ConditionBRVisitor" ? Importance::Important
                                                      : Importance::Essential;
-  case PathDiagnosticPiece::Kind::ControlFlow:
+  case PathDiagnosticPiece::ControlFlow:
     return Importance::Unimportant;
   }
   return Importance::Unimportant;
 }
 
 static json::Object createThreadFlow(const PathPieces &Pieces,
                                      json::Array &Files) {
   const SourceManager &SMgr = Pieces.front()->getLocation().getManager();

test/Analysis/analyzer-config.c

 // RUN: %clang_analyze_cc1 -analyzer-checker=debug.ConfigDumper > %t 2>&1
 // RUN: FileCheck --input-file=%t %s --match-full-lines
 
 // CHECK: [config]
+// CHECK-NEXT: add-pop-up-notes = true
 // CHECK-NEXT: aggressive-binary-operation-simplification = false
 // CHECK-NEXT: alpha.clone.CloneChecker:IgnoredFilesPattern = ""
 // CHECK-NEXT: alpha.clone.CloneChecker:MinimumCloneComplexity = 50
 // CHECK-NEXT: alpha.clone.CloneChecker:ReportNormalClones = true
 // CHECK-NEXT: alpha.security.MmapWriteExec:MmapProtExec = 0x04
 // CHECK-NEXT: alpha.security.MmapWriteExec:MmapProtRead = 0x01
 // CHECK-NEXT: avoid-suppressing-null-argument-paths = false
 // CHECK-NEXT: c++-allocator-inlining = true

 // CHECK-NEXT: stable-report-filename = false
 // CHECK-NEXT: suppress-c++-stdlib = true
 // CHECK-NEXT: suppress-inlined-defensive-checks = true
 // CHECK-NEXT: suppress-null-return-paths = true
 // CHECK-NEXT: unix.DynamicMemoryModeling:Optimistic = false
 // CHECK-NEXT: unroll-loops = false
 // CHECK-NEXT: widen-loops = false
 // CHECK-NEXT: [stats]
-// CHECK-NEXT: num-entries = 84
+// CHECK-NEXT: num-entries = 85