<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 12.03.2013 22:06, Anna Zaks wrote:<br>
</div>
<blockquote
cite="mid:44CDB5FC-2B2F-4AAE-ADA3-EE06C22B0F3B@apple.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<div>
<div>Thanks Anton! The patch looks good overall. Have you
evaluated it on some real C++ codebases?</div>
</div>
</blockquote>
Not yet. Failed to launch scan-build with my Perl 5.16.2.<br>
<br>
<blockquote
cite="mid:44CDB5FC-2B2F-4AAE-ADA3-EE06C22B0F3B@apple.com"
type="cite">
<div>
<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>
</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
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>
<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>
<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>
<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>
<div><br>
</div>
</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>
<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>
<div>+ // Rest of 32-bit word, currently just an allocation
family.</div>
</div>
<div>
<div>+ unsigned Family : 30; </div>
</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>
<br>
void test(unichar *characters) {<br>
NSString *string = [[NSString alloc]
initWithCharactersNoCopy:characters length:12 freeWhenDone:1];<br>
if (!string) {free(characters);}<br>
}<br>
<br>
<blockquote
cite="mid:44CDB5FC-2B2F-4AAE-ADA3-EE06C22B0F3B@apple.com"
type="cite">
<div>
<div>
<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>
</div>
</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>
<div><br>
</div>
<div>Thanks,</div>
<div>Anna.</div>
</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>
</body>
</html>