[cfe-dev] [analyzer] Zombie symbols.

[Artem Dergachev via cfe-dev]

Hmm, this sounds like a very reasonable approach. Will see what i can 
do, shouldn't be very hard. In fact, !isLive() should be enough once all 
the liveness info is collected. Due to first-class relations, amounts of 
live and dead symbols are typically infinite, and this approach takes 
care of that. Still, by automating the old approach by exploring the 
checker GDM we could achieve better performance due to the short-path 
optimization below (but not due to lookup direction in checkers, which 
is seemingly optimized through dead_begin()..dead_end(), because we'd 
have to iterate through the state in checkLiveSymbols phase anyway).

On 31.03.2016 04:23, Anna Zaks wrote:
> Ted pointed out that isLive covers the symbols that do not exist in 
> the state. Have you tried addressing the issue with something like this:
>
> diff --git 
> a/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h 
> b/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
> index 30481ea..1c5a984 100644
> --- a/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
> +++ b/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
> @@ -550,8 +550,8 @@ public:
>    ///
>    /// This should only be called once all marking of dead symbols has 
> completed.
>    /// (For checkers, this means only in the evalDeadSymbols callback.)
> - bool isDead(SymbolRef sym) const {
> - return TheDead.count(sym);
> + bool isDead(SymbolRef sym) {
> + return TheDead.count(sym) || !isLive(sym);
>    }
>
> Almost all tests pass with this change and the one that does not looks 
> like a legitimate new leak reported.
>
> There are a couple of issues to further investigate here:
> -  What are the performance implications? I'd expect some, but they 
> are probably not very bad. Still we should measure.
> -  We should probably eliminate the use of dead_begin dead_end, which 
> some checkers reply on.
> -  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.
>
> diff --git a/lib/StaticAnalyzer/Core/ExprEngine.cpp 
> b/lib/StaticAnalyzer/Core/ExprEngine.cpp
> index 0b66933..e43c5b7 100644
> --- a/lib/StaticAnalyzer/Core/ExprEngine.cpp
> +++ b/lib/StaticAnalyzer/Core/ExprEngine.cpp
> @@ -380,14 +380,7 @@ void ExprEngine::removeDead(ExplodedNode *Pred, 
> ExplodedNodeSet &Out,
>    // Process any special transfer function for dead symbols.
>    // A tag to track convenience transitions, which can be removed at 
> cleanup.
>    static SimpleProgramPointTag cleanupTag(TagProviderName, "Clean Node");
> -  if (!SymReaper.hasDeadSymbols()) {
> - // Generate a CleanedNode that has the environment and store cleaned
> - // up. Since no symbols are dead, we can optimize and not clean out
> - // the constraint manager.
> - StmtNodeBuilder Bldr(Pred, Out, *currBldrCtx);
> - Bldr.generateNode(DiagnosticStmt, Pred, CleanedState, &cleanupTag, K);
> -
> -  } else {
> +  {
>
> Artem, let me know if you'd like to finish the work on this; otherwise 
> either me or Devin could conduct the remaining investigations.
>
> Cheers,
> Anna.
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20160331/392fe9f0/attachment.html>