This revision was automatically updated to reflect the committed changes.
Closed by commit rL255236: [analyzer] Fix symbolic element index lifetime.
(authored by dergachev).
Changed prior to commit:
http://reviews.llvm.org/D12726?vs=37267=42400#toc
Repository:
rL LLVM
NoQ added a comment.
In http://reviews.llvm.org/D12726#303122, @zaks.anna wrote:
> > So the real question is whether (or rather how) the Store should maintain
> > correct region liveness information
>
> > after completing its internal garbage collection pass, because there are,
> > in fact,
zaks.anna accepted this revision.
zaks.anna added a comment.
This revision is now accepted and ready to land.
LGTM.
(Feel free to add comments to the existing code!)
> So the real question is whether (or rather how) the Store should maintain
> correct region liveness information
> after
NoQ updated this revision to Diff 37267.
NoQ added a comment.
A, another mis-submit. Sorry for the noise.
http://reviews.llvm.org/D12726
Files:
docs/analyzer/DebugChecks.rst
include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
NoQ updated this revision to Diff 37263.
NoQ added a comment.
Hmm, i think i'm ready to explain most of the stuff.
- First of all, the piece of code in `EnvironmentManager::removeDeadBindings()`
i mentioned above is truly useless; everything would be marked as live anyway
on the next line.
-
NoQ updated this revision to Diff 37266.
NoQ added a comment.
Whoops accidentally left out a dead code line in tests.
http://reviews.llvm.org/D12726
Files:
docs/analyzer/DebugChecks.rst
include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
NoQ updated this revision to Diff 36946.
NoQ marked 4 inline comments as done.
NoQ added a comment.
Thanks for the review! Fixed all comments. I don't notice any significant
performance hit with this patch (+/- 2% on the whole Android codebase runs).
I guess i won't break this into separate
zaks.anna added a comment.
Now that we have a way to test symbol reaper, please, add more coverage to the
symbol-reaper.c test, including the test that Jordan mentioned. Even if it is
not fixed, it's good to include it with a FIXME note.
What is the performance impact of this change?
The
NoQ updated this revision to Diff 35067.
NoQ added a comment.
Thanks for the quick reply, sorry for the delay! Was afk for a couple of days.
Yeah, right, in fact i didn't even fix the issue for store keys at all; only
for store values and environment values.
It also seems much harder to test
jordan_rose added a comment.
Hm, interesting. I'm not sure this is even sufficient, though: what if I have a
FieldRegion that's a sub-region of an ElementRegion with a symbolic index?
RegionStore does everything by base regions, so we won't ever see that
intermediate region with the symbolic
NoQ created this revision.
NoQ added reviewers: zaks.anna, krememek.
NoQ added subscribers: cfe-commits, dergachev.a.
In Clang Static Analyzer, when the symbol is referenced by an index value of an
element region, it does not prevent garbage collection (reaping) of that
symbol. Hence, if the
11 matches
Mail list logo
