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

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin added a comment.

Thanks!

Another round of comments inline. With those addressed it looks good to me -- but you should wait on Artem's go-ahead before committing.



================
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:121
+    QualType Q = C.getVariable()->getType();
+    if (Q->isObjCObjectPointerType() &&
+        Q->getPointeeType()
----------------
I think here you will need to check whether Q is a TypedefType and compare the IdentifierInfo for its TypedefNameDecl to 'dispatch_semaphore_t' instead. 

The reason is that `NSObject<OS_dispatch_semaphore>` is an implementation detail and in fact `dispatch_semaphore_t` is typedef'd to another type depending on whether the file is Objective-C(++) or not.

There is an example of the idiom we use to do IdentifierInfo comparisons in ObjCDeallocChecker. We cache the identifier in the checker class. This lets us get constant-time comparisons on identifiers (since they are interned strings).

Also: It would be good to add a comment explaining why we care whether a semaphore was captured.


================
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:316
 
-  for (unsigned i = 0, e = cb.V.size(); i != e; ++i) {
+  for (const auto &P : Cb.V) {
     // Generate a report for this bug.
----------------
We generally prefer to avoid combining style cleanups like these with functional changes except in the code that is required to be updated for the functional change. This makes it easier for future maintainer to use the commit history to understand what a commit did and why.



================
Comment at: test/Analysis/stack-async-leak.m:1
+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core -fblocks -fobjc-arc -verify %s
+
----------------
alexshap wrote:
> dcoughlin wrote:
> > Can you add an additional run line testing the expected behavior when ARC is not enabled? You can pass a -DARC_ENABLED=1 or similar to distinguish between the two (for example, to test to make sure that a diagnostic is emitted under MRR but not ARC).
> i've tried this, it somehow gets more complicated to read / change the tests - decided to create a separate file.
That's fine!


================
Comment at: test/Analysis/stack-capture-leak-arc.mm:144
+  block();
+  return block;
+}
----------------
Can you add a `// no-warning` comment to this line? We use that to indicate that we explicitly don't expect a warning there.


Repository:
  rL LLVM

https://reviews.llvm.org/D39438