Page MenuHomePhabricator

[analyzer] Enable c++-allocator-inlining by default?
ClosedPublic

Authored by NoQ on Jan 17 2018, 5:15 PM.

Details

Summary

Once D42192 lands, i believe it should be safe to flip the default value of -analyzer-config c++-allocator-inlining to true by default.

My evaluation (on a large mostly internal codebase, including WebKit, more than a day of real-world time of continuous analysis) shows that around twice as much false positives are getting fixed (~35) as new false positives added (~20). Additionally, the reason for newly added false positives is our increased precision and coverage due to more aggressive inlining, which exposes other, unrelated problems in the analyzer, which is an expected outcome of enabling more coverage on codebases that already have high false positive rates (such as many C++ codebases). Gradually improving C++ support should be mitigating those. One true positive was lost due to analyzer reaching the max-nodes limit due to more aggressive inlining. I didn't immediately find any new positives that were definitely true, but some looked fairly suspicious.

The old mode would be available for a while, so it's possible, if necessary, to turn the new mode off with -analyzer-config c++-allocator-inlining=false.

All changes in tests in the patch are FIXMEs that were resolved by enabling the new mode.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.Jan 17 2018, 5:15 PM
This revision was not accepted when it landed; it landed in state Needs Review.Jan 24 2018, 1:01 PM
This revision was automatically updated to reflect the committed changes.
NoQ added a comment.Jan 24 2018, 6:30 PM

I committed this for now, but i'm totally open for post-commit review and reacting on any "unknown unknowns" that may be found.