On 27.03.2013 20:45, Jordan Rose wrote:
On Mar 27, 2013, at 5:32 , Anton Yartsev <[email protected]
<mailto:[email protected]>> wrote:
On 27.03.2013 5:23, Jordan Rose wrote:
On Mar 26, 2013, at 17:10 , 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.
I don't think this is safe -- what if the user has custom libraries
in their system include path? We can really only assume heap
allocation for the implicit case and for ::operator new(std::size_t)
and ::operator new(std::size_t, std::nothrow_t).
However, I think just checking "global namespace" might be a good
enough approximation. If the user overrides one of the default
allocators, it ought to behave like the real one, so we'd only be
messing up if they had a new global "allocate from pool" or something.
Also, in theory we'd have to check placement new, but that's handled
specially below anyway.
Updated the logic, improved test coverage.
+ // Skip all operator new/delete methods (including static ones).
All operator new/delete methods are implicitly static, so you can just
leave out the parenthetical. On the other hand, it might be worth
noting that we realize this is an approximation that might not
correctly model a custom global allocator.
Fixed at r178244. The note gone to ExprEngine::VisitCXXNewExpr().
--
Anton
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
