[PATCH] D81407: [Analyzer][StreamChecker] Add note tags for file opening.

[Balázs Kéri via Phabricator via cfe-commits]

balazske marked 3 inline comments as done.
balazske added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:406
 
+const ExplodedNode *StreamChecker::getAcquisitionSite(const ExplodedNode *N,
+                                                      SymbolRef StreamSym,
----------------
NoQ wrote:
> NoQ wrote:
> > Ok, so this is a tiny auxiliary visitor.
> > 
> > I wish we could set uniqueing location post-factum from within the note tag. Unfortunately we can't because notes are generated after uniqueing :(
> > 
> > I'd like you to experiment with tracking this information as part of the state instead so that you didn't need to perform an additional scan. I'd be happy if it helps you avoid performing this additional pass.
> > 
> > I.e., when you're opening the file, add all the information you need for building your uniqueing location into the stream state (not the node though, just the program point or something like that). Then retrieve it in O(1) when you're emitting the report.
> Another thing we could try is to implement such post-processing pass globally for all checkers. I.e., instead of an optional uniqueing location accept an optional lambda that looks at a node and answers whether this node should be used as a uniqueing location. Then before report deduplication scan each report that supplies such lambda and update its uniqueing location. That'll probably require some work on BugReporter but that sounds like a nice way to avoid all the boilerplate.
Storing the "acquisition site" in the state is the natural way of doing this. Probably I should not think that existing checkers do things the best way, this function is taken from  `FuchsiaHandleChecker` (or here it has a specific reason?). And not storing the data in the state is a bit less memory consumption if this matters.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:406
 
+const ExplodedNode *StreamChecker::getAcquisitionSite(const ExplodedNode *N,
+                                                      SymbolRef StreamSym,
----------------
balazske wrote:
> NoQ wrote:
> > NoQ wrote:
> > > Ok, so this is a tiny auxiliary visitor.
> > > 
> > > I wish we could set uniqueing location post-factum from within the note tag. Unfortunately we can't because notes are generated after uniqueing :(
> > > 
> > > I'd like you to experiment with tracking this information as part of the state instead so that you didn't need to perform an additional scan. I'd be happy if it helps you avoid performing this additional pass.
> > > 
> > > I.e., when you're opening the file, add all the information you need for building your uniqueing location into the stream state (not the node though, just the program point or something like that). Then retrieve it in O(1) when you're emitting the report.
> > Another thing we could try is to implement such post-processing pass globally for all checkers. I.e., instead of an optional uniqueing location accept an optional lambda that looks at a node and answers whether this node should be used as a uniqueing location. Then before report deduplication scan each report that supplies such lambda and update its uniqueing location. That'll probably require some work on BugReporter but that sounds like a nice way to avoid all the boilerplate.
> Storing the "acquisition site" in the state is the natural way of doing this. Probably I should not think that existing checkers do things the best way, this function is taken from  `FuchsiaHandleChecker` (or here it has a specific reason?). And not storing the data in the state is a bit less memory consumption if this matters.
We should check how many checkers can benefit from such a "uniqueing location callback". Normally the checker should know what the uniqueing location is, at least when the bug report is created. The location is naturally obtained from the state that the checker maintains, at least if we want to avoid scans like `getAcquisitionSite`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81407