<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Mar 25, 2013, at 8:01 , Anton Yartsev <<a href="mailto:anton.yartsev@gmail.com">anton.yartsev@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">
<meta content="text/html; charset=windows-1252" http-equiv="Content-Type">
<div text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Committed as r177849</div>
<br>
Manage to find several random real bugs (report-843813.html,
report-230257.html, recursive case in report-727931.html), but for
now it is hard to detect real bugs among tons of false-positives.<br>
<br>
There are two types of false-positives that form the majority of
reports:<br>
1) Illustrated by the following test (added similar test to
NewDelete-checker-test.mm):<br>
int *global;<br>
void testMemIsOnHeap() {<br>
int *p = new int; // FIXME: currently not heap allocated! <br>
if (global != p) // evaluates to UnknownVal() rather then 'true'<br>
global = p;<br>
} // report false-positive leak<br>
<br>
As I understand the problem is that currently a memory region for
'new' is not a heap region and this lead to false-positives like
report-863595.html and others. (e.g. that causes 'global != p'
evaluate to UnknownVal() rather then 'true' (logic of
SimpleSValBuilder::evalBinOpLL))<br>
<br>
Attached is the proposed patch that fixes these issues. </div></blockquote><div><br></div><div>There are two reasons I didn't use getConjuredHeapSymbol when I originally put in this code:</div><div>(1) It handles all CXXNewExprs, even if the allocator is not one of the global ones.</div><div>(2) Heap symbols weren't used yet (Anna added them later for MallocChecker).</div><div><br></div><div>Obviously #2 is bogus now. #1 worries me a bit because it requires duplicating some of the checks you just added to MallocChecker.</div><div><br></div><div>In the long run, the design would be to get the appropriate memory from the allocator call, and we have PR12014's restructuring of the CFG blocking that. I'm not sure if we'd then move the heap symbol logic into a checker, or if it would still stay in Core.</div><div><br></div><div>In the short term, I guess the best idea is to duplicate some of the checks (or refactor them to a helper function somewhere...though probably <i>not</i> into AST) and then conjure a heap symbol if we know we can.</div><div><br></div><br><blockquote type="cite"><div text="#000000" bgcolor="#FFFFFF">2) The second type of false-positives is illustrated by the
following minimal test:<br>
void f(const int & x);<br>
<br>
void test() {<br>
int *p = (int *)malloc(sizeof(int));<br>
f(*p);<br>
} // report false-positive leak<br>
<br>
report-218274.html shows how it looks like in reality.<br>
Haven't addressed this yet. Removing 'const' from the declaration
eliminates the leak report. </div></blockquote><div><br></div><div>Interesting. You can't change a const region (and pedantically you can't free() it either), but you certainly can 'delete' it. (C++11 [expr.delete]p2)</div><div><br></div><div>Anna, any thoughts on this? Should these also count as "pointer escaped" even though their contents have not been invalidated?</div><div><br></div><br><blockquote type="cite"><div text="#000000" bgcolor="#FFFFFF">I had a cold and my performance decreased significantly so sorry for
delays and inattention :)<br></div></blockquote></div><br><div>Please take care of yourself! Hope you are feeling better.</div><div>Jordan</div></body></html>