[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:731
+  }
+  return C.getNoteTag([Text, Name](BugReport &BR) -> std::string {
+      SmallString<256> Msg;
----------------
baloghadamsoftware wrote:
> Szelethus wrote:
> > baloghadamsoftware wrote:
> > > NoQ wrote:
> > > > Szelethus wrote:
> > > > > NoQ wrote:
> > > > > > baloghadamsoftware wrote:
> > > > > > > NoQ wrote:
> > > > > > > > You'll need to check whether the container is actually of interest to the bug report. We don't want notes to be added about changes to irrelevant containers.
> > > > > > > > 
> > > > > > > > You can use a combination of "Report `BR` was emitted by one of the iterator checkers" and "The memory region of the container is marked as interesting" (while also actually marking it as interesting in the checker).
> > > > > > > > 
> > > > > > > > Ideally we should instead make a new generic storage inside the `BugReport` object, in order to pass down the interesting information from the call site of `emitReport` ("Hi, i'm an iterator checker who emitted this report and i'm interested in changes made to the size of this container").
> > > > > > > Are you sure in this? I already wondered how it works so I added a test that checks one container and changes another one and there were no note tags displayed for the one we did not check but change. See the last test.
> > > > > > That's because you didn't do
> > > > > > ```lang=c++
> > > > > >   V2.cbegin();
> > > > > >   V2.cend();
> > > > > > ```
> > > > > > in the beginning.
> > > > > A similar conversation sparked up recently in between @boga95, @steakhal and me regarding reporting taintedness. Bug reports are fine up to the point where (in reverse) the first propagation happens, but finding out which value tainted the one that caused the report isn't handled at the moment. One idea was to mark the initial (again, in reverse) value as interesting, create a `NoteTag` at the point of propagation, where we should know which value was the cause of the spread, mark that interesting as well, etc.
> > > > > 
> > > > > If `NoteTag`s only emit a message when the concerning value is interesting, this should theoretically solve that problem. I guess you could say that we're propagating interestingness in reverse.
> > > > > 
> > > > > I'm not immediately sure if this idea was ever mentioned or implemented here.
> > > > Yes, that's the intended solution to such problems. `trackExpressionValue` works similarly, just with assignments instead of taint propagations. And in both cases note tags are a much more straightforward solution to the problem.
> > > Yes, you are right. My problem now is that how to mark interesting when debugging? I I filter for interesting containers only, I lose my ability to debug. Should I create a debug function just for marking a container as interesting. Or is there such function already?
> > I'm not immediately sure how interetingness ties into debugging, what specific scenario are you thinking about?
> In the test of the modeling checker we use debug checkers. They should be able to mark the container interesting to be able to test the not tags. I managed to solve problem, even in a somewhat unorthodox way.
The core issue with NoteTag it does not know about interestingness and nor about MemRegion. I believe everything based on MemRegions already and when you emit the report you know exactly which MemRegion raised an error. So I think first we need to solve that the NoteTags only report on given MemRegions and those regions of course mega-interesting: we do not need to keep around the interestingness then.


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

https://reviews.llvm.org/D73720