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! > 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
