<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On 12 April 2018 at 22:50, Artem Dergachev <span dir="ltr"><<a href="mailto:noqnoqneo@gmail.com" target="_blank">noqnoqneo@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF"><div><div class="h5">
<br>
<br>
<div class="m_7021133860125552746moz-cite-prefix">On 4/12/18 1:10 PM, Gábor Horváth
wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr"><br>
<div class="gmail_extra"><br>
<div class="gmail_quote">On 14 February 2018 at 22:20, Artem
Dergachev via cfe-dev <span dir="ltr"><<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">More explanations on
how the analyzer keeps making its way around the C++ AST.<br>
<br>
== Lifetime extension ==<br>
<br>
This is a brain dump of how (and how much) lifetime
extension of temporary objects is currently broken in the
static analyzert. Spoilers: not too much, it seems, but
annoying nevertheless.<br>
<br>
Consider an example:<br>
<br>
1 class C {<br>
2 public:<br>
3 C() {}<br>
4 ~C() {}<br>
5 };<br>
6<br>
7 void foo() {<br>
8 const C &c = C();<br>
9 }<br>
<br>
With the AST for the variable declaration:<br>
<br>
DeclStmt 0x7fa5ac85cba0 <line:8:3, col:19><br>
`-VarDecl 0x7fa5ac85c878 <col:3, col:18>
col:12 c 'const C &' cinit<br>
`-ExprWithCleanups 0x7fa5ac85cb30 <col:16,
col:18> 'const C' lvalue<br>
`-MaterializeTemporaryExpr 0x7fa5ac85cb00
<col:16, col:18> 'const C' lvalue extended by Var
0x7fa5ac85c878 'c' 'const C &'<br>
`-ImplicitCastExpr 0x7fa5ac85cae8 <col:16,
col:18> 'const C' <NoOp><br>
`-CXXBindTemporaryExpr 0x7fa5ac85cac8
<col:16, col:18> 'C' (CXXTemporary 0x7fa5ac85cac0)<br>
`-CXXTemporaryObjectExpr 0x7fa5ac85ca88
<col:16, col:18> 'C' 'void ()'<br>
<br>
*here goes a periodic reminder that CXXTemporaryObjectExpr
is a sub-class of CXXConstructExpr*<br>
<br>
Notice how MaterializeTemporaryExpr is the innermost
expression (the first one in the order of execution) that
is an lvalue. Essentially, you can think of it as the
mythical "rvalue-to-lvalue" cast that takes in a C++
object rvalue and returns the this-value for that object.
Because all C++ objects have a this-value that never
changes throughout their lifetime, it is essentially their
identity. Otherwise you can't call methods on them.<br>
<br>
MaterializeTemporaryExpr also contains information about
the lifetime extension process: we needed the this-value
in order to bind it to variable 'c'. You see that in the
AST.<br>
<br>
In the analyzer, however, MaterializeTemporaryExpr does a
different thing, as a temporary solution (no pun
intended). It constructs a new temporary region out of
thin air and binds the rvalue object to that temporary in
the Store. The respective function in our code is called
"createTemporaryRegionIfNeeded<wbr>". It also has a
separate purpose of converting rvalue sub-object
adjustments into lvalue sub-object adjustments, which we
wouldn't discuss this time.<br>
<br>
Now that we learned how to inline temporary constructors
and destructors, it essentially means that the this-value
in the constructor and in the destructor would be
different. Because first we construct the object into
temporary region R1, then we take lazyCompoundVal{R1} to
represent the value of CXXTemporaryObjectExpr, then we
materialize lazyCompoundVal{R1} to R2, then we bind R2 to
variable 'c', then we call the automatic(!) destructor for
'c' which contains R2. To be clear, the region store at
the time of destruction would be:<br>
<br>
c: R2,<br>
R2: lazyCompoundVal{R1}.<br>
<br>
It means that fields of the object would contain the
correct values, there would be the correct number of
destructors called (no temporary destructors, just one
automatic destructor), but the identity of the object
(this-value) would change in the process. Unless the
object actually makes any decisions or does any
manipulations that involve its this-value, the modeling
should be correct. When the object starts to actively use
its this-value in its inlined methods, the analysis would
do wrong stuff. Additionally, it causes a mess in the
checkers when they try to track objects by their
this-values - i.e. IteratorChecker has a lot of additional
code to work around the fact that the region for the
object constantly changes.<br>
<br>
From the above it is clear that MaterializeTemporaryExpr
should not construct any new regions, at least not in this
case. We already have the correct region, namely R1, which
should be re-used.<br>
</blockquote>
<div><br>
</div>
<div>Hi Artem!<br>
<br>
</div>
<div>We found a strange false positive that might be related
to what you describe above but not sure though. Could you
take a look?<br>
</div>
<div>It looks like we are seeing a null pointer dereference
error, and the null value comes from the destructor which
was invoked on a temporary. <br>
</div>
<div>If this is not the case, the path diagnostic might be
misleading.<br>
</div>
<div><br>
</div>
<div>Here is the finding: <a href="http://cc.elte.hu:15010/Default/#run=Xerces_Xerces-C_3_2_1_unexplored_first&checker-name=unix.MismatchedDeallocator&checker-name=cplusplus.NewDeleteLeaks&checker-name=core.uninitialized.UndefReturn&checker-name=core.UndefinedBinaryOperatorResult&checker-name=core.NullDereference&checker-name=core.NonNullParamChecker&checker-name=core.DivideZero&checker-name=core.CallAndMessage&tab=allReports&reportHash=2f8548f84329734e674f15d844caeb38&report=6764&subtab=2f8548f84329734e674f15d844caeb38" target="_blank">http://cc.elte.hu:15010/<wbr>Default/#run=Xerces_Xerces-C_<wbr>3_2_1_unexplored_first&<wbr>checker-name=unix.<wbr>MismatchedDeallocator&checker-<wbr>name=cplusplus.NewDeleteLeaks&<wbr>checker-name=core.<wbr>uninitialized.UndefReturn&<wbr>checker-name=core.<wbr>UndefinedBinaryOperatorResult&<wbr>checker-name=core.<wbr>NullDereference&checker-name=<wbr>core.NonNullParamChecker&<wbr>checker-name=core.DivideZero&<wbr>checker-name=core.<wbr>CallAndMessage&tab=allReports&<wbr>reportHash=<wbr>2f8548f84329734e674f15d844caeb<wbr>38&report=6764&subtab=<wbr>2f8548f84329734e674f15d844caeb<wbr>38</a><br>
<br>
</div>
<div>Regards,<br>
</div>
<div>Gábor<br>
</div>
</div>
</div>
</div>
</blockquote>
<br></div></div>
Hmm, i don't fully understand it yet. It seems to me that
RefHash3KeysIdPoolEnumerator drains the whole pool upon destruction
(<span class="m_7021133860125552746pl-c">fIdPtrs is not a member of the temporary, it's
an external entity)</span>, so it indeed can't really be safely
passed around or copied (i.e. elidable copies are not moves).<br></div></blockquote><div><br></div><div>Oh, I think you are right! Somehow I assumed the there is a deep copy going on but after looking at the copy constructor, it looks like that is not the case. Sorry for bothering :)<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF">
<br>
So i'd easily believe that this code only works because all copies
are elided, i.e. the code is not portable (at least not until C++17)
but usually works because most compilers are clever enough. We'll
probably get rid of such positives when we implement copy elision.<br></div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF">
<br>
In case i got it all wrong, do you accidentally have a reproducer
for me to have a closer look? If CTU stuff is hard to provide a
reproducer for, you should be able to write a small code snippet
directly into RefHash3KeysIdPool.c to reproduce the problem. I'll
also accept a trimmed (as in -trim-egraph) exploded graph dot file
even if it's huge.<br>
<br>
P.S. The "Calling..." icon is hilarious :D<div><div class="h5"><br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
It is tempting to take R1 directly from
lazyCompoundVal{R1} - it already has memories about once
being a value of R1. I'm not sure it's not going to work -
it may work, at least i'm not ready to come up with a
counterexample. But the meaning of LazyCompoundVal's
parent region is different and coincides with what we want
just accidentally. Namely, lazyCompoundVal{R1} is a value
of an object that was bound to R1 in some particular
moment of time in the past, without any explanation of
when this moment of time was - but there's no indication
if R1 is the region of the temporary we've just
constructed, or a region of an unrelated object that used
to have the same value back then. As we'd see later,
MaterializeTemporaryExpr doesn't always contain a
constructor within it - there are a lot of cases to cover,
and if the trick doesn't work even once, it's going to be
hard, so i'd probably not going to commit into maintaining
this invariant. Though it might be plausible to modify add
an SVal kind that does exactly what we mean here - i.e. a
value that does indeed correspond to a specific C++ object
identified by region. It might be a beautiful solution,
but it may also fail miserably if tricky cornercases show
up - so i'm not ready to commit to that. Also the correct
way of dealing with such values (i.e. for how long are
they relevant?) would be extremely difficult to explain to
checker developers.<br>
<br>
The more straightforward approach here is to include
MaterializeTemporaryExpr (hereinafter MTE) into the
construction context. It means, if a temporary that we're
about to construct would be lifetime-extended later, we'd
rather know about that during construction, and maintain a
map in the program state from MTE to their respective
temporary regions that were used for representing the
respective construction targets. Upon encountering the
respective MTE we'd simply retrieve the implicit temporary
storage for the value from the program state and declare
that temporary region to be the value of the MTE. This
would mimic the approach we have with
CXXBindTemporaryExprs (hereinafter BTE) and their
respective regions that allows temporary destructors to
work - but this time it's going to be about
MaterializeTemporaryExprs and automatic destructors. I
imagine that on the checker side this can potentially be
exposed via some sort of State->getTemporaryStorage(Exp<wbr>r)
call, but i believe that generally this process should be
as transparent to the checkers as possible.<br>
<br>
It sounds as if both of these maps could be eliminated by
always making sure that the target temporary is
constructed "with" the MTE (for lifetime-extended
temproraries) or BTE (for temporaries that require
destruction at the end of full-expression). In this case,
with the help of construction context-assisted lookahead,
we declare that the target of the construction is
CXXTempObjectRegion(MTE, LC) or CXXTempObjectRegion(BTE,
LC) respectively, rather than
CXXTempObjectRegion(CXXConstru<wbr>ctExpr). Then during
evaluation of MTE or BTE we'd simply construct the same
region with the expression we're currently evaluating, and
it's automagically going to be the correct region. This
approach, however, becomes confusing when we start dealing
with elidable constructors (explained below). So for now i
believe that it is quite irrelevant which expression is
identifying the temporary region.<br>
<br>
== Elidable constructors ==<br>
<br>
While it doesn't sound like an immediately important task
to implement copy elision in the analyzer, it may help
with making some things easier. And it'd also make some
reports fancier, as mentioned in <a href="https://reviews.llvm.org/D43144" rel="noreferrer" target="_blank">https://reviews.llvm.org/D4314<wbr>4</a>.<br>
<br>
Elidable copy-constructors can be explained as a form of
lifetime extension. Instead of copying the temporary, they
keep using the original value of the temporary, which in
some pretty twisted sense means that they are
lifetime-extending it to be able to use it. For example,
if we modify our example by replacing the
lifetime-extending reference variable with a value-type
variable:<br>
<br>
1 class C {<br>
2 public:<br>
3 C() {}<br>
4 ~C() {}<br>
5 };<br>
6<br>
7 void foo() {<br>
8 C c = C();<br>
9 }<br>
<br>
...then we'd still have an MTE, even though lifetime
extension would seem to be gone:<br>
<br>
DeclStmt 0x7fb8f005afb8 <line:8:3, col:12><br>
`-VarDecl 0x7fb8f005ac50 <col:3, col:11> col:5
c 'C' cinit<br>
`-ExprWithCleanups 0x7fb8f005afa0 <col:9,
col:11> 'C'<br>
`-CXXConstructExpr 0x7fb8f005af68 <col:9,
col:11> 'C' 'void (const C &) noexcept' elidable<br>
`-MaterializeTemporaryExpr 0x7fb8f005af00
<col:9, col:11> 'const C' lvalue<br>
`-ImplicitCastExpr 0x7fb8f005aee8 <col:9,
col:11> 'const C' <NoOp><br>
`-CXXBindTemporaryExpr 0x7fb8f005aec8
<col:9, col:11> 'C' (CXXTemporary 0x7fb8f005aec0)<br>
`-CXXTemporaryObjectExpr 0x7fb8f005ae88
<col:9, col:11> 'C' 'void ()'<br>
<br>
In this case the MTE is expressing the fact that the
temporary constructed via CXXTemporaryObjectExpr can be
"lifetime-extended" (by actually merging it with the stack
variable) rather than copied, if the CXXConstructExpr
surrounding it would be chosen to be elided. The AST
doesn't make the elision choice for us - but is compatible
with both choices. The MTE essentially overrides the
necessity of immediate destruction provided by the BTE,
and lets the surrounding AST decide upon the lifetime of
the object.<br>
<br>
Because the analyzer currently does not do copy elision,
it will use the MTE only to compute the argument for the
elidable copy-constructor, and then perform the
copy-construction, and then destroy the original temporary
at the end of the full-expression. Note, however, that in
this case we need to properly support both the BTE (for
the temporary destructor to work) and the MTE (for
computing its value). We need to implement the MTE's
ability to perform "rvalue-to-lvalue-cast" even if the
temporary destruction is still necessary. For this reason,
if we rely on constructing temporary regions with the
correct BTEs or MTEs, at least one of these tasks becomes
impossible to perform.<br>
<br>
If we were to support copy elision, then the
CXXTemporaryObjectExpr constructor would go directly into
the variable region. For the purposes of modeling, it'd
mean that only CXXTemporaryObjectExpr would actually need
to be modeled. But this would require additional coding in
the construction context to be able to realize that the
target is the variable while modeling the
CXXTemporaryObjectExpr.<br>
<br>
For the sake of completeness, let's consider the ternary
operator example:<br>
<br>
1 class C {<br>
2 public:<br>
3 C(int) {}<br>
4 ~C() {}<br>
5 };<br>
6<br>
7 void foo(int coin) {<br>
8 const C &c = coin ? C(1) : C(2);<br>
9 }<br>
<br>
The respective AST would be:<br>
<br>
DeclStmt 0x7fc1e20023e0 <line:8:3, col:34><br>
`-VarDecl 0x7fc1e2001dc8 <col:3, col:33>
col:12 c 'const C &' cinit<br>
`-ExprWithCleanups 0x7fc1e2002370 <col:16,
col:33> 'const C' lvalue<br>
`-MaterializeTemporaryExpr 0x7fc1e2002340
<col:16, col:33> 'const C' lvalue extended by Var
0x7fc1e2001dc8 'c' 'const C &'<br>
`-ImplicitCastExpr 0x7fc1e2002328 <col:16,
col:33> 'const C' <NoOp><br>
`-ConditionalOperator 0x7fc1e20022f8
<col:16, col:33> 'C'<br>
|-ImplicitCastExpr 0x7fc1e2002170
<col:16> 'bool' <IntegralToBoolean><br>
| `-ImplicitCastExpr 0x7fc1e2002158
<col:16> 'int' <LValueToRValue><br>
| `-DeclRefExpr 0x7fc1e2001e28
<col:16> 'int' lvalue ParmVar 0x7fc1e2001c18 'coin'
'int'<br>
|-CXXBindTemporaryExpr 0x7fc1e2002248
<col:23, col:26> 'C' (CXXTemporary 0x7fc1e2002240)<br>
| `-CXXConstructExpr 0x7fc1e2002208
<col:23, col:26> 'C' 'void (const C &) noexcept'
elidable<br>
| `-MaterializeTemporaryExpr
0x7fc1e20021a0 <col:23, col:26> 'const C' lvalue<br>
| `-ImplicitCastExpr 0x7fc1e2002188
<col:23, col:26> 'const C' <NoOp><br>
| `-CXXFunctionalCastExpr
0x7fc1e2002078 <col:23, col:26> 'C' functional cast
to class C <ConstructorConversion><br>
| `-CXXBindTemporaryExpr
0x7fc1e2002058 <col:23, col:26> 'C' (CXXTemporary
0x7fc1e2002050)<br>
| `-CXXConstructExpr
0x7fc1e2002018 <col:23, col:26> 'C' 'void (int)'<br>
| `-IntegerLiteral
0x7fc1e2001e60 <col:25> 'int' 1<br>
`-CXXBindTemporaryExpr 0x7fc1e20022d8
<col:30, col:33> 'C' (CXXTemporary 0x7fc1e20022d0)<br>
`-CXXConstructExpr 0x7fc1e2002298
<col:30, col:33> 'C' 'void (const C &) noexcept'
elidable<br>
`-MaterializeTemporaryExpr
0x7fc1e2002280 <col:30, col:33> 'const C' lvalue<br>
`-ImplicitCastExpr 0x7fc1e2002268
<col:30, col:33> 'const C' <NoOp><br>
`-CXXFunctionalCastExpr
0x7fc1e2002130 <col:30, col:33> 'C' functional cast
to class C <ConstructorConversion><br>
`-CXXBindTemporaryExpr
0x7fc1e2002110 <col:30, col:33> 'C' (CXXTemporary
0x7fc1e2002108)<br>
`-CXXConstructExpr
0x7fc1e20020d0 <col:30, col:33> 'C' 'void (int)'<br>
`-IntegerLiteral
0x7fc1e20020b0 <col:32> 'int' 2<br>
<br>
Each branch contains two constructors: the temporary and
the elidable copy. The temporaries are surrounded with
their respective BTEs and copy-elision-kind MTEs, which
indicates that they need to be either destroyed as
temporaries, or, if copy elision is chosen, have their
lifetime decided upon by the surrounding AST. The elidable
copy constructors also, being temporaries, have their
respective BTEs. Note, however, that there is only one MTE
for both BTEs for the elidable constructors.<br>
<br>
So after the conditional operator is resolved (which is
the first thing we need to do, according to the CFG), we'd
go ahead and perform the constructors, and their trigger
would be their respective BTE in the non-elide case, and
the single top-level MTE in the elide case. In the
non-elide case, copy constructors would be triggered by
the top-level MTE.<br>
<br>
It means that, once again, copy elision would prevent us
from handling both the BTE and the copy-elision-kind MTE
in the single construction, allowing the "predictable
target region" trick to work: when we need the temporary
destructor, we construct directly into CXXTempObjectRegion
of the BTE and it gets automatically picked up during
destruction, and when we need the automatic destructor, we
construct directly into CXXTempObjectRegion of the MTE and
we can easily compute the value of the MTE. But when we
don't do copy elision, we'd have to keep at least one of
those in the program state map.<br>
<br>
== Return by value ==<br>
<br>
Returning C++ objects by value is actually very similar to
constructing it. Consider:<br>
<br>
1 class C {<br>
2 public:<br>
3 C() {}<br>
4 ~C() {}<br>
5 };<br>
6<br>
7 C bar() {<br>
8 C c;<br>
9 return c;<br>
10 }<br>
11<br>
12 void foo() {<br>
13 const C &c = bar();<br>
14 }<br>
<br>
With the respective AST for DeclStmt in foo():<br>
<br>
DeclStmt 0x7fe62c84f8e8 <line:13:3, col:21><br>
`-VarDecl 0x7fe62c84f6b8 <col:3, col:20>
col:12 c 'const C &' cinit<br>
`-ExprWithCleanups 0x7fe62c84f878 <col:16,
col:20> 'const C' lvalue<br>
`-MaterializeTemporaryExpr 0x7fe62c84f848
<col:16, col:20> 'const C' lvalue extended by Var
0x7fe62c84f6b8 'c' 'const C &'<br>
`-ImplicitCastExpr 0x7fe62c84f830 <col:16,
col:20> 'const C' <NoOp><br>
`-CXXBindTemporaryExpr 0x7fe62c84f810
<col:16, col:20> 'C' (CXXTemporary 0x7fe62c84f808)<br>
`-CallExpr 0x7fe62c84f7e0 <col:16,
col:20> 'C'<br>
`-ImplicitCastExpr 0x7fe62c84f7c8
<col:16> 'C (*)()' <FunctionToPointerDecay><br>
`-DeclRefExpr 0x7fe62c84f770
<col:16> 'C ()' lvalue Function 0x7fe62c84f190 'bar'
'C ()'<br>
<br>
And for the ReturnStmt in bar():<br>
<br>
ReturnStmt 0x7fe62c84f5b0 <line:9:3, col:10><br>
`-CXXConstructExpr 0x7fe62c84f578 <col:10> 'C'
'void (const C &) noexcept' elidable<br>
`-ImplicitCastExpr 0x7fe62c84f518 <col:10>
'const C' lvalue <NoOp><br>
`-DeclRefExpr 0x7fe62c84f4f0 <col:10> 'C'
lvalue Var 0x7fe62c84f280 'c' 'C'<br>
<br>
Since <a href="https://reviews.llvm.org/D42875" rel="noreferrer" target="_blank">https://reviews.llvm.org/D4287<wbr>5</a>
we can already realize that the constructor in bar(),
assuming that we're inlining bar() during analysis, would
be constructed into something that is a return value of
bar(). This allows us, by looking that the
StackFrameContext's call site, to figure out that it is
being constructed into the CallExpr in foo(). Now if only
we knew that that the call site is a lifetime-extended
temporary, i.e. if only we had a pointer to the foo()'s
MTE at the CallExpr's CFG element, we'd be able to find
the correct target region for construction: the
CXXTempObjectRegion for the MTE in the StackFrameContext
of foo(). So i'm proposing to add some sort of
construction context to not only constructors, but also to
functions that return objects, and then during
construction perform the lookup in three easy steps:<br>
<br>
1. in the callee's CFG from constructor to return
statement,<br>
2. through the location from the return statement to the
call site,<br>
3. then through the caller's CFG from the call site to
the MTE.<br>
<br>
If the function is not inlined, we can still make use of
the construction context to represent the return value as
a LazyCompoundValue of the MTE's temporary. It would
eliminate the need to replace the return value with
another value while evaluating the MTE, and of course the
need to re-bind the object to a different this-region.<br>
<br>
So i believe that this is a good way to eliminate the need
for the "createTemporaryRegionIfNeeded<wbr>" thing in the
function calls as well.
<div class="m_7021133860125552746gmail-HOEnZb">
<div class="m_7021133860125552746gmail-h5"><br>
<br>
<br>
On 06/02/2018 1:41 PM, Artem Dergachev wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
A bit of an update.<br>
<br>
== Temporary destructors ==<br>
<br>
Adding some initial support for temporary
destructors seems pretty easy and straightforward at
this point, given the awesome work done by Manuel
Klimek on our CFG a few years ago.<br>
<br>
1. We already have the awesome CFGTemporaryDtor
elements, which have the backreference to the
CXXBindTemporaryExpr for their temporaries.<br>
<br>
2. In simple cases CXXBindTemporaryExprs have an
immediate constructor within them, and therefore we
can provide the CXXBindTemporaryExprs as the
construction context's trigger statements, and
therefore have a reliable CXXTempObjectRegion for
constructors.<br>
<br>
3. Then we already track the CXXBindTemporaryExprs
for the active temporaries in the program state. We
can attach any constructor information to them, such
as the target region, if we need to (probably we can
reconstruct it by looking at the expression and the
location context, not sure if we want to).<br>
<br>
4. So when we reach the CFGTemporaryDtor element, we
can just lookup all the info we need, and perform
the destruction properly.<br>
<br>
5. There's a bit of a liveness problem, because it
seems that our liveness analysis tends to throw away
the temporary too early. I can easily hack this
problem away by marking all regions that correspond
to active temporaries as live. I'll see if there's a
better solution.<br>
<br>
== CXXDefaultArgExpr problems ==<br>
<br>
There's a known problem with those. Consider:<br>
<br>
void foo(const C &c = C()) {<br>
}<br>
<br>
void bar() {<br>
foo();<br>
foo();<br>
}<br>
<br>
Each call of foo() contains a CXXDefaultArgExpr for
c. The default argument value C() is constructed
before we enter foo() and destroyed after we leave
foo(). However, c's initializer, "= C()", is *not
part of the AST of bar()*. In particular, when foo()
is called twice, the initializer for the two calls
is the same, only CXXDefaultArgExprs are different.
This screws a lot of invariants in the analyzer:
program points start coinciding (causing the
analysis to loop and cache out), Environment doesn't
expect the same expression in the same location
context have two different values (suppose calls are
nested into each other), analysis taking wrong
branches, and so on.<br>
<br>
Luckily, default-arg expressions that aren't zero
integers or null pointers are pretty rare. Still,
we'd need to eventually think how to avoid any
really bad practical problems with them.<br>
<br>
On 25/01/2018 9:08 AM, Artem Dergachev wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Handling C++ temporary object construction and
destruction seems to be the biggest cause of false
positives on C++ code at the moment. I'd be
looking into this, even though for now i don't see
the whole scale of problem.<br>
<br>
== CFG, destructors, and ProgramPoints ==<br>
<br>
We should probably enable `-analyzer-config
cfg-temporary-dtors=true` by default soon. It is a
fairly low-impact change because it only alters
the CFG but the analyzer rarely actually uses the
new nodes. Destructors for the most part are still
evaluated conservatively, with improper object
regions. So it causes almost no changes in the
analyzer's positives for now, but it definitely
opens up room for further improvements.<br>
<br>
I'm observing a couple of problems with this mode
at the moment, namely the rich CFG destructor
element hierarchy is not currently complemented by
an equally rich ProgramPoint hierarchy. This
causes the analysis to merge nodes which are not
equivalent, for example two implicit destructors
of the same type (with the same function
declaration) may sometimes cause the ExplodedGraph
to coil up and stop analysis (lost coverage)
because of having the same program state at the
erroneously-same program point. Because situations
when we can guarantee a change in the program
state are pretty rare, we'd have to produce more
program point kinds to handle this correctly.<br>
<br>
CallEvent hierarchy is also very much affected in
a similar manner - because apparently we're
constructing program points by looking at
CallEvents, so they'd need to carry all the
information that's needed to construct the
pre-call/post-call program point.<br>
<br>
== Construction contexts ==<br>
<br>
We are correctly modeling "this" object region
during construction/destruction of variables with
automatic storage duration, fields and base
classes, and on operator new() since recently, as
long as these aren't arrays of objects. It was not
yet implemented for other cases such as
temporaries, initializer lists, fields or C++17
base classes within aggregates, and pass-by-value
from/to functions (the latter being a slightly
different problem than temporaries).<br>
<br>
First of all, by "not yet implemented" i mean that
instead of constructing into (destroying) the
correct object (in the correct memory region), we
come up with a "temporary region", which looks
exactly like a region of a valid C++ temporary but
is only used for communicating that it is not the
right region. Then we look at the region, see that
it is a temporary, and avoid inlining
constructors, because it would make little sense
when the region is not correct. However, if we are
to model temporaries, we need another way of
communicating our failure to find the correct
region, which is being addressed by <a href="https://reviews.llvm.org/D42457" rel="noreferrer" target="_blank">https://reviews.llvm.org/D4245<wbr>7</a><br>
<br>
Also in the cases when the correct region is used,
it is being computed in a funky way: in order to
figure out where do we construct the object, we
walk forward in the CFG (or from child to parent
in the AST) to find the next/parent statement that
would accomodate the newly constructed object.
This means that the CFG, while perfectly telling
us what to do in what order (which we, unlike
CodeGen, cannot easily take from AST because we
cannot afford recursive evaluation of statements,
mainly because of inlining), discards too much
context to be helpful in understanding how to do
it.<br>
<br>
I tried to add the information about such "trigger
statements" for constructors into CFG and it is
extremely helpful and easy to both use and extend.
This assumes adding a new sub-class of CFGElement
for constructors, which maintain a "construction
context" - for now it's just a trigger statement.
For instance, in<br>
<br>
class C { ... };<br>
void foo() {<br>
C c;<br>
}<br>
<br>
...the trigger for constructor C() would be
DeclStmt `C c`, and once we know this we can
immediately figure out that the construction
target is the region of variable `c`. Construction
trigger is not necessarily a statement - it may be
a CXXCtorInitializer, which is an AST node kind of
its own. Additionally, when constructing
aggregates via initializer lists, we may have the
same statement trigger multiple constructors, eg.
in<br>
<br>
class C { public: C(int); ~C(); };<br>
struct S { C c1, c2, c3; };<br>
void foo() {<br>
S s { 1, 2, 3 };<br>
}<br>
<br>
... we have three different constructors (actually
six different constructors if we include elidable
copy constructors) for c1, c2, c3 (and lack of
constructor for `s` because of the aggregate
thing). It would be more natural to declare that
the specific field or index would be a part of the
CFG's construction context, as well as the
intermediate InitListExpr, so even in these simple
cases the construction context may get quite
bulky. And this example is quite popular on actual
code - probably the second worst cause of false
positives after temporaries.<br>
<br>
For now i have no specific plan on what would
construction context for temporaries contain in
the general case. I might not be able to get the
data structures right from the start. In any case,
it might be necessary to perform additional memory
allocations for these CFG elements (for analyzer's
CFG only, so it wouldn't affect compilation time
or warnings).<br>
<br>
I believe that getting the correct target region
in as many cases as possible would be the main
part of my work for the nearest future. And i want
to move most this work to CFG, while letting the
analyzer pick up the trigger statement from it and
trust it as blindly as possible.<br>
<br>
== Workflow ==<br>
<br>
When working on operator new, i tried hard to
maintain a reasonable history, making around 15
local rebases. It was not awesome because it was
hard for the reviewers to understand the context
of the new changes, and changes could have easily
kicked in during rebases. A few lessons learned
here would be to commit more aggressively, i.e.
avoiding stockpiling a large history of patches
(essentially a large branch), which in turn would
be possible through trying harder to avoid
unrelated hard-to-test core changes (as long as it
doesn't require weird workarounds) that aren't
covered by a flag (such as those evalCast fixes),
in order to make sure reviewing take less
responsibility. It's fine if some parts would
later be fixed (assuming they would indeed be
fixed), if it means making the turnaround faster
and the tail of patches shorter - that's also the
attitude i'd try to maintain when reviewing stuff
myself.<br>
</blockquote>
<br>
</blockquote>
<br>
______________________________<wbr>_________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-dev</a><br>
</div>
</div>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
<br>
</div></div></div>
</blockquote></div><br></div></div>