<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 22.03.2013 21:22, Anna Zaks wrote:<br>
</div>
<blockquote
cite="mid:6BED0B8B-0AF8-42FA-A387-017F9584BD66@apple.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<br>
<div>
<div>On Mar 21, 2013, at 8:01 PM, 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" style="letter-spacing:
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 class="moz-cite-prefix">On 14.03.2013 21:42, Anna Zaks
wrote:<br>
</div>
<blockquote
cite="mid:29C2DC67-546F-4D8B-B74C-EBF84547969B@apple.com"
type="cite">One more concern. I do not see any tests with
path notes. It's very important to test not only the
existence of the warning, but also the notes along the
path.
<div> - malloc-path.c currently tests the path notes
produced by the warnings from the MallocChecker</div>
<div> -
test/Analysis/diagnostics/deref-track-symbolic-region.c
is a good example of how to test for the notes in text
and plist formats(used to draw the path in Xcode).</div>
<div><br>
</div>
<div>I'd add a new test file and test the diagnostics in
both text and plist format. I am OK with this test and
any related work going in as a separate commit.</div>
</blockquote>
New test added,
MallocChecker::MallocBugVisitor::isAllocated() and
MallocChecker::MallocBugVisitor::isReleased() changed a
little.<br>
Nothing else changed since the last patch.<br>
OK to commit?<br>
</div>
</blockquote>
<div><br>
</div>
<div>Anton, </div>
<div><br>
</div>
<div>Jordan has noticed that you have Windows line endings
('\r') in the new file. Please, make sure you do not commit
these.</div>
<div><br>
</div>
<div>Otherwise, we are OK with post commit review.</div>
<div><br>
</div>
<div>I am curious if you were able to find any real bugs with
the new checker.</div>
</div>
</blockquote>
Committed as r177849<br>
<br>
Manage to find several random real bugs (report-843813.html,
report-230257.html, report-444177.html, report-737879.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.<br>
<br>
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.<br>
<br>
I had a cold and my performance decreased significantly so sorry for
delays and inattention :)
<blockquote
cite="mid:6BED0B8B-0AF8-42FA-A387-017F9584BD66@apple.com"
type="cite">
<div><br>
<blockquote type="cite">
<div text="#000000" bgcolor="#FFFFFF" style="letter-spacing:
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;">
<blockquote
cite="mid:29C2DC67-546F-4D8B-B74C-EBF84547969B@apple.com"
type="cite">
<div>
<div><br>
<div>
<div>On Mar 14, 2013, at 8:38 AM, 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"
style="letter-spacing: 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 class="moz-cite-prefix">On 12.03.2013
22:06, Anna Zaks wrote:<br>
</div>
<blockquote
cite="mid:44CDB5FC-2B2F-4AAE-ADA3-EE06C22B0F3B@apple.com"
type="cite">Thanks Anton! The patch looks good
overall. Have you evaluated it on some real
C++ codebases?</blockquote>
Not yet. Failed to launch scan-build with my
Perl 5.16.2.<br>
<br>
</div>
</blockquote>
<div><br>
</div>
<div>Let's do this ASAP. It might point out issues
we have not thought of yet.</div>
<br>
<blockquote type="cite">
<div text="#000000" bgcolor="#FFFFFF"
style="letter-spacing: 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;">
<blockquote
cite="mid:44CDB5FC-2B2F-4AAE-ADA3-EE06C22B0F3B@apple.com"
type="cite">
<div><br>
</div>
<div>Below are some comments.</div>
<div><br>
</div>
<div>
<div>
<div>---</div>
<div>+ // The return value from operator
new is already bound and we don't want
to </div>
<div>+ // break this binding so we call
MallocUpdateRefState instead of
MallocMemAux.</div>
<div>+ State = MallocUpdateRefState(C,
NE, State, NE->isArray() ?
AF_CXXNewArray </div>
<div>+
: AF_CXXNew);</div>
</div>
<div>Why is this different from handling
malloc? <span style="font-family: Menlo;">MallocMemAux</span> does
break the binding formed by default
handling of malloc and forms a new binding
with more semantic information. (I am fine
with addressing this after the initial
commit/commits.)</div>
</div>
</blockquote>
In case of 'new' the memory can be initialized
to a non-default value (e.g. int *p = new
int(3); ). The existing logic of<span
class="Apple-converted-space"> </span><span
style="font-family: Menlo;">MallocMemAux</span>()
breaks the binding and information about the
initialization value is lost. This causes
several tests to fail.<br>
Changed the comment to be more informative.<br>
</div>
</blockquote>
<div><br>
</div>
I see. I am concerned about the inconsistency of
processing malloc and new. On the other hand, it
probably does not matter right now.</div>
<div><br>
<blockquote type="cite">
<div text="#000000" bgcolor="#FFFFFF"
style="letter-spacing: 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;"><br>
<blockquote
cite="mid:44CDB5FC-2B2F-4AAE-ADA3-EE06C22B0F3B@apple.com"
type="cite">
<div>
<div><br>
</div>
<div>---</div>
<div> def MallocOptimistic :
Checker<"MallocWithAnnotations">,</div>
<div>- HelpText<"Check for memory leaks,
double free, and use-after-free problems.
Assumes that all user-defined functions
which might free a pointer are
annotated.">,</div>
<div>+ HelpText<"Check for memory leaks,
double free, and use-after-free problems.
Traces memory managed by malloc()/free().
Assumes that all user-defined functions
which might free a pointer are
annotated.">,</div>
</div>
<div><br>
</div>
<div>Shouldn't the MallocWithAnnotations only
check the functions which are explicitly
annotated? I think it might be better to
change the code rather than the comment.</div>
</blockquote>
Currently MallocWithAnnotations is designed as
extended unix.Malloc and it is not a single line
change to make it distinct. Can we address this
after primary commits?<br>
<br>
</div>
</blockquote>
<div><br>
</div>
<div>Addressing after the initial patch is fine.</div>
<br>
<blockquote type="cite">
<div text="#000000" bgcolor="#FFFFFF"
style="letter-spacing: 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;">
<blockquote
cite="mid:44CDB5FC-2B2F-4AAE-ADA3-EE06C22B0F3B@apple.com"
type="cite">
<div><br>
</div>
<div>---</div>
<div>+ unsigned K : 2; // Kind enum, but
stored as a bitfield.</div>
<div>
<div>+ unsigned Family : 30; // Rest of
32-bit word, currently just an allocation </div>
<div>+ // family.</div>
</div>
<div>
<div><br>
</div>
<div>We usually add the comment on a line
preceding the declaration, like this:</div>
<div>
<div>+ // Kind enum, but stored as a
bitfield.</div>
<div>+ unsigned K : 2; </div>
<div>+ // Rest of 32-bit word, currently
just an allocation family.</div>
<div>+ unsigned Family : 30; </div>
<div><br>
</div>
<div>
<div>---</div>
<div>+ // Check if an expected
deallocation function matches the real
one.</div>
<div>+ if (RsBase && </div>
<div>+
RsBase->getAllocationFamily() !=
AF_None &&</div>
<div>+
RsBase->getAllocationFamily() !=
getAllocationFamily(C, ParentExpr) ) {</div>
</div>
<div>Is it possible to have AF_None family
here? Shouldn't "
RsBase->getAllocationFamily() !=
AF_None" be inside an assert?</div>
</div>
</div>
</blockquote>
It is possible. In the example below
initWithCharactersNoCopy:length:freeWhenDone
takes ownership of memory allocated by unknown
means, RefState with AF_None is bound to the
call and after, when free() is processed, we
have RsBase->getAllocationFamily() ==
AF_None.<br>
</div>
</blockquote>
<div><br>
</div>
<div>My understanding is that this API assumes that
the memory belongs to the malloc family. So, for
example, we would warn on freeing with a regular
"free" after freeing with
"initWithCharactersNoCopy.. freeWhenDone".</div>
<div><br>
</div>
<div>If the family is unknown, we should not be
tracking the memory at all.</div>
</div>
</div>
</div>
</blockquote>
Great idea, I'll include corresponding changes in the next
patch devoted to unix.MismatchedDeallocator<br>
<br>
<blockquote
cite="mid:29C2DC67-546F-4D8B-B74C-EBF84547969B@apple.com"
type="cite">
<div><br>
<blockquote type="cite">
<div text="#000000" bgcolor="#FFFFFF"
style="letter-spacing: 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;"><br>
void test(unichar *characters) {<br>
<span class="Apple-converted-space"> </span>NSString
*string = [[NSString alloc]
initWithCharactersNoCopy:characters length:12
freeWhenDone:1];<br>
<span class="Apple-converted-space"> </span>if
(!string) {free(characters);}<br>
}<br>
<br>
<blockquote
cite="mid:44CDB5FC-2B2F-4AAE-ADA3-EE06C22B0F3B@apple.com"
type="cite">
<div><br>
</div>
<div>
<div>---</div>
<div>+// Used to check correspondence between
allocators and deallocators.</div>
<div>+enum AllocationFamily {</div>
</div>
<div>The comment should describe what family is.
It is a central notion for the checker and I do
not think we explain it anywhere.</div>
<div><br>
</div>
<div>---</div>
<div>The patch is very long. Is it possible to
split it up into smaller chunks (<a
moz-do-not-send="true"
href="http://llvm.org/docs/DeveloperPolicy.html#incremental-development">http://llvm.org/docs/DeveloperPolicy.html#incremental-development</a>)?</div>
</blockquote>
Committed the initial refactoring as r176949.<br>
<br>
Let's start! Attached is the cplusplus.NewDelete
checker separated from the patch.<br>
<br>
<blockquote
cite="mid:44CDB5FC-2B2F-4AAE-ADA3-EE06C22B0F3B@apple.com"
type="cite">
<div>
<div><br>
</div>
<div>Thanks,</div>
<div>Anna.</div>
</div>
<div>On Mar 12, 2013, at 8:56 AM, Anton Yartsev
<<a moz-do-not-send="true"
href="mailto:anton.yartsev@gmail.com">anton.yartsev@gmail.com</a>>
wrote:</div>
<div><br class="Apple-interchange-newline">
<blockquote type="cite">
<div text="#000000" bgcolor="#FFFFFF"
style="letter-spacing: 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 class="moz-cite-prefix">On 12.03.2013
2:24, Jordan Rose wrote:<br>
</div>
<blockquote
cite="mid:959015FF-D6AE-47A7-97D8-B64A0E600CD0@apple.com"
type="cite">Looking over this one last
time...
<div><br>
</div>
<blockquote type="cite">
<div>- os << "Argument to free()
is offset by "</div>
<div>+ os << "Argument to ";</div>
<div>+ if (!printAllocDeallocName(os,
C, DeallocExpr))</div>
<div>+ os << "deallocator";</div>
<div>+ os << " is offset by "</div>
<div> <span
class="Apple-converted-space"> </span><<
offsetBytes</div>
<div> <span
class="Apple-converted-space"> </span><<
" "</div>
<div> <span
class="Apple-converted-space"> </span><<
((abs(offsetBytes) > 1) ? "bytes" :
"byte")</div>
<div>- << " from the start of
memory allocated by malloc()";</div>
<div>+ << " from the start of
";</div>
<div>+ if (AllocExpr) {</div>
<div>+ SmallString<100>
TempBuf;</div>
<div>+ llvm::raw_svector_ostream
TempOs(TempBuf);</div>
<div> </div>
<div>+ if
(printAllocDeallocName(TempOs, C,
AllocExpr))</div>
<div>+ os << "memory
allocated by " << TempOs.str();</div>
<div>+ else</div>
<div>+ os << "allocated
memory";</div>
<div>+ } else</div>
<div>+ printExpectedAllocName(os, C,
DeallocExpr);</div>
<div>+</div>
</blockquote>
<div><br>
</div>
The way you've set it up, AllocExpr will
never be NULL (which is good, because
otherwise you'd get "...from the start of
malloc()" rather than "from the start of
memory allocated by malloc()").</blockquote>
Strange logic. Fixed.<br>
<br>
<blockquote
cite="mid:959015FF-D6AE-47A7-97D8-B64A0E600CD0@apple.com"
type="cite">
<div><br>
<div><br>
</div>
<blockquote type="cite">
<div>+@interface Wrapper : NSData</div>
<div>+- (id)initWithBytesNoCopy:(void
*)bytes length:(NSUInteger)len;</div>
<div>+@end</div>
</blockquote>
<div><br>
</div>
<div>As I discovered with the rest of
the ObjC patch, this isn't a great
test case because the analyzer doesn't
consider it a system method. However,
I don't see you use it anywhere in the
file anyway, so you can probably just
remove it.</div>
</div>
<div><br>
</div>
<div><br>
</div>
<div>
<blockquote type="cite">+void
testNew11(NSUInteger dataLength) {</blockquote>
<blockquote type="cite">+ int *data =
new int;</blockquote>
<blockquote type="cite">+ NSData
*nsdata = [NSData
dataWithBytesNoCopy:data
length:sizeof(int) freeWhenDone:1]; //
expected-warning{{Memory allocated by
'new' should be deallocated by
'delete', not
+dataWithBytesNoCopy:length:freeWhenDone:}}</blockquote>
<blockquote type="cite">+}</blockquote>
</div>
<div><br>
</div>
<div>Hm, that is rather unwieldy, but what
bothers me more is that
+dataWithBytesNoCopy:length:freeWhenDone:<span
class="Apple-converted-space"> </span><i>doesn't</i> free
the memory; it just takes ownership of
it. I guess it's okay to leave that as a
FIXME for now, but in the long run we
should say something like
"+dataWithBytesNoCopy:length:freeWhenDone:
cannot take ownership of memory
allocated by 'new'." (In the "hold"
cases, most likely the user wasn't
intending to free </div>
<div><br>
</div>
<div>But, this doesn't have to block the
patch; you/we can fix it post-commit.</div>
<div><br>
</div>
<div><br>
</div>
<div>
<blockquote type="cite">+ delete x; //
FIXME: Shoud detect pointer escape and
keep silent. checkPointerEscape() is
not currently invoked for delete.</blockquote>
</div>
<div><br>
</div>
<div>Pedantic note: the real issue here is
that we don't model delete at all
(see ExprEngine::VisitCXXDeleteExpr).
The correct model won't explicitly
invoke checkPointerEscape, but it will
construct a CallEvent for the deletion
operator and then try to evaluate that
call—or at least invalidate the
arguments like VisitCXXNewExpr does for
placement args—which will cause the
argument region to get invalidated and
checkPointerEscape to be triggered.</div>
<div><br>
</div>
<div>Jordan</div>
</blockquote>
Updated patch attached.<br>
<pre class="moz-signature" cols="72">--
Anton</pre>
<span><MallocChecker_v11.patch></span></div>
</blockquote>
</div>
<br>
</blockquote>
<pre class="moz-signature" cols="72">--
Anton</pre>
<span><cplusplus.NewDelete_v1.patch></span></div>
</blockquote>
</div>
<br>
</blockquote>
<br>
<br>
<pre class="moz-signature" cols="72">--
Anton</pre>
<span><cplusplus.NewDelete_v2.patch></span></div>
</blockquote>
</div>
<br>
</blockquote>
<br>
<br>
<pre class="moz-signature" cols="72">--
Anton</pre>
</body>
</html>