<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body 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=ISO-8859-1">
<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=ISO-8859-1">
<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=ISO-8859-1"
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=ISO-8859-1">
<meta http-equiv="Content-Type" content="text/html;
charset=ISO-8859-1">
<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>
<br>
<blockquote
cite="mid:254908E4-011B-4100-BE1A-48A980CDD204@apple.com"
type="cite">
<div><br>
</div>
<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">Jordan, what do
you think about this?</div>
</blockquote>
<blockquote type="cite">
<div text="#000000" bgcolor="#FFFFFF"> <br>
<blockquote
cite="mid:2CAEFC0E-8B7A-4301-9143-CC1EC37A582E@apple.com"
type="cite">
<div>
<div>+void MallocChecker::checkPostStmt(const
CXXNewExpr *NE, </div>
<div>+
CheckerContext &C) const {NE</div>
<div>+</div>
<div>+ FunctionDecl *OperatorNew =
NE->getOperatorNew();</div>
<div>+ if (!OperatorNew)</div>
<div>+ return;</div>
<div>+</div>
<div>+ // Skip custom new operators</div>
<div>+ if (!OperatorNew->isImplicit()
&&</div>
<div>+
!C.getSourceManager().isInSystemHeader(OperatorNew->getLocStart())
&&</div>
<div>+ !NE->isGlobalNew())</div>
<div>+ return;</div>
<div>+</div>
<div>+ // Skip standard global placement operator
new/new[](std::size_t, void * p);</div>
<div>+ // process all other standard new/new[]
operators including placement</div>
<div>+ // operators new/new[](std::size_t, const
std::nothrow_t&)</div>
<div>+ if
(OperatorNew->isReservedGlobalPlacementOperator())</div>
<div>+ return;</div>
</div>
<div><br>
</div>
<div>Is there a reason why we first process each
operator new in "checkPostStmt(const callExpr" and
finish processing in "checkPostStmt(const
CXXNewExpr" ? I think the code would be simpler if
everything could be done in a single callback. <br>
</div>
</blockquote>
Code added to "checkPostStmt(const callExpr" is for
processing direct calls to operator new/delete
functions, "checkPostStmt(const CXXNewExpr" is for
handling new expressions. Either first or second
callback is called in each particular case but not
both of them. Am I right?<br>
<br>
</div>
</blockquote>
<div><br>
</div>
I see; makes sense. Please, add a comment in
"checkPostStmt(const callExpr*".</div>
<div><br>
<blockquote type="cite">
<div text="#000000" bgcolor="#FFFFFF">
<blockquote
cite="mid:2CAEFC0E-8B7A-4301-9143-CC1EC37A582E@apple.com"
type="cite">
<div><br>
</div>
<div>
<div>+void
MallocChecker::PrintExpectedAllocName(raw_ostream
&os, CheckerContext &C, </div>
<div>+
const Expr *E) const {</div>
<div>+ DeallocatorKind dKind = GetDeallocKind(C,
E);</div>
<div>+</div>
<div>+ switch(dKind) {</div>
<div>+ case D_free: os << "malloc()";
return;</div>
<div>+ case D_delete: os << "operator
new"; return;</div>
<div>+ case D_deleteArray: os <<
"operator new[]"; return;</div>
<div>+ case D_unknown: os << "unknown
means"; return;</div>
</div>
<div><br>
</div>
<div>This function is used to form user visible
warnings. Do we ever expect it to print "unknown
means"? Can this be based on the Kind stored
inside of RefState, where there is no D_unknown?</div>
</blockquote>
Right, changed the case to assert. There is actually
implicit D_unknown in RefState - case when 2nd and 3rd
bits are set to zero.<br>
<br>
<pre class="moz-signature" cols="72">--
Anton</pre>
</div>
<span><MallocChecker_v3.patch></span></blockquote>
</div>
<br>
</div>
_______________________________________________<br>
cfe-commits mailing list<br>
<a moz-do-not-send="true"
href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a class="moz-txt-link-freetext" href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote>
</div>
<br>
</blockquote>
<br>
<br>
<pre class="moz-signature" cols="72">--
Anton</pre>
</body>
</html>