Page MenuHomePhabricator

[analyzer] Extend bug reports with extra notes.
ClosedPublic

Authored by NoQ on Sep 6 2016, 1:20 PM.

Details

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:

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.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ updated this revision to Diff 70457.Sep 6 2016, 1:20 PM
NoQ retitled this revision from to [analyzer] Extend bug reports with extra notes..
NoQ updated this object.
NoQ added a subscriber: cfe-commits.
v.g.vassilev edited edge metadata.Sep 8 2016, 2:17 PM

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 ↗(On Diff #70457)

Extra new line?

a.sidorin edited edge metadata.Sep 9 2016, 11:25 AM

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 ↗(On Diff #70457)

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 ↗(On Diff #70457)

const auto &Piece : path?

lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
117 ↗(On Diff #70457)

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

119 ↗(On Diff #70457)

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

...
...

}
?

bcraig added a subscriber: bcraig.Sep 9 2016, 11:31 AM

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 added a comment.Sep 9 2016, 11:46 AM

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 ↗(On Diff #70457)

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

zaks.anna edited edge metadata.Sep 9 2016, 8:32 PM

Thanks!

Looks good overall. Several comments below.

lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
160 ↗(On Diff #70457)

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 ↗(On Diff #70457)

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

lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
119 ↗(On Diff #70457)
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 edited edge metadata.Sep 16 2016, 1:05 PM

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 updated this revision to Diff 72480.Sep 26 2016, 7:31 AM
NoQ edited edge metadata.

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 updated this object.Sep 26 2016, 7:45 AM
NoQ updated this object.
NoQ updated this revision to Diff 72519.Sep 26 2016, 10:20 AM
NoQ marked 10 inline comments as done.

Addressed inline comments. Renamed extra notes to notes.

zaks.anna accepted this revision.Sep 27 2016, 11:04 AM
zaks.anna edited edge metadata.

I have no further comments.

This revision is now accepted and ready to land.Sep 27 2016, 11:04 AM
This revision was automatically updated to reflect the committed changes.