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

[NoQ]

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.

[dcoughlin]

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.

[NoQ]

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.

[dcoughlin]

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.

cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp

if (TrimmedFName != FName)
       FName = TrimmedFName;
   }
 
   SmallString<256> S;
   llvm::raw_svector_ostream os(S);
   bool SuggestStatic = false;
   os << "Call to '" << FName << "' uses";
   if (const VarRegion *VR = dyn_cast<VarRegion>(RB)) {
+    const VarDecl *VD = VR->getDecl();
+    // FIXME: These should have correct memory space and thus should be filtered
+    // out earlier. This branch only fires when we're looking from a block,
+    // which we analyze as a top-level declaration, onto a static local
+    // in a function that contains the block.
+    if (VD->isStaticLocal())
+      return;
     // We filtered out globals earlier, so it must be a local variable
     // or a block variable which is under UnknownSpaceRegion.
     if (VR != R)
       os << " memory within";
-    if (VR->getDecl()->hasAttr<BlocksAttr>())
+    if (VD->hasAttr<BlocksAttr>())
       os << " the block variable '";
     else
       os << " the local variable '";
     os << VR->getDecl()->getName() << '\'';
     SuggestStatic = true;
   } else if (const ObjCIvarRegion *IVR = getParentIvarRegion(R)) {
     if (IVR != R)
       os << " memory within";

cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp

// Finally handle static locals.
     llvm::PointerUnion<const StackFrameContext *, const VarRegion *> V =
       getStackOrCaptureRegionForDeclContext(LC, DC, D);
 
     if (V.is<const VarRegion*>())
       return V.get<const VarRegion*>();
 
     const StackFrameContext *STC = V.get<const StackFrameContext*>();
 
-    if (!STC)
+    if (!STC) {
+      // FIXME: Assign a more sensible memory space to static locals
+      // we see from within blocks that we analyze as top-level declarations.
       sReg = getUnknownRegion();
-    else {
+    } else {
       if (D->hasLocalStorage()) {
         sReg = isa<ParmVarDecl>(D) || isa<ImplicitParamDecl>(D)
                ? static_cast<const MemRegion*>(getStackArgumentsRegion(STC))
                : static_cast<const MemRegion*>(getStackLocalsRegion(STC));
       }
       else {
         assert(D->isStaticLocal());
         const Decl *STCD = STC->getDecl();

cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp

SVal RegionStoreManager::getBindingForVar(RegionBindingsConstRef B,
   if (isa<UnknownSpaceRegion>(MS))
     return svalBuilder.getRegionValueSymbolVal(R);
 
   if (isa<GlobalsSpaceRegion>(MS)) {
     QualType T = VD->getType();
 
     // Function-scoped static variables are default-initialized to 0; if they
     // have an initializer, it would have been processed by now.
+    // FIXME: This is only true when we're starting analysis from main().
+    // We're losing a lot of coverage here.
     if (isa<StaticGlobalSpaceRegion>(MS))
       return svalBuilder.makeZeroVal(T);
 
     if (Optional<SVal> V = getBindingForDerivedDefaultValue(B, MS, R, T)) {
       assert(!V->getAs<nonloc::LazyCompoundVal>());
       return V.getValue();
     }
 

cfe/trunk/test/Analysis/dispatch-once.m

 
 void test_block_var_from_outside_block() {
   __block dispatch_once_t once;
   ^{
     use_block_var(&once);
   };
   dispatch_once(&once, ^{}); // expected-warning{{Call to 'dispatch_once' uses the block variable 'once' for the predicate value.}}
 }
+
+void test_static_var_from_outside_block() {
+  static dispatch_once_t once;
+  ^{
+    dispatch_once(&once, ^{}); // no-warning
+  };
+}

cfe/trunk/test/Analysis/null-deref-static.m

+// RUN: %clang_cc1 -w -fblocks -analyze -analyzer-checker=core,deadcode,alpha.core,debug.ExprInspection -verify %s
+
+void *malloc(unsigned long);
+void clang_analyzer_warnIfReached();
+
+void test_static_from_block() {
+  static int *x;
+  ^{
+    *x; // no-warning
+  };
+}
+
+void test_static_within_block() {
+  ^{
+    static int *x;
+    *x; // expected-warning{{Dereference of null pointer}}
+  };
+}
+
+void test_static_control_flow(int y) {
+  static int *x;
+  if (x) {
+    // FIXME: Should be reachable.
+    clang_analyzer_warnIfReached(); // no-warning
+  }
+  if (y) {
+    // We are not sure if this branch is possible, because the developer
+    // may argue that function is always called with y == 1 for the first time.
+    // In this case, we can only advise the developer to add assertions
+    // for suppressing such path.
+    *x; // expected-warning{{Dereference of null pointer}}
+  } else {
+    x = malloc(1);
+  }
+}