[PATCH] D56632: [analyzer] Track region liveness only through base regions.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, george.karpenkov, rnkovacs, mikhail.ramalho, Szelethus, baloghadamsoftware.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, a.sidorin, szepet, mgorny.

This is a follow-up to D56042 <https://reviews.llvm.org/D56042>.

When a memory region is live, all its sub-regions and super-regions are automatically live (and vice versa), so it is only necessary to track liveness of base regions. This is exactly how we imagined this to work, but it turns out that it didn't.

The reason why it works correctly most of the time because the reachable symbol scanner is automatically marks all parent regions as reachable - and therefore they are marked as live by adding all of them to region roots every time a sub-region is marked as live. However, enumerating all *child* regions this way is problematic (there may be infinitely many of them).

In the test from D56042 <https://reviews.llvm.org/D56042>, the symbol for `p.x` dies because when `.get()` is called for the last time only `p` is kept alive by the Environment, but not `p.x`. Due to that,  `reg_$0<p.x>` is believed to be dead - recall that `SymbolRegionValue` is kept alive by its parent region, and additionally notice that there are no explicit bindings anywhere else to keep it alive (because `SymbolRegionValue` is simply assumed to be there as long as the parent region is alive, rather than bound explicitly).

Now, when the `RegionRoots` test fails, `isLiveRegion()` falls through to see if the region is "lexically" alive. Here it correctly jumps to the base region `p` and looks if live variables analysis thinks it's alive. It doesn't! Because there's no need to compute expression 'p' anymore anywhere in the program.

What `isLiveRegion()` should have done is look up the base region in `RegionRoots`. But it doesn't. Hence the patch.

The newly added `test_A` demonstrates the problem even more clearly: having the symbol for `a.x` die before the call to `a.foo()` is definitely incorrect.

The other test, `test_B`, is an attempt to figure out whether the problem is also there "in the opposite direction". That is, when `b.a` is marked live, is `b` marked live automatically? Otherwise the lookup in `RegionRoots` would still fail.

The answer is yes, it does work correctly, because `scanReachableSymbols` always scans the whole super-region chain of every region. Which means that every time the Environment or the Store marks a region as live, all of its super-regions are added to `RegionRoots`. However, i would still like to add conversion to the base region into `markLive()`, because this behavior is something that should be guaranteed by `SymbolReaper` rather implied by callers manually, even if the callers have to do that anyway.

So for now the change in `markLive()` does not affect functionality at all, but it will be important when checkers use the `checkLiveSymbols()` functionality more aggressively. Additionally it slightly decreases the size of the `RegionRoots` map for faster lookups but adds an extra time overhead for marking something as live (need to ascend to the base region). I didn't try to figure out whether it gives a net gain in performance.

For that reason the unit test as well. Also a few convenient getters were added to `ExprEngine` in order to make the test more concise.


Repository:
  rC Clang

https://reviews.llvm.org/D56632

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  lib/StaticAnalyzer/Core/SymbolManager.cpp
  test/Analysis/diagnostics/dtors.cpp
  test/Analysis/symbol-reaper.cpp
  unittests/StaticAnalyzer/CMakeLists.txt
  unittests/StaticAnalyzer/SymbolReaperTest.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D56632.181418.patch
Type: text/x-patch
Size: 11167 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190112/013092e9/attachment-0001.bin>