[analyzer] RetainCount: When diagnosing overrelease, mention if it's coming from a nested block.

[NoQ]

Summary

RetainCountChecker's warning message "Incorrect decrement of the reference count of an object that is not owned at this point by the caller" does not explicitly mention the caller, which may be confusing when there is a nested block, especially when the block is hard to notice. It should be obvious to which caller it refers. The patch tries to improve on that.

By the way, plist-based tests in retain-release.m are disabled since r163536 (~2012), and need to be updated. It's trivial to re-enable them but annoying to maintain - would we prefer to re-enable or delete them or replace with -analyzer-output=text tests?

[dcoughlin]

By the way, plist-based tests in retain-release.m are disabled since r163536 (~2012), and need to be updated. It's trivial to re-enable them but annoying to maintain - would we prefer to re-enable or delete them or replace with -analyzer-output=text tests?

This is rdar://problem/33514142

My preference would be to factor out/re-target some specific tests into their own file and check that with -verify + -analyzer-output=text and with plist comparisons

lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp

89

Is it possible to detect this from the location context in RetainCountChecker::processNonLeakError() rather than encoding it in the analysis state? This would avoid a multiplicity of 'ByBlock' kinds.

[dcoughlin]

I forgot to say this looks like a nice usability improvement!

[NoQ]

Avoid creating a new RefVal kind.

In D36750#843427, @dcoughlin wrote:

By the way, plist-based tests in retain-release.m are disabled since r163536 (~2012), and need to be updated. It's trivial to re-enable them but annoying to maintain - would we prefer to re-enable or delete them or replace with -analyzer-output=text tests?

My preference would be to factor out/re-target some specific tests into their own file and check that with -verify + -analyzer-output=text and with plist comparisons

Hmm, which specific tests do you have in mind? And is it worth it to try to recover the intended arrows in plists from the old plist tests?

[NoQ]

This is all wrong. While RetainCountChecker is more function-local than, say, MallocChecker, we still can't say for sure that it is the bottom frame's function (or block) that should be owning the object in this case. Ideally it should, but that's not the pattern that the checker is de facto trying to find. The checker is usually fine seeing a pointer allocated in a top function and released in a sub-block. If the bottom frame is over-releasing, we'd warn, but it wouldn't be simply because the bottom frame is over-releasing, so mentioning the particular stack frame may end up being misleading.

lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp

enum Kind {
     Released,  // Object has been released.
     ReturnedOwned, // Returned object passes ownership to caller.
     ReturnedNotOwned, // Return object does not pass ownership to caller.
     ERROR_START,
     ErrorDeallocNotOwned, // -dealloc called on non-owned object.
     ErrorDeallocGC, // Calling -dealloc with GC enabled.
     ErrorUseAfterRelease, // Object used after released.
     ErrorReleaseNotOwned, // Release of an object that was not owned.
     ERROR_LEAK_START,
     ErrorLeak,  // A memory leak due to excessive reference counts.
     ErrorLeakReturned, // A memory leak due to the returning method not having
                        // the correct naming conventions.
     ErrorGCLeakReturned,
     ErrorOverAutorelease,
     ErrorReturnedNotOwned
   };
 
public:
     BadRelease(const CheckerBase *checker) : CFRefBug(checker, "Bad release") {}
 
     const char *getDescription() const override {
       return "Incorrect decrement of the reference count of an object that is "
              "not owned at this point by the caller";
     }
   };
 
+  class BadReleaseByBlock : public CFRefBug {
+  public:
+    BadReleaseByBlock(const CheckerBase *checker)
+        : CFRefBug(checker, "Bad release") {}
+
+    const char *getDescription() const override {
+      return "Incorrect decrement of the reference count of an object that is "
+             "not owned at this point by the current block";
+    }
+  };
+
   class DeallocGC : public CFRefBug {
   public:
     DeallocGC(const CheckerBase *checker)
         : CFRefBug(checker, "-dealloc called while using garbage collection") {}
 
     const char *getDescription() const override {
       return "-dealloc called while using garbage collection";
     }
: public Checker< check::Bind,
                     check::PostStmt<ObjCDictionaryLiteral>,
                     check::PostStmt<ObjCBoxedExpr>,
                     check::PostStmt<ObjCIvarRefExpr>,
                     check::PostCall,
                     check::PreStmt<ReturnStmt>,
                     check::RegionChanges,
                     eval::Assume,
                     eval::Call > {
-  mutable std::unique_ptr<CFRefBug> useAfterRelease, releaseNotOwned;
+  mutable std::unique_ptr<CFRefBug> useAfterRelease;
+  mutable std::unique_ptr<CFRefBug> releaseNotOwned, releaseNotOwnedByBlock;
   mutable std::unique_ptr<CFRefBug> deallocGC, deallocNotOwned;
   mutable std::unique_ptr<CFRefBug> overAutorelease, returnNotOwnedForOwned;
   mutable std::unique_ptr<CFRefBug> leakWithinFunction, leakAtReturn;
   mutable std::unique_ptr<CFRefBug> leakWithinFunctionGC, leakAtReturnGC;
 
   typedef llvm::DenseMap<SymbolRef, const CheckerProgramPointTag *> SymbolTagMap;
 
   // This map is only used to ensure proper deletion of any allocated tags.
switch (ErrorKind) {
     default:
       llvm_unreachable("Unhandled error.");
     case RefVal::ErrorUseAfterRelease:
       if (!useAfterRelease)
         useAfterRelease.reset(new UseAfterRelease(this));
       BT = useAfterRelease.get();
       break;
     case RefVal::ErrorReleaseNotOwned:
+      if (isa<BlockDecl>(C.getLocationContext()->getDecl())) {
+        if (!releaseNotOwnedByBlock)
+          releaseNotOwnedByBlock.reset(new BadReleaseByBlock(this));
+        BT = releaseNotOwnedByBlock.get();
+      } else {
         if (!releaseNotOwned)
           releaseNotOwned.reset(new BadRelease(this));
         BT = releaseNotOwned.get();
+      }
       break;
     case RefVal::ErrorDeallocGC:
       if (!deallocGC)
         deallocGC.reset(new DeallocGC(this));
       BT = deallocGC.get();
       break;
     case RefVal::ErrorDeallocNotOwned:
       if (!deallocNotOwned)

test/Analysis/retain-release.m

 void testCFReturnsRetainedError() {
   extern int copyViaParam(CFTypeRef * CF_RETURNS_RETAINED outObj);
   CFTypeRef obj;
   if (copyViaParam(&obj) == -42)
     return; // no-warning
   CFRelease(obj);
 }
 
+//===----------------------------------------------------------------------===//
+// When warning within blocks make it obvious that warnings refer to blocks.
+//===----------------------------------------------------------------------===//
+
+int useBlock(int (^block)());
+void rdar31699502_hardToNoticeBlocks() {
+  if (useBlock(^{
+    NSAutoreleasePool * pool = [[NSAutoreleasePool alloc] init];
+    NSArray *array = [NSArray array];
+    [array release]; // expected-warning{{Incorrect decrement of the reference count of an object that is not owned at this point by the current block}}
+    [pool drain];
+    return 0;
+  })) {
+    return;
+  }
+}
+
 // CHECK:  <key>diagnostics</key>
 // CHECK-NEXT:  <array>
 // CHECK-NEXT:   <dict>
 // CHECK-NEXT:    <key>path</key>
 // CHECK-NEXT:    <array>
 // CHECK-NEXT:     <dict>
 // CHECK-NEXT:      <key>kind</key><string>control</string>
 // CHECK-NEXT:      <key>edges</key>