jordan_rose added a comment.
So we just throw out the whole notion of "metadata in use"? That can work.
There are two things I'd be concerned about changing here:
- What happens when you take a string's length, test it against something,
throw it away, then remeasure the string? In this case we'd be okay because
CStringChecker still hangs on to the symbol in its map until the region is
invalidated.
- Will we keep more symbols alive than we did previously, increasing the
analyzer's memory use? I think the answer is "yes", but not by very much: the
live/dead checking is two-pass, so we'll mark a metadata symbol live in
`checkLiveSymbols` but then find out we could have dropped it in
`checkDeadSymbols` when the region is found to be dead. It'll get cleaned up
the next time around, but I think that sort of conditional liveness is what the
old mechanism was trying to accomplish.
Do you have any ideas for how we could make that better? I can think of a
complicated case where you use `markInUse` if the metadata symbol exactly
matches the key, but `markLive` otherwise…but at some point I'm not sure it's
worth the complexity cost. (In particular, after this change, I'm not sure
metadata symbols behave any differently from regular conjured symbols. This is
a good thing.)
(We've also talked about having better REGISTER_MAP_WITH_PROGRAMSTATE that
auto-remove any keys for dead symbols or dead regions. I'm sure Anna and Devin
would be happy to see someone pick that up.)
================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2137-2141
@@ -2139,10 +2136,7 @@
- CStringLengthTy::Factory &F = state->get_context<CStringLength>();
- for (CStringLengthTy::iterator I = Entries.begin(), E = Entries.end();
- I != E; ++I) {
- SVal Len = I.getData();
- if (SymbolRef Sym = Len.getAsSymbol()) {
- if (SR.isDead(Sym))
- Entries = F.remove(Entries, I.getKey());
- }
+ auto &F = state->get_context<CStringLength>();
+ for (auto I = Entries.begin(), E = Entries.end(); I != E; ++I) {
+ const MemRegion *MR = I.getKey();
+ if (!SR.isLiveRegion(MR))
+ Entries = F.remove(Entries, MR);
}
----------------
Huh, this is probably an improvement anyway!
Repository:
rL LLVM
http://reviews.llvm.org/D14277
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
