[analyzer] StackAddrEscape: Delay turning on by default a little bit?

[NoQ]

Summary

I'm seeing false positives on the new check added in D39438 of the following kind:

void foo() {
  T buf[16];
  for (int n = 0; n < 16; ++n) {
    T *ptr = &buf[n];
    dispatch_async(queue, ^{ use(ptr); });
  }
  dispatch_barrier_sync(queue, ^{});
}

In this case, pointer to the local variable buf is captured by a block that goes into dispatch_async, yet it is not a bug because synchronization ensures that this block quits before the variable goes out of scope.

I guess it's kinda similar to the semaphore synchronization case. We could probably add another suppression for functions that contain any dispatch_barrier_async calls. The ideal solution is to delay the warning until checkEndFunction (like the escape-to-global check does) to see if things get synced up until then (of course, once we have scopes we can make it even more fine-grained).

Alexander, do you think we should keep the checker under a flag (not enabled for regular users) until we get a better understanding of what kinds of synchronizations can get on the way? This diff splits your new check into alpha.core.StackAddressAsyncEscape. Or do you already have good quick fixes coming?

[NoQ]

Update the other run-line.

[alexander-shaposhnikov]

i see, to be honest, this is kind of unfortunate, if i am not mistaken, i've seen these false-positives, but not too many, most reports were real bugs. But if it's annoying, than i think it's fine to make it "alpha" until i figure out a good way to workaround this issue.

[NoQ]

Yeah, we usually try to avoid omissions of modeling in on-by-default checkers because the user may accidentally run into projects in which the unmodeled idiom is common, and then he'd get false positives all over the place. In my case it was just two new positives, both false due to this dispatch_barrier_sync idiom.

[NoQ]

Add a FIXME test.

cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td

 def TestAfterDivZeroChecker : Checker<"TestAfterDivZero">,
   HelpText<"Check for division by variable that is later compared against 0. Either the comparison is useless or there is division by zero.">,
   DescFile<"TestAfterDivZeroChecker.cpp">;
 
 def DynamicTypeChecker : Checker<"DynamicTypeChecker">,
   HelpText<"Check for cases where the dynamic and the static type of an object are unrelated.">,
   DescFile<"DynamicTypeChecker.cpp">;
 
+def StackAddrAsyncEscapeChecker : Checker<"StackAddressAsyncEscape">,
+  HelpText<"Check that addresses to stack memory do not escape the function">,
+  DescFile<"StackAddrEscapeChecker.cpp">;
+
 } // end "alpha.core"
 
 let ParentPackage = Nullability in {
 
 def NullPassedToNonnullChecker : Checker<"NullPassedToNonnull">,
   HelpText<"Warns when a null pointer is passed to a pointer which has a _Nonnull type.">,
   DescFile<"NullabilityChecker.cpp">;
 

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

: public Checker<check::PreCall, check::PreStmt<ReturnStmt>,
                      check::EndFunction> {
   mutable IdentifierInfo *dispatch_semaphore_tII;
   mutable std::unique_ptr<BuiltinBug> BT_stackleak;
   mutable std::unique_ptr<BuiltinBug> BT_returnstack;
   mutable std::unique_ptr<BuiltinBug> BT_capturedstackasync;
   mutable std::unique_ptr<BuiltinBug> BT_capturedstackret;
 
 public:
+  enum CheckKind {
+    CK_StackAddrEscapeChecker,
+    CK_StackAddrAsyncEscapeChecker,
+    CK_NumCheckKinds
+  };
+
+  DefaultBool ChecksEnabled[CK_NumCheckKinds];
+
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
   void checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const;
   void checkEndFunction(CheckerContext &Ctx) const;
 
 private:
   void checkReturnedBlockCaptures(const BlockDataRegion &B,
                                   CheckerContext &C) const;
   void checkAsyncExecutedBlockCaptures(const BlockDataRegion &B,
for (const MemRegion *Region : getCapturedStackRegions(B, C)) {
     if (Range.isValid())
       Report->addRange(Range);
     C.emitReport(std::move(Report));
   }
 }
 
 void StackAddrEscapeChecker::checkPreCall(const CallEvent &Call,
                                           CheckerContext &C) const {
+  if (!ChecksEnabled[CK_StackAddrAsyncEscapeChecker])
+    return;
   if (!Call.isGlobalCFunction("dispatch_after") &&
       !Call.isGlobalCFunction("dispatch_async"))
     return;
   for (unsigned Idx = 0, NumArgs = Call.getNumArgs(); Idx < NumArgs; ++Idx) {
     if (const BlockDataRegion *B = dyn_cast_or_null<BlockDataRegion>(
             Call.getArgSVal(Idx).getAsRegion()))
       checkAsyncExecutedBlockCaptures(*B, C);
   }
 }
 
 void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS,
                                           CheckerContext &C) const {
+  if (!ChecksEnabled[CK_StackAddrEscapeChecker])
+    return;
 
   const Expr *RetE = RS->getRetValue();
   if (!RetE)
     return;
   RetE = RetE->IgnoreParens();
 
   const LocationContext *LCtx = C.getLocationContext();
   SVal V = C.getState()->getSVal(RetE, LCtx);
if (isa<BlockDataRegion>(R) &&
       return;
     }
   }
 
   EmitStackError(C, R, RetE);
 }
 
 void StackAddrEscapeChecker::checkEndFunction(CheckerContext &Ctx) const {
+  if (!ChecksEnabled[CK_StackAddrEscapeChecker])
+    return;
+
   ProgramStateRef State = Ctx.getState();
 
   // Iterate over all bindings to global variables and see if it contains
   // a memory region in the stack space.
   class CallBack : public StoreManager::BindingsHandler {
   private:
     CheckerContext &Ctx;
     const StackFrameContext *CurSFC;
for (const auto &P : Cb.V) {
     auto Report = llvm::make_unique<BugReport>(*BT_stackleak, Out.str(), N);
     if (Range.isValid())
       Report->addRange(Range);
 
     Ctx.emitReport(std::move(Report));
   }
 }
 
-void ento::registerStackAddrEscapeChecker(CheckerManager &Mgr) {
-  Mgr.registerChecker<StackAddrEscapeChecker>();
+#define REGISTER_CHECKER(name) \
+  void ento::register##name(CheckerManager &Mgr) { \
+    StackAddrEscapeChecker *Chk = \
+        Mgr.registerChecker<StackAddrEscapeChecker>(); \
+    Chk->ChecksEnabled[StackAddrEscapeChecker::CK_##name] = true; \
   }
+
+REGISTER_CHECKER(StackAddrEscapeChecker)
+REGISTER_CHECKER(StackAddrAsyncEscapeChecker)

cfe/trunk/test/Analysis/stack-capture-leak-arc.mm

-// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core -fblocks -fobjc-arc -verify %s
+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core,alpha.core.StackAddressAsyncEscape -fblocks -fobjc-arc -verify %s
 
 typedef struct dispatch_queue_s *dispatch_queue_t;
 typedef void (^dispatch_block_t)(void);
 void dispatch_async(dispatch_queue_t queue, dispatch_block_t block);
 typedef long dispatch_once_t;
 void dispatch_once(dispatch_once_t *predicate, dispatch_block_t block);
 typedef long dispatch_time_t;
 void dispatch_after(dispatch_time_t when, dispatch_queue_t queue, dispatch_block_t block);
+void dispatch_barrier_sync(dispatch_queue_t queue, dispatch_block_t block);
 
 extern dispatch_queue_t queue;
 extern dispatch_once_t *predicate;
 extern dispatch_time_t when;
 
 void test_block_expr_async() {
   int x = 123;
   int *p = &x;
dispatch_async(queue, ^{
     // Some work.
     dispatch_semaphore_signal(semaphore);
   }); // no-warning
 
   // Do some other work concurrently with the asynchronous work
   // Wait for the asynchronous work to finish
   dispatch_semaphore_wait(semaphore, 1000);
 }
+
+void test_dispatch_barrier_sync() {
+  int buf[16];
+  for (int n = 0; n < 16; ++n) {
+    int *ptr = &buf[n];
+    // FIXME: Should not warn. The dispatch_barrier_sync() call ensures
+    // that the block does not outlive 'buf'.
+    dispatch_async(queue, ^{ // expected-warning{{Address of stack memory associated with local variable 'buf' is captured by an asynchronously-executed block}}
+      (void)ptr;
+    });
+  }
+  dispatch_barrier_sync(queue, ^{});
+}

cfe/trunk/test/Analysis/stack-capture-leak-no-arc.mm

-// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core -fblocks -verify %s
+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core,alpha.core.StackAddressAsyncEscape -fblocks -verify %s
 
 typedef struct dispatch_queue_s *dispatch_queue_t;
 typedef void (^dispatch_block_t)(void);
 void dispatch_async(dispatch_queue_t queue, dispatch_block_t block);
 extern dispatch_queue_t queue;
 
 void test_block_inside_block_async_no_leak() {
   int x = 123;