On Feb 19, 2013, at 10:18 PM, Anton Yartsev <[email protected]> wrote:
> Hi, Jordan. Thanx for the review!
>
> 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()
>
> There is currently one problem with handling of operator delete(). The
> following test
>
> void testDeleteOp1() {
> int *p = (int *)malloc(sizeof(int));
> operator delete(p); // FIXME: should complain "Argument to operator delete()
> was allocated by malloc(), not operator new"
> }
>
> 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).
> I haven't investigated the problem deeply yet, intend to address it parallel
> with the review.
>
>> + if (NE->getNumPlacementArgs())
>> + return;
>> + // skip placement new operators as they may not allocate memory
>>
>> Two comments here:
>> - Please make sure all comments are complete, capitalized, and punctuated
>> sentences. (This has the important one, "complete"....just missing
>> capitalization and punctuation.)
> I'll try. Unfortunately I am not as good in English as I want to be, so sorry
> for my grammar, syntax, and punctuation.
>
> --
> Anton
>
> <MallocChecker_v2.patch>
Anton, it's great that you are working on this! See the comments/feedback below.
+// Numeration is added for convenient mapping to RefState::Kind
Should be "Enumeration"
enum Kind { // Reference to allocated memory.
Allocated,
....
+
+ // Mapped DeallocatorKind.
+ // Expected kind of a deallocator; used to check if a real
+ // kind of a deallocator matches expected one
+ Free = 0x4,
+ Delete = 0x8,
+ DeleteArray = 0xC
Wouldn't it be cleaner and simpler if the Kind enumeration just had something
like this:
AllocatedWithMalloc
AllocatedWithNew
AllocatedWithNewArray
Or, if we want the name to be based on the deallocator function, we could use
something like "ToBeReleasedWithFree, ToBeReleasedWithNew, ..".
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 (MacOSKeychainAPIChecker
uses this approach). This is probably an overkill for now, but is another
option.
+ const FunctionDecl *getCalleeDecl() const { return CalleeDecl; }
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?
One thing to keep in mind here, is that it would be beneficial to make this all
work well with ownership annotations (even though they are not supported now,
they are something we might want to have in the future). So feel free to push
back on simplification suggestions if there is a clear benefit with respect to
the ownership annotations support. On the other hand, we don't need to design a
solution that would fit the annotations now either.
+ bool isDefaultNonptrplacementNewDelete(const FunctionDecl *FD,
Camel case should be used
+ } else if (isDefaultNonptrplacementNewDelete(FD, C)) {
+ OverloadedOperatorKind K = FD->getDeclName().getCXXOverloadedOperator();
+ if (K == OO_New)
+ State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State,
+ D_delete);
+ else if (K == OO_Array_New)
+ State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State,
+ D_deleteArray);
+ else
+ State = FreeMemAux(C, CE, State, 0, false, ReleasedAllocatedMemory);
In the last else, we assume that delete/delete[] was called based on the logic
in isDefaultNonptrplacementNewDelete. Please either check the kind again or
assert this. We want to make sure they don't go out of sync.
+void MallocChecker::checkPostStmt(const CXXNewExpr *NE,
+ CheckerContext &C) const {NE
+
+ FunctionDecl *OperatorNew = NE->getOperatorNew();
+ if (!OperatorNew)
+ return;
+
+ // Skip custom new operators
+ if (!OperatorNew->isImplicit() &&
+ !C.getSourceManager().isInSystemHeader(OperatorNew->getLocStart()) &&
+ !NE->isGlobalNew())
+ return;
+
+ // Skip standard global placement operator new/new[](std::size_t, void * p);
+ // process all other standard new/new[] operators including placement
+ // operators new/new[](std::size_t, const std::nothrow_t&)
+ if (OperatorNew->isReservedGlobalPlacementOperator())
+ return;
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.
Also, the tests above are similar to what is done in
isDefaultNonptrplacementNewDelete. These should be factored out into a
subroutine to avoid inconsistency. (Ex: Shouldn't "NE->isGlobalNew()" check be
done in both?)
Same remarks go for "checkPreStmt(const CXXDeleteExpr".
+void MallocChecker::PrintExpectedAllocName(raw_ostream &os, CheckerContext &C,
+ const Expr *E) const {
+ DeallocatorKind dKind = GetDeallocKind(C, E);
+
+ switch(dKind) {
+ case D_free: os << "malloc()"; return;
+ case D_delete: os << "operator new"; return;
+ case D_deleteArray: os << "operator new[]"; return;
+ case D_unknown: os << "unknown means"; return;
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?
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
