r178890 - [analyzer] Split new/delete checker into use-after-free and leaks parts.

[Anna Zaks]

On Apr 5, 2013, at 2:05 PM, Anton Yartsev <anton.yartsev at gmail.com> wrote:

> On 06.04.2013 0:30, Jordan Rose wrote:
>> On Apr 5, 2013, at 13:22 , Anton Yartsev <anton.yartsev at gmail.com> wrote:
>> 
>>> On 05.04.2013 21:55, Jordan Rose wrote:
>>>>    if (!Filter.CMallocOptimistic && !Filter.CMallocPessimistic &&
>>>> -      !Filter.CNewDeleteChecker)
>>>> +      !Filter.CNewDeleteLeaksChecker)
>>>>      return;
>>>>  -  if (!isTrackedFamily(C, Sym))
>>>> +  const RefState *RS = C.getState()->get<RegionState>(Sym);
>>>> +  assert(RS && "cannot leak an untracked symbol");
>>>> +  AllocationFamily Family = RS->getAllocationFamily();
>>>> +  if (!isTrackedFamily(Family))
>>>>      return;
>>>>  +  // Special case for new and new[]; these are controlled by a separate checker
>>>> +  // flag so that they can be selectively disabled.
>>>> +  if (Family == AF_CXXNew || Family == AF_CXXNewArray)
>>>> +    if (!Filter.CNewDeleteLeaksChecker)
>>>> +      return;
>>> We already checked for Filter.CNewDeleteLeaksChecker at the beginning.
>>> 
>>> Now this works properly just because isTrackedFamily() does not know about CNewDeleteLeaksChecker and returns false.
>>> What about adding bug types at this point?
>> The first check says "any of {MallocOptimistic,MallocPessimistic,NewDeleteLeaks}". This one checks exclusively for NewDeleteLeaks and AF_CXXNew[Array].
> Oops, sorry.
> 
>> This is basically a quick-and-dirty "bug types" implementation that's used because today we only have one bug type that's different. I'm not sure the added generalization will actually make things clearer—here we implicitly know the "type" is a leak because we're in reportLeak. None of the rest of the code has to know that leaks are any different from anything else. But maybe I'm missing some benefit of the general bug types.
> No benefits other then just looks clearer to me :) Passing a bug kind shows, on the one hand, that sometimes we depend on the bug type, and allows to freely add other bug-dependent code on the other.
> I quickly returned the bug types while waited for your replay, just to see, if it simplify the appearance, attached the patch so you could also see how it looks like.
> I am under no circumstances insist on adding bug types, sending just for information, perhaps you like it. :)
> 

Pulling all the decisions about when to warn based on the family into a single function seems to be an improvement. However, introducing an enum of 6 bug types, while only one of the values is used is odd and probably redundant.

> In any case, these may be slightly simplified as isTrackedFamily() now treats AF_None as unacceptable (after r178899 )
>> I'll admit it's a bit kludgy that the family is effectively checked twice, but I actually like the implication that alpha.cplusplus.NewDeleteLeaks is a "sub-checker" of cplusplus.NewDelete.
>> 
>> Jordan
> 
> -- 
> Anton
> 
> <MallocChecker.cpp.BugTypes.patch>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130405/2943e4f1/attachment.html>