<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 2:24, Jordan Rose wrote:<br>
</div>
<blockquote
cite="mid:959015FF-D6AE-47A7-97D8-B64A0E600CD0@apple.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
Looking over this one last time...
<div><br>
</div>
<blockquote type="cite">
<div>
<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> << offsetBytes</div>
<div> << " "</div>
<div> << ((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>
</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>
<div>+@interface Wrapper : NSData</div>
<div>+- (id)initWithBytesNoCopy:(void *)bytes
length:(NSUInteger)len;</div>
<div>+@end</div>
</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: <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>
<div>
<blockquote type="cite">+ delete x; // FIXME: Shoud detect
pointer escape and keep silent. checkPointerEscape() is not
currently invoked for delete.</blockquote>
</div>
</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>
</body>
</html>