This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Fix memory space for block-captured static locals.
ClosedPublic

Authored by NoQ on Jan 20 2017, 5:22 AM.

Details

Summary

When a block that is being analyzed as top-level captures static local variables declared in a function (which itself is not being analyzed) surrounding the block, it puts such variables into the unknown memory space, similarly to the stack locals of those functions (that may have been moved to the heap by the time the block starts executing).

However, because blocks don't move such variables to the heap, but capture them by reference instead, we are quite sure that we should put those into the static memspace.

This patch fixes a false positive in MacOSXAPIChecker.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.Jan 20 2017, 5:22 AM
dcoughlin accepted this revision.Jan 20 2017, 10:43 AM

LGTM!

It would be good to get this into the clang 4.0 svn branch, too. I think Hans Wennborg is managing this. See http://clang-developers.42468.n3.nabble.com/4-0-0-Release-The-branch-is-here-the-release-process-starts-td4054864.html.

This revision is now accepted and ready to land.Jan 20 2017, 10:43 AM
This revision was automatically updated to reflect the committed changes.
NoQ updated this revision to Diff 85588.Jan 24 2017, 7:53 AM

Reopening due to a revert.

This time i reduce the scope of the fix to the checker, and add FIXMEs to the problems that showed up while testing it.

NoQ reopened this revision.Jan 24 2017, 7:54 AM
This revision is now accepted and ready to land.Jan 24 2017, 7:54 AM
NoQ requested review of this revision.Jan 24 2017, 7:54 AM
NoQ edited edge metadata.
dcoughlin accepted this revision.Jan 24 2017, 1:43 PM

LGTM with some minor comments.

lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
98 ↗(On Diff #85588)

"namespace" --> "memory space"?

lib/StaticAnalyzer/Core/RegionStore.cpp
1853 ↗(On Diff #85588)

If I understand correctly, I think 'losing' is a better word here instead of 'wasting', since we're not spending a finite resource unwisely.

test/Analysis/dispatch-once.m
116 ↗(On Diff #85588)

It would be good to add a test that trips up the reverted fix from r292800 with a null pointer false positive. This will make sure that someone who tries to address the FIXME in the same way in the future will detect it.

This revision is now accepted and ready to land.Jan 24 2017, 1:43 PM
This revision was automatically updated to reflect the committed changes.