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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
Herald added subscribers: cfe-commits, a.sidorin, szepet, xazax.hun.

I'm seeing false positives on the new check added in https://reviews.llvm.org/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?


Repository:
  rC Clang

https://reviews.llvm.org/D41042

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
  test/Analysis/stack-capture-leak-arc.mm


Index: test/Analysis/stack-capture-leak-arc.mm
===================================================================
--- test/Analysis/stack-capture-leak-arc.mm
+++ test/Analysis/stack-capture-leak-arc.mm
@@ -1,4 +1,4 @@
-// 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);
Index: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -37,6 +37,14 @@
   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;
@@ -225,6 +233,8 @@
 
 void StackAddrEscapeChecker::checkPreCall(const CallEvent &Call,
                                           CheckerContext &C) const {
+  if (!ChecksEnabled[CK_StackAddrAsyncEscapeChecker])
+    return;
   if (!Call.isGlobalCFunction("dispatch_after") &&
       !Call.isGlobalCFunction("dispatch_async"))
     return;
@@ -237,6 +247,8 @@
 
 void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS,
                                           CheckerContext &C) const {
+  if (!ChecksEnabled[CK_StackAddrEscapeChecker])
+    return;
 
   const Expr *RetE = RS->getRetValue();
   if (!RetE)
@@ -277,6 +289,9 @@
 }
 
 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
@@ -346,6 +361,12 @@
   }
 }
 
-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)
Index: include/clang/StaticAnalyzer/Checkers/Checkers.td
===================================================================
--- include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -188,6 +188,10 @@
   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 {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D41042.126233.patch
Type: text/x-patch
Size: 3326 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20171209/269664b1/attachment.bin>