[PATCH] D42672: [CFG] [analyzer] Heavier CFGConstructor elements.

Mon Jan 29 17:52:57 PST 2018

NoQ created this revision.
NoQ added reviewers: rsmith, dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet.
Herald added subscribers: cfe-commits, rnkovacs.

I'm planning to introduce some sort of `CFGConstructor` element, which would help the analyzer (and otherwise whoever enables it - i.e. i don't have plans to enable it for sema warnings) to understand how to perform construction of a C++ object without peeking at the next `CFGElement`(s) or ascending through `ParentMap` to the parent statements. Because we're doing the worklist thing rather than recursive traversal of the AST, we cannot easily guess, by looking at the `CXXConstructExpr`, where does it construct into, so having some sort of context seems to be extremely handy.

Because there are so many ways of constructing an object in C++, this element may eventually become quite beefy. For now in the analyzer we only have simple cases in the analyzer, such as construction into a single variable's `DeclStmt` or into `CXXCtorInitializer` or into `CXXNewExpr`; in these cases having just a single parent statement is enough - so the plan is to improve upon this (http://lists.llvm.org/pipermail/cfe-dev/2018-January/056691.html). Aggregate initialization expressions may contain multiple constructions at once, so for those we'd need extra data to figure out what part of the "whole thing" (element, field, base) is being constructed by the current constructor; in these cases the "whole thing" is not necessarily the initializer list as a temporary - it may still be, say, eventually turned into a plain variable, so this whole context would be expressed. Hopefully this'd also help us solve the problematic `CXXDefaultArgExpr` problem - where the constructor nested within it is in fact not even a part of the current function's AST, which screws a //lot// of our invariants.

So for now i propose to allow me to //allocate extra data for// `CFGConstructor` //in the CFG's// `BumpPtrAllocator`, so that i had enough room for incremental development without constantly thinking about bit manipulations; i eventually revert this decision if i notice that two pointers are indeed enough to hold all the data we need, but for now it seems highly unlikely. On the other hand, i'm fine with replacing it with a pair of regular AST node pointers until i actually need more room.

The auxiliary data class is called `ConstructionContext` (not prefixing it with `CFG` or whatever) because i don't plan on keeping it contained within CFG, but rather passing it around actively in the analyzer. In particular, i believe that `ConstructionContext` should replace `CXXBindTemporaryExpr` as a (more) //unique identifier of a temporary object// (for instance, with a backreference to the call site in the `CXXDefaultArgExpr` case). So, hopefully we'd even be able to use it during destruction.

`CFGConstructor` is not a sub-class of `CFGStmt`. It'd be much easier to use in the analyzer if it was a sub-class, but it'd make `CFGStmt`'s `isKind()` slower, which will affect compilation time regardless of whether `CFGConstructor`s are actually enabled.

The new `CFGConstructor` element //takes place of// the respective `CFGStmt` element whenever we managed to compute its respective `ConstructionContext`. In order to demonstrate how it works, i added computation of `ConstructionContext` for constructors into `CXXNewExpr`s. This is far from enough for replacing existing mechanisms in the analyzer, but in a couple of patches i hope i'd be able to actually convert the analyzer to the new behavior. For now the analyzer remains pretty much untouched - just prepared to see the new element.

`LiveVariables` needed a slight fix as well. If my grep-fu doesn't fail me, it's only used by the analyzer. So only analyzer should be affected.

It might be possible to replace `CFGAllocator` completely with the new `ConstructionContext` (inline both the allocator and the constructor during evaluation of `CFGConstructor`), but i didn't try, and i'm not convinced that it's a good idea.

I made the constructor of `CFGStmt` and `CFGInitializer` explicit because it took me an hour to catch a bug when i tried to re-use `ExprEngine::ProcessStmt()` to process the construct-expression.

Repository:
  rC Clang

https://reviews.llvm.org/D42672

Files:
  include/clang/Analysis/AnalysisDeclContext.h
  include/clang/Analysis/CFG.h
  include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  lib/Analysis/AnalysisDeclContext.cpp
  lib/Analysis/CFG.cpp
  lib/Analysis/LiveVariables.cpp
  lib/StaticAnalyzer/Core/AnalysisManager.cpp
  lib/StaticAnalyzer/Core/CoreEngine.cpp
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  lib/StaticAnalyzer/Core/PathDiagnostic.cpp
  test/Analysis/cfg.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D42672.131903.patch
Type: text/x-patch
Size: 22330 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180130/cf8f4fd1/attachment-0001.bin>