[PATCH] D14277: [Analyzer] Make referenced SymbolMetadata live even if its region is dead

[Jordan Rose via cfe-commits]

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