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

Artem Dergachev via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Nov 10 00:01:26 PST 2017

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?



More information about the cfe-commits mailing list