[PATCH] D58367: [analyzer] NFC: Improve upon the concept of BugReporterVisitor.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ marked an inline comment as done.
NoQ added a comment.

In D58367#1425922 <https://reviews.llvm.org/D58367#1425922>, @Szelethus wrote:

> Would `NoteTag`s be displayed in a dumped exploded graph?


That's a great question. Tags themselves are always printed out, together with their description, and description for note tags is defined by this patch as follows:

  StringRef getTagDescription() const override {
    if (MemoizedMessage)
      return *MemoizedMessage;
    else
      return "Untriggered Note Tag";
  }

This "tag description" thing is only visible in exploded graph dumps; it doesn't have any other meaning. It is clear that it's very appealing to not only mention that the tag is attached, but also to print out the message it would produce. However, it is impossible to obtain the message until the specific bug report is provided. Not only the report must already be emitted, but also it needs to be chosen to represent its class of equivalent bug reports.

For now it means that if you simply do the usual `-analyzer-viz-egraph-graphviz` thing or the `debug.ViewExplodedGraph` thing, the graph will be visualized *before* the note tag lambdas are invoked, and it won't be able to show you the text:

F8456827: Screen Shot 2019-03-14 at 5.35.34 PM.png <https://reviews.llvm.org/F8456827>

However, if you take your debugger, break at the end of `BugReporter::FlushReport()` and execute `p ((GRBugReporter *)this)->Eng.ViewGraph(0)` (or `1` if you want it trimmed), it'll show you the exact message:

F8456835: Screen Shot 2019-03-14 at 5.33.55 PM.png <https://reviews.llvm.org/F8456835>

I think it'd be great to delay `-analyzer-viz-egraph-graphviz` so that all note tags were resolved by the time the graph is printed.

Now, why was this a great question? This question was great because it helped me realize that the memoization is a broken idea because the same lambda may produce multiple different messages if it participates in multiple bug reports. Which means we need a more sophisticated memoization, i.e. `map<BugReport *, Optional<string>>` or something like that (assuming that bug reports can actually be identified by pointers).

> In D58367#1402847 <https://reviews.llvm.org/D58367#1402847>, @NoQ wrote:
> 
>> In D58367#1402796 <https://reviews.llvm.org/D58367#1402796>, @Szelethus wrote:
>>
>> > 2. I guess most of the visitors could be changed to this format, do you have plans to convert them? I'll happily land a hand, because it sounds like a big chore. I guess that would also test this implementation fairly well.
>>
>>
>> I don't have an immediate plan but i'll definitely convert visitors when i touch them and get annoyed. Also i believe that this new functionality is much more useful for //core// visitors than for checker visitors, simply because there's much more information to reverse-engineer in every visitor.
> 
> 
> As I understand it, this solution could be used to entirely get rid of the current bugreporter visitor structure (at least for checkers), right? The discussion seems to conclude that this is just as general, far easier to understand, far easier to implement, and is basically better in every regard without an (//edit: significant//) hit to performance? Because if so, I'm definitely against supporting two concurrent implementations of the same functionality -- in fact, we should even just forbid checkers to add custom visitors.

I suspect so. Supporting two different implementations doesn't sounds scary because both of them are fairly easy (at least ever since George has eradicated the fix-point-iteration for mutually recursive visitors), but once note tags start landing and settling down, i'll definitely try to avoid accepting new visitors :)



================
Comment at: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp:165
 
-  C.addTransition(C.getState()->set<ReleasedParameter>(PVD));
+  const NoteTag *T = C.getNoteTag([this, PVD](BugReport &BR) -> std::string {
+    if (&BR.getBugType() != &BT)
----------------
xazax.hun wrote:
> I am not very familiar with this check but wonder don't you miss an "isInteresting" check somewhere? Where do we filter the notes that are unrelated to the bug paths?
Yeah, this check is special; it only tracks a single boolean flag in the program state and doesn't have a notion of interesting symbols or regions. Which is kinda why i started with this checker - it's easier than other checkers with the new approach (and harder than other checkers with the old approach).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58367/new/

https://reviews.llvm.org/D58367