<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body 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>
<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?
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.<br>
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>
Jordan, what do you think about this?<br>
<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>
<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>
</body>
</html>