<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 27.03.2013 20:45, Jordan Rose wrote:<br>
</div>
<blockquote
cite="mid:587549F8-11B9-463F-ACC0-E0D7E5D67455@apple.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=ISO-8859-1">
<br>
<div>
<div>On Mar 27, 2013, at 5:32 , Anton Yartsev <<a
moz-do-not-send="true" 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=ISO-8859-1"
http-equiv="Content-Type">
<div text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 27.03.2013 5:23, Jordan Rose
wrote:<br>
</div>
<blockquote
cite="mid:9BEA794A-8D25-4F43-B742-A62F39837FFD@apple.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=ISO-8859-1">
<br>
<div>
<div>On Mar 26, 2013, at 17:10 , Anton Yartsev <<a
moz-do-not-send="true"
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=ISO-8859-1"
http-equiv="Content-Type">
<div text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 25.03.2013 21:39,
Anna Zaks wrote:<br>
</div>
<blockquote
cite="mid:9084B92B-44BB-497B-9F7D-2A1AAA599822@apple.com"
type="cite">
<meta http-equiv="Content-Type"
content="text/html; charset=ISO-8859-1">
<div>
<div>
<div>On Mar 25, 2013, at 9:30 AM, Jordan Rose
<<a moz-do-not-send="true"
href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>>
wrote:</div>
<br class="Apple-interchange-newline">
<blockquote type="cite">
<div style="font-family: Helvetica;
font-size: 12px; font-style: normal;
font-variant: normal; font-weight: normal;
letter-spacing: normal; line-height:
normal; orphans: auto; text-align: start;
text-indent: 0px; text-transform: none;
white-space: normal; widows: auto;
word-spacing: 0px;
-webkit-text-size-adjust: auto;
-webkit-text-stroke-width: 0px;">
<div><br class="Apple-interchange-newline">
On Mar 25, 2013, at 8:01 , Anton Yartsev
<<a moz-do-not-send="true"
href="mailto:anton.yartsev@gmail.com">anton.yartsev@gmail.com</a>>
wrote:</div>
<br class="Apple-interchange-newline">
<blockquote type="cite">
<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!<span
class="Apple-converted-space"> </span><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<span
class="Apple-converted-space"> </span><i>not</i> into
AST) and then conjure a heap symbol if
we know we can.</div>
</div>
</blockquote>
</div>
</div>
</blockquote>
Failed to find any suitable place other then AST :)
Eventually noticed, that actually only a single
check should be duplicated. Decided to leave the
wide comment added when I tried to find the proper
place for isStandardNewDelete().<br>
New fix attached.<br>
</div>
</blockquote>
<div><br>
</div>
<div>I don't think this is safe -- what if the user has
custom libraries in their system include path? We can
really only assume heap allocation for the implicit
case and for ::operator new(std::size_t) and
::operator new(std::size_t, std::nothrow_t).</div>
<div><br>
</div>
<div>However, I think just checking "global namespace"
might be a good enough approximation. If the user
overrides one of the default allocators, it ought to
behave like the real one, so we'd only be messing up
if they had a new global "allocate from pool" or
something.</div>
<div><br>
</div>
<div>Also, in theory we'd have to check placement new,
but that's handled specially below anyway.</div>
</div>
</blockquote>
Updated the logic, improved test coverage.<br>
</div>
</blockquote>
</div>
<br>
<div>
<div>+ // Skip all operator new/delete methods (including
static ones).</div>
</div>
<div><br>
</div>
<div>All operator new/delete methods are implicitly static, so you
can just leave out the parenthetical. On the other hand, it
might be worth noting that we realize this is an approximation
that might not correctly model a custom global allocator.</div>
</blockquote>
Fixed at r178244. The note gone to ExprEngine::VisitCXXNewExpr().<br>
<br>
<pre class="moz-signature" cols="72">--
Anton</pre>
</body>
</html>