<div dir="auto"><span style="font-family:sans-serif;font-size:12.8px">Hi Artem!</span><div dir="auto" style="font-family:sans-serif;font-size:12.8px"><br></div><div dir="auto" style="font-family:sans-serif;font-size:12.8px">My favourite is B or D. At least, I feel like those are the most intuitive for checker writers. The reason why I like B because if someone looks up how new expression works in the language, this callback sequence will match exactly what is writtem there.</div><div dir="auto" style="font-family:sans-serif;font-size:12.8px"><br></div><div dir="auto" style="font-family:sans-serif;font-size:12.8px">For this reason, I think using new expr's callback only for the allocation is unexpected for those who already have good knowledge about how the language works. (And the difference between new expression and operator new.)</div><div dir="auto" style="font-family:sans-serif;font-size:12.8px"><br></div><div dir="auto" style="font-family:sans-serif;font-size:12.8px">I already feel like there are too much divergence between the language wording and the analyzer one (eg loc and nonloc vs rvalue/lvalue). </div><div dir="auto" style="font-family:sans-serif;font-size:12.8px"><br></div><div dir="auto" style="font-family:sans-serif;font-size:12.8px">And while I agree that for a+b it is good to have the pre binary operator callback after both of the operands are evaluated, the same logic feels counterintuitive to me in case of new expression. In case of a+b it is very onlikely that a check want to store something to the state in pre operator+ callback that is relevant to the evaluation of a or b. In case of new expression, however, I think a check might make a change to the state that is relevant to the allocation. This could be worked around by making the checks use another callback of course, so it is not the strongest argument. </div><div dir="auto" style="font-family:sans-serif;font-size:12.8px"><br></div><div dir="auto" style="font-family:sans-serif;font-size:12.8px">Regards,</div><div dir="auto" style="font-family:sans-serif;font-size:12.8px">Gábor</div></div><div class="gmail_extra"><br><div class="gmail_quote">2017. dec. 8. 15:46 ezt írta ("Artem Dergachev via cfe-dev" <<a href="mailto:cfe-dev@lists.llvm.org">cfe-dev@lists.llvm.org</a>>):<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">All right, so how do we want to equip C++ operator new() with checker callbacks?<br>
<br>
~~~<br>
<br>
First of all, a quick note about how do we evaluate call-expressions. Typical code to do that in the analyzer (eg. ExprEngine::VisitCallExpr) kinda looks like this:<br>
<br>
```<br>
// Take node `Pred` from the CoreEngine's worklist.<br>
NodeSet1 = { Pred };<br>
<br>
NodeSet2;<br>
for (N in NodeSet1) {<br>
// Checkers evaluate check::PreStmt<CallExpr> event, put their transitions to NodeSet2.<br>
runCheckersForPreStmt(N, &NodeSet2);<br>
}<br>
<br>
NodeSet3;<br>
for (N in NodeSet2) {<br>
// Checkers evaluate check::PreCall event, put their transitions to NodeSet3.<br>
runCheckersForPreCall(N, &NodeSet3);<br>
}<br>
<br>
NodeSet4;<br>
for (N in NodeSet3) {<br>
// Either eval::Call event in Checkers, or inlineCall, or conservativeEvalCall.<br>
evalCall(N, &NodeSet4);<br>
}<br>
<br>
NodeSet5;<br>
for (N in NodeSet4) {<br>
// Checkers evaluate check::PostCall event, put their transitions to NodeSet5.<br>
runCheckersForPostCall(N, &NodeSet5);<br>
}<br>
<br>
NodeSet6;<br>
for (N in NodeSet5) {<br>
// Checkers evaluate check::PostStmt<CallExpr> event, put their transitions to NodeSet6.<br>
runCheckersForPreStmt(N, &NodeSet6);<br>
}<br>
<br>
// Put nodes from NodeSet6 back to the worklist.<br>
```<br>
<br>
During evalCall(), if any checker's eval::Call succeeds, that checker simply puts their transitions to NodeSet4. During conservativeEvalCall, the core puts exactly one new node to NodeSet4.<br>
<br>
The interesting part here is that in case of inlineCall(), *the call is not actually evaluated* (!). The only thing that happens here is that inlineCall enters the stack frame and puts the node with the new stack frame *directly to the worklist* (!!), completely bypassing NodeSet4. Because of that, in inlineCall case, NodeSet{4,5,6} are all empty, and the remaining code does nothing, so no PostCall callbacks get called here until the call is actually evaluated. However, when the call is fully inlined, at the CallExit program point (ExprEngine::processCallExit()<wbr>), PostCall and PostStmt callbacks are called manually. So we get the correct sequence of callbacks regardless.<br>
<br>
~~~<br>
<br>
Now, suppose we have operator new:<br>
<br>
new (args1...) C(args2...)<br>
<br>
The semantics of this code can be expressed in the following "statement-expression" pseudo-code:<br>
<br>
```<br>
01 C *_this = (C *) operator new(sizeof(C), args1...);<br>
02 if (_this) _this->C(args2...);<br>
03 return _this;<br>
```<br>
<br>
Note that the constructor is simply not called when the operator returns nullptr.<br>
Note the cast on the first line - needs to be modeled, because operator new returns `void *`.<br>
Note that i did ask George if he thinks that expressing this piece of code as a body-farm-thing is a good idea, and he didn't think so :)<br>
<br>
By looking at this construct, it should be obvious that the relationship between CXXNewExpr and its respective CXXConstructExpr is more complicated than that of a "normal" expression and its sub-expression. They are kinda computed in the counter-intuitive order, and the easiest way to explain that would be to announce that there are actually three "expressions" involved: operator new call fake expression, constructor call expression, and the "big new-expression" of which both of these are children, in that order.<br>
<br>
This is how our CFG currently sees it, in case of `-analyzer-config c++-allocator-inlining=true`:<br>
<br>
1. CFGNewAllocator // evaluate `operator new(sizeof(C), args1...)`<br>
2. CXXConstructExpr // evaluate `_this->C(args2...)`<br>
3. CXXNewExpr // bind `_this` to the expression<br>
<br>
So i guess it's so far so good.<br>
<br>
At 1., we do defaultEvalCall for operator new. FIXME: do a regular evalCall, i.e. allow checkers to evaluate operator new. Also in <a href="https://reviews.llvm.org/D40560" rel="noreferrer" target="_blank">https://reviews.llvm.org/D4056<wbr>0</a> we add a new program state trait to hold the artificial variable "_this", since there's no room for it in the Environment, since it's not an expression but something we made up. Also we perform the cast from `void *` to `C *`.<br>
<br>
At 2., we evalCall for the constructor with the help of our fake variable `_this`. FIXME: we should also do the if() part of it, i.e. don't evaluate the constructor when the operator returns null. It shouldn't be a state split though, i guess we should just suppress the branch on which the return value is null, unless we inlined the operator new and sure it's null.<br>
<br>
At 3., we take `_this` and declare that the value stored in it would from now on be the value of the "big new-expression" that unites them all, regardless of whether it's null or not.<br>
<br>
~~~<br>
<br>
Now when it comes to checker callbacks, it's a bit messy right now. On CFGNewAllocator call evaluation, the call site is CXXNewExpr. It means that if the operator is inlined, and we hit processCallExit(), as explained above, we'd be triggering PostStmt<CXXNewExpr>, even though we didn't ever trigger PreStmt<CXXNewExpr>. Then at 3., we'd have PreStmt<CXXNewExpr> and then another(!!!) PostStmt<CXXNewExpr>, which drives MallocChecker crazy. This, of course, can be trivially fixed. But i wanted to write this long explanation to specifically highlight this bug, with the hope that in the future it would be less likely to get reintroduced.<br>
<br>
And then, now that we realize that we have three kinda-statements here, the question is, do we want all three equipped with checker callbacks? We could plan to make a new callback that'd be surrounding CFGNewAllocator similarly to how PreStmt/PostStmt surrounds CXXConstructExpr and CXXNewExpr.<br>
<br>
It may look like this:<br>
<br>
(A)<br>
<br>
-> check::PreCXXAllocator // new callback<br>
-> check::PreCall<br>
1. CFGNewAllocator<br>
<- check::PostCall<br>
<- check::PostCXXAllocator // new callback<br>
<br>
-> check::PreStmt<CXXConstructExp<wbr>r><br>
-> check::PreCall<br>
2. CXXConstructExpr<br>
<- check::PostCall<br>
<- check::PostStmt<CXXConstructEx<wbr>pr><br>
<br>
-> check::PreStmt<CXXNewExpr><br>
3. CXXNewExpr<br>
<- check::PostStmt<CXXNewExpr><br>
<br>
Or like this:<br>
<br>
(B)<br>
<br>
-> check::PreStmt<CXXNewExpr><br>
<br>
-> check::PreCXXAllocator // new callback<br>
-> check::PreCall<br>
1. CFGNewAllocator<br>
<- check::PostCall<br>
<- check::PostCXXAllocator // new callback<br>
<br>
-> check::PreStmt<CXXConstructExp<wbr>r><br>
-> check::PreCall<br>
2. CXXConstructExpr<br>
<- check::PostCall<br>
<- check::PostStmt<CXXConstructEx<wbr>pr><br>
<br>
3. CXXNewExpr<br>
<- check::PostStmt<CXXNewExpr><br>
<br>
Or like this:<br>
<br>
(C)<br>
<br>
-> check::PreStmt<CXXNewExpr><br>
-> check::PreCall<br>
1. CFGNewAllocator<br>
<- check::PostCall<br>
<- check::PostStmt<CXXNewExpr><br>
<br>
-> check::PreStmt<CXXConstructExp<wbr>r><br>
-> check::PreCall<br>
2. CXXConstructExpr<br>
<- check::PostCall<br>
<- check::PostStmt<CXXConstructEx<wbr>pr><br>
<br>
-> check::PreBigNewExpr // new callback (needs better name)<br>
3. CXXNewExpr<br>
<- check::PostBigNewExpr // new callback<br>
<br>
Or even like this:<br>
<br>
(D)<br>
<br>
-> check::PreWholeNewThing // new callback<br>
<br>
-> check::PreStmt<CXXNewExpr><br>
-> check::PreCall<br>
1. CFGNewAllocator<br>
<- check::PostCall<br>
<- check::PostStmt<CXXNewExpr><br>
<br>
-> check::PreStmt<CXXConstructExp<wbr>r><br>
-> check::PreCall<br>
2. CXXConstructExpr<br>
<- check::PostCall<br>
<- check::PostStmt<CXXConstructEx<wbr>pr><br>
<br>
3. CXXNewExpr<br>
<- check::PostWholeNewThing // new callback<br>
<br>
Or maybe even like this if we want:<br>
<br>
(E)<br>
<br>
-> check::PreWholeNewThing // new callback (needs better name)<br>
<br>
-> check::PreCXXAllocator // another new callback<br>
-> check::PreCall<br>
1. CFGNewAllocator<br>
<- check::PostCall<br>
<- check::PostCXXAllocator // new callback<br>
<br>
-> check::PreStmt<CXXConstructExp<wbr>r><br>
-> check::PreCall<br>
2. CXXConstructExpr<br>
<- check::PostCall<br>
<- check::PostStmt<CXXConstructEx<wbr>pr><br>
<br>
-> check::PreStmt<CXXNewExpr><br>
3. CXXNewExpr<br>
<- check::PostStmt<CXXNewExpr><br>
<br>
-> check::PostWholeNewThing // new callback<br>
<br>
Variant (A) is what we commonly do for all other expressions. For instance, if we have an expression `a + b`, we only do PreStmt<BinaryOperator> *after* evaluation of both `a` and `b`. It kind of represents the semantics exactly. However, it would be completely counter-intuitive for checker authors to subscribe to PreStmt<CXXNewExpr> and find out that by the time their callback fires, both operator new() and the constructor(!) have already been called. So i expect confusion between CXXNewExpr in our sense (the big new-expression) and CXXNewExpr in a common person's sense (the allocator call). In particular, it would be nice to move MallocChecker to the new callback - eg. we're already done with region extent when we're constructing.<br>
<br>
Variant (B) tries to go around this confusion while keeping the "user-facing meaning" of CXXNewExpr as the big new-expression, but it goes against what we normally do, which would anyway be confusing.<br>
<br>
In variants (C) and (D) we have the "user-facing meaning" of CXXNewExpr changed into "here's where the allocation occurs". This is how MallocChecker currently understands things. Like (A), variant (C) tries to do what we usually do by only doing Pre-thingy after sub-thingies were evaluated (except evaluating CXXNewExpr before its child CXXConstructExpr, but we kinda agreed that they're not actually parent-child-related). However, because this time Pre-thingy is not a PreStmt-thingy, i find variant (D) fancier: the new callback is saying "we begin unleashing the operator new hell" and "we're done with the operator new hell nice and clean". Still, both (C) and (D) have an unpleasant downside of being unable to reliably read the value of CXXNewExpr at PostStmt<CXXNewExpr>, as the expression is not yet evaluated. This can probably be fixed, but it brings us back to the problem of whether we want to keep the value of the fake `_this` variable in the Environment as the value of CXXNewExpr this whole time. We probably do.<br>
<br>
Variant (E) is kinda a combination of all approaches, as it provides the "big" callback like (B) and (D), while still saying that CXXNewExpr is the big new-expression, while not messing up the usual PreStmt/PostStmt order like (B) did. It still has the problem of check::PreStmt<CXXNewExpr> being confusingly late, but otherwise seems usable.<br>
<br>
We can delay the choices between (A) and (E) and also between (C) and (D) until we actually want to introduce the new callbacks, but i guess it's reasonable to try to agree on what do we want to mean by CXXNewExpr.<br>
<br>
I personally feel that (D) is the friendliest approach. Checkers subscribe for new - they get operator new. Checkers subscribe for constructor - they get their constructor. Checkers want the whole thing - sure, here's our fancy custom callback. They may probably never even think about this whole hassle.<br>
<br>
My second favorite is (E), which has the strength-slash-weakness of consistency with the AST's CXXNewExpr being a parent of CXXConstructExpr. I believe that the reorder in the analysis order compared to the AST, even if it's not an actual reorder, would be quite intuitively acceptable. While the positioning of the new operator after the constructor wouldn't be.<br>
<br>
Then (A) is simply a trimmed-down variant of (E), (B) is just weird, and (C) doesn't seem to be anyhow better than (D).<br>
<br>
We can also combine (C) and (D), which is even better:<br>
<br>
(F)<br>
<br>
-> check::PreWholeNewThing // new callback (needs better name)<br>
<br>
-> check::PreStmt<CXXNewExpr><br>
-> check::PreCall<br>
1. CFGNewAllocator<br>
<- check::PostCall<br>
<- check::PostStmt<CXXNewExpr><br>
<br>
-> check::PreStmt<CXXConstructExp<wbr>r><br>
-> check::PreCall<br>
2. CXXConstructExpr<br>
<- check::PostCall<br>
<- check::PostStmt<CXXConstructEx<wbr>pr><br>
<br>
-> check::PreBigNewExpr // another new callback (needs better name)<br>
3. CXXNewExpr<br>
<- check::PostBigNewExpr // new callback<br>
<br>
<- check::PostWholeNewThing // new callback<br>
<br>
Which by far seems to be the best (not necessarily good) idea i could have come up with on this subject.<br>
<br>
Thoughts are welcome!~<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>
</blockquote></div></div>