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

[Anna Zaks via cfe-commits]

zaks.anna added a comment.

Thanks for working on this!

The main unfinished task here is to investigate ways of reducing the performance hit. See more info below.

> The patch was tested on Android open-source platform source code.


Just to double check, have you compare the pre and after results and all of them but this one issue are the same?

>   Performance has slightly degraded (~5%) - hmm


5% is a considerable regression :(


================
Comment at: lib/StaticAnalyzer/Core/Environment.cpp:180
@@ -179,3 +177,1 @@
-      for (; SI != SE; ++SI)
-        SymReaper.maybeDead(*SI);
     }
----------------
We are removing this because the maybeDead is no longer used, correct?

================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:390
@@ -389,3 @@
-
-  } else {
-    // Call checkers with the non-cleaned state so that they could query the
----------------
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.

================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:2452
@@ -2451,3 +2440,1 @@
-        SymReaper.maybeDead(*SI);
-    }
   }
----------------
Looks like we are saying that we should no longer add to maybeDead because it's not used.


http://reviews.llvm.org/D18860