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

[Anton Yartsev]

On 30.03.2013 1:01, Jordan Rose wrote:
>
> On Mar 29, 2013, at 13:50 , Anna Zaks <ganna at apple.com 
> <mailto:ganna at apple.com>> wrote:
>
>>
>> On Mar 29, 2013, at 11:57 AM, Anton Yartsev <anton.yartsev at gmail.com 
>> <mailto:anton.yartsev at gmail.com>> wrote:
>>
>>> On 29.03.2013 22:04, Anna Zaks wrote:
>>>>
>>>> On Mar 28, 2013, at 5:42 PM, Anton Yartsev <anton.yartsev at gmail.com 
>>>> <mailto:anton.yartsev at gmail.com>> wrote:
>>>>
>>>>>>> Evolved one more problem to solve: if overloaded standard 
>>>>>>> operator new is defined and is called as a function, then it is 
>>>>>>> not recognized as overloaded operator for some reason. Tests 
>>>>>>> testOpNewArray() and testOpNew() in NewDelete-custom.cpp cover 
>>>>>>> these issue.
>>>>>>
>>>>>> You can check if it has to do with redeclarations of the 
>>>>>> allocator function, but I wouldn't expect that. Definitely 
>>>>>> something we need to fix before putting out another open-source 
>>>>>> checker build.
>>>>> Addressed the issue. Actually the problem is caused by the fact, 
>>>>> that overloaded operator new was inlined and has not been 
>>>>> processed by checkPostStmt(const CallExpr) at all as it skips 
>>>>> inlined calls.
>>>>> What Is the reason for skipping inline calls?
>>>>>
>>>>
>>>> The idea is that if a function has been inlined, it will be modeled 
>>>> through inlining where we will know exactly what it does.
>>>>
>>>> It is unclear if we should enforce the malloc/free, new/delete 
>>>> rules on inlined functions. For example, in this case, if someone 
>>>> overloaded the operator new, do we want to ensure that delete was 
>>>> called on the object regardless of what new's custom implementation is?
>>> It seems to me that if we just simply handle inline functions 
>>> without analyzing the implementation
>>
>> Currently, C.wasInlined() == true means that we did analyze the 
>> implementation.
>>
>>> it'll give less false-positives then false-negatives if we just skip 
>>> them as most malloc/new functions are expected to allocate memory. 
>>> Or I haven't got something?
>>
>> The implementation could be more precise in telling you what happened 
>> (did the function allocate memory or not).
>> You'd get less false negatives if you just do not track regions 
>> allocated by inlined functions - less tracking -> less bug reports -> 
>> less false positives. No? However, you might miss real bugs(have 
>> false negatives).
I just wanted to say, that we likely to miss more real bugs if we stay 
conservative then get false-positives if we handle inline cases. Sorry 
for confusing :)

>>
>> Said that, I do not think that the current decision is supported by 
>> real world examples, as it should be….
>
> I agree with Anna: for now, we should be conservative and not track 
> things that come out of a custom definition of the global operator 
> new. We can revisit this later, since it's /usually/ the case that 
> they should still be deleted, but for now let's keep this behavior. 
> You can change your test cases to just not include the definition of 
> the global operators, which matches both libc++ and libstdc++.
> Sorry for not noticing this sooner, which would have saved you a 
> little runaround.
>
> Jordan
>
Committed as r178388.

-- 
Anton

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130330/4dc0917b/attachment.html>