On Feb 21, 2013, at 6:00 PM, Anton Yartsev <[email protected]> wrote:
>> >> 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> >> > > Hi Anna. Thanks for your comments! Attached is the new patch. > >> 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. > Not sure that I have got an idea. > 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. > Left for now. 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? > >> + const FunctionDecl *getCalleeDecl() const { return CalleeDecl; } >> Do we only store the name of the allocator declaration here? If the Decl is always an allocator Decl, we should change the name of the method to be more descriptive. >> Do we need to store this in the state? (Growing states implies memory >> overhead.) Can't this be easily implied from the kind? > 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. > 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. Ok, I see. > Jordan, what do you think about this? > >> +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. > 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? > I see; makes sense. Please, add a comment in "checkPostStmt(const callExpr*". >> >> +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? > Right, changed the case to assert. There is actually implicit D_unknown in > RefState - case when 2nd and 3rd bits are set to zero. > > -- > Anton > <MallocChecker_v3.patch>
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
