[PATCH] D36750: [analyzer] RetainCount: When diagnosing overrelease, mention if it's coming from a nested block.
NoQ planned changes to this revision. NoQ added a comment. 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. https://reviews.llvm.org/D36750 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36750: [analyzer] RetainCount: When diagnosing overrelease, mention if it's coming from a nested block.
NoQ updated this revision to Diff 113102.
NoQ marked an inline comment as done.
NoQ added a comment.
Avoid creating a new `RefVal` kind.
In https://reviews.llvm.org/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?
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: diagnostics
// CHECK-NEXT:
// CHECK-NEXT:
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
@@ -1722,6 +1722,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)
@@ -2560,7 +2571,8 @@
check::RegionChanges,
eval::Assume,
eval::Call > {
- mutable std::unique_ptr useAfterRelease, releaseNotOwned;
+ mutable std::unique_ptr useAfterRelease;
+ mutable std::unique_ptr releaseNotOwned, releaseNotOwnedByBlock;
mutable std::unique_ptr deallocGC, deallocNotOwned;
mutable std::unique_ptr overAutorelease, returnNotOwnedForOwned;
mutable std::unique_ptr leakWithinFunction, leakAtReturn;
@@ -3396,9 +3408,15 @@
BT = useAfterRelease.get();
break;
case RefVal::ErrorReleaseNotOwned:
- if (!releaseNotOwned)
-releaseNotOwned.reset(new BadRelease(this));
- BT = releaseNotOwned.get();
+ if (isa(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)
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: diagnostics
// CHECK-NEXT:
// CHECK-NEXT:
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
@@ -1722,6 +1722,17 @@
}
};
+ class
[PATCH] D36750: [analyzer] RetainCount: When diagnosing overrelease, mention if it's coming from a nested block.
dcoughlin added a comment. I forgot to say this looks like a nice usability improvement! https://reviews.llvm.org/D36750 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36750: [analyzer] RetainCount: When diagnosing overrelease, mention if it's coming from a nested block.
dcoughlin added a comment. > 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 Comment at: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp:89 ErrorReleaseNotOwned, // Release of an object that was not owned. +ErrorReleaseNotOwnedByBlock, // Release of an object not owned by a block. ERROR_LEAK_START, 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. https://reviews.llvm.org/D36750 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36750: [analyzer] RetainCount: When diagnosing overrelease, mention if it's coming from a nested block.
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: diagnostics
// CHECK-NEXT:
// CHECK-NEXT:
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 useAfterRelease, releaseNotOwned;
+ mutable std::unique_ptr useAfterRelease;
+ mutable std::unique_ptr releaseNotOwned, releaseNotOwnedByBlock;
mutable std::unique_ptr deallocGC, deallocNotOwned;
mutable std::unique_ptr overAutorelease, returnNotOwnedForOwned;
mutable std::unique_ptr leakWithinFunction, leakAtReturn;
@@ -3299,7 +3315,10 @@
return removeRefBinding(state, sym);
V = V.releaseViaIvar() ^ RefVal::Released;
} else {
-V = V ^ RefVal::ErrorReleaseNotOwned;
+if (isa(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
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
