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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 181961.
NoQ marked 12 inline comments as done.
NoQ added a comment.

Fix stuff.

In D56632#1356163 <https://reviews.llvm.org/D56632#1356163>, @baloghadamsoftware wrote:

> Did you measure decrease in the false-positive rate or an increase in the true-positive rate on real code? I expect some.


In progress :)

In D56632#1356249 <https://reviews.llvm.org/D56632#1356249>, @xazax.hun wrote:

> > 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.
>
>
>
>   I think this is non-obvious. If we had all the information it would make perfect sense to have a dead field in an alive struct. But since the liveness analysis is intraprocedural and we cannot know what happens to a struct when it escapes we have no choice but keep the field alive. A more sophisticated analysis (which is also likely to be more expensive) could have dead fields in an alive struct in case the struct never escapes. 


Great point indeed! I vaguely remember that some other tools do actually work that way. If a field is not referenced anywhere in the path and there's no weird pointer arithmetic going on upon the variable, we can actually diagnose a leak of the value within the field *before* the variable itself dies. Even intraprocedurally, we can just handle escapes and still do slightly better than we do now. This gets really valuable when the variable is, say, a non-escaping static (local or global). The variable never dies, but there may still be no deallocation for the field anywhere in the code and we may be able to see this within the translation unit. This doesn't have to have anything to do with fields though, the variable itself may carry the leaking pointer. Same with private fields regardless of storage. Neat food for thought.

Added a comment. Is there anything else worth documenting here, other than "the whole damn thing"?

>> 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.
> 
> I did not really follow, but probably my understanding of how memory regions work is not correct. If we work with base regions, why do we still need to scan the whole super-region chain?

It's just that `scanReachableSymbols` is written that way, and it's used everywhere for these kind of purposes. I.e., we (i.e., `SymbolReaper`) don't (i.e., doesn't) need to scan the whole chain of regions (apart from the `markElementIndicesLive()` thing... wait a minute, does it work in the opposite direction? - i.e., if an array-typed region is live, does it automatically mean that all index symbols in all element regions within it are actually treated as live everywhere in the program state? - need to check), but this behavior is re-used from other users `scanReachableSymbols` (wait a minute, do any of the other users actually need that? - i'm not immediately seeing any user actually need that, it seems that *everybody* operates on base regions only - but probably it's anyway not that much slower than jumping to the base region directly - need to check).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56632/new/

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.181961.patch
Type: text/x-patch
Size: 11456 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190116/d335acca/attachment-0001.bin>