<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 14.03.2013 21:42, Anna Zaks wrote:<br>
</div>
<blockquote
cite="mid:29C2DC67-546F-4D8B-B74C-EBF84547969B@apple.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
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>
<br>
<blockquote
cite="mid:29C2DC67-546F-4D8B-B74C-EBF84547969B@apple.com"
type="cite">
<div><br>
</div>
<div>Thanks,</div>
<div>Anna.</div>
<div><br>
<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>
</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>
<div>
<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>
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><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>
</div>
</div>
</div>
</blockquote>
<br>
<br>
<pre class="moz-signature" cols="72">--
Anton</pre>
</body>
</html>