[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: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.
----------------
NoQ wrote:
> dcoughlin wrote:
> > 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.
> Hmm, that seems to be exactly the thing i'm looking for: heap-based regions that may alias.
> 
> The property of a region's staying on the heap has little to do with the property of being able to alias.
> 
> I've a feeling that we should have avoided using C++ inheritance in the memregion hierarchy, and instead went for a bunch of constraints. Eg., memory space is essentially a constraint (it may be unknown or get known later through exploring aliasing), region's value type is essentially a constraint (as seen during dynamic type propagation, it may be unknown, it may be partially known, we may get to know it better during the analysis by observing successful dynamic casts), extent is essentially a constraint (that we currently impose on SymbolExtent), offset of a symbolic region inside its true parent region is a constraint, and so on.
> 
> But that's too vague. I've no well-defined idea how to make this better at the moment.
If you feel strongly about this, I would suggest putting the FIXME in the core, perhaps in SimpleSValBuilder where the dis-aliasing assumption is introduced or perhaps in the declaration of HeapSpaceRegion. This will make it clear to future maintainers that it is not a defect in the checker.


================
Comment at: lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp:70
 
   // Check if the first argument is stack allocated.  If so, issue a warning
   // because that's likely to be bad news.
----------------
I guess this comment needs to be updated.


================
Comment at: test/Analysis/dispatch-once.m:26
+  // Use regexps to check that we're NOT suggesting to make this static.
+  dispatch_once(once, ^{}); // expected-warning-re{{{{^Call to 'dispatch_once' uses heap-allocated memory for the predicate value.  Using such transient memory for the predicate is potentially dangerous$}}}}
+}
----------------
Clever.


================
Comment at: test/Analysis/dispatch-once.m:62
+- (void)test_ivar_struct_from_inside {
+  dispatch_once(&s.once, ^{}); // expected-warning{{Call to 'dispatch_once' uses the instance variable 's' for the predicate value.}}
+}
----------------
Interesting. Have you seen this pattern in the wild?

I think this diagnostic is a little bit confusing since the ivar itself isn't being used for the predicate value.

Maybe "... uses a subfield of the instance variable 's' for the predicate value"? 






https://reviews.llvm.org/D25909