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

[Jordan Rose]

>>> Memory allocated by malloc() should be deallocated by free(), not operator delete[].
>> I'm really not sure whether it's better to mention the deallocator's expected allocator, or the allocated region's expected deallocator. Which mistake do you think people are more likely to make?
>> 
>> (Also, I came up with that phrasing in ten seconds; it could totally change.)
> The second one of course. I definitely prefer to print the allocated region's expected deallocator.
> The single downside I see is that this breaks the uniformity of reports and complicate ReportBadFree(). (other reports about, for example, freeing addresses of labels, could not be printed in the new fashion)

Hm. I think we should still go with the second one.

One last major comment: in talking offline to Ted (Kremenek) about this, he pointed out that it has the potential to be a very noisy new warning. In the same way that we have unix.Malloc and alpha.unix.MallocWithAnnotations, it might be good to separate out cplusplus.NewDelete and...we don't have a good category for MismatchedFree, but maybe unix.MismatchedFree. That way we can control these checks individually; look for "MallocOptimistic" to see how it's currently being used.


Comments comments comments:

+// Used to check correspondence between allocators and deallocators.
+enum AllocationFamilies {
+  AF_None,
+  AF_Malloc,
+  AF_CXXNew,
+  AF_CXXNewArray
+};

Not sure if I introduced this or you, but we generally name enums in the singular (AllocationFamily rather than AllocationFamilies).


-  static RefState getReleased(const Stmt *s) { return RefState(Released, s); }
+  static RefState getReleased(const Stmt *s) { 
+    return RefState(Released, s, AF_None);
+  }
   static RefState getRelinquished(const Stmt *s) {
-    return RefState(Relinquished, s);
+    return RefState(Relinquished, s, AF_None);
   }

I'm wondering if you might as well preserve the families here. We're not using them now, but at the very least it could be nice for debugging.


+  if (FD->getDeclName().getNameKind() != DeclarationName::CXXOperatorName)
+    return false;
+
+  OverloadedOperatorKind Kind = FD->getOverloadedOperator();
+  if (Kind != OO_New && Kind != OO_Array_New && 
+      Kind != OO_Delete && Kind != OO_Array_Delete)
+    return false;

The second check takes care of the first here.


+      OverloadedOperatorKind K = FD->getDeclName().getCXXOverloadedOperator();

Can be shortened to FD->getOverloadedOperator() again.


+      SmallString<100> TempBuf;
+      llvm::raw_svector_ostream TempOs(TempBuf);
+
+      if (printAllocDeallocName(TempOs, C, AllocExpr))
+        os << " was allocated by " << TempOs.str() << ", not ";
+      else
+        os << " was not allocated by ";

Ah, I see why you were avoiding this. Nevertheless, I think this is a better experience for the user, and if we go with the new output this will get restructured a bit anyway. (As a tiny comment, you can probably shrink the SmallString to use less stack space, since most functions have much shorter names.)

Jordan