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

[Anna Zaks]

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.)

Anna.

On Mar 5, 2013, at 10:53 PM, Anna Zaks <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.
> 
> 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?
> 
> Thanks,
> Anna.
> 
> On Mar 5, 2013, at 6:25 PM, Jordan Rose <jordan_rose at apple.com> wrote:
> 
>> 
>> On Mar 5, 2013, at 5:12 , Anton Yartsev <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.)
>> 
>> 
>>> +  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
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130306/93183d3a/attachment.html>