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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Looks great, thanks! Minor inline stuff.



================
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:116
+  const StackSpaceRegion *S = cast<StackSpaceRegion>(R->getMemorySpace());
+  return S->getStackFrame() != C.getLocationContext()->getCurrentStackFrame();
+}
----------------
Because stack address escapes are not fatal errors, it is not necessarily true that any other stack frame context here would be an ancestor of the current context. It may be an escaped stack variable from a function that has exited long time ago. You might want to check the `getParent()` chain.

Hmm, or rather rename the function to "isNotInCurrentFrame" because that's how you seem to be using it.


================
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:173
+  // To avoid false-positives (for now) we ignore all the blocks
+  // which have capatured a variable of the type "dispatch_semaphore_t".
+  if (isSemaphoreCaptured(*B.getDecl()))
----------------
Typo: cap~~a~~tured -> captured.


================
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:177
+  for (const MemRegion *Region : getCapturedStackRegions(B, C)) {
+    if (isa<BlockDataRegion>(Region))
+      continue;
----------------
Hmm, why would such region even be in the list?


================
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:184
+      BT_capturedstackasync = llvm::make_unique<BuiltinBug>(
+          this, "Address of stack-allocated memory");
+    SmallString<512> Buf;
----------------
I think bugtype should describe the bug, so that the user knew what sort of bug it is by looking at the list. Maybe add "...is captured" here?


Repository:
  rL LLVM

https://reviews.llvm.org/D39438