[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

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.

lib/StaticAnalyzer/Core/MemRegion.cpp

if (const BlockInvocationContext *BC =
       }
     }
 
     LC = LC->getParent();
   }
   return (const StackFrameContext *)nullptr;
 }
 
+static CanQualType getBlockPointerType(const BlockDecl *BD, ASTContext &C) {
+  // FIXME: The fallback type here is totally bogus -- though it should
+  // never be queried, it will prevent uniquing with the real
+  // BlockCodeRegion. Ideally we'd fix the AST so that we always had a
+  // signature.
+  QualType T;
+  if (const TypeSourceInfo *TSI = BD->getSignatureAsWritten())
+    T = TSI->getType();
+  if (T.isNull())
+    T = C.VoidTy;
+  if (!T->getAs<FunctionType>())
+    T = C.getFunctionNoProtoType(T);
+  T = C.getBlockPointerType(T);
+  return C.getCanonicalType(T);
+}
+
 const VarRegion* MemRegionManager::getVarRegion(const VarDecl *D,
                                                 const LocationContext *LC) {
   const MemRegion *sReg = nullptr;
 
   if (D->hasGlobalStorage() && !D->isStaticLocal()) {
 
     // First handle the globals defined in system headers.
     if (C.getSourceManager().isInSystemHeader(D->getLocation())) {
// Treat other globals as GlobalInternal unless they are constants.
       // TODO: We could walk the complex types here and see if everything is
       // constified.
       if (GT && GQT.isConstQualified() && GT->isArithmeticType())
         sReg = getGlobalsRegion(MemRegion::GlobalImmutableSpaceRegionKind);
       else
         sReg = getGlobalsRegion();
     }
 
-  // Finally handle static locals.
+  // Finally handle locals.
   } else {
     // FIXME: Once we implement scope handling, we will need to properly lookup
     // 'D' to the proper LocationContext.
     const DeclContext *DC = D->getDeclContext();
     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) {
+      if (D->isStaticLocal()) {
+        const CodeTextRegion *fReg = nullptr;
+        if (const auto *ND = dyn_cast<NamedDecl>(DC))
+          fReg = getFunctionCodeRegion(ND);
+        else if (const auto *BD = dyn_cast<BlockDecl>(DC))
+          fReg = getBlockCodeRegion(BD, getBlockPointerType(BD, getContext()),
+                                    LC->getAnalysisDeclContext());
+        assert(fReg && "Unable to determine code region for a static local!");
+        sReg = getGlobalsRegion(MemRegion::StaticGlobalSpaceRegionKind, fReg);
+      } else {
+        // We're looking at a block-captured local variable, which may be either
+        // still local, or already moved to the heap. So we're not sure.
         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();
         if (isa<FunctionDecl>(STCD) || isa<ObjCMethodDecl>(STCD))
           sReg = getGlobalsRegion(MemRegion::StaticGlobalSpaceRegionKind,
                                   getFunctionCodeRegion(cast<NamedDecl>(STCD)));
         else if (const BlockDecl *BD = dyn_cast<BlockDecl>(STCD)) {
-          // FIXME: The fallback type here is totally bogus -- though it should
-          // never be queried, it will prevent uniquing with the real
-          // BlockCodeRegion. Ideally we'd fix the AST so that we always had a
-          // signature.
-          QualType T;
-          if (const TypeSourceInfo *TSI = BD->getSignatureAsWritten())
-            T = TSI->getType();
-          if (T.isNull())
-            T = getContext().VoidTy;
-          if (!T->getAs<FunctionType>())
-            T = getContext().getFunctionNoProtoType(T);
-          T = getContext().getBlockPointerType(T);
-
           const BlockCodeRegion *BTR =
-            getBlockCodeRegion(BD, C.getCanonicalType(T),
+              getBlockCodeRegion(BD, getBlockPointerType(BD, getContext()),
                                  STC->getAnalysisDeclContext());
           sReg = getGlobalsRegion(MemRegion::StaticGlobalSpaceRegionKind,
                                   BTR);
         }
         else {
           sReg = getGlobalsRegion();
         }
       }
     }

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
+  };
+}