[PATCH] D39438: [analyzer] Diagnose stack leaks via block captures

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:128
+      !Call.isGlobalCFunction("dispatch_async") &&
+      !Call.isGlobalCFunction("dispatch_once"))
+    return;
----------------
`dispatch_once` isn't really asynchronous; i think it's out of place here.

Also we model it in the body farm, so i guess any errors inside its block would be caught anyway.


================
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:136-139
+    BlockDataRegion::referenced_vars_iterator I =
+        Block->referenced_vars_begin();
+    BlockDataRegion::referenced_vars_iterator E = Block->referenced_vars_end();
+    for (; I != E; ++I) {
----------------
If you like, feel free to add support for `for (const auto &Item: Block->referenced_vars())` loops into `BlockDataRegion` :)


================
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:144
+        return;
+      if (dyn_cast_or_null<StackSpaceRegion>(Region->getMemorySpace())) {
+        ExplodedNode *N = C.generateErrorNode();
----------------
`getMemorySpace()` is never null. Every region resides in a memory space.


================
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:153
+        llvm::raw_svector_ostream Out(Buf);
+        genName(Out, Region, C.getASTContext());
+        Out << " was captured by the block which will be executed "
----------------
Not sure - the rest of the checker's code seems to be making use of the returned `SourceRange` by adding to the report, do we need that here?


================
Comment at: test/Analysis/stack-async-leak.m:1
+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core -analyzer-store=region -fblocks -verify %s
+
----------------
We don't add `-analyzer-store=region` nowadays; i don't think it's useful (though it doesn't hurt).


================
Comment at: test/Analysis/stack-async-leak.m:60
+  void (^b)(void) = ^void(void) {
+    *p = 1; 
+  };
----------------
You may enjoy adding an extra note piece (as in D24278) to the offending statement within the block. It might be helpful if it's not obvious which block is being executed or which pointer points to the leaking variable.

Also it might be a good idea to think about a visitor that would track the block pointer to highlight where it was initialized with a concrete block code.

Though probably it's not super useful because we're most likely staying within a single function with this checker.

By the way, do we warn (or want to warn) on returning a block from a function, when this block captures a local variable?


Repository:
  rL LLVM

https://reviews.llvm.org/D39438