[PATCH][Review request] unix.Malloc checker improvement: +handling new/delete, +memory.MismatchedFree, +memory.MismatchedDelete, +improved display names for allocators/deallocators

[Anton Yartsev]

On 06.03.2013 21:46, Anna Zaks wrote:
> CC-ing the patch author!
>
> Also,
>
> Could you split out the ObjC NoCopy + FreeWhenDone change into a 
> separate patch. It does not seem to be directly related to the other 
> changes. Also, I am not 100% sure what changes we are making there. 
> One part is refactoring, however, you've also removed the check for 
> the message calls from the doesNotFreeMemory(). Do we expect any 
> behavior changes from this?
> (Sorry if I've missed something; the patch is getting big.)
Splitted ObjC NoCopy + FreeWhenDone change, kept changes in 
doesNotFreeMemory().
The logic of doesNotFreeMemory() was broken - it treated all 'NoCopy' 
and 'FreeWhenDone==1' methods as freeing memory and unknown to us. This 
lead to removal of RefState from the State and impossibility for further 
alloc/dealloc matching analysis.

Added the test that covers this change among other things:
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:}}
}
>
> Anna.
>
> On Mar 5, 2013, at 10:53 PM, Anna Zaks <ganna at apple.com 
> <mailto:ganna at apple.com>> wrote:
>
>> Looks good. Thanks for working on this!
>>
>> -        case OwnershipAttr::Returns:
>> -          State = MallocMemReturnsAttr(C, CE, *i);
>> -          break;
>> ..
>> +          case OwnershipAttr::Returns:
>> +            State = MallocMemReturnsAttr(C, CE, *i);
>> +            break;
>> ..
>> Is the indentation the only diff here? If yes, please keep the 
>> previous version; most of the case statements are not indented in the 
>> llvm/clang codebases. We usually try to keep the patches focused on 
>> the problem solved. If cleanup of existing code is necessary, it 
>> should go into a separate commit.
>>
>> -  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.">,
>> This is getting too long.
Haven't got anything better then to remove 'Traces memory managed by 
malloc()/free().' sentence.
Do you have an idea how to shorten this?

>>
>> Jordan had previously pointed out that you might need to add 'new' 
>> and 'delete' to doesNotFreeMemOrKnown. I do not see that changed. Has 
>> it been done or is it not necessary?
The change gone to isMemFunction() that is called from 
doesNotFreeMemOrKnown()

>>
>> Thanks,
>> Anna.
>>
>> On Mar 5, 2013, at 6:25 PM, Jordan Rose <jordan_rose at apple.com 
>> <mailto:jordan_rose at apple.com>> wrote:
>>
>>>
>>> On Mar 5, 2013, at 5:12 , Anton Yartsev <anton.yartsev at gmail.com 
>>> <mailto:anton.yartsev at gmail.com>> wrote:
>>>
>>>> Attached is the new patch with unix.MismatchedDeallocator and 
>>>> cplusplus.NewDelete splitted from unix.Malloc plus comments 
>>>> addressed and several other improvements.
>>>
>>> Awesome. At this point I think this is basically ready to go in. Anna?
>>>
>>> (We need to update SATestBuild.py and 
>>> lib/Frontend/CompilerInvocation.cpp to enable the "cplusplus" 
>>> package again.)
Have not found any suitable place in lib/Frontend/CompilerInvocation.cpp
Did you mean to update clang/lib/Driver/Tools.cpp ?
Attached patch contain the supposed change.

>>>
>>>
>>>> +  delete p; // expected-warning{{Memory allocated by operator 
>>>> new[] should be deallocated by operator delete[], not operator delete}}
>>>
>>>
>>> Bikeshedding again on the warning text: do we really need the word 
>>> "operator" in there? Maybe just putting quotes around the operator 
>>> name would be good enough:
>>>
>>
>> +1
>> It's best to keep the diagnostics as concise as possible.
>>
>>> "Memory allocated by 'new[]' should be deallocated by 'delete[]', 
>>> not 'delete'".
>>>
>>> That's this part of printAllocDeallocName (which may have been 
>>> suggested by me):
>>>
>>>> +  if (const CXXNewExpr *NE = dyn_cast<CXXNewExpr>(E)) {
>>>> +    os << *NE->getOperatorNew();
>>>> +    return true;
>>>> +  }
>>>> +
>>>> +  if (const CXXDeleteExpr *DE = dyn_cast<CXXDeleteExpr>(E)) {
>>>> +    os << *DE->getOperatorDelete();
>>>> +    return true;
>>>> +  }
>>>
>>> It looks like there's a function getOperatorSpelling that will get 
>>> the name without the word "operator". (It makes sense to leave in 
>>> the "operator" when it appears in an explicit call expr, just not a 
>>> normal expression or the "expected" case.
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu <mailto:cfe-commits at cs.uiuc.edu>
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>


-- 
Anton

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130307/f7348218/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: MallocChecker_v9.patch
Type: text/x-diff
Size: 56415 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130307/f7348218/attachment.patch>