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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to