Thanks Anton! The patch looks good overall. Have you evaluated it on some real
C++ codebases?
Below are some comments.
---
+ // The return value from operator new is already bound and we don't want to
+ // break this binding so we call MallocUpdateRefState instead of
MallocMemAux.
+ State = MallocUpdateRefState(C, NE, State, NE->isArray() ? AF_CXXNewArray
+ : AF_CXXNew);
Why is this different from handling malloc? MallocMemAux does break the binding
formed by default handling of malloc and forms a new binding with more semantic
information. (I am fine with addressing this after the initial commit/commits.)
---
def MallocOptimistic : Checker<"MallocWithAnnotations">,
- 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.">,
Shouldn't the MallocWithAnnotations only check the functions which are
explicitly annotated? I think it might be better to change the code rather than
the comment.
---
+ unsigned K : 2; // Kind enum, but stored as a bitfield.
+ unsigned Family : 30; // Rest of 32-bit word, currently just an allocation
+ // family.
We usually add the comment on a line preceding the declaration, like this:
+ // Kind enum, but stored as a bitfield.
+ unsigned K : 2;
+ // Rest of 32-bit word, currently just an allocation family.
+ unsigned Family : 30;
---
+ // Check if an expected deallocation function matches the real one.
+ if (RsBase &&
+ RsBase->getAllocationFamily() != AF_None &&
+ RsBase->getAllocationFamily() != getAllocationFamily(C, ParentExpr) ) {
Is it possible to have AF_None family here? Shouldn't "
RsBase->getAllocationFamily() != AF_None" be inside an assert?
---
+// Used to check correspondence between allocators and deallocators.
+enum AllocationFamily {
The comment should describe what family is. It is a central notion for the
checker and I do not think we explain it anywhere.
---
The patch is very long. Is it possible to split it up into smaller chunks
(http://llvm.org/docs/DeveloperPolicy.html#incremental-development)?
Thanks,
Anna.
On Mar 12, 2013, at 8:56 AM, Anton Yartsev <[email protected]> wrote:
> On 12.03.2013 2:24, Jordan Rose wrote:
>> Looking over this one last time...
>>
>>> - os << "Argument to free() is offset by "
>>> + os << "Argument to ";
>>> + if (!printAllocDeallocName(os, C, DeallocExpr))
>>> + os << "deallocator";
>>> + os << " is offset by "
>>> << offsetBytes
>>> << " "
>>> << ((abs(offsetBytes) > 1) ? "bytes" : "byte")
>>> - << " from the start of memory allocated by malloc()";
>>> + << " from the start of ";
>>> + if (AllocExpr) {
>>> + SmallString<100> TempBuf;
>>> + llvm::raw_svector_ostream TempOs(TempBuf);
>>>
>>> + if (printAllocDeallocName(TempOs, C, AllocExpr))
>>> + os << "memory allocated by " << TempOs.str();
>>> + else
>>> + os << "allocated memory";
>>> + } else
>>> + printExpectedAllocName(os, C, DeallocExpr);
>>> +
>>
>> The way you've set it up, AllocExpr will never be NULL (which is good,
>> because otherwise you'd get "...from the start of malloc()" rather than
>> "from the start of memory allocated by malloc()").
> Strange logic. Fixed.
>
>>
>>
>>> +@interface Wrapper : NSData
>>> +- (id)initWithBytesNoCopy:(void *)bytes length:(NSUInteger)len;
>>> +@end
>>
>> As I discovered with the rest of the ObjC patch, this isn't a great test
>> case because the analyzer doesn't consider it a system method. However, I
>> don't see you use it anywhere in the file anyway, so you can probably just
>> remove it.
>>
>>
>>> +void testNew11(NSUInteger dataLength) {
>>> + int *data = new int;
>>> + NSData *nsdata = [NSData dataWithBytesNoCopy:data length:sizeof(int)
>>> freeWhenDone:1]; // expected-warning{{Memory allocated by 'new' should be
>>> deallocated by 'delete', not +dataWithBytesNoCopy:length:freeWhenDone:}}
>>> +}
>>
>>
>> Hm, that is rather unwieldy, but what bothers me more is that
>> +dataWithBytesNoCopy:length:freeWhenDone: doesn't free the memory; it just
>> takes ownership of it. I guess it's okay to leave that as a FIXME for now,
>> but in the long run we should say something like
>> "+dataWithBytesNoCopy:length:freeWhenDone: cannot take ownership of memory
>> allocated by 'new'." (In the "hold" cases, most likely the user wasn't
>> intending to free
>>
>> But, this doesn't have to block the patch; you/we can fix it post-commit.
>>
>>
>>> + delete x; // FIXME: Shoud detect pointer escape and keep silent.
>>> checkPointerEscape() is not currently invoked for delete.
>>
>>
>> Pedantic note: the real issue here is that we don't model delete at all (see
>> ExprEngine::VisitCXXDeleteExpr). The correct model won't explicitly invoke
>> checkPointerEscape, but it will construct a CallEvent for the deletion
>> operator and then try to evaluate that call—or at least invalidate the
>> arguments like VisitCXXNewExpr does for placement args—which will cause the
>> argument region to get invalidated and checkPointerEscape to be triggered.
>>
>> Jordan
> Updated patch attached.
> --
> Anton
> <MallocChecker_v11.patch>
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
