[cfe-dev] [analyzer] Temporaries.

[Artem Dergachev via cfe-dev]

A bit of an update. Previous messages for easier navigation through this 
thread:

http://lists.llvm.org/pipermail/cfe-dev/2018-January/056691.html
http://lists.llvm.org/pipermail/cfe-dev/2018-February/056813.html
http://lists.llvm.org/pipermail/cfe-dev/2018-February/056898.html
http://lists.llvm.org/pipermail/cfe-dev/2018-February/056909.html
http://lists.llvm.org/pipermail/cfe-dev/2018-February/056929.html

== Overall status ==

Previous changes had minimal effects on the false positives i observed. 
However, once return-by-value and construction-conversion support has 
landed, improved modeling has finally yielded significant results, 
fixing at least 150 false positives (around 40% of all positives) on a 
few sub-projects of WebKit (incl. WebCore, JavaScriptCore).

Large chunks of the remaining false positives are related to brace 
initialization constructors which i'll try to support at least in simple 
cases (eg. a single layer of braces).

Many of the remaining reports are hard to understand - it seems that 
trackNullOrUndefValue isn't always doing a good job. Other reports are 
over-saturated with inlined temporary destructors: path pruning may be 
working incorrectly.

On other projects i've seen interesting bug reports against code that 
works incorrectly when copy elision isn't happening (of a poorly 
implemented class that performs memory management). We're capable of 
finding such bugs because we don't model copy elision yet. Such code is 
not portable, but many users that use just one compiler can afford 
relying on its implementation details, and copy elision is usually 
supported by most major compilers. These reports would probably go away 
because i'd like to model copy elision anyway because it's generally a 
good thing: it improves analyzer performance and reduces path lengths. 
And we'd still catch these bugs when non-elidable copies will be made.

These results are with temporary destructor inlining turned on. I'm 
holding off on destructor inlining for now because i'm seeing false 
positives on other projects, even though the reference counting 
suppression is working fairly well.

Lack of default argument support is still causing a small but noticeable 
coverage drop. The CFG for this syntax seems usable but i guess full 
support would require introducing a new LocationContext sub-class. 
Binary conditional operators are still not supported (even in CFG), but 
it doesn't seem to cause many problems.

== C++17 problems ==

 > http://en.cppreference.com/w/cpp/language/copy_elision
 > (since C++17)
 > Under the following circumstances, the compilers are REQUIRED to omit 
the copy- and move- construction of class objects even if the copy/move 
constructor and the destructor have observable side-effects. They need 
not be present or accessible, as the language rules ensure that no 
copy/move operation takes place, even conceptually:
 > In initialization, if the initializer expression is a prvalue and the 
cv-unqualified version of the source type is the same class as the class 
of the destination, the initializer expression is used to initialize the 
destination object...
 > In a function call, if the operand of a return statement is a prvalue 
and the return type of the function is the same as the type of that 
prvalue...

This is going to be fun to support. Specifically, this means that 
neither initialization 'C c = C();' nor return-statement 'return C()' 
(where 'C()' may as well be replaced with a function call of type 'C') 
contains an elidable copy in the AST anymore, but only a single 
constructor of variable 'c' (or the returned-to object) - probably with 
an intermediate CXXBindTemporaryExpr if the class has a non-trivial 
destructor. A few consequences of this change:

* For now the CFG itself is broken in this case because it includes a 
temporary destructor that shouldn't be there.

* The construction context for a function that returns an object by 
value may now be not only a temporary object context, but also a simple 
variable context or a returned value context - we'll need to handle 
these cases.

* Liveness does not work correctly - previously it was relying on 
CXXBindTemporaryExpr or MaterializeTemporaryExpr but now these aren't 
guaranteed to even exist, so by the time we reach 'c' from 'C()' we may 
already destroy bindings to the not-yet-alive variable 'c' we've added 
in the constructor. This is especially obvious for the conditional 
operator case, i.e. 'C c = x ? C(1) : C(2);' - in this case in C++17 
there is just one constructor happening on every particular path.

* Returned values may now chain together - the target object of a 
constructor that's called as part of the return statement may be 
multiple stack frames above the constructor call, which we'd need to 
unwind recursively.

For now i'll disable temporary- and returned-value- construction 
contexts in C++17.