[PATCH] D18860: [analyzer] Fix the "Zombie symbols" issue.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: lib/StaticAnalyzer/Core/Environment.cpp:180
-      for (; SI != SE; ++SI)
-        SymReaper.maybeDead(*SI);
     }
----------------
zaks.anna wrote:
> We are removing this because the maybeDead is no longer used, correct?
Yup.


================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:390
-
-  } else {
-    // Call checkers with the non-cleaned state so that they could query the
----------------
zaks.anna wrote:
> This removes an optimization to address the following issue: 
> "removeDeadBindings is not run right after the last reference to the object is lost, which leads to imprecise error reports and failure to report the leak in some cases. It's because of how hasDeadSymbols() is implemented. That problem is solved if we disable the optimization, but I do not know how that effects performance. Maybe we can come up with something more clever.
> "
> I suspect the removal of this optimization causes the performance regression. In the patch I sent to the list, this was just a hack to demonstrate what causes the issue. I am not sure we should not just remove the optimization... The best proposal I have is to trigger remove dead bindings at the end of every basic block. This would degrade diagnostics (you will see leaks only at the end of the basic block), but should give us performance back or even improve performance.
> 
> Artem and Devin, WDYT?
> 
> Artem, can you experiment with this and investigate if the diagnostics become much worse? Maybe send a couple of examples? I suggest we implement this mode behind a flag as a separate patch.
This cannot be kept around because `.hasDeadSymbols()` cannot be implemented correctly.


================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:2452
-        SymReaper.maybeDead(*SI);
-    }
   }
----------------
zaks.anna wrote:
> Looks like we are saying that we should no longer add to maybeDead because it's not used.
Yup.


https://reviews.llvm.org/D18860