On Feb 28, 2013, at 6:48 PM, Anton Yartsev <[email protected]> wrote:
> 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?
NewDelete checker would check for use-after-free and leaks involving the new
and delete "family".
MismatchedDeallocator would check for mismatched deallocators (better name than
MismatchedFree). This checker could consult with the state that the other
checkers populate, for example, to check which function was used to allocate a
symbol.
>
>>
>>
>> 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
>
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
