<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Attached is the new version of the
patch, several comments below.<br>
</div>
<blockquote
cite="mid:1BCDB3E6-F065-458F-9A83-E93381CD9BFC@apple.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<br>
<div>
<div>On Feb 22, 2013, at 7:24 , 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=windows-1252"
http-equiv="Content-Type">
<div text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 22.02.2013 9:30, Anna Zaks
wrote:<br>
</div>
<blockquote
cite="mid:254908E4-011B-4100-BE1A-48A980CDD204@apple.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<br>
<div>
<div>On Feb 21, 2013, at 6:26 PM, Anna Zaks <<a
moz-do-not-send="true" href="mailto:ganna@apple.com">ganna@apple.com</a>>
wrote:</div>
<br class="Apple-interchange-newline">
<blockquote type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<div style="word-wrap: break-word; -webkit-nbsp-mode:
space; -webkit-line-break: after-white-space;"><br>
<div>
<div>On Feb 21, 2013, at 6:00 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">
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
<div text="#000000" bgcolor="#FFFFFF">
<blockquote
cite="mid:2CAEFC0E-8B7A-4301-9143-CC1EC37A582E@apple.com"
type="cite">
<meta http-equiv="Content-Type"
content="text/html; charset=windows-1252">
<meta http-equiv="Content-Type"
content="text/html; charset=windows-1252">
<br>
<div>
<div>On Feb 19, 2013, at 10:18 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">Hi, Jordan. Thanx
for the review!<br>
<br>
Attached is the new version of the patch
with all the comments addressed. Also
added support for directly called
operator new()/new[]() and operator
delete()<br>
<br>
There is currently one problem with
handling of operator delete(). The
following test<br>
<br>
void testDeleteOp1() {<br>
int *p = (int *)malloc(sizeof(int));<br>
operator delete(p); // FIXME: should
complain "Argument to operator delete()
was allocated by malloc(), not operator
new"<br>
}<br>
<br>
produce no warnings as attached RefState
seem to be missing at the point when
checkPostStmt(const CallExpr *CE,
CheckerContext &C) callback is
called for operator delete(p).<br>
I haven't investigated the problem
deeply yet, intend to address it
parallel with the review.<br>
<br>
<blockquote type="cite">+ if
(NE->getNumPlacementArgs())<br>
+ return;<br>
+ // skip placement new operators as
they may not allocate memory<br>
<br>
Two comments here:<br>
- Please make sure all comments are
complete, capitalized, and punctuated
sentences. (This has the important
one, "complete"....just missing
capitalization and punctuation.)<br>
</blockquote>
I'll try. Unfortunately I am not as good
in English as I want to be, so sorry for
my grammar, syntax, and punctuation.<br>
<br>
-- <br>
Anton<br>
<br>
<span><MallocChecker_v2.patch></span></blockquote>
<div><br>
</div>
</div>
</blockquote>
<br>
Hi Anna. Thanks for your comments! Attached is
the new patch.<br>
<br>
<blockquote
cite="mid:2CAEFC0E-8B7A-4301-9143-CC1EC37A582E@apple.com"
type="cite">
<div>Just adding another kind as extra
enumeration values does not seem right.
Another option is to make Kind be a
pointer to a static array, which contains
objects recording all necessary info about
each kind (<span style="color: rgb(0, 97,
65); font-family: Monaco; font-size:
11px; ">MacOSKeychainAPIChecker</span> uses
this approach). This is probably an
overkill for now, but is another option.</div>
</blockquote>
Not sure that I have got an idea.<br>
The memory and deallocator kind are both set
for a RefState. Do you mean creating a static
array with 'memory kinds' * 'deallocator kind'
elements for all possible combinations? Also
there is no necessary info other then the kind
itself.<br>
Left for now.<br>
</div>
</blockquote>
<div><br>
</div>
<div>I think of ToBeReleasedWith* as being
different types of Allocate; I don't think they
should be separate values in the same enum. It's
also unfortunate to have to copy the constant
values in both places - DeallocatorKind and
RefState::Kind. Maybe you could restructure this
similarly to how this is done in SVals.h?<br>
<blockquote type="cite">
<div text="#000000" bgcolor="#FFFFFF"><br>
<blockquote
cite="mid:2CAEFC0E-8B7A-4301-9143-CC1EC37A582E@apple.com"
type="cite">
<div>+ const FunctionDecl
*getCalleeDecl() const { return
CalleeDecl; }</div>
<div>Do we only store the name of the
allocator declaration here? </div>
</blockquote>
</div>
</blockquote>
<div><br>
</div>
<div>If the Decl is always an allocator Decl, we
should change the name of the method to be
more descriptive.</div>
</div>
</div>
</div>
</blockquote>
</div>
</blockquote>
<blockquote
cite="mid:254908E4-011B-4100-BE1A-48A980CDD204@apple.com"
type="cite">
<div>
<blockquote type="cite">
<div style="word-wrap: break-word; -webkit-nbsp-mode:
space; -webkit-line-break: after-white-space;">
<div>
<blockquote type="cite">
<div text="#000000" bgcolor="#FFFFFF">
<blockquote
cite="mid:2CAEFC0E-8B7A-4301-9143-CC1EC37A582E@apple.com"
type="cite">
<div>Do we need to store this in the state?
(Growing states implies memory overhead.)
Can't this be easily implied from the
kind?</div>
</blockquote>
Kind can't give us information about the name
of an allocator that can be malloc(),
realloc(), a user function with
ownership_takes attribute, etc.</div>
</blockquote>
<blockquote type="cite">
<div text="#000000" bgcolor="#FFFFFF"> One
solution to avoid keeping a CalleeDecl in
RefState is to rollback to
CallExpr::getDirectCallee() from
CheckerContext::getCalleeDec() and to print
"malloc()" in case of indirect calls.<br>
</div>
</blockquote>
<div><br>
</div>
Ok, I see.<br>
</div>
</div>
</blockquote>
<div><br>
</div>
After thinking about it some more, I do not think we
should add an extra pointer to the state to
differentiate between few allocator functions. In the
case when we do not have ownership attributes, we only
have few possible allocators, whose names we know ahead
of time. In case we support ownership attributes, we are
likely to have few allocator functions whose names we
could just store in the checker state on the first
encounter (like we store the IdentifierInfo). </div>
<div><br>
</div>
<div>In addition, we could change the ownership attributes
in such a way that each allocator would have a
corresponding deallocator; for example, if we wanted to
check matching allocators and deallocators. Annotated
deallocators won't necessarily be one of the functions
you know at compile time, so the DeallocatorKind enum
would not cover it. I think, it's best if we kept a
table on a side that would store this info (allocation
function name, deallocator) and refer to the entries in
the table from the state. (Take a look at
MacOSKeychainAPIChecker - it's very similar to malloc,
but it handles different allocator/deallocator pairs. I
think a similar solution could work in this case as
well. Other solutions that address these issues are
welcome as well!)</div>
</blockquote>
Attached is the patch that uses approach with a dynamic
table that holds both allocator name and expected
deallocator kind. This approach allows to keep any allocator
names, either known or new ones. The table could be easily
extended to hold additional data such as info about special
deallocators, etc.<br>
</div>
</blockquote>
</div>
<br>
<div>Please do not do this. The analyzer is not thread-safe today,
but that's no reason why we should make it harder to make it
thread-safe tomorrow.</div>
<div><br>
</div>
<div>Stepping back, there are many problems with this, the main
one being that just keeping track of the function name isn't
really good enough. MacOSKeychainAPIChecker can get away with it
because its functions are </div>
<div><br>
</div>
<div>I think all Anna meant is to have a <i>static</i> table,
like MacOSKeychainAPIChecker. If/when we support allocator
families defined at runtime, we can do this properly, with a
mutable table as a field of the checker, but for now this is
both overkill and error-prone.</div>
<br>
</blockquote>
<br>
<blockquote
cite="mid:1BCDB3E6-F065-458F-9A83-E93381CD9BFC@apple.com"
type="cite">
<div><br>
</div>
<div>Capitalization, period.</div>
<div><br>
</div>
</blockquote>
Checked and fixed everything, sorry.<br>
<br>
<br>
<blockquote
cite="mid:1BCDB3E6-F065-458F-9A83-E93381CD9BFC@apple.com"
type="cite">
<div>This is <i>still</i> the wrong check. You need to check if
there are <i>any</i> placement arguments, and you need to check
that the Decl is not part of a class. Both are easy:</div>
</blockquote>
Changed logic to skip all placement operators except the standard
placement nothrow versions that we know to allocate/deallocate
memory.<br>
<br>
<br>
<blockquote
cite="mid:1BCDB3E6-F065-458F-9A83-E93381CD9BFC@apple.com"
type="cite">
<div><br>
</div>
<div>
<div>+ if (isa<ObjCMessageExpr>(E))</div>
<div>+ return D_free;</div>
</div>
<div><br>
</div>
<div>There are no Objective-C messages that free memory
immediately; just take this out.</div>
<div><br>
</div>
</blockquote>
Returning DK_free for Objective-C messages just mean that they
belong to the 'malloc/free' family. Consider the following example:<br>
void test() {<br>
int *p = (int *)malloc(sizeof(int));<br>
[NSData dataWithBytesNoCopy:bytes length:sizeof(int)];<br>
}<br>
Here we just check that dataWithBytesNoCopy:length: is from
'malloc/free' family and keep silent. <br>
This also implements '// TODO: Check that the memory was allocated
with malloc.' from the checkPostObjCMessage().<br>
Do you want to handle this situation in some other way?<br>
<br>
<pre class="moz-signature" cols="72">--
Anton</pre>
</body>
</html>