<div dir="ltr"><div>What are your feelings on this run? I'm personally pleased with how things look like, and the only thing I'd like to add is pruning operator!=, though I do not plan on doing any more analyses.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, 10 Aug 2019 at 03:48, Artem Dergachev <<a href="mailto:noqnoqneo@gmail.com">noqnoqneo@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div bgcolor="#FFFFFF">
<br>
<br>
<div class="gmail-m_5652446046771540064moz-cite-prefix">On 8/8/19 2:05 PM, Kristóf Umann wrote:</div><blockquote type="cite"><div dir="ltr"><div dir="ltr">
<div class="gmail_attr"><br>
</div>
<div class="gmail_attr">The negatives:</div>
<div class="gmail_attr">1. Extra notes to functions following
this pattern were very-very common:</div>
<div class="gmail_attr"><br>
</div>
<div class="gmail_attr"><font face="monospace">bool a(int b) {<br>
return b == 0;</font>
<div class="gmail_attr"><font face="monospace"> // Assuming
'b' is equal to 0</font></div>
<div class="gmail_attr"><font face="monospace">
// Returning the value 1, which will be (a part of a)
condition</font></div>
<font face="monospace">}<br>
<br>
void f(int b) {<br>
int *x = 0;<br>
if (a(b)) // call to 'a', returning from 'a'<br>
*x = 5; // nullptr dereference<br>
}</font></div>
<div class="gmail_attr"><br>
</div>
<div class="gmail_attr">These notes are totally meaningless,
and they aren't pruned out by [1] because the assumption
notes are non-prunable (they explain a variable, b in this
case, that is marked as interesting, as the return value is
tracked by ReturnVisitor). A possible solution would be to
prune linear functions in ConditionBRVisitor as well
similarly to [1], but then we'd lose the assumption note.
Instead, moving the non-prunable assumption note to the
actual collapse point would be better, but a quick look at
the generated ExplodedGraph reveals that this no longer an
bug report construction issue: the collapse point is in fact
at the return statement, rather then the condition.</div>
<div class="gmail_attr"><br>
</div>
<div class="gmail_attr">So, I think the bug report should
always explain how the analyzer thinks. Ideally, this should
be identical to how the bug could be reproduced, but if it
is not (in this case the assumption should be made when a(b)
is the condition), maybe the logic of the analysis should be
changed instead.</div>
<div class="gmail_attr"><br>
</div>
<div class="gmail_attr">Example: Notes 19-23, <a href="http://cc.elte.hu:15001/GSoC2019-July-5/#is-unique=on&run=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&review-status=Confirmed&review-status=False%20positive&review-status=Intentional&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&reportHash=8c536e75492d2588898e03178a704efb&report=12672&subtab=8c536e75492d2588898e03178a704efb" target="_blank">http://cc.elte.hu:15001/GSoC2019-July-5/#is-unique=on&run=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&review-status=Confirmed&review-status=False%20positive&review-status=Intentional&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&reportHash=8c536e75492d2588898e03178a704efb&report=12672&subtab=8c536e75492d2588898e03178a704efb</a></div>
</div>
</div>
</blockquote>
<br>
This is due to the eagerly-assume behavior. The state split does
actually happen at == or ! rather than at the if-statement.<br>
<br>
I guess you could check if the node is wearing an "ExprEngine:
Eagerly Assume" tag. <br>
<br></div></blockquote><div><br></div><div>Then I guess we should leave this as-is.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF">
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_attr">2. I didn't come across this too
often, but it happened:</div>
<div class="gmail_attr"><br>
</div>
<div class="gmail_attr"><font face="monospace">int getInt();
int getInt2();<br>
<br>
bool a() {<br>
return getInt2() && getInt();</font></div>
<div class="gmail_attr"><font face="monospace"> // Returning
value, which will be (a part of a) condition<br>
}<br>
<br>
void f() {<br>
int *x = 0;</font><br style="font-family:monospace">
<span style="font-family:monospace"> if (a()) // call to
'a', returning from 'a'</span><br style="font-family:monospace">
<span style="font-family:monospace"> *x = 5; // nullptr
dereference</span><font face="monospace"><br>
}</font><br>
</div>
<div class="gmail_attr"><br>
</div>
<div class="gmail_attr"><font face="arial, sans-serif">This
note wasn't pruned because a() has in fact more than 3
basic blocks, but it really doesn't change anything. I
guess we *could* prune it, but I'm leaning on accepting it
as a sacrifice.</font></div>
<div class="gmail_attr"><br>
</div>
<div class="gmail_attr">Example: Notes 65-68, <a href="http://cc.elte.hu:15001/GSoC2019-July-5/#is-unique=on&run=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&review-status=Confirmed&review-status=False%20positive&review-status=Intentional&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&reportHash=8cc2c55ab79efc36fcd2b1725648c2c3&report=12678&subtab=8cc2c55ab79efc36fcd2b1725648c2c3" target="_blank">http://cc.elte.hu:15001/GSoC2019-July-5/#is-unique=on&run=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&review-status=Confirmed&review-status=False%20positive&review-status=Intentional&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&reportHash=8cc2c55ab79efc36fcd2b1725648c2c3&report=12678&subtab=8cc2c55ab79efc36fcd2b1725648c2c3</a></div>
</div>
</div>
</blockquote>
<br>
I guess it depends:<br>
<br>
- If we're assuming that the condition is true, and the function
returns A && B, the note is indeed redundant.<br>
- If we're assuming that the condition is true, and the function
returns A || B, it's worth mentioning whether we think that A is
true or we think that B is true.<br>
- If we're assuming that the condition is false, and the function
returns A && B, it's worth mentioning whether we think that
A is false or we think that B is false.<br>
- If we're assuming that the condition is false, and the function
returns A || B, the note is indeed redundant.<br></div></blockquote><div><br></div><div>Yea, I may fix it up later, though I'm not *immediately* worried about it since this problem isn't condition tracking specific. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF"><blockquote type="cite"><div dir="ltr"><div dir="ltr">
<div class="gmail_attr"><br>
</div>
<div class="gmail_attr">4. Calls to operator!= in foreach
loops -- Are these necessary? Is there any, even articial
cases where this is actually meaningful? This is pretty much
the only non-constructive addition of notes that are
introduced only because of condition tracking.</div>
<div class="gmail_attr"><br>
</div>
<div class="gmail_attr">Example: Notes 13-17, 22-26, 37-41: <a href="http://cc.elte.hu:15001/GSoC2019-July-5/#is-unique=on&run=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&review-status=Confirmed&review-status=False%20positive&review-status=Intentional&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&reportHash=50cc2bed223dc4ba62f46d0665346a5e&report=12743&subtab=50cc2bed223dc4ba62f46d0665346a5e" target="_blank">http://cc.elte.hu:15001/GSoC2019-July-5/#is-unique=on&run=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&review-status=Confirmed&review-status=False%20positive&review-status=Intentional&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&reportHash=50cc2bed223dc4ba62f46d0665346a5e&report=12743&subtab=50cc2bed223dc4ba62f46d0665346a5e</a></div>
</div>
</div>
</blockquote>
<br>
Mmm, yeah, i'd rather skip them. We don't make much sense out of
iterators anyway. I wish IteratorChecker could provide us with
better notes.<br>
<br></div></blockquote><div><br></div><div>I guess I'll fix this up quickly before making this on-by-default. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF">
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_attr">Positives:</div>
<div class="gmail_attr"><br>
</div>
<div class="gmail_attr">In general, the wording as the
analyses were run may be imperfect, but they indicate what
the message is about very precisely. The path of execution
is much better explained, and even if the extra function
call isn't meaningful, the note messages state clearly that
we're only talking about a condition, I can skip them
without being bothered by it too much. I am given extra
information without the cost of polluting the rest of the
report.</div>
<div class="gmail_attr"><br>
</div>
<div class="gmail_attr">Here is a collection of my favorites,
though I found most of the extra notes nice:</div>
<div class="gmail_attr"><br>
</div>
<div class="gmail_attr">1. Here, we encounter a nullpointer
dereference issue. Op0 is assumed to be equal to Op1, but
only due to condition tracking do we explain that Op1 is
believed to be null, and THAT is why this assumption (Op1 ==
Op0) is so important!</div>
<div class="gmail_attr"><br>
</div>
<div class="gmail_attr">Notes 23-31: <a href="http://cc.elte.hu:15001/GSoC2019-July-5/#is-unique=on&run=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&review-status=Confirmed&review-status=False%20positive&review-status=Intentional&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&reportHash=6b5f3e43213b64eaef4a25a249c59baf&report=12955&subtab=6b5f3e43213b64eaef4a25a249c59baf" target="_blank">http://cc.elte.hu:15001/GSoC2019-July-5/#is-unique=on&run=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&review-status=Confirmed&review-status=False%20positive&review-status=Intentional&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&reportHash=6b5f3e43213b64eaef4a25a249c59baf&report=12955&subtab=6b5f3e43213b64eaef4a25a249c59baf</a><br>
</div>
<div class="gmail_attr"><br>
</div>
</div>
</div>
</blockquote>
<br>
The note is indeed extremely helpful, but the warning itself should
have been suppressed by the "inlined defensive check" heuristic. I
guess the heuristic doesn't know how to carry over to the other side
of the `==`.<br><blockquote type="cite"><div dir="ltr"><div dir="ltr">
<div class="gmail_attr"><br>
</div>
<div class="gmail_attr">3. This one is weird, I'm not yet sure
how this note came about, but yet again, it isn't clear AT
ALL in the original report why CountS will be a source of a
null dereference error. The extra note tells us that CountS
is non-null, but its pointee is.</div>
<div class="gmail_attr"><br>
</div>
<div class="gmail_attr">Note 19: <a href="http://cc.elte.hu:15001/GSoC2019-July-5/#is-unique=on&run=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&review-status=Confirmed&review-status=False%20positive&review-status=Intentional&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&reportHash=356d88cb062474fdf17584bf1451bffc&report=13412&subtab=356d88cb062474fdf17584bf1451bffc" target="_blank">http://cc.elte.hu:15001/GSoC2019-July-5/#is-unique=on&run=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&review-status=Confirmed&review-status=False%20positive&review-status=Intentional&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=LLVM%2FClang%20WITH%20condition%20tracking%20FINAL&reportHash=356d88cb062474fdf17584bf1451bffc&report=13412&subtab=356d88cb062474fdf17584bf1451bffc</a></div>
</div>
</div>
</blockquote>
<br>
You're talking about the "Assuming pointer value is null" thingy? I
suspect that it's a bug and the note is in fact meaningless. See
also <a class="gmail-m_5652446046771540064moz-txt-link-freetext" href="https://reviews.llvm.org/D65889?id=214186#inline-590619" target="_blank">https://reviews.llvm.org/D65889?id=214186#inline-590619</a> - i've
seen them before but i suspect that i suddenly see more of them
these days. Generally, i still don't understand the report after
seeing these notes. Even if the notes tell anything about the
pointee, the code they're attached to doesn't.<br>
<br>
I also don't understand why did we bring in the copy-constructor.
Was there an invalidation of all heap going on that also touched
*CountS?<br></div></blockquote><div><br></div><div>The answer lies in the DEBUG notes run: We already track CollectionS to the point where it was initialized, and FindLastStoreBRVisitor tracks the right hand side of it. This will trigger control dependency calculation again, resulting in State being tracked, which leads to that monster of a copy constructor call, where we explain why we believed State to be null. The call to IntrusiveRefCntPtr::operator bool was pruned, but it explains why we placed a note about Obj, because thats what it returns. I do wonder however why it wasn't accompanied with ", which will be (a part of a) condition", I'll take a look.</div><div> </div></div></div>