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

[Anna Zaks]

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.