[PATCH] D25909: [analyzer] MacOSXApiChecker: Disallow dispatch_once predicates on heap and in ivars.

[Devin Coughlin via cfe-commits]

dcoughlin added inline comments.


================
Comment at: lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp:92
+  else if (isa<HeapSpaceRegion>(RS))
+    os << " heap allocated memory";
+  else if (isa<UnknownSpaceRegion>(RS)) {
----------------
"heap allocated" --> "heap-allocated"


================
Comment at: lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp:94
+  else if (isa<UnknownSpaceRegion>(RS)) {
+    // FIXME: Presence of an IVar region has priority over this branch, because
+    // ObjC objects are on the heap even if the core doesn't realize this.
----------------
It is not clear to me that this FIXME is a good idea. I would remove it so someone doesn't spend a lot of time trying to address it.

Objective-C objects don't have the strong dis-aliasing guarantee that the analyzer assumes for heap base regions. In other words, two calls to [[Foo alloc] init] may yield exactly the same instance.  This is because, unlike malloc() and C++ global new, ObjC initializers can (and frequently do) return instances other than the passed-in, freshly-allocated self.


================
Comment at: test/Analysis/dispatch-once.m:13
+
+void test_stack() {
+  dispatch_once_t once;
----------------
Should the tests for dispatch_once in unix-fns.c be moved here?


================
Comment at: test/Analysis/dispatch-once.m:15
+  dispatch_once_t once;
+  dispatch_once(&once, ^{}); // expected-warning{{Call to 'dispatch_once' uses the local variable 'once' for the predicate value.}}
+}
----------------
I think it would be good to include the full diagnostic text for the stack local variable case because there is control flow in its construction.


https://reviews.llvm.org/D25909