On 28.03.2013 1:18, Anna Zaks wrote:
On Mar 27, 2013, at 10:13 AM, Anton Yartsev <[email protected]
<mailto:[email protected]>> wrote:
On 27.03.2013 20:58, Anna Zaks wrote:
On Mar 26, 2013, at 6:10 PM, Anna Zaks <[email protected]
<mailto:[email protected]>> wrote:
On Mar 26, 2013, at 5:10 PM, Anton Yartsev <[email protected]
<mailto:[email protected]>> wrote:
On 25.03.2013 21:39, Anna Zaks wrote:
On Mar 25, 2013, at 9:30 AM, Jordan Rose <[email protected]
<mailto:[email protected]>> wrote:
On Mar 25, 2013, at 8:01 , Anton Yartsev
<[email protected] <mailto:[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.
Committed at r178250
--
Anton
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
