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

[Anton Yartsev]

On 01.03.2013 5:55, Jordan Rose wrote:
>>>> 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.
Have not got the idea yet. Please clarify what should 
cplusplus.NewDelete and unix.MismatchedFree check for individually, and 
what about other allocation families.
Can we just separate out the whole new check to something like 
.MismatchedDeallocator?

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


-- 
Anton