One more concern. I do not see any tests with path notes. It's very important to test not only the existence of the warning, but also the notes along the path. - malloc-path.c currently tests the path notes produced by the warnings from the MallocChecker - test/Analysis/diagnostics/deref-track-symbolic-region.c is a good example of how to test for the notes in text and plist formats(used to draw the path in Xcode).
I'd add a new test file and test the diagnostics in both text and plist format. I am OK with this test and any related work going in as a separate commit. Thanks, Anna. On Mar 14, 2013, at 8:38 AM, Anton Yartsev <[email protected]> wrote: > On 12.03.2013 22:06, Anna Zaks wrote: >> Thanks Anton! The patch looks good overall. Have you evaluated it on some >> real C++ codebases? > Not yet. Failed to launch scan-build with my Perl 5.16.2. > Let's do this ASAP. It might point out issues we have not thought of yet. >> >> Below are some comments. >> >> --- >> + // The return value from operator new is already bound and we don't want >> to >> + // break this binding so we call MallocUpdateRefState instead of >> MallocMemAux. >> + State = MallocUpdateRefState(C, NE, State, NE->isArray() ? AF_CXXNewArray >> + : AF_CXXNew); >> Why is this different from handling malloc? MallocMemAux does break the >> binding formed by default handling of malloc and forms a new binding with >> more semantic information. (I am fine with addressing this after the initial >> commit/commits.) > In case of 'new' the memory can be initialized to a non-default value (e.g. > int *p = new int(3); ). The existing logic of MallocMemAux() breaks the > binding and information about the initialization value is lost. This causes > several tests to fail. > Changed the comment to be more informative. I see. I am concerned about the inconsistency of processing malloc and new. On the other hand, it probably does not matter right now. > >> >> --- >> def MallocOptimistic : Checker<"MallocWithAnnotations">, >> - HelpText<"Check for memory leaks, double free, and use-after-free >> problems. Assumes that all user-defined functions which might free a pointer >> are annotated.">, >> + HelpText<"Check for memory leaks, double free, and use-after-free >> problems. Traces memory managed by malloc()/free(). Assumes that all >> user-defined functions which might free a pointer are annotated.">, >> >> Shouldn't the MallocWithAnnotations only check the functions which are >> explicitly annotated? I think it might be better to change the code rather >> than the comment. > Currently MallocWithAnnotations is designed as extended unix.Malloc and it is > not a single line change to make it distinct. Can we address this after > primary commits? > Addressing after the initial patch is fine. >> >> --- >> + unsigned K : 2; // Kind enum, but stored as a bitfield. >> + unsigned Family : 30; // Rest of 32-bit word, currently just an >> allocation >> + // family. >> >> We usually add the comment on a line preceding the declaration, like this: >> + // Kind enum, but stored as a bitfield. >> + unsigned K : 2; >> + // Rest of 32-bit word, currently just an allocation family. >> + unsigned Family : 30; >> >> --- >> + // Check if an expected deallocation function matches the real one. >> + if (RsBase && >> + RsBase->getAllocationFamily() != AF_None && >> + RsBase->getAllocationFamily() != getAllocationFamily(C, ParentExpr) ) >> { >> Is it possible to have AF_None family here? Shouldn't " >> RsBase->getAllocationFamily() != AF_None" be inside an assert? > It is possible. In the example below > initWithCharactersNoCopy:length:freeWhenDone takes ownership of memory > allocated by unknown means, RefState with AF_None is bound to the call and > after, when free() is processed, we have RsBase->getAllocationFamily() == > AF_None. My understanding is that this API assumes that the memory belongs to the malloc family. So, for example, we would warn on freeing with a regular "free" after freeing with "initWithCharactersNoCopy.. freeWhenDone". If the family is unknown, we should not be tracking the memory at all. > > void test(unichar *characters) { > NSString *string = [[NSString alloc] initWithCharactersNoCopy:characters > length:12 freeWhenDone:1]; > if (!string) {free(characters);} > } > >> >> --- >> +// Used to check correspondence between allocators and deallocators. >> +enum AllocationFamily { >> The comment should describe what family is. It is a central notion for the >> checker and I do not think we explain it anywhere. >> >> --- >> The patch is very long. Is it possible to split it up into smaller chunks >> (http://llvm.org/docs/DeveloperPolicy.html#incremental-development)? > Committed the initial refactoring as r176949. > > Let's start! Attached is the cplusplus.NewDelete checker separated from the > patch. > >> >> Thanks, >> Anna. >> On Mar 12, 2013, at 8:56 AM, Anton Yartsev <[email protected]> wrote: >> >>> On 12.03.2013 2:24, Jordan Rose wrote: >>>> Looking over this one last time... >>>> >>>>> - os << "Argument to free() is offset by " >>>>> + os << "Argument to "; >>>>> + if (!printAllocDeallocName(os, C, DeallocExpr)) >>>>> + os << "deallocator"; >>>>> + os << " is offset by " >>>>> << offsetBytes >>>>> << " " >>>>> << ((abs(offsetBytes) > 1) ? "bytes" : "byte") >>>>> - << " from the start of memory allocated by malloc()"; >>>>> + << " from the start of "; >>>>> + if (AllocExpr) { >>>>> + SmallString<100> TempBuf; >>>>> + llvm::raw_svector_ostream TempOs(TempBuf); >>>>> >>>>> + if (printAllocDeallocName(TempOs, C, AllocExpr)) >>>>> + os << "memory allocated by " << TempOs.str(); >>>>> + else >>>>> + os << "allocated memory"; >>>>> + } else >>>>> + printExpectedAllocName(os, C, DeallocExpr); >>>>> + >>>> >>>> The way you've set it up, AllocExpr will never be NULL (which is good, >>>> because otherwise you'd get "...from the start of malloc()" rather than >>>> "from the start of memory allocated by malloc()"). >>> Strange logic. Fixed. >>> >>>> >>>> >>>>> +@interface Wrapper : NSData >>>>> +- (id)initWithBytesNoCopy:(void *)bytes length:(NSUInteger)len; >>>>> +@end >>>> >>>> As I discovered with the rest of the ObjC patch, this isn't a great test >>>> case because the analyzer doesn't consider it a system method. However, I >>>> don't see you use it anywhere in the file anyway, so you can probably just >>>> remove it. >>>> >>>> >>>>> +void testNew11(NSUInteger dataLength) { >>>>> + int *data = new int; >>>>> + NSData *nsdata = [NSData dataWithBytesNoCopy:data length:sizeof(int) >>>>> freeWhenDone:1]; // expected-warning{{Memory allocated by 'new' should be >>>>> deallocated by 'delete', not +dataWithBytesNoCopy:length:freeWhenDone:}} >>>>> +} >>>> >>>> >>>> Hm, that is rather unwieldy, but what bothers me more is that >>>> +dataWithBytesNoCopy:length:freeWhenDone: doesn't free the memory; it just >>>> takes ownership of it. I guess it's okay to leave that as a FIXME for now, >>>> but in the long run we should say something like >>>> "+dataWithBytesNoCopy:length:freeWhenDone: cannot take ownership of memory >>>> allocated by 'new'." (In the "hold" cases, most likely the user wasn't >>>> intending to free >>>> >>>> But, this doesn't have to block the patch; you/we can fix it post-commit. >>>> >>>> >>>>> + delete x; // FIXME: Shoud detect pointer escape and keep silent. >>>>> checkPointerEscape() is not currently invoked for delete. >>>> >>>> >>>> Pedantic note: the real issue here is that we don't model delete at all >>>> (see ExprEngine::VisitCXXDeleteExpr). The correct model won't explicitly >>>> invoke checkPointerEscape, but it will construct a CallEvent for the >>>> deletion operator and then try to evaluate that call—or at least >>>> invalidate the arguments like VisitCXXNewExpr does for placement >>>> args—which will cause the argument region to get invalidated and >>>> checkPointerEscape to be triggered. >>>> >>>> Jordan >>> Updated patch attached. >>> -- >>> Anton >>> <MallocChecker_v11.patch> >> > -- > Anton > <cplusplus.NewDelete_v1.patch>
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
