On Mar 21, 2013, at 8:01 PM, Anton Yartsev <[email protected]> wrote:
> On 14.03.2013 21:42, Anna Zaks wrote:
>> One more concern. I do not see any tests with path notes. It's very
>> important to test not only the existence of the warning, but also the notes
>> along the path.
>> - malloc-path.c currently tests the path notes produced by the warnings
>> from the MallocChecker
>> - test/Analysis/diagnostics/deref-track-symbolic-region.c is a good example
>> of how to test for the notes in text and plist formats(used to draw the path
>> in Xcode).
>>
>> I'd add a new test file and test the diagnostics in both text and plist
>> format. I am OK with this test and any related work going in as a separate
>> commit.
> New test added, MallocChecker::MallocBugVisitor::isAllocated() and
> MallocChecker::MallocBugVisitor::isReleased() changed a little.
> Nothing else changed since the last patch.
> OK to commit?
Anton,
Jordan has noticed that you have Windows line endings ('\r') in the new file.
Please, make sure you do not commit these.
Otherwise, we are OK with post commit review.
I am curious if you were able to find any real bugs with the new checker.
Thanks!
Anna.
>
>>
>> Thanks,
>> Anna.
>>
>>
>> On Mar 14, 2013, at 8:38 AM, Anton Yartsev <[email protected]> wrote:
>>
>>> On 12.03.2013 22:06, Anna Zaks wrote:
>>>> Thanks Anton! The patch looks good overall. Have you evaluated it on some
>>>> real C++ codebases?
>>> Not yet. Failed to launch scan-build with my Perl 5.16.2.
>>>
>>
>> Let's do this ASAP. It might point out issues we have not thought of yet.
>>
>>>>
>>>> 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.)
>>> In case of 'new' the memory can be initialized to a non-default value (e.g.
>>> int *p = new int(3); ). The existing logic of MallocMemAux() breaks the
>>> binding and information about the initialization value is lost. This causes
>>> several tests to fail.
>>> Changed the comment to be more informative.
>>
>> I see. I am concerned about the inconsistency of processing malloc and new.
>> On the other hand, it probably does not matter right now.
>>
>>>
>>>>
>>>> ---
>>>> 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.
>>> Currently MallocWithAnnotations is designed as extended unix.Malloc and it
>>> is not a single line change to make it distinct. Can we address this after
>>> primary commits?
>>>
>>
>> Addressing after the initial patch is fine.
>>
>>>>
>>>> ---
>>>> + 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?
>>> It is possible. In the example below
>>> initWithCharactersNoCopy:length:freeWhenDone takes ownership of memory
>>> allocated by unknown means, RefState with AF_None is bound to the call and
>>> after, when free() is processed, we have RsBase->getAllocationFamily() ==
>>> AF_None.
>>
>> My understanding is that this API assumes that the memory belongs to the
>> malloc family. So, for example, we would warn on freeing with a regular
>> "free" after freeing with "initWithCharactersNoCopy.. freeWhenDone".
>>
>> If the family is unknown, we should not be tracking the memory at all.
> Great idea, I'll include corresponding changes in the next patch devoted to
> unix.MismatchedDeallocator
>
>>
>>>
>>> void test(unichar *characters) {
>>> NSString *string = [[NSString alloc] initWithCharactersNoCopy:characters
>>> length:12 freeWhenDone:1];
>>> if (!string) {free(characters);}
>>> }
>>>
>>>>
>>>> ---
>>>> +// 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)?
>>> Committed the initial refactoring as r176949.
>>>
>>> Let's start! Attached is the cplusplus.NewDelete checker separated from the
>>> patch.
>>>
>>>>
>>>> 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>
>>>>
>>> --
>>> Anton
>>> <cplusplus.NewDelete_v1.patch>
>>
>
>
> --
> Anton
> <cplusplus.NewDelete_v2.patch>
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
