[PATCH] D55804: [analyzer] C++17: Fix leak false positives when an object with destructor is returned from the top frame.
Artem Dergachev via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Dec 17 18:49:20 PST 2018
NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, george.karpenkov, rnkovacs, Szelethus, mikhail.ramalho, baloghadamsoftware.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, a.sidorin, szepet.
The `return_from_top_frame` test demonstrates what happens. The object bound to the sub-expression of the return statement is a lazy compound value in this case. Liveness of that expression ends slightly before the end of the analysis, leaving time for `MallocChecker` to diagnose a leak.
This is fixed by admitting that we do not know how to properly model the memory region in which the return value is constructed. Neither `CXXTempObjectRegion` nor `SymbolicRegion` seems suitable, because the region needs to be in an unknown memory space and must also be live after the last reference disappears. The fix is only applied to C++17 when the object has a destructor (which produces a `CXX17ElidedCopyReturnedValueConstructionContext`) because other cases seem to work due to liveness analysis behaving correctly.
We could (and probably should) teach liveness analysis that with C++17 AST with `CXXBindTemporaryExpr` it is still a good idea to keep the top-frame returned expression alive forever. But we still don't have the correct region to represent construction target. Either we should modify liveness analysis and use `SymbolicRegion`, or we need to come up with a new kind of region (either for the return-to object itself, or for an implicit parameter through which its address is passed into the callee, so that the return-to object would be a `SymbolicRegion` of its `SymbolRegionValue`, similarly to how `CXXThisRegion` works).
Before i forget: re-enable tests for temporaries on C++17. Notice that some still fail. The current patch doesn't regress any of the old tests in this file that are now FIXME'd out.
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 6660 bytes
Desc: not available
More information about the cfe-commits