Szelethus added a comment.
Balázs, could you please add the checker option within this patch? If we find
that the option works well (removes a lot of useless reports) I'd be happy to
help implement that uniqueing pass.
CmdLineOption<Boolean,
"UniqueLeaks",
"Only display a single report for each leaked stream object,
rather than a report for each path of execution on which the same stream was
leaked",
"false",
InAlpha>,
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:406
+const ExplodedNode *StreamChecker::getAcquisitionSite(const ExplodedNode *N,
+ SymbolRef StreamSym,
----------------
NoQ wrote:
> balazske wrote:
> > 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`.
> > Probably I should not think that existing checkers do things the best way
>
> Well, if you ever find yourself copy-pasting a large chunk of code from a
> different checker and using it almost unchanged, it's a good indication that
> the checker API needs to be improved. You can't do things "the best way" in a
> single checker in isolation, it's a collective effort. You're a programmer,
> you can change everything, no need to be confined in your checker.
>
> > 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.
>
> No, i don't think it ever happens automagically. It's either tracked in the
> state specifically for that purpose or scanned backwards. So i'd rather
> believe that every checker that has non-default uniqueing locations will
> benefit from such facility.
> 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.
I've lately found the uniqueing location quite wonky, because its hardly ever
the location we want to unique by, but it is the only way to pull it off with
the existing infrastructure. More often then not the uniqueing point is a
variable, a stream object, or something that isn't a location, but can sort of
be tied to a location. So, lets go with this suggestion!
@balazske As long as this is an off-by-default hidden checker option, I think
we can commit this code for experimentation purposes. I believe it is correct
for 99% of the cases. But I agree, we need to tie its enabling to a more robust
solution.
Lets add some comments:
// HACK: This is essentially a tiny bugreporter visitor, but before the
trimming of the exploded graph (so it may contain directed cycles still). We
use it to acquire a uniqueing location, but it would be better if we could
unique by the actual ExplodedNode instead. We should probably implement a pass
before bug report generation that takes a lambda that looks at a node and
answers whether this node should be used as a uniqueing node in favor of the
currect location-based technique. Note that FuchsiaHandleChecker does something
very similar as well.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81407/new/
https://reviews.llvm.org/D81407
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
