NoQ created this revision.
Herald added a subscriber: xazax.hun.
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?
https://reviews.llvm.org/D36750
Files:
lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
test/Analysis/retain-release.m
Index: test/Analysis/retain-release.m
===================================================================
--- test/Analysis/retain-release.m
+++ test/Analysis/retain-release.m
@@ -2300,6 +2300,23 @@
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>
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
@@ -86,6 +86,7 @@
ErrorDeallocGC, // Calling -dealloc with GC enabled.
ErrorUseAfterRelease, // Object used after released.
ErrorReleaseNotOwned, // Release of an object that was not owned.
+ ErrorReleaseNotOwnedByBlock, // Release of an object not owned by a block.
ERROR_LEAK_START,
ErrorLeak, // A memory leak due to excessive reference counts.
ErrorLeakReturned, // A memory leak due to the returning method not having
@@ -331,6 +332,9 @@
Out << "Release of Not-Owned [ERROR]";
break;
+ case ErrorReleaseNotOwnedByBlock:
+ Out << "Release of Not-Owned by Block [ERROR]";
+
case RefVal::ErrorOverAutorelease:
Out << "Over-autoreleased";
break;
@@ -1714,6 +1718,17 @@
}
};
+ 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)
@@ -2510,7 +2525,8 @@
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;
@@ -3299,7 +3315,10 @@
return removeRefBinding(state, sym);
V = V.releaseViaIvar() ^ RefVal::Released;
} else {
- V = V ^ RefVal::ErrorReleaseNotOwned;
+ if (isa<BlockDecl>(C.getLocationContext()->getDecl()))
+ V = V ^ RefVal::ErrorReleaseNotOwnedByBlock;
+ else
+ V = V ^ RefVal::ErrorReleaseNotOwned;
hasErr = V.getKind();
}
break;
@@ -3349,6 +3368,11 @@
releaseNotOwned.reset(new BadRelease(this));
BT = releaseNotOwned.get();
break;
+ case RefVal::ErrorReleaseNotOwnedByBlock:
+ if (!releaseNotOwnedByBlock)
+ releaseNotOwnedByBlock.reset(new BadReleaseByBlock(this));
+ BT = releaseNotOwnedByBlock.get();
+ break;
case RefVal::ErrorDeallocGC:
if (!deallocGC)
deallocGC.reset(new DeallocGC(this));
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
