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
10 matches
Mail list logo