[PATCH] D12726: [analyzer] A fix for symbolic element region index lifetime.

[Artem Dergachev via cfe-commits]

NoQ updated this revision to Diff 37263.
NoQ added a comment.

Hmm, i think i'm ready to explain most of the stuff.

- First of all, the piece of code in `EnvironmentManager::removeDeadBindings()` i mentioned above is truly useless; everything would be marked as live anyway on the next line.

- Then, in `RegionStore`, in `VisitBinding()`, `markLive()` of the whole region value inside the binding is indeed correct, and i provide a pretty straightforward test;

- Finally, in `VisitCluster()`, it's a bit more complicated, and i suggest that `markLive()` is unnecessary here, and in fact does very little if anything, so it's hard to test even with stuff we have now:
  - i guess we shouldn't think of a region as live, simply because it has bindings, in a procedure designed for removing bindings;
  - however, marking the region as live here wouldn't save the bindings; they would later be removed simply because they weren't visited;
  - liveness of the region would also extend lifetime of a few symbols that depend on it, such as its `SymbolRegionValue`, `SymbolExtent`, and `SymbolMetadata`, however because the binding would be dead anyway, these symbols would die on the next pass;
    - note that `SymbolRegionValue` should explicitly be dead when the region has other bindings, so we certainly shouldn't try to preserve the region only to save `SymbolRegionValue`;
      - see also http://reviews.llvm.org/D5104 ; i also think i accidentally wrote the missing test for this ticket, will put it there;
  - another place that relies on region liveness information collected on these passes is Dynamic Type Propagation, however the same arguments apply; it would only delay the inevitable.

So the real question is whether (or rather how) the `Store` should maintain correct region liveness information after completing its internal garbage collection pass, because there are, in fact, other users of this information later in the chain, but this seems to be a larger problem without instantly noticeable effects.

The new diff removes dead code in `Environment` and tests and fixes the separate bug in the `Store` that caused reaping of constraints on `SymbolRegionValue` (and a few other kinds of `SymbolData`) too early when the only place where the related region is stored is a store value.


http://reviews.llvm.org/D12726

Files:
  docs/analyzer/DebugChecks.rst
  include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
  lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
  lib/StaticAnalyzer/Core/Environment.cpp
  lib/StaticAnalyzer/Core/RegionStore.cpp
  lib/StaticAnalyzer/Core/SymbolManager.cpp
  test/Analysis/return-ptr-range.cpp
  test/Analysis/symbol-reaper.c

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D12726.37263.patch
Type: text/x-patch
Size: 10763 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20151013/152bbbe8/attachment-0001.bin>