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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
