Page MenuHomePhabricator

[analyzer] Fix a couple of bugs in HTML report generation.
ClosedPublic

Authored by NoQ on Feb 4 2020, 1:27 PM.

Details

Summary

This patch fixes an accidental redundant </td> and disables generation of variable popups within macro popups due to https://bugs.llvm.org/show_bug.cgi?id=44782

I also added some tests for our HTML files because those were pretty much non-existent so far. Given that we can now also test scan-build (i actually landed D69781 yesterday), there are no more excuses for not writing tests. Test variable-popups.c was passing previously, so it's here just for the sake of finally having a test, while the other two tests were failing on their respective CHECK-NOT directives.

Another interesting thing i did was apply tidy-html5 to our HTML files. They seem to satisfy the linter now, and btw that's how i found the first issue (while trying to reduce the second issue by running tidy-html5 under creduce).

@Charusso: I didn't manage to understand how PopUpRanges get passed around and why, so i removed them. If you have an example of where it matters, please send tests :)

Diff Detail

Event Timeline

NoQ created this revision.Feb 4 2020, 1:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2020, 1:27 PM

Thanks for the fix! The PopUpRanges is very important, please revert it back in its original shape. Sorry for the inconvenience.

I have ran a quick scan-build with this patch on LLVM because I wanted to give you a real world example (which you cannot visibly see at the test file). Here it is:

The idea of ranges was that to add a new entry into a pop-up note as a table so you could inject any kind of pop-up information in order on the same range. Also if we highlighted the range as a pop-up do not highlight it in gray as well (which could break the HTML). Back in the days I did not realize the importance of the Doxygen and test files, I did not really know how to do so. Here I have adjusted the comments a little-bit:

+ /// Create the pop-up notes' table's start tag.
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);
  }
}

+ /// Inject a new entry into the pop-up notes' table or add the table's end tag.
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 no report made at the current range \c Range we could inject the table's end tag as this is the last report on that range. Also this is the first encounter of the range, after that it is safe to insert new entries to the table.
  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);
  } else {
-    // Otherwise inject just the new row at the end of the range.
+    // Otherwise we are working with multiple notes at the same range, so inject a new row to the table at the end of the range which is the beginning of the table. With that we fill the table "upwards" which is the same order as we emit reports.
    html::HighlightRange(R, Range.getBegin(), Range.getEnd(), "", Buf.c_str(),
                         /*IsTokenRange=*/true);
  }
}

Please rename the variable-popups.c to variable-popups-simple.c, variable-popups-2.c to variable-popups-macro.c, and here variable-popups-multiple.c comes:

// RUN: rm -fR %t
// RUN: mkdir %t
// RUN: %clang_analyze_cc1 -analyzer-checker=core \
// RUN:                    -analyzer-output=html -o %t -verify %s
// RUN: cat %t/report-*.html | FileCheck %s

void bar(int);

void foo() {
  int a;
  for (unsigned i = 0; i < 3; ++i)
    if (i)
      bar(a); // expected-warning{{1st function call argument is an uninitialized value}}
}

// CHECK:      <span class='variable'>i
// CHECK-SAME:   <table class='variable_popup'><tbody><tr>
// CHECK-SAME:     <td valign='top'>
// CHECK-SAME:       <div class='PathIndex PathIndexPopUp'>2.1</div>
// CHECK-SAME:     </td>
// CHECK-SAME:     <td>'i' is 0</td>
// CHECK-SAME:   </tr>
// CHECK-SAME:   <tr>
// CHECK-SAME:     <td valign='top'>
// CHECK-SAME:       <div class='PathIndex PathIndexPopUp'>4.1</div>
// CHECK-SAME:     </td>
// CHECK-SAME:     <td>'i' is 1</td>
// CHECK-SAME:   </tr></tbody></table>
// CHECK-SAME: </span>
Charusso added inline comments.Feb 4 2020, 3:20 PM
clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
967

Here the gray highlighting goes, so the PopUpRanges store whether we have already highlighted the range and we early-continue. The cool thing is, we do not check for intersections because the pop-up range has a smaller scope than the entire expression's gray range so the HTML handles the colors/mouse-hover for us. In case of macro pop-ups the coloring and-or pop-upping was already buggy, they are handled differently, where I have not found such a cool workaround.

NoQ updated this revision to Diff 242466.Feb 4 2020, 4:43 PM

Aha, that's what those are! Great. I thought they're only for resolving conflicts with range highlights, which seemed kinda redundant.

Addressed all comments :)

Charusso accepted this revision.Feb 4 2020, 5:00 PM

Cool, thanks you!

clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
612

Side note: I like the other form of De Morgan's laws because here I have to apply it in my head every time I see such a construct. Also we are using this function in negation, so I would write:

static bool isMacro(const SourceRange &Range) {
  return Range.getBegin().isMacroID() || Range.getEnd().isMacroID();
}

The idiom is to write code for readability so that understandability over everything else.

This revision is now accepted and ready to land.Feb 4 2020, 5:00 PM
This revision was automatically updated to reflect the committed changes.
NoQ marked an inline comment as done.Feb 5 2020, 6:21 AM
NoQ added inline comments.
clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
612

Objection :) I want this function to figure out if a pop-up range should be displayed, not whether the range is in a macro. Like, if we come up with other excuses for not displaying a range, we'll update this function, not make a new one. And if we need to check whether something is a macro in another place, we'll make a new function, not re-use this one.

I could do something like shouldSkipPopUpRange() but that'd move the confusing negation into the function name, making call sites harder to read.