On Mar 27, 2013, at 2:18 PM, Anna Zaks <[email protected]> wrote:
>
> On Mar 27, 2013, at 10:13 AM, Anton Yartsev <[email protected]> wrote:
>
>> On 27.03.2013 20:58, Anna Zaks wrote:
>>>
>>> On Mar 26, 2013, at 6:10 PM, Anna Zaks <[email protected]> wrote:
>>>
>>>>
>>>> On Mar 26, 2013, at 5:10 PM, Anton Yartsev <[email protected]> wrote:
>>>>
>>>>> On 25.03.2013 21:39, Anna Zaks wrote:
>>>>>> On Mar 25, 2013, at 9:30 AM, Jordan Rose <[email protected]> wrote:
>>>>>>
>>>>>>>
>>>>>>> On Mar 25, 2013, at 8:01 , Anton Yartsev <[email protected]>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> Committed as r177849
>>>>>>>>
>>>>>>>> Manage to find several random real bugs (report-843813.html,
>>>>>>>> report-230257.html, recursive case in report-727931.html), but for now
>>>>>>>> it is hard to detect real bugs among tons of false-positives.
>>>>>>>>
>>>>>>>> There are two types of false-positives that form the majority of
>>>>>>>> reports:
>>>>>>>> 1) Illustrated by the following test (added similar test to
>>>>>>>> NewDelete-checker-test.mm):
>>>>>>>> int *global;
>>>>>>>> void testMemIsOnHeap() {
>>>>>>>> int *p = new int; // FIXME: currently not heap allocated!
>>>>>>>> if (global != p) // evaluates to UnknownVal() rather then 'true'
>>>>>>>> global = p;
>>>>>>>> } // report false-positive leak
>>>>>>>>
>>>>>>>> As I understand the problem is that currently a memory region for
>>>>>>>> 'new' is not a heap region and this lead to false-positives like
>>>>>>>> report-863595.html and others. (e.g. that causes 'global != p'
>>>>>>>> evaluate to UnknownVal() rather then 'true' (logic of
>>>>>>>> SimpleSValBuilder::evalBinOpLL))
>>>>>>>>
>>>>>>>> Attached is the proposed patch that fixes these issues.
>>>>>>>
>>>>>>> There are two reasons I didn't use getConjuredHeapSymbol when I
>>>>>>> originally put in this code:
>>>>>>> (1) It handles all CXXNewExprs, even if the allocator is not one of the
>>>>>>> global ones.
>>>>>>> (2) Heap symbols weren't used yet (Anna added them later for
>>>>>>> MallocChecker).
>>>>>>>
>>>>>>> Obviously #2 is bogus now. #1 worries me a bit because it requires
>>>>>>> duplicating some of the checks you just added to MallocChecker.
>>>>>>>
>>>>>>> In the long run, the design would be to get the appropriate memory from
>>>>>>> the allocator call, and we have PR12014's restructuring of the CFG
>>>>>>> blocking that. I'm not sure if we'd then move the heap symbol logic
>>>>>>> into a checker, or if it would still stay in Core.
>>>>>>>
>>>>>>> In the short term, I guess the best idea is to duplicate some of the
>>>>>>> checks (or refactor them to a helper function somewhere...though
>>>>>>> probably not into AST) and then conjure a heap symbol if we know we can.
>>>>> Failed to find any suitable place other then AST :) Eventually noticed,
>>>>> that actually only a single check should be duplicated. Decided to leave
>>>>> the wide comment added when I tried to find the proper place for
>>>>> isStandardNewDelete().
>>>>> New fix attached.
>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> 2) The second type of false-positives is illustrated by the following
>>>>>>>> minimal test:
>>>>>>>> void f(const int & x);
>>>>>>>>
>>>>>>>> void test() {
>>>>>>>> int *p = (int *)malloc(sizeof(int));
>>>>>>>> f(*p);
>>>>>>>> } // report false-positive leak
>>>>>>>>
>>>>>>>> report-218274.html shows how it looks like in reality.
>>>>>>>> Haven't addressed this yet. Removing 'const' from the declaration
>>>>>>>> eliminates the leak report.
>>>>>>>
>>>>>>> Interesting. You can't change a const region (and pedantically you
>>>>>>> can't free() it either), but you certainly can 'delete' it. (C++11
>>>>>>> [expr.delete]p2)
>>>>>>>
>>>>>>> Anna, any thoughts on this? Should these also count as "pointer
>>>>>>> escaped" even though their contents have not been invalidated?
>>>>>>>
>>>>>>
>>>>>> I think handling this similarly to pointer escape is the best. However,
>>>>>> I am concerned that if we simply extend pointer escape to trigger on
>>>>>> another "Kind" of escape, all the other users of pointerEscape will need
>>>>>> to know about it (and do nothing for this kind of escape). How about a
>>>>>> new checker callback, which will rely on the same core code as
>>>>>> _checkPointerEscape? This way the checker developers would not need to
>>>>>> know about this special case, and we can reuse all the code that
>>>>>> determines when the escape should be triggered.
>>>>> As for me it would be better to leave the single callback as this is just
>>>>> another type of pointer escape, if I understand correctly. Have no other
>>>>> arguments for this :)
>>>>> In addition, the "Kind" approach is relatively new, so hopefully a few
>>>>> users of pointerEscape be affected.
>>>> The main concern is that every user of the callback will now have to check
>>>> if the kind is ConstPointer (or whatever we call it, maybe multiple
>>>> kinds..) and do nothing in that case. So every user will need to know
>>>> about this special case and handle it specially.
>>> Anton,
>>>
>>> If you don't mind, I am going to work on this one. I have some spare time
>>> and we'd like to get the new/delete checker out in the next open source
>>> build.
>> I am OK with this, thanx!
>>
I've committed the new callback. Looks like no email was sent out.. So just
look at r178310.
> Anton,
>
> I've realized that I need the part of your new/delete work that tracks
> families for fixing this. Specifically, I want to assume that a const pointer
> cannot be freed but could be deleted. Can you commit the remaining patches?
> Specifically, I need the part that performs family tracking, but you can
> commit the mismatched deallocators work as well.
>
> Thanks!
> Anna.
>>>
>>> Cheers,
>>> Anna.
>>>
>>>>>
>>>>>
>>>>> Evolved another issue, that I first thought to be related to case 1),
>>>>> here is the minimal test:
>>>>> struct S {
>>>>> int **p;
>>>>> };
>>>>> void testOk(S &s) {
>>>>> new(s.p) (int*)(new int); // false-positive leak
>>>>> }
>>>>> void testLeak() {
>>>>> S s;
>>>>> new(s.p) (int*)(new int); // real leak
>>>>> }
>>>>>
>>>>> Haven't addressed these yet. The leak is reported for cases of the form
>>>>> 'small_vector.push_back(new Something)', where push_back() uses placement
>>>>> new to store data.
>>>>>
>>>>> --
>>>>> Anton
>>>>> <FixForCase1.patch>
>>>>
>>>> _______________________________________________
>>>> cfe-commits mailing list
>>>> [email protected]
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>
>> --
>> Anton
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
