[PATCH] D36441: Add Support for Reference Counting of Parameters on the Callee Side in RetainCountChecker
malhar1995 updated this revision to Diff 111420.
malhar1995 added a comment.
This patch adds the functionality of performing reference counting on the
callee side for Integer Set Library (ISL) to Clang Static Analyzer's
RetainCountChecker.
Reference counting on the callee side can be extensively used to perform
debugging within a function (For example: Finding leaks on error paths).
Repository:
rL LLVM
https://reviews.llvm.org/D36441
Files:
lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
test/Analysis/retain-release-inline.m
Index: test/Analysis/retain-release-inline.m
===
--- test/Analysis/retain-release-inline.m
+++ test/Analysis/retain-release-inline.m
@@ -302,6 +302,9 @@
__attribute__((annotate("rc_ownership_returns_retained"))) isl_basic_map *isl_basic_map_cow(__attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap);
void free(void *);
+void callee_side_parameter_checking_leak(__attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap) { // expected-warning {{Potential leak of an object}}
+}
+
// As 'isl_basic_map_free' is annotated with 'rc_ownership_trusted_implementation', RetainCountChecker trusts its
// implementation and doesn't analyze its body. If the annotation 'rc_ownership_trusted_implementation' is removed,
// a leak warning is raised by RetainCountChecker as the analyzer is unable to detect a decrement in the reference
@@ -348,6 +351,37 @@
isl_basic_map_free(bmap);
}
+void callee_side_parameter_checking_incorrect_rc_decrement(isl_basic_map *bmap) {
+ isl_basic_map_free(bmap); // expected-warning {{Incorrect decrement of the reference count}}
+}
+
+__attribute__((annotate("rc_ownership_returns_retained"))) isl_basic_map *callee_side_parameter_checking_return_notowned_object(isl_basic_map *bmap) {
+ return bmap; // expected-warning {{Object with a +0 retain count returned to caller where a +1 (owning) retain count is expected}}
+}
+
+__attribute__((annotate("rc_ownership_returns_retained"))) isl_basic_map *callee_side_parameter_checking_assign_consumed_parameter_leak_return(__attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap1, __attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap2) { // expected-warning {{Potential leak of an object}}
+ bmap1 = bmap2;
+ isl_basic_map_free(bmap2);
+ return bmap1;
+}
+
+__attribute__((annotate("rc_ownership_returns_retained"))) isl_basic_map *callee_side_parameter_checking_assign_consumed_parameter_leak(__attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap1, __attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap2) { // expected-warning {{Potential leak of an object}}
+ bmap1 = bmap2;
+ isl_basic_map_free(bmap1);
+ return bmap2;
+}
+
+__attribute__((annotate("rc_ownership_returns_retained"))) isl_basic_map *error_path_leak(__attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap1, __attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap2) { // expected-warning {{Potential leak of an object}}
+ bmap1 = isl_basic_map_cow(bmap1);
+ if (!bmap1 || !bmap2)
+goto error;
+
+ isl_basic_map_free(bmap2);
+ return bmap1;
+error:
+ return isl_basic_map_free(bmap1);
+}
+
//===--===//
// Test returning retained and not-retained values.
//===--===//
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
@@ -462,6 +462,7 @@
ArgEffect getDefaultArgEffect() const { return DefaultArgEffect; }
friend class RetainSummaryManager;
+ friend class RetainCountChecker;
};
} // end anonymous namespace
@@ -1319,6 +1320,13 @@
return hasRCAnnotation(FD, "rc_ownership_trusted_implementation");
}
+static bool isGeneralizedObjectRef(QualType Ty) {
+ if (Ty.getAsString().substr(0, 4) == "isl_")
+return true;
+ else
+return false;
+}
+
//===--===//
// Summary creation for Selectors.
//===--===//
@@ -1848,6 +1856,15 @@
class CFRefLeakReport : public CFRefReport {
const MemRegion* AllocBinding;
+const Stmt *AllocStmt;
+
+// Finds the function declaration where a leak warning for the parameter 'sym' should be raised.
+void deriveParamLocation(CheckerContext &Ctx, SymbolRef sym);
+// Finds the location where a leak warning for 'sym' should be raised.
+void deriveAllocLocation(CheckerContext &Ctx, SymbolRef sym);
+// Produces description of a leak warning which is printed on the console.
+void createDescription(CheckerContext &Ctx, bool GCEnabled, bool
[PATCH] D36441: Add Support for Reference Counting of Parameters on the Callee Side in RetainCountChecker
malhar1995 updated this revision to Diff 110514.
malhar1995 marked 7 inline comments as done.
malhar1995 added a comment.
Addressed comments.
P.S: I'm yet to test this callee-side parameter checking functionality on the
actual ISL codebase. I'll do that ASAP.
Repository:
rL LLVM
https://reviews.llvm.org/D36441
Files:
lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
test/Analysis/retain-release-inline.m
Index: test/Analysis/retain-release-inline.m
===
--- test/Analysis/retain-release-inline.m
+++ test/Analysis/retain-release-inline.m
@@ -302,6 +302,9 @@
__attribute__((annotate("rc_ownership_returns_retained"))) isl_basic_map *isl_basic_map_cow(__attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap);
void free(void *);
+void callee_side_parameter_checking_leak(__attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap) { // expected-warning {{Potential leak of an object}}
+}
+
// As 'isl_basic_map_free' is annotated with 'rc_ownership_trusted_implementation', RetainCountChecker trusts its
// implementation and doesn't analyze its body. If the annotation 'rc_ownership_trusted_implementation' is removed,
// a leak warning is raised by RetainCountChecker as the analyzer is unable to detect a decrement in the reference
@@ -348,6 +351,26 @@
isl_basic_map_free(bmap);
}
+void callee_side_parameter_checking_incorrect_rc_decrement(isl_basic_map *bmap) {
+ isl_basic_map_free(bmap); // expected-warning {{Incorrect decrement of the reference count}}
+}
+
+__attribute__((annotate("rc_ownership_returns_retained"))) isl_basic_map *callee_side_parameter_checking_return_notowned_object(isl_basic_map *bmap) {
+ return bmap; // expected-warning {{Object with a +0 retain count returned to caller where a +1 (owning) retain count is expected}}
+}
+
+ __attribute__((annotate("rc_ownership_returns_retained"))) isl_basic_map *callee_side_parameter_checking_assign_consumed_parameter_leak_return(__attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap1, __attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap2) { // expected-warning {{Potential leak of an object}}
+ bmap1 = bmap2;
+ isl_basic_map_free(bmap2);
+ return bmap1;
+}
+
+ __attribute__((annotate("rc_ownership_returns_retained"))) isl_basic_map *callee_side_parameter_checking_assign_consumed_parameter_leak(__attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap1, __attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap2) { // expected-warning {{Potential leak of an object}}
+ bmap1 = bmap2;
+ isl_basic_map_free(bmap1);
+ return bmap2;
+}
+
//===--===//
// Test returning retained and not-retained values.
//===--===//
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
@@ -462,6 +462,7 @@
ArgEffect getDefaultArgEffect() const { return DefaultArgEffect; }
friend class RetainSummaryManager;
+ friend class RetainCountChecker;
};
} // end anonymous namespace
@@ -1848,6 +1849,15 @@
class CFRefLeakReport : public CFRefReport {
const MemRegion* AllocBinding;
+const Stmt *AllocStmt;
+
+// Finds the function declaration where a leak warning for the parameter 'sym' should be raised.
+void deriveParamLocation(CheckerContext &Ctx, SymbolRef sym);
+// Finds the location where a leak warning for 'sym' should be raised.
+void deriveAllocLocation(CheckerContext &Ctx, SymbolRef sym);
+// Produces description of a leak warning which is printed on the console.
+void createDescription(CheckerContext &Ctx, bool GCEnabled, bool IncludeAllocationLine);
+
public:
CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts, bool GCEnabled,
const SummaryLogTy &Log, ExplodedNode *n, SymbolRef sym,
@@ -2427,13 +2437,25 @@
return llvm::make_unique(L, os.str());
}
-CFRefLeakReport::CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts,
- bool GCEnabled, const SummaryLogTy &Log,
- ExplodedNode *n, SymbolRef sym,
- CheckerContext &Ctx,
- bool IncludeAllocationLine)
- : CFRefReport(D, LOpts, GCEnabled, Log, n, sym, false) {
+void CFRefLeakReport::deriveParamLocation(CheckerContext &Ctx, SymbolRef sym) {
+ const SourceManager& SMgr = Ctx.getSourceManager();
+
+ if (!sym->getOriginRegion())
+return;
+ auto *Region = dyn_cast(sym->getOriginRegion());
+ if (Region) {
+const Decl *PDecl = Region->getDecl();
+if (PDecl && isa(PDecl)) {
+ PathDiagnosticLocation ParamLocation = PathDiagnosti
[PATCH] D36441: Add Support for Reference Counting of Parameters on the Callee Side in RetainCountChecker
malhar1995 added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp:2521-2523
+ deriveAllocLocation(Ctx, sym);
+ if (!AllocBinding)
+deriveParamLocation(Ctx, sym);
I'm not sure what difference it will make if I change the ordering of the
invocations of `deriveAllocLocation` and 'deriveParamLocation`.
Comment at: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp:2683
+ DefaultBool PerformCalleeSideParameterChecking;
+
This might be used in the future in case callee side parameter checking is
added for Core Foundation and Objective-C objects.
Comment at: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp:3960-3971
+ for (unsigned idx = 0, e = FD->getNumParams(); idx != e; ++idx) {
+const ParmVarDecl *Param = FD->getParamDecl(idx);
+SymbolRef Sym = state->getSVal(state->getRegion(Param,
LCtx)).getAsSymbol();
+
+QualType Ty = Param->getType();
+if (hasRCAnnotation(Param, "rc_ownership_consumed"))
+ state = setRefBinding(state, Sym,
Getting function summary and checking for `ArgEffects` doesn't seem like an
option to me as currently there is no way to differentiate between Core
Foundation and Generalized objects just by looking at their ArgEffects.
However, this loop results in `check-clang-analysis` failure in
`retain-release.m` on lines `1140` and `2237`.
Repository:
rL LLVM
https://reviews.llvm.org/D36441
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36441: Add Support for Reference Counting of Parameters on the Callee Side in RetainCountChecker
malhar1995 created this revision.
Herald added a subscriber: eraman.
Current RetainCountChecker performs reference counting of function
arguments/parameters only on the caller side and not on the callee side.
This patch aims to add support for reference counting on the callee-side for
objects of 'Generalized' ObjKind.
This patch is still a work in progress.
Repository:
rL LLVM
https://reviews.llvm.org/D36441
Files:
lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
test/Analysis/retain-release-inline.m
Index: test/Analysis/retain-release-inline.m
===
--- test/Analysis/retain-release-inline.m
+++ test/Analysis/retain-release-inline.m
@@ -302,6 +302,9 @@
__attribute__((annotate("rc_ownership_returns_retained"))) isl_basic_map *isl_basic_map_cow(__attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap);
void free(void *);
+void callee_side_parameter_checking_leak(__attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap) { // expected-warning {{Potential leak}}
+}
+
// As 'isl_basic_map_free' is annotated with 'rc_ownership_trusted_implementation', RetainCountChecker trusts its
// implementation and doesn't analyze its body. If the annotation 'rc_ownership_trusted_implementation' is removed,
// a leak warning is raised by RetainCountChecker as the analyzer is unable to detect a decrement in the reference
@@ -348,6 +351,10 @@
isl_basic_map_free(bmap);
}
+void callee_side_parameter_checking_incorrect_rc_decrement(isl_basic_map *bmap) {
+ isl_basic_map_free(bmap); // expected-warning {{Incorrect decrement of the reference count}}
+}
+
//===--===//
// Test returning retained and not-retained values.
//===--===//
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
@@ -1848,6 +1848,12 @@
class CFRefLeakReport : public CFRefReport {
const MemRegion* AllocBinding;
+const Stmt *AllocStmt;
+
+void deriveParamLocation(CheckerContext &Ctx, SymbolRef sym);
+void deriveAllocLocation(CheckerContext &Ctx, SymbolRef sym);
+void createDescription(CheckerContext &Ctx, bool GCEnabled, bool IncludeAllocationLine);
+
public:
CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts, bool GCEnabled,
const SummaryLogTy &Log, ExplodedNode *n, SymbolRef sym,
@@ -2427,13 +2433,25 @@
return llvm::make_unique(L, os.str());
}
-CFRefLeakReport::CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts,
- bool GCEnabled, const SummaryLogTy &Log,
- ExplodedNode *n, SymbolRef sym,
- CheckerContext &Ctx,
- bool IncludeAllocationLine)
- : CFRefReport(D, LOpts, GCEnabled, Log, n, sym, false) {
+void CFRefLeakReport::deriveParamLocation(CheckerContext &Ctx, SymbolRef sym) {
+ const SourceManager& SMgr = Ctx.getSourceManager();
+
+ if (!sym->getOriginRegion())
+return;
+ const DeclRegion *Region = dyn_cast(sym->getOriginRegion());
+ if (Region) {
+const Decl *PDecl = Region->getDecl();
+if (PDecl && isa(PDecl)) {
+ PathDiagnosticLocation ParamLocation = PathDiagnosticLocation::create(PDecl, SMgr);
+ Location = ParamLocation;
+ UniqueingLocation = ParamLocation;
+ UniqueingDecl = Ctx.getLocationContext()->getDecl();
+}
+ }
+}
+
+void CFRefLeakReport::deriveAllocLocation(CheckerContext &Ctx,SymbolRef sym) {
// Most bug reports are cached at the location where they occurred.
// With leaks, we want to unique them by the location where they were
// allocated, and only report a single path. To do this, we need to find
@@ -2457,8 +2475,13 @@
// FIXME: This will crash the analyzer if an allocation comes from an
// implicit call (ex: a destructor call).
// (Currently there are no such allocations in Cocoa, though.)
- const Stmt *AllocStmt = PathDiagnosticLocation::getStmt(AllocNode);
- assert(AllocStmt && "Cannot find allocation statement");
+ AllocStmt = PathDiagnosticLocation::getStmt(AllocNode);
+ // assert(AllocStmt && "Cannot find allocation statement");
+
+ if (!AllocStmt) {
+AllocBinding = nullptr;
+return;
+ }
PathDiagnosticLocation AllocLocation =
PathDiagnosticLocation::createBegin(AllocStmt, SMgr,
@@ -2469,8 +2492,9 @@
// leaks should be uniqued on the allocation site.
UniqueingLocation = AllocLocation;
UniqueingDecl = AllocNode->getLocationContext()->getDecl();
+}
- // Fill in the description of the bug.
+void CFRefLeakReport::createDescription(CheckerContext &Ctx, bool GCEnabled, bool IncludeAllocationLine) {
Description.clear();
[PATCH] D35613: Add Support for Generic Reference Counting Annotations in RetainCountChecker
malhar1995 added a comment. @dcoughlin Ping. Repository: rL LLVM https://reviews.llvm.org/D35613 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35613: Add Support for Generic Reference Counting Annotations in RetainCountChecker
malhar1995 updated this revision to Diff 107471.
malhar1995 added a comment.
Removed the checks to see if the symbol type is NULL while printing diagnostics
as they are unnecessary.
Repository:
rL LLVM
https://reviews.llvm.org/D35613
Files:
include/clang/StaticAnalyzer/Checkers/ObjCRetainCount.h
lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
test/Analysis/retain-release-inline.m
test/Analysis/retain-release.m
Index: test/Analysis/retain-release.m
===
--- test/Analysis/retain-release.m
+++ test/Analysis/retain-release.m
@@ -325,6 +325,9 @@
extern
void *CFPlugInInstanceCreate(CFAllocatorRef allocator, CFUUIDRef factoryUUID, CFUUIDRef typeUUID);
+typedef struct {
+ int ref;
+} isl_basic_map;
//===--===//
// Test cases.
@@ -574,6 +577,14 @@
}
}
+__attribute__((annotate("rc_ownership_returns_retained"))) isl_basic_map *isl_basic_map_cow(__attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap);
+
+// Test custom diagnostics for generalized objects.
+void f18(__attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap) {
+ // After this call, 'bmap' has a +1 reference count.
+ bmap = isl_basic_map_cow(bmap); // expected-warning {{Potential leak of an object}}
+}
+
// Test basic tracking of ivars associated with 'self'. For the retain/release
// checker we currently do not want to flag leaks associated with stores
// of tracked objects to ivars.
Index: test/Analysis/retain-release-inline.m
===
--- test/Analysis/retain-release-inline.m
+++ test/Analysis/retain-release-inline.m
@@ -299,15 +299,15 @@
bar(s);
}
-__attribute__((cf_returns_retained)) isl_basic_map *isl_basic_map_cow(__attribute__((cf_consumed)) isl_basic_map *bmap);
+__attribute__((annotate("rc_ownership_returns_retained"))) isl_basic_map *isl_basic_map_cow(__attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap);
void free(void *);
// As 'isl_basic_map_free' is annotated with 'rc_ownership_trusted_implementation', RetainCountChecker trusts its
// implementation and doesn't analyze its body. If the annotation 'rc_ownership_trusted_implementation' is removed,
// a leak warning is raised by RetainCountChecker as the analyzer is unable to detect a decrement in the reference
// count of 'bmap' along the path in 'isl_basic_map_free' assuming the predicate of the second 'if' branch to be
// true or assuming both the predicates in the function to be false.
-__attribute__((annotate("rc_ownership_trusted_implementation"))) isl_basic_map *isl_basic_map_free(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+__attribute__((annotate("rc_ownership_trusted_implementation"))) isl_basic_map *isl_basic_map_free(__attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap) {
if (!bmap)
return NULL;
@@ -322,15 +322,15 @@
// implementation and doesn't analyze its body. If that annotation is removed, a 'use-after-release' warning might
// be raised by RetainCountChecker as the pointer which is passed as an argument to this function and the pointer
// which is returned from the function point to the same memory location.
-__attribute__((annotate("rc_ownership_trusted_implementation"))) __attribute__((cf_returns_retained)) isl_basic_map *isl_basic_map_copy(isl_basic_map *bmap) {
+__attribute__((annotate("rc_ownership_trusted_implementation"))) __attribute__((annotate("rc_ownership_returns_retained"))) isl_basic_map *isl_basic_map_copy(isl_basic_map *bmap) {
if (!bmap)
return NULL;
bmap->ref++;
return bmap;
}
-void test_use_after_release_with_trusted_implementation_annotate_attribute(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+void test_use_after_release_with_trusted_implementation_annotate_attribute(__attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap) {
// After this call, 'bmap' has a +1 reference count.
bmap = isl_basic_map_cow(bmap);
// After the call to 'isl_basic_map_copy', 'bmap' has a +1 reference count.
@@ -341,7 +341,7 @@
isl_basic_map_free(temp);
}
-void test_leak_with_trusted_implementation_annotate_attribute(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+void test_leak_with_trusted_implementation_annotate_attribute(__attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap) {
// After this call, 'bmap' has a +1 reference count.
bmap = isl_basic_map_cow(bmap); // no-warning
// After this call, 'bmap' has a +0 reference count.
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
@@ -1340,6 +1340,8 @@
if (D->hasAttr())
return RetEffect::MakeOwned(RetEffect::CF);
+ else if (hasR
[PATCH] D35613: Add Support for Generic Reference Counting Annotations in RetainCountChecker
malhar1995 added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp:2012
+ } else if (CurrV.getObjKind() == RetEffect::GenericC) {
+if (Sym->getType().isNull()) {
+ os << " returns an object with a ";
NoQ wrote:
> I don't think this can happen. Symbols always have a type, see
> `isValidTypeForSymbol()`. These branches can be removed from the surrounding
> code as well.
So, you're saying `Sym->getType()` can never be NULL?
If so, do you want me to insert `assert(isValidTypeForSymbol(Sym->getType()))`
instead?
Repository:
rL LLVM
https://reviews.llvm.org/D35613
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35613: Add Support for Generic Reference Counting Annotations in RetainCountChecker
malhar1995 updated this revision to Diff 107347.
malhar1995 added a comment.
Addressed all comments except the one where I had to remove the test to see if
the return type was null while emitting diagnostics.
Repository:
rL LLVM
https://reviews.llvm.org/D35613
Files:
include/clang/StaticAnalyzer/Checkers/ObjCRetainCount.h
lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
test/Analysis/retain-release-inline.m
test/Analysis/retain-release.m
Index: test/Analysis/retain-release.m
===
--- test/Analysis/retain-release.m
+++ test/Analysis/retain-release.m
@@ -325,6 +325,9 @@
extern
void *CFPlugInInstanceCreate(CFAllocatorRef allocator, CFUUIDRef factoryUUID, CFUUIDRef typeUUID);
+typedef struct {
+ int ref;
+} isl_basic_map;
//===--===//
// Test cases.
@@ -574,6 +577,14 @@
}
}
+__attribute__((annotate("rc_ownership_returns_retained"))) isl_basic_map *isl_basic_map_cow(__attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap);
+
+// Test custom diagnostics for generalized objects.
+void f18(__attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap) {
+ // After this call, 'bmap' has a +1 reference count.
+ bmap = isl_basic_map_cow(bmap); // expected-warning {{Potential leak of an object}}
+}
+
// Test basic tracking of ivars associated with 'self'. For the retain/release
// checker we currently do not want to flag leaks associated with stores
// of tracked objects to ivars.
Index: test/Analysis/retain-release-inline.m
===
--- test/Analysis/retain-release-inline.m
+++ test/Analysis/retain-release-inline.m
@@ -299,15 +299,15 @@
bar(s);
}
-__attribute__((cf_returns_retained)) isl_basic_map *isl_basic_map_cow(__attribute__((cf_consumed)) isl_basic_map *bmap);
+__attribute__((annotate("rc_ownership_returns_retained"))) isl_basic_map *isl_basic_map_cow(__attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap);
void free(void *);
// As 'isl_basic_map_free' is annotated with 'rc_ownership_trusted_implementation', RetainCountChecker trusts its
// implementation and doesn't analyze its body. If the annotation 'rc_ownership_trusted_implementation' is removed,
// a leak warning is raised by RetainCountChecker as the analyzer is unable to detect a decrement in the reference
// count of 'bmap' along the path in 'isl_basic_map_free' assuming the predicate of the second 'if' branch to be
// true or assuming both the predicates in the function to be false.
-__attribute__((annotate("rc_ownership_trusted_implementation"))) isl_basic_map *isl_basic_map_free(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+__attribute__((annotate("rc_ownership_trusted_implementation"))) isl_basic_map *isl_basic_map_free(__attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap) {
if (!bmap)
return NULL;
@@ -322,15 +322,15 @@
// implementation and doesn't analyze its body. If that annotation is removed, a 'use-after-release' warning might
// be raised by RetainCountChecker as the pointer which is passed as an argument to this function and the pointer
// which is returned from the function point to the same memory location.
-__attribute__((annotate("rc_ownership_trusted_implementation"))) __attribute__((cf_returns_retained)) isl_basic_map *isl_basic_map_copy(isl_basic_map *bmap) {
+__attribute__((annotate("rc_ownership_trusted_implementation"))) __attribute__((annotate("rc_ownership_returns_retained"))) isl_basic_map *isl_basic_map_copy(isl_basic_map *bmap) {
if (!bmap)
return NULL;
bmap->ref++;
return bmap;
}
-void test_use_after_release_with_trusted_implementation_annotate_attribute(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+void test_use_after_release_with_trusted_implementation_annotate_attribute(__attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap) {
// After this call, 'bmap' has a +1 reference count.
bmap = isl_basic_map_cow(bmap);
// After the call to 'isl_basic_map_copy', 'bmap' has a +1 reference count.
@@ -341,7 +341,7 @@
isl_basic_map_free(temp);
}
-void test_leak_with_trusted_implementation_annotate_attribute(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+void test_leak_with_trusted_implementation_annotate_attribute(__attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap) {
// After this call, 'bmap' has a +1 reference count.
bmap = isl_basic_map_cow(bmap); // no-warning
// After this call, 'bmap' has a +0 reference count.
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
@@ -1340,6 +1340,8 @@
if (D->hasAttr())
return RetEffect::MakeOwned(RetEf
[PATCH] D35613: Add Support for Generic Reference Counting Annotations in RetainCountChecker
malhar1995 created this revision.
malhar1995 added a project: clang.
As part of my Google Summer of Code project, I am working on adding support for
Integer Set Library (ISL) annotations to the current RetainCountChecker.
Note about ISL:
ISL has annotations __isl_give and __isl_take which are analogous to
cf_returns_retained and cf_consumed but in case of ISL, annotations precede
datatypes of function parameters.
So far, to build the ISL codebase using scan-build, I was #define-ing
__isl_give and __isl_take to __attribute__((cf_returns_retained)) and
__attribute__((cf_consumed)) respectively which resulted in the analyzer
treating ISL objects as Core Foundation objects while performing reference
counting.
This patch aims to add support for generic annotations
(__attribute__(annotate("give"))) and __attribute__(annotate("take" in
RetainCountChecker hence enabling the RetainCountChecker to perform reference
counting for any codebase written in C.
Let me know your comments on the same.
Repository:
rL LLVM
https://reviews.llvm.org/D35613
Files:
include/clang/StaticAnalyzer/Checkers/ObjCRetainCount.h
lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
test/Analysis/retain-release-inline.m
Index: test/Analysis/retain-release-inline.m
===
--- test/Analysis/retain-release-inline.m
+++ test/Analysis/retain-release-inline.m
@@ -299,15 +299,15 @@
bar(s);
}
-__attribute__((cf_returns_retained)) isl_basic_map *isl_basic_map_cow(__attribute__((cf_consumed)) isl_basic_map *bmap);
+__attribute__((annotate("give"))) isl_basic_map *isl_basic_map_cow(__attribute__((annotate("take"))) isl_basic_map *bmap);
void free(void *);
// As 'isl_basic_map_free' is annotated with 'rc_ownership_trusted_implementation', RetainCountChecker trusts its
// implementation and doesn't analyze its body. If the annotation 'rc_ownership_trusted_implementation' is removed,
// a leak warning is raised by RetainCountChecker as the analyzer is unable to detect a decrement in the reference
// count of 'bmap' along the path in 'isl_basic_map_free' assuming the predicate of the second 'if' branch to be
// true or assuming both the predicates in the function to be false.
-__attribute__((annotate("rc_ownership_trusted_implementation"))) isl_basic_map *isl_basic_map_free(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+__attribute__((annotate("rc_ownership_trusted_implementation"))) isl_basic_map *isl_basic_map_free(__attribute__((annotate("take"))) isl_basic_map *bmap) {
if (!bmap)
return NULL;
@@ -322,15 +322,15 @@
// implementation and doesn't analyze its body. If that annotation is removed, a 'use-after-release' warning might
// be raised by RetainCountChecker as the pointer which is passed as an argument to this function and the pointer
// which is returned from the function point to the same memory location.
-__attribute__((annotate("rc_ownership_trusted_implementation"))) __attribute__((cf_returns_retained)) isl_basic_map *isl_basic_map_copy(isl_basic_map *bmap) {
+__attribute__((annotate("rc_ownership_trusted_implementation"))) __attribute__((annotate("give"))) isl_basic_map *isl_basic_map_copy(isl_basic_map *bmap) {
if (!bmap)
return NULL;
bmap->ref++;
return bmap;
}
-void test_use_after_release_with_trusted_implementation_annotate_attribute(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+void test_use_after_release_with_trusted_implementation_annotate_attribute(__attribute__((annotate("take"))) isl_basic_map *bmap) {
// After this call, 'bmap' has a +1 reference count.
bmap = isl_basic_map_cow(bmap);
// After the call to 'isl_basic_map_copy', 'bmap' has a +1 reference count.
@@ -341,7 +341,7 @@
isl_basic_map_free(temp);
}
-void test_leak_with_trusted_implementation_annotate_attribute(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+void test_leak_with_trusted_implementation_annotate_attribute(__attribute__((annotate("take"))) isl_basic_map *bmap) {
// After this call, 'bmap' has a +1 reference count.
bmap = isl_basic_map_cow(bmap); // no-warning
// After this call, 'bmap' has a +0 reference count.
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
@@ -1340,6 +1340,8 @@
if (D->hasAttr())
return RetEffect::MakeOwned(RetEffect::CF);
+ else if (hasRCAnnotation(D, "give"))
+return RetEffect::MakeOwned(RetEffect::GenericC);
if (D->hasAttr())
return RetEffect::MakeNotOwned(RetEffect::CF);
@@ -1363,9 +1365,10 @@
const ParmVarDecl *pd = *pi;
if (pd->hasAttr())
Template->addArg(AF, parm_idx, DecRefMsg);
-else if (pd->hasAttr())
+else if (pd->hasAttr() || hasRCAnnotation(pd, "take"))
Template->addArg(AF, parm_idx, DecRef);
-else if (pd->hasAt
[PATCH] D34937: Suppressing Reference Counting Diagnostics for Functions Containing 'rc_ownership_trusted_implementation' Annotate Attribute
malhar1995 updated this revision to Diff 107020.
malhar1995 added a comment.
Added isTrustedReferenceCountAnnotation() again.
Repository:
rL LLVM
https://reviews.llvm.org/D34937
Files:
lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
test/Analysis/retain-release-inline.m
Index: test/Analysis/retain-release-inline.m
===
--- test/Analysis/retain-release-inline.m
+++ test/Analysis/retain-release-inline.m
@@ -12,7 +12,7 @@
//
// It includes the basic definitions for the test cases below.
//===--===//
-
+#define NULL 0
typedef unsigned int __darwin_natural_t;
typedef unsigned long uintptr_t;
typedef unsigned int uint32_t;
@@ -267,6 +267,10 @@
extern CFStringRef CFStringCreateWithCStringNoCopy(CFAllocatorRef alloc, const char *cStr, CFStringEncoding encoding, CFAllocatorRef contentsDeallocator);
+typedef struct {
+ int ref;
+} isl_basic_map;
+
//===--===//
// Test cases.
//===--===//
@@ -285,6 +289,7 @@
foo(s);
bar(s);
}
+
void test_neg() {
NSString *s = [[NSString alloc] init]; // no-warning
foo(s);
@@ -294,6 +299,55 @@
bar(s);
}
+__attribute__((cf_returns_retained)) isl_basic_map *isl_basic_map_cow(__attribute__((cf_consumed)) isl_basic_map *bmap);
+void free(void *);
+
+// As 'isl_basic_map_free' is annotated with 'rc_ownership_trusted_implementation', RetainCountChecker trusts its
+// implementation and doesn't analyze its body. If the annotation 'rc_ownership_trusted_implementation' is removed,
+// a leak warning is raised by RetainCountChecker as the analyzer is unable to detect a decrement in the reference
+// count of 'bmap' along the path in 'isl_basic_map_free' assuming the predicate of the second 'if' branch to be
+// true or assuming both the predicates in the function to be false.
+__attribute__((annotate("rc_ownership_trusted_implementation"))) isl_basic_map *isl_basic_map_free(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+ if (!bmap)
+return NULL;
+
+ if (--bmap->ref > 0)
+return NULL;
+
+ free(bmap);
+ return NULL;
+}
+
+// As 'isl_basic_map_copy' is annotated with 'rc_ownership_trusted_implementation', RetainCountChecker trusts its
+// implementation and doesn't analyze its body. If that annotation is removed, a 'use-after-release' warning might
+// be raised by RetainCountChecker as the pointer which is passed as an argument to this function and the pointer
+// which is returned from the function point to the same memory location.
+__attribute__((annotate("rc_ownership_trusted_implementation"))) __attribute__((cf_returns_retained)) isl_basic_map *isl_basic_map_copy(isl_basic_map *bmap) {
+ if (!bmap)
+return NULL;
+
+ bmap->ref++;
+ return bmap;
+}
+
+void test_use_after_release_with_trusted_implementation_annotate_attribute(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+ // After this call, 'bmap' has a +1 reference count.
+ bmap = isl_basic_map_cow(bmap);
+ // After the call to 'isl_basic_map_copy', 'bmap' has a +1 reference count.
+ isl_basic_map *temp = isl_basic_map_cow(isl_basic_map_copy(bmap));
+ // After this call, 'bmap' has a +0 reference count.
+ isl_basic_map *temp2 = isl_basic_map_cow(bmap); // no-warning
+ isl_basic_map_free(temp2);
+ isl_basic_map_free(temp);
+}
+
+void test_leak_with_trusted_implementation_annotate_attribute(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+ // After this call, 'bmap' has a +1 reference count.
+ bmap = isl_basic_map_cow(bmap); // no-warning
+ // After this call, 'bmap' has a +0 reference count.
+ isl_basic_map_free(bmap);
+}
+
//===--===//
// Test returning retained and not-retained values.
//===--===//
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
@@ -1304,6 +1304,21 @@
DoNothing, DoNothing);
}
+/// Returns true if the declaration 'D' is annotated with 'rcAnnotation'.
+bool hasRCAnnotation(const Decl *D, StringRef rcAnnotation) {
+ for (const auto *Ann : D->specific_attrs()) {
+if (Ann->getAnnotation() == rcAnnotation)
+ return true;
+ }
+ return false;
+}
+
+/// Returns true if the function declaration 'FD' contains
+/// 'rc_ownership_trusted_implementation' annotate attribute.
+bool isTrustedReferenceCountImplementation(const FunctionDecl *FD) {
+ return hasRCAnnotation(FD, "rc_ownership_trusted_implementation");
+}
+
//===--===//
// Summary crea
[PATCH] D34937: Suppressing Reference Counting Diagnostics for Functions Containing 'rc_ownership_trusted_implementation' Annotate Attribute
malhar1995 updated this revision to Diff 106911.
malhar1995 added a comment.
Addressed comments.
Changed the function from isTrustedReferenceCountImplementation() to
hasRCAnnotation() (I'm unable to come up with a better name) as this will be
useful when I add support to RetainCountChecker to check for generic
annotations corresponding to __isl_give(cf_returns_retained) and
__isl_take(cf_consumed).
Repository:
rL LLVM
https://reviews.llvm.org/D34937
Files:
lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
test/Analysis/retain-release-inline.m
Index: test/Analysis/retain-release-inline.m
===
--- test/Analysis/retain-release-inline.m
+++ test/Analysis/retain-release-inline.m
@@ -12,7 +12,7 @@
//
// It includes the basic definitions for the test cases below.
//===--===//
-
+#define NULL 0
typedef unsigned int __darwin_natural_t;
typedef unsigned long uintptr_t;
typedef unsigned int uint32_t;
@@ -267,6 +267,10 @@
extern CFStringRef CFStringCreateWithCStringNoCopy(CFAllocatorRef alloc, const char *cStr, CFStringEncoding encoding, CFAllocatorRef contentsDeallocator);
+typedef struct {
+ int ref;
+} isl_basic_map;
+
//===--===//
// Test cases.
//===--===//
@@ -285,6 +289,7 @@
foo(s);
bar(s);
}
+
void test_neg() {
NSString *s = [[NSString alloc] init]; // no-warning
foo(s);
@@ -294,6 +299,55 @@
bar(s);
}
+__attribute__((cf_returns_retained)) isl_basic_map *isl_basic_map_cow(__attribute__((cf_consumed)) isl_basic_map *bmap);
+void free(void *);
+
+// As 'isl_basic_map_free' is annotated with 'rc_ownership_trusted_implementation', RetainCountChecker trusts its
+// implementation and doesn't analyze its body. If the annotation 'rc_ownership_trusted_implementation' is removed,
+// a leak warning is raised by RetainCountChecker as the analyzer is unable to detect a decrement in the reference
+// count of 'bmap' along the path in 'isl_basic_map_free' assuming the predicate of the second 'if' branch to be
+// true or assuming both the predicates in the function to be false.
+__attribute__((annotate("rc_ownership_trusted_implementation"))) isl_basic_map *isl_basic_map_free(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+ if (!bmap)
+return NULL;
+
+ if (--bmap->ref > 0)
+return NULL;
+
+ free(bmap);
+ return NULL;
+}
+
+// As 'isl_basic_map_copy' is annotated with 'rc_ownership_trusted_implementation', RetainCountChecker trusts its
+// implementation and doesn't analyze its body. If that annotation is removed, a 'use-after-release' warning might
+// be raised by RetainCountChecker as the pointer which is passed as an argument to this function and the pointer
+// which is returned from the function point to the same memory location.
+__attribute__((annotate("rc_ownership_trusted_implementation"))) __attribute__((cf_returns_retained)) isl_basic_map *isl_basic_map_copy(isl_basic_map *bmap) {
+ if (!bmap)
+return NULL;
+
+ bmap->ref++;
+ return bmap;
+}
+
+void test_use_after_release_with_trusted_implementation_annotate_attribute(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+ // After this call, 'bmap' has a +1 reference count.
+ bmap = isl_basic_map_cow(bmap);
+ // After the call to 'isl_basic_map_copy', 'bmap' has a +1 reference count.
+ isl_basic_map *temp = isl_basic_map_cow(isl_basic_map_copy(bmap));
+ // After this call, 'bmap' has a +0 reference count.
+ isl_basic_map *temp2 = isl_basic_map_cow(bmap); // no-warning
+ isl_basic_map_free(temp2);
+ isl_basic_map_free(temp);
+}
+
+void test_leak_with_trusted_implementation_annotate_attribute(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+ // After this call, 'bmap' has a +1 reference count.
+ bmap = isl_basic_map_cow(bmap); // no-warning
+ // After this call, 'bmap' has a +0 reference count.
+ isl_basic_map_free(bmap);
+}
+
//===--===//
// Test returning retained and not-retained values.
//===--===//
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
@@ -1304,6 +1304,15 @@
DoNothing, DoNothing);
}
+/// Returns true if the declaration 'D' is annotated with 'rcAnnotation'.
+bool hasRCAnnotation(const Decl *D, StringRef rcAnnotation) {
+ for (const auto *Ann : D->specific_attrs()) {
+if (Ann->getAnnotation() == rcAnnotation)
+ return true;
+ }
+ return false;
+}
+
//===--===//
//
[PATCH] D34937: Suppressing Reference Counting Diagnostics for Functions Containing 'rc_ownership_trusted_implementation' Annotate Attribute
malhar1995 added a comment. In https://reviews.llvm.org/D34937#811500, @dcoughlin wrote: > Looks good to me, other than a nit on indentation and a request for a little > bit more info in a comment! Currently, I have added support for generic annotations for __isl_take and __isl_give and I have also added a generic diagnostic note when the function returns an object which is neither Core-Foundation nor Objective-C. For emitting a generic diagnostic note, I have added an object kind to the enum ObjKind in ObjCRetainCount.h. Now, for adding the aforementioned support, I have made some changes to the patch which I last submitted. So, should I submit another patch on the same revision after modifying the title, summary, etc. or should I create another revision for that? Repository: rL LLVM https://reviews.llvm.org/D34937 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34937: Suppressing Reference Counting Diagnostics for Functions Containing 'rc_ownership_trusted_implementation' Annotate Attribute
malhar1995 updated this revision to Diff 106765.
malhar1995 added a comment.
Checked for "trusted" annotation on the function declaration corresponding to
the function definition.
Repository:
rL LLVM
https://reviews.llvm.org/D34937
Files:
lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
test/Analysis/retain-release-inline.m
Index: test/Analysis/retain-release-inline.m
===
--- test/Analysis/retain-release-inline.m
+++ test/Analysis/retain-release-inline.m
@@ -12,7 +12,7 @@
//
// It includes the basic definitions for the test cases below.
//===--===//
-
+#define NULL 0
typedef unsigned int __darwin_natural_t;
typedef unsigned long uintptr_t;
typedef unsigned int uint32_t;
@@ -267,6 +267,10 @@
extern CFStringRef CFStringCreateWithCStringNoCopy(CFAllocatorRef alloc, const char *cStr, CFStringEncoding encoding, CFAllocatorRef contentsDeallocator);
+typedef struct {
+ int ref;
+} isl_basic_map;
+
//===--===//
// Test cases.
//===--===//
@@ -285,6 +289,7 @@
foo(s);
bar(s);
}
+
void test_neg() {
NSString *s = [[NSString alloc] init]; // no-warning
foo(s);
@@ -294,6 +299,56 @@
bar(s);
}
+__attribute__((cf_returns_retained)) isl_basic_map *isl_basic_map_cow(__attribute__((cf_consumed)) isl_basic_map *bmap);
+void free(void *);
+
+// As 'isl_basic_map_free' is annotated with 'rc_ownership_trusted_implementation', RetainCountChecker trusts its
+// implementation and doesn't analyze its body. If the annotation 'rc_ownership_trusted_implementation' is removed,
+// a leak warning is raised by RetainCountChecker as the analyzer is unable to detect a decrement in the reference
+// count of 'bmap' along the path in 'isl_basic_map_free' assuming the predicate of the second 'if' branch to be
+// true or assuming both the predicates in the function to be false.
+__attribute__((annotate("rc_ownership_trusted_implementation"))) isl_basic_map *isl_basic_map_free(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+ if (!bmap)
+return NULL;
+
+ if (--bmap->ref > 0)
+return NULL;
+
+ free(bmap);
+ return NULL;
+}
+
+// As 'isl_basic_map_copy' is annotated with 'rc_ownership_trusted_implementation', RetainCountChecker trusts its
+// implementation and doesn't analyze its body. If that annotation is removed, a 'use-after-release' warning might
+// be raised by RetainCountChecker as the pointer which is passed as an argument to this function and the pointer
+// which is returned from the function point to the same memory location.
+__attribute__((annotate("rc_ownership_trusted_implementation"))) __attribute__((cf_returns_retained)) isl_basic_map *isl_basic_map_copy(isl_basic_map *bmap)
+{
+ if (!bmap)
+return NULL;
+
+ bmap->ref++;
+ return bmap;
+}
+
+void test_use_after_release_with_trusted_implementation_annotate_attribute(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+ // After this call, 'bmap' has a +1 reference count.
+ bmap = isl_basic_map_cow(bmap);
+ // After the call to 'isl_basic_map_copy', 'bmap' has a +1 reference count.
+ isl_basic_map *temp = isl_basic_map_cow(isl_basic_map_copy(bmap));
+ // After this call, 'bmap' has a +0 reference count.
+ isl_basic_map *temp2 = isl_basic_map_cow(bmap); // no-warning
+ isl_basic_map_free(temp2);
+ isl_basic_map_free(temp);
+}
+
+void test_leak_with_trusted_implementation_annotate_attribute(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+ // After this call, 'bmap' has a +1 reference count.
+ bmap = isl_basic_map_cow(bmap); // no-warning
+ // After this call, 'bmap' has a +0 reference count.
+ isl_basic_map_free(bmap);
+}
+
//===--===//
// Test returning retained and not-retained values.
//===--===//
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
@@ -1894,6 +1894,16 @@
return SFC->getAnalysisDeclContext()->isBodyAutosynthesized();
}
+/// Returns true if the function declaration 'FD' contains
+/// 'rc_ownership_trusted_implementation' annotate attribute.
+bool isTrustedReferenceCountImplementation(const FunctionDecl *FD) {
+ for (const auto *Ann : FD->specific_attrs()) {
+if (Ann->getAnnotation() == "rc_ownership_trusted_implementation")
+ return true;
+ }
+ return false;
+}
+
std::shared_ptr
CFRefReportVisitor::VisitNode(const ExplodedNode *N, const ExplodedNode *PrevN,
BugReporterContext &BRC, BugReport &BR) {
@@ -3380,6 +3390,9 @@
// See if it's one
[PATCH] D34937: Suppressing Reference Counting Diagnostics for Functions Containing 'rc_ownership_trusted_implementation' Annotate Attribute
malhar1995 updated this revision to Diff 106685.
malhar1995 added a comment.
Suppresses false positives involving functions which perform reference counting.
Added relevant test-cases to test/Analysis/retain-release-inline.m
https://reviews.llvm.org/D34937
Files:
lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
test/Analysis/retain-release-inline.m
Index: test/Analysis/retain-release-inline.m
===
--- test/Analysis/retain-release-inline.m
+++ test/Analysis/retain-release-inline.m
@@ -12,7 +12,7 @@
//
// It includes the basic definitions for the test cases below.
//===--===//
-
+#define NULL 0
typedef unsigned int __darwin_natural_t;
typedef unsigned long uintptr_t;
typedef unsigned int uint32_t;
@@ -267,6 +267,10 @@
extern CFStringRef CFStringCreateWithCStringNoCopy(CFAllocatorRef alloc, const char *cStr, CFStringEncoding encoding, CFAllocatorRef contentsDeallocator);
+typedef struct {
+ int ref;
+} isl_basic_map;
+
//===--===//
// Test cases.
//===--===//
@@ -285,6 +289,7 @@
foo(s);
bar(s);
}
+
void test_neg() {
NSString *s = [[NSString alloc] init]; // no-warning
foo(s);
@@ -294,6 +299,56 @@
bar(s);
}
+__attribute__((cf_returns_retained)) isl_basic_map *isl_basic_map_cow(__attribute__((cf_consumed)) isl_basic_map *bmap);
+void free(void *);
+
+// As 'isl_basic_map_free' is annotated with 'rc_ownership_trusted_implementation', RetainCountChecker trusts its
+// implementation and doesn't analyze its body. If the annotation 'rc_ownership_trusted_implementation' is removed,
+// a leak warning is raised by RetainCountChecker as the analyzer is unable to detect a decrement in the reference
+// count of 'bmap' along the path in 'isl_basic_map_free' assuming the predicate of the second 'if' branch to be
+// true or assuming both the predicates in the function to be false.
+__attribute__((annotate("rc_ownership_trusted_implementation"))) isl_basic_map *isl_basic_map_free(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+ if (!bmap)
+return NULL;
+
+ if (--bmap->ref > 0)
+return NULL;
+
+ free(bmap);
+ return NULL;
+}
+
+// As 'isl_basic_map_copy' is annotated with 'rc_ownership_trusted_implementation', RetainCountChecker trusts its
+// implementation and doesn't analyze its body. If that annotation is removed, a 'use-after-release' warning might
+// be raised by RetainCountChecker as the pointer which is passed as an argument to this function and the pointer
+// which is returned from the function point to the same memory location.
+__attribute__((annotate("rc_ownership_trusted_implementation"))) __attribute__((cf_returns_retained)) isl_basic_map *isl_basic_map_copy(isl_basic_map *bmap)
+{
+ if (!bmap)
+return NULL;
+
+ bmap->ref++;
+ return bmap;
+}
+
+void test_use_after_release_with_trusted_implementation_annotate_attribute(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+ // After this call, 'bmap' has a +1 reference count.
+ bmap = isl_basic_map_cow(bmap);
+ // After the call to 'isl_basic_map_copy', 'bmap' has a +1 reference count.
+ isl_basic_map *temp = isl_basic_map_cow(isl_basic_map_copy(bmap));
+ // After this call, 'bmap' has a +0 reference count.
+ isl_basic_map *temp2 = isl_basic_map_cow(bmap); // no-warning
+ isl_basic_map_free(temp2);
+ isl_basic_map_free(temp);
+}
+
+void test_leak_with_trusted_implementation_annotate_attribute(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+ // After this call, 'bmap' has a +1 reference count.
+ bmap = isl_basic_map_cow(bmap); // no-warning
+ // After this call, 'bmap' has a +0 reference count.
+ isl_basic_map_free(bmap);
+}
+
//===--===//
// Test returning retained and not-retained values.
//===--===//
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
@@ -1894,6 +1894,16 @@
return SFC->getAnalysisDeclContext()->isBodyAutosynthesized();
}
+/// Returns true if the function declaration 'FD' contains
+/// 'rc_ownership_trusted_implementation' annotate attribute.
+bool isTrustedReferenceCountImplementation(const FunctionDecl *FD) {
+ for (const auto *Ann : FD->specific_attrs()) {
+if (Ann->getAnnotation() == "rc_ownership_trusted_implementation")
+ return true;
+ }
+ return false;
+}
+
std::shared_ptr
CFRefReportVisitor::VisitNode(const ExplodedNode *N, const ExplodedNode *PrevN,
BugReporterContext &BRC, BugReport &BR) {
@@ -3380,6 +3390,9 @@
[PATCH] D34937: Suppressing Reference Counting Diagnostics for Functions Containing 'rc_ownership_trusted_implementation' Annotate Attribute
malhar1995 updated this revision to Diff 105292.
malhar1995 added a comment.
Corrected one of the two test-cases added in the last-diff.
Repository:
rL LLVM
https://reviews.llvm.org/D34937
Files:
lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
test/Analysis/retain-release-inline.m
Index: test/Analysis/retain-release-inline.m
===
--- test/Analysis/retain-release-inline.m
+++ test/Analysis/retain-release-inline.m
@@ -12,7 +12,7 @@
//
// It includes the basic definitions for the test cases below.
//===--===//
-
+#define NULL 0
typedef unsigned int __darwin_natural_t;
typedef unsigned long uintptr_t;
typedef unsigned int uint32_t;
@@ -267,6 +267,10 @@
extern CFStringRef CFStringCreateWithCStringNoCopy(CFAllocatorRef alloc, const char *cStr, CFStringEncoding encoding, CFAllocatorRef contentsDeallocator);
+typedef struct {
+ int ref;
+} isl_basic_map;
+
//===--===//
// Test cases.
//===--===//
@@ -285,6 +289,7 @@
foo(s);
bar(s);
}
+
void test_neg() {
NSString *s = [[NSString alloc] init]; // no-warning
foo(s);
@@ -294,6 +299,48 @@
bar(s);
}
+__attribute__((cf_returns_retained)) isl_basic_map *isl_basic_map_copy(isl_basic_map *bmap);
+__attribute__((cf_returns_retained)) isl_basic_map *isl_basic_map_cow(__attribute__((cf_consumed)) isl_basic_map *bmap);
+isl_basic_map *isl_basic_map_decrement_reference_count(__attribute__((cf_consumed)) isl_basic_map *bmap);
+void free(void *);
+
+__attribute__((annotate("rc_ownership_trusted_implementation"))) __attribute__((cf_returns_retained)) isl_basic_map *test_use_after_release_with_trusted_implementation_annotate_attribute(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+ // After this call, 'temp' has a +1 reference count.
+ isl_basic_map *temp = isl_basic_map_copy(bmap);
+ // After this call, 'bmap' has a +1 reference count.
+ bmap = isl_basic_map_cow(bmap);
+ // After this call, 'bmap' has a +0 reference count.
+ isl_basic_map_decrement_reference_count(bmap);
+ isl_basic_map_decrement_reference_count(bmap); // no-warning {{A 'use-after-release' warning is not raised here as this function has 'rc_ownership_trusted_implementation' annotate attribute}}
+ return temp;
+}
+
+isl_basic_map *isl_basic_map_free(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+ if (!bmap)
+return NULL;
+
+ if (--bmap->ref > 0)
+return NULL;
+
+ free(bmap);
+ return NULL;
+}
+
+__attribute__((cf_returns_retained)) isl_basic_map *test_leak_with_trusted_implementation_annotate_attribute(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+ // After this call, 'temp' has a +1 reference count.
+ isl_basic_map *temp = isl_basic_map_copy(bmap);
+ // After this call, 'bmap' has a +1 reference count.
+ bmap = isl_basic_map_cow(bmap); // no-warning {{A 'leak' warning is not raised here as 'foo_bar' (annotated with 'rc_ownership_trusted_implementation') is on the call-stack}}
+ // After this call, assuming the predicate of the second if branch to be true, 'bmap' has a +1 reference count.
+ isl_basic_map_free(bmap);
+ return temp;
+}
+
+__attribute__((annotate("rc_ownership_trusted_implementation"))) __attribute__((cf_returns_retained)) isl_basic_map *foo_bar(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+ bmap = test_leak_with_trusted_implementation_annotate_attribute(bmap);
+ return bmap;
+}
+
//===--===//
// Test returning retained and not-retained values.
//===--===//
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
@@ -1894,6 +1894,23 @@
return SFC->getAnalysisDeclContext()->isBodyAutosynthesized();
}
+/// Returns true if any declaration on the call-stack contains
+/// 'rc_ownership_trusted_implementation' annotate attribute.
+bool isTrustedReferenceCountImplementation(const LocationContext *LCtx) {
+ while (LCtx) {
+if (const StackFrameContext *SFC = dyn_cast(LCtx)) {
+ const Decl *D = SFC->getDecl();
+ for (const auto *Ann : D->specific_attrs()) {
+if (Ann->getAnnotation() == "rc_ownership_trusted_implementation") {
+ return true;
+}
+ }
+}
+LCtx = LCtx->getParent();
+ }
+ return false;
+}
+
std::shared_ptr
CFRefReportVisitor::VisitNode(const ExplodedNode *N, const ExplodedNode *PrevN,
BugReporterContext &BRC, BugReport &BR) {
@@ -3345,11 +3362,13 @@
}
assert(BT);
- auto report = std::unique_ptr(
- new CFRefReport(*BT
[PATCH] D34937: Suppressing Reference Counting Diagnostics for Functions Containing 'rc_ownership_trusted_implementation' Annotate Attribute
malhar1995 added a comment. In https://reviews.llvm.org/D34937#799504, @dcoughlin wrote: > Thanks for the tests. > > Have you tried this on the ISL codebase to make sure it is suppressing the > diagnostics in related to reference counting implementation that you expect? > > I think it would be good to add some tests that reflect the reference > counting implementation patterns in ISL that you want to make sure not to > warn on. Can you simplify those patterns to their essence and add them as > tests? Although, I have not tried running the new RetainCountChecker on the entire ISL codebase, I ran it on small chunks of it and it seemed to be working as expected. Also, the test-cases that I have added in my latest patch are indeed representative of the reference counting implementation in ISL. Ideally, in case of ISL, we don't want the analyzer to analyze the bodies of functions of the type obj_free(), obj_cow(), etc. as they result in a large number of 'leak' false positives but adding 'rc_ownership_trusted_implementation' annotate attribute to just these functions will not do the trick as when warnings are raised, these functions are no longer present on the call-stack. Also, I just realized that I made a mistake with one of the two test-cases that I added. I'll fix that ASAP. Repository: rL LLVM https://reviews.llvm.org/D34937 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34937: Suppressing Reference Counting Diagnostics for Functions Containing 'rc_ownership_trusted_implementation' Annotate Attribute
malhar1995 updated this revision to Diff 105186.
malhar1995 added a comment.
Added relevant test-cases to verify the added functionality.
Repository:
rL LLVM
https://reviews.llvm.org/D34937
Files:
lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
test/Analysis/retain-release-inline.m
Index: test/Analysis/retain-release-inline.m
===
--- test/Analysis/retain-release-inline.m
+++ test/Analysis/retain-release-inline.m
@@ -12,7 +12,7 @@
//
// It includes the basic definitions for the test cases below.
//===--===//
-
+#define NULL 0
typedef unsigned int __darwin_natural_t;
typedef unsigned long uintptr_t;
typedef unsigned int uint32_t;
@@ -267,6 +267,10 @@
extern CFStringRef CFStringCreateWithCStringNoCopy(CFAllocatorRef alloc, const char *cStr, CFStringEncoding encoding, CFAllocatorRef contentsDeallocator);
+typedef struct {
+int ref;
+} isl_basic_map;
+
//===--===//
// Test cases.
//===--===//
@@ -285,6 +289,7 @@
foo(s);
bar(s);
}
+
void test_neg() {
NSString *s = [[NSString alloc] init]; // no-warning
foo(s);
@@ -294,6 +299,48 @@
bar(s);
}
+__attribute__((cf_returns_retained)) isl_basic_map *isl_basic_map_copy(isl_basic_map *bmap);
+__attribute__((cf_returns_retained)) isl_basic_map *isl_basic_map_cow(__attribute__((cf_consumed)) isl_basic_map *bmap);
+isl_basic_map *isl_basic_map_decrement_reference_count(__attribute__((cf_consumed)) isl_basic_map *bmap);
+void free(void *);
+
+__attribute__((annotate("rc_ownership_trusted_implementation"))) __attribute__((cf_returns_retained)) isl_basic_map *test_use_after_release_with_trusted_implementation_annotate_attribute(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+ // After this call, 'temp' has a +1 reference count.
+ isl_basic_map *temp = isl_basic_map_copy(bmap);
+ // After this call, 'bmap' has a +1 reference count.
+ bmap = isl_basic_map_cow(bmap);
+ // After this call, 'bmap' has a +0 reference count.
+ isl_basic_map_decrement_reference_count(bmap);
+ isl_basic_map_decrement_reference_count(bmap); // no-warning {{A 'use-after-release' warning is not raised here as this function has 'rc_ownership_trusted_implementation' annotate attribute}}
+ return temp;
+}
+
+isl_basic_map *isl_basic_map_free(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+ if (!bmap)
+return NULL;
+
+ if (--bmap->ref > 0)
+return NULL;
+
+ free(bmap);
+ return NULL;
+}
+
+__attribute__((cf_returns_retained)) isl_basic_map *test_leak_with_trusted_implementation_annotate_attribute(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+ // After this call, 'temp' has a +1 reference count.
+ isl_basic_map *temp = isl_basic_map_copy(bmap);
+ // After this call, 'bmap' has a +1 reference count.
+ bmap = isl_basic_map_cow(bmap); // no-warning {{A 'leak' warning is not raised here as 'foo_bar' (annotated with 'rc_ownership_trusted_implementation') is on the call-stack}}
+ // After this call, assuming the predicate of the second if branch to be true, 'bmap' has a +1 reference count.
+ isl_basic_map_free(bmap);
+ return temp;
+}
+
+__attribute__((annotate("rc_ownership_trusted_implementation"))) __attribute__((cf_returns_retained)) isl_basic_map *foo_bar(__attribute__((cf_consumed)) isl_basic_map *bmap) {
+ bmap = test_leak_with_trusted_annotate_attribute(bmap);
+ return bmap;
+}
+
//===--===//
// Test returning retained and not-retained values.
//===--===//
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
@@ -1894,6 +1894,23 @@
return SFC->getAnalysisDeclContext()->isBodyAutosynthesized();
}
+/// Returns true if any declaration on the call-stack contains
+/// 'rc_ownership_trusted_implementation' annotate attribute.
+bool isTrustedReferenceCountImplementation(const LocationContext *LCtx) {
+ while (LCtx) {
+if (const StackFrameContext *SFC = dyn_cast(LCtx)) {
+ const Decl *D = SFC->getDecl();
+ for (const auto *Ann : D->specific_attrs()) {
+if (Ann->getAnnotation() == "rc_ownership_trusted_implementation") {
+ return true;
+}
+ }
+}
+LCtx = LCtx->getParent();
+ }
+ return false;
+}
+
std::shared_ptr
CFRefReportVisitor::VisitNode(const ExplodedNode *N, const ExplodedNode *PrevN,
BugReporterContext &BRC, BugReport &BR) {
@@ -3345,11 +3362,13 @@
}
assert(BT);
- auto report = std::unique_ptr(
- new CFRefReport(*BT, C.getASTC
[PATCH] D34937: Suppressing Reference Counting Diagnostics for Functions Containing 'rc_ownership_trusted_implementation' Annotate Attribute
malhar1995 updated this revision to Diff 105160.
malhar1995 added a comment.
Herald added a subscriber: eraman.
Changed function name from 'isAnnotatedToSkipDiagnostics' to
'isTrustedReferenceCountImplementation'.
Added some test-cases to test/Analysis/retain-release-inline.m.
Applied clang-format to the changed code.
Repository:
rL LLVM
https://reviews.llvm.org/D34937
Files:
lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
test/Analysis/retain-release-inline.m
Index: test/Analysis/retain-release-inline.m
===
--- test/Analysis/retain-release-inline.m
+++ test/Analysis/retain-release-inline.m
@@ -285,6 +285,14 @@
foo(s);
bar(s);
}
+
+__attribute__((annotate("rc_ownership_trusted_implementation"))) void test_with_trusted_implementation_annotate_attribute() {
+ NSString *s = [[NSString alloc] init]; // no-warning {{Ideally, a "leak" warning should've been raised here. But, as this function contains "rc_ownership_trusted_implementation" annotate attribute, the warning is suppressed}}
+ foo(s);
+ foo(s);
+ bar(s);
+}
+
void test_neg() {
NSString *s = [[NSString alloc] init]; // no-warning
foo(s);
@@ -294,6 +302,34 @@
bar(s);
}
+void test2() {
+ NSString *s = [[NSString alloc] init];
+ foo(s);
+ foo(s);
+ bar(s);
+ bar(s);
+ bar(s);
+ [s release]; // expected-warning {{Reference-counted object is used after it is released}}
+}
+
+__attribute__((annotate("rc_ownership_trusted_implementation"))) void test2_with_trusted_implementation_annotate_attribute() {
+ NSString *s = [[NSString alloc] init];
+ foo(s);
+ foo(s);
+ bar(s);
+ bar(s);
+ bar(s);
+ [s release]; // no-warning {{Ideally, a "use-after-release" warning should've been raised here. But, as this function contains "rc_ownership_trusted_implementation" annotate attribute, the warning is suppressed}}
+}
+
+void foo_bar() {
+ NSString *s = [[NSString alloc] init]; // no-warning {{Ideally, a "leak" warning should've been raised. But, as "test3" on the callstack has annotate attribute "rc_ownership_trusted_implementation", the warning is suppressed}}
+}
+
+__attribute__((annotate("rc_ownership_trusted_implementation"))) void test3_with_trusted_implementation_annotate_attribute() {
+ foo_bar();
+}
+
//===--===//
// Test returning retained and not-retained values.
//===--===//
@@ -343,6 +379,14 @@
CFRelease(str);
}
+__attribute__((annotate("rc_ownership_trusted_implementation"))) void test_test_return_inline_2_with_trusted_implementation_annotate_attribute(char *bytes) {
+ CFStringRef str = CFStringCreateWithCStringNoCopy(0, bytes, NSNEXTSTEPStringEncoding, 0); // no-warning {{Ideally, a "leak" warning should've been raised here. But, as this function contains "rc_ownership_trusted_implementation" annotate attribute, the warning is suppressed}}
+ // After this call, 'str' really has +2 reference count.
+ CFStringRef str2 = test_return_inline(str);
+ // After this call, 'str' really has a +1 reference count.
+ CFRelease(str);
+}
+
extern CFStringRef getString(void);
CFStringRef testCovariantReturnType(void) __attribute__((cf_returns_retained));
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
@@ -1894,6 +1894,23 @@
return SFC->getAnalysisDeclContext()->isBodyAutosynthesized();
}
+/// Returns true if any declaration on the call-stack contains
+/// 'rc_ownership_trusted_implementation' annotate attribute.
+bool isTrustedReferenceCountImplementation(const LocationContext *LCtx) {
+ while (LCtx) {
+if (const StackFrameContext *SFC = dyn_cast(LCtx)) {
+ const Decl *D = SFC->getDecl();
+ for (const auto *Ann : D->specific_attrs()) {
+if (Ann->getAnnotation() == "rc_ownership_trusted_implementation") {
+ return true;
+}
+ }
+}
+LCtx = LCtx->getParent();
+ }
+ return false;
+}
+
std::shared_ptr
CFRefReportVisitor::VisitNode(const ExplodedNode *N, const ExplodedNode *PrevN,
BugReporterContext &BRC, BugReport &BR) {
@@ -3345,11 +3362,13 @@
}
assert(BT);
- auto report = std::unique_ptr(
- new CFRefReport(*BT, C.getASTContext().getLangOpts(), C.isObjCGCEnabled(),
- SummaryLog, N, Sym));
- report->addRange(ErrorRange);
- C.emitReport(std::move(report));
+ if (!isTrustedReferenceCountImplementation(N->getLocationContext())) {
+auto report = std::unique_ptr(
+new CFRefReport(*BT, C.getASTContext().getLangOpts(),
+C.isObjCGCEnabled(), SummaryLog, N, Sym));
+report->addRange(ErrorRange);
+C.emitReport(std::move(report));
+ }
}
//===--
[PATCH] D34937: Suppressing Reference Counting Diagnostics for Functions Containing 'rc_ownership_trusted_implementation' Annotate Attribute
malhar1995 created this revision.
As part of my Google Summer of Code project, I am working on adding support for
Integer Set Library (ISL) annotations to the current RetainCountChecker.
Hence, to begin with, Dr. Devin Coughlin gave me a task to suppress reference
counting diagnostics for all ISL functions by preceding them with some annotate
attribute.
The attached diff aims to do that by not emitting reports if any function on
the call stack has 'rc_ownership_trusted_implementation' annotate attribute.
Note about ISL:
ISL has annotations __isl_give and __isl_take which are analogous to
cf_returns_retained and cf_consumed but in case of ISL, annotations precede
datatypes of function parameters.
Let me know your thoughts on the same.
Repository:
rL LLVM
https://reviews.llvm.org/D34937
Files:
lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
@@ -1894,6 +1894,22 @@
return SFC->getAnalysisDeclContext()->isBodyAutosynthesized();
}
+bool
+isAnnotatedToSkipDiagnostics(const LocationContext *LCtx) {
+ while (LCtx) {
+if (const StackFrameContext *SFC = dyn_cast(LCtx)) {
+ const Decl *D = SFC->getDecl();
+ for (const auto *Ann : D->specific_attrs()){
+if (Ann->getAnnotation() == "rc_ownership_trusted_implementation") {
+ return true;
+}
+ }
+}
+LCtx = LCtx->getParent();
+ }
+ return false;
+}
+
std::shared_ptr
CFRefReportVisitor::VisitNode(const ExplodedNode *N, const ExplodedNode *PrevN,
BugReporterContext &BRC, BugReport &BR) {
@@ -3345,11 +3361,13 @@
}
assert(BT);
- auto report = std::unique_ptr(
- new CFRefReport(*BT, C.getASTContext().getLangOpts(), C.isObjCGCEnabled(),
- SummaryLog, N, Sym));
- report->addRange(ErrorRange);
- C.emitReport(std::move(report));
+ if (!isAnnotatedToSkipDiagnostics(N->getLocationContext())){
+auto report = std::unique_ptr(
+new CFRefReport(*BT, C.getASTContext().getLangOpts(), C.isObjCGCEnabled(),
+SummaryLog, N, Sym));
+report->addRange(ErrorRange);
+C.emitReport(std::move(report));
+ }
}
//===--===//
@@ -3579,9 +3597,10 @@
if (N) {
const LangOptions &LOpts = C.getASTContext().getLangOpts();
bool GCEnabled = C.isObjCGCEnabled();
- C.emitReport(std::unique_ptr(new CFRefLeakReport(
- *getLeakAtReturnBug(LOpts, GCEnabled), LOpts, GCEnabled,
- SummaryLog, N, Sym, C, IncludeAllocationLine)));
+ if (!isAnnotatedToSkipDiagnostics(N->getLocationContext()))
+C.emitReport(std::unique_ptr(new CFRefLeakReport(
+*getLeakAtReturnBug(LOpts, GCEnabled), LOpts, GCEnabled,
+SummaryLog, N, Sym, C, IncludeAllocationLine)));
}
}
}
@@ -3606,9 +3625,10 @@
if (!returnNotOwnedForOwned)
returnNotOwnedForOwned.reset(new ReturnedNotOwnedForOwned(this));
- C.emitReport(std::unique_ptr(new CFRefReport(
- *returnNotOwnedForOwned, C.getASTContext().getLangOpts(),
- C.isObjCGCEnabled(), SummaryLog, N, Sym)));
+ if (!isAnnotatedToSkipDiagnostics(N->getLocationContext()))
+C.emitReport(std::unique_ptr(new CFRefReport(
+*returnNotOwnedForOwned, C.getASTContext().getLangOpts(),
+C.isObjCGCEnabled(), SummaryLog, N, Sym)));
}
}
}
@@ -3811,9 +3831,10 @@
overAutorelease.reset(new OverAutorelease(this));
const LangOptions &LOpts = Ctx.getASTContext().getLangOpts();
-Ctx.emitReport(std::unique_ptr(
-new CFRefReport(*overAutorelease, LOpts, /* GCEnabled = */ false,
-SummaryLog, N, Sym, os.str(;
+if (!isAnnotatedToSkipDiagnostics(N->getLocationContext()))
+ Ctx.emitReport(std::unique_ptr(
+ new CFRefReport(*overAutorelease, LOpts, /* GCEnabled = */ false,
+ SummaryLog, N, Sym, os.str(;
}
return nullptr;
@@ -3865,9 +3886,10 @@
: getLeakAtReturnBug(LOpts, GCEnabled);
assert(BT && "BugType not initialized.");
- Ctx.emitReport(std::unique_ptr(
- new CFRefLeakReport(*BT, LOpts, GCEnabled, SummaryLog, N, *I, Ctx,
- IncludeAllocationLine)));
+ if (!isAnnotatedToSkipDiagnostics(N->getLocationContext()))
+Ctx.emitReport(std::unique_ptr(
+new CFRefLeakReport(*BT, LOpts, GCEnabled, SummaryLog, N, *I, Ctx,
+IncludeAllocationLine)));
}
}
___
cfe-commits mailing list
cfe-commits@l
[PATCH] D34937: Suppressing Reference Counting Diagnostics for Functions Containing 'rc_ownership_trusted_implementation' Annotate Attribute
malhar1995 updated this revision to Diff 105043.
Repository:
rL LLVM
https://reviews.llvm.org/D34937
Files:
lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
@@ -1894,6 +1894,22 @@
return SFC->getAnalysisDeclContext()->isBodyAutosynthesized();
}
+bool
+isAnnotatedToSkipDiagnostics(const LocationContext *LCtx) {
+ while (LCtx) {
+if (const StackFrameContext *SFC = dyn_cast(LCtx)) {
+ const Decl *D = SFC->getDecl();
+ for (const auto *Ann : D->specific_attrs()){
+if (Ann->getAnnotation() == "rc_ownership_trusted_implementation") {
+ return true;
+}
+ }
+}
+LCtx = LCtx->getParent();
+ }
+ return false;
+}
+
std::shared_ptr
CFRefReportVisitor::VisitNode(const ExplodedNode *N, const ExplodedNode *PrevN,
BugReporterContext &BRC, BugReport &BR) {
@@ -3345,11 +3361,13 @@
}
assert(BT);
- auto report = std::unique_ptr(
- new CFRefReport(*BT, C.getASTContext().getLangOpts(), C.isObjCGCEnabled(),
- SummaryLog, N, Sym));
- report->addRange(ErrorRange);
- C.emitReport(std::move(report));
+ if (!isAnnotatedToSkipDiagnostics(N->getLocationContext())){
+auto report = std::unique_ptr(
+new CFRefReport(*BT, C.getASTContext().getLangOpts(), C.isObjCGCEnabled(),
+SummaryLog, N, Sym));
+report->addRange(ErrorRange);
+C.emitReport(std::move(report));
+ }
}
//===--===//
@@ -3579,9 +3597,10 @@
if (N) {
const LangOptions &LOpts = C.getASTContext().getLangOpts();
bool GCEnabled = C.isObjCGCEnabled();
- C.emitReport(std::unique_ptr(new CFRefLeakReport(
- *getLeakAtReturnBug(LOpts, GCEnabled), LOpts, GCEnabled,
- SummaryLog, N, Sym, C, IncludeAllocationLine)));
+ if (!isAnnotatedToSkipDiagnostics(N->getLocationContext()))
+C.emitReport(std::unique_ptr(new CFRefLeakReport(
+*getLeakAtReturnBug(LOpts, GCEnabled), LOpts, GCEnabled,
+SummaryLog, N, Sym, C, IncludeAllocationLine)));
}
}
}
@@ -3606,9 +3625,10 @@
if (!returnNotOwnedForOwned)
returnNotOwnedForOwned.reset(new ReturnedNotOwnedForOwned(this));
- C.emitReport(std::unique_ptr(new CFRefReport(
- *returnNotOwnedForOwned, C.getASTContext().getLangOpts(),
- C.isObjCGCEnabled(), SummaryLog, N, Sym)));
+ if (!isAnnotatedToSkipDiagnostics(N->getLocationContext()))
+C.emitReport(std::unique_ptr(new CFRefReport(
+*returnNotOwnedForOwned, C.getASTContext().getLangOpts(),
+C.isObjCGCEnabled(), SummaryLog, N, Sym)));
}
}
}
@@ -3811,9 +3831,10 @@
overAutorelease.reset(new OverAutorelease(this));
const LangOptions &LOpts = Ctx.getASTContext().getLangOpts();
-Ctx.emitReport(std::unique_ptr(
-new CFRefReport(*overAutorelease, LOpts, /* GCEnabled = */ false,
-SummaryLog, N, Sym, os.str(;
+if (!isAnnotatedToSkipDiagnostics(N->getLocationContext()))
+ Ctx.emitReport(std::unique_ptr(
+ new CFRefReport(*overAutorelease, LOpts, /* GCEnabled = */ false,
+ SummaryLog, N, Sym, os.str(;
}
return nullptr;
@@ -3865,9 +3886,10 @@
: getLeakAtReturnBug(LOpts, GCEnabled);
assert(BT && "BugType not initialized.");
- Ctx.emitReport(std::unique_ptr(
- new CFRefLeakReport(*BT, LOpts, GCEnabled, SummaryLog, N, *I, Ctx,
- IncludeAllocationLine)));
+ if (!isAnnotatedToSkipDiagnostics(N->getLocationContext()))
+Ctx.emitReport(std::unique_ptr(
+new CFRefLeakReport(*BT, LOpts, GCEnabled, SummaryLog, N, *I, Ctx,
+IncludeAllocationLine)));
}
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32449: Modifying PthreadLockChecker.cpp to reduce false positives.
malhar1995 updated this revision to Diff 99970.
malhar1995 added a comment.
Applied clang-format only to the changed lines in the final code.
Repository:
rL LLVM
https://reviews.llvm.org/D32449
Files:
.gitignore
lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
test/Analysis/pthreadlock.c
Index: test/Analysis/pthreadlock.c
===
--- test/Analysis/pthreadlock.c
+++ test/Analysis/pthreadlock.c
@@ -176,6 +176,42 @@
pthread_mutex_unlock(pmtx); // no-warning
}
+void ok23(void) {
+ if (pthread_mutex_destroy(&mtx1) != 0) // no-warning
+pthread_mutex_destroy(&mtx1);// no-warning
+}
+
+void ok24(void) {
+ if (pthread_mutex_destroy(&mtx1) != 0) // no-warning
+pthread_mutex_lock(&mtx1); // no-warning
+}
+
+void ok25(void) {
+ if (pthread_mutex_destroy(&mtx1) != 0) // no-warning
+pthread_mutex_unlock(&mtx1); // no-warning
+}
+
+void ok26(void) {
+ pthread_mutex_unlock(&mtx1); // no-warning
+ if (pthread_mutex_destroy(&mtx1) != 0) // no-warning
+pthread_mutex_lock(&mtx1); // no-warning
+}
+
+void ok27(void) {
+ pthread_mutex_unlock(&mtx1); // no-warning
+ if (pthread_mutex_destroy(&mtx1) != 0) // no-warning
+pthread_mutex_lock(&mtx1); // no-warning
+ else
+pthread_mutex_init(&mtx1, NULL); // no-warning
+}
+
+void ok28(void) {
+ if (pthread_mutex_destroy(&mtx1) != 0) { // no-warning
+pthread_mutex_lock(&mtx1); // no-warning
+pthread_mutex_unlock(&mtx1); // no-warning
+pthread_mutex_destroy(&mtx1); // no-warning
+ }
+}
void
bad1(void)
@@ -392,3 +428,46 @@
pthread_mutex_unlock(&mtx1); // no-warning
pthread_mutex_init(&mtx1, NULL); // expected-warning{{This lock has already been initialized}}
}
+
+void bad27(void) {
+ pthread_mutex_unlock(&mtx1);// no-warning
+ int ret = pthread_mutex_destroy(&mtx1); // no-warning
+ if (ret != 0) // no-warning
+pthread_mutex_lock(&mtx1);// no-warning
+ else
+pthread_mutex_unlock(&mtx1); // expected-warning{{This lock has already been destroyed}}
+}
+
+void bad28(void) {
+ pthread_mutex_unlock(&mtx1);// no-warning
+ int ret = pthread_mutex_destroy(&mtx1); // no-warning
+ if (ret != 0) // no-warning
+pthread_mutex_lock(&mtx1);// no-warning
+ else
+pthread_mutex_lock(&mtx1); // expected-warning{{This lock has already been destroyed}}
+}
+
+void bad29(void) {
+ pthread_mutex_lock(&mtx1); // no-warning
+ pthread_mutex_unlock(&mtx1); // no-warning
+ if (pthread_mutex_destroy(&mtx1) != 0) // no-warning
+pthread_mutex_init(&mtx1, NULL); // expected-warning{{This lock has already been initialized}}
+ else
+pthread_mutex_init(&mtx1, NULL); // no-warning
+}
+
+void bad30(void) {
+ pthread_mutex_lock(&mtx1); // no-warning
+ pthread_mutex_unlock(&mtx1); // no-warning
+ if (pthread_mutex_destroy(&mtx1) != 0) // no-warning
+pthread_mutex_init(&mtx1, NULL); // expected-warning{{This lock has already been initialized}}
+ else
+pthread_mutex_destroy(&mtx1); // expected-warning{{This lock has already been destroyed}}
+}
+
+void bad31(void) {
+ int ret = pthread_mutex_destroy(&mtx1); // no-warning
+ pthread_mutex_lock(&mtx1); // expected-warning{{This lock has already been destroyed}}
+ if (ret != 0)
+pthread_mutex_lock(&mtx1);
+}
Index: lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
+++ lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
@@ -25,30 +25,49 @@
namespace {
struct LockState {
- enum Kind { Destroyed, Locked, Unlocked } K;
+ enum Kind {
+Destroyed,
+Locked,
+Unlocked,
+UntouchedAndPossiblyDestroyed,
+UnlockedAndPossiblyDestroyed
+ } K;
private:
LockState(Kind K) : K(K) {}
public:
static LockState getLocked() { return LockState(Locked); }
static LockState getUnlocked() { return LockState(Unlocked); }
static LockState getDestroyed() { return LockState(Destroyed); }
+ static LockState getUntouchedAndPossiblyDestroyed() {
+return LockState(UntouchedAndPossiblyDestroyed);
+ }
+ static LockState getUnlockedAndPossiblyDestroyed() {
+return LockState(UnlockedAndPossiblyDestroyed);
+ }
bool operator==(const LockState &X) const {
return K == X.K;
}
bool isLocked() const { return K == Locked; }
bool isUnlocked() const { return K == Unlocked; }
bool isDestroyed() const { return K == Destroyed; }
+ bool isUntouchedAndPossiblyDestroyed() const {
+return K == UntouchedAndPossiblyDestroyed;
+ }
+ bool isUnlockedAndPossiblyDestroyed() const {
+return K == UnlockedAndPossiblyDestroyed;
+ }
void Profile(llvm::FoldingSetNodeID &ID) const {
ID.A
[PATCH] D32449: Modifying PthreadLockChecker.cpp to reduce false positives.
malhar1995 added a comment. In https://reviews.llvm.org/D32449#761303, @NoQ wrote: > Thanks, this is great! Two more things: > > - You have touched other code, unrelated to your patch, with clang-format; > we're usually trying to avoid that, because it creates merge conflicts out of > nowhere, and because some of that code actually seems formatted by hand > intentionally. It's better to revert these changes; you can use the `git > clang-format` thing to format only actual changes. I did not apply clang-format to any file except for PthreadLockChecker.cpp. Do you think the merge conflict is due to me not applying clang-format to test/Analysis/pthreadlock.c? The only files I changed were .gitignore, PthreadLockChecker.cpp and test/Analysis/pthreadlock.c. Also, when you asked me to revert the changes, did you mean revert the changes made by clang-format? If yes, how do I do that? I apologize for asking such silly questions. The thing is I'm new to all this and I don't really know how to proceed further. > > > - Updating .gitignore sounds like the right thing to do (llvm's .gitignore > already has this), but i guess we'd better make a separate commit for that. Repository: rL LLVM https://reviews.llvm.org/D32449 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32449: Modifying PthreadLockChecker.cpp to reduce false positives.
malhar1995 updated this revision to Diff 99750.
malhar1995 added a comment.
Addressed previous comments (removed Schrodinger from lock state names, changed
method name setAppropriateLockState to resolvePossiblyDestroyedMutex, added an
assert in resolvePossiblyDestroyedMutex, formatted the code using clang-format
and added some test-cases to test/Analysis/pthreadlock.c)
Repository:
rL LLVM
https://reviews.llvm.org/D32449
Files:
.gitignore
lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
test/Analysis/pthreadlock.c
Index: test/Analysis/pthreadlock.c
===
--- test/Analysis/pthreadlock.c
+++ test/Analysis/pthreadlock.c
@@ -176,6 +176,49 @@
pthread_mutex_unlock(pmtx); // no-warning
}
+void
+ok23(void) {
+ if(pthread_mutex_destroy(&mtx1) != 0) // no-warning
+ pthread_mutex_destroy(&mtx1); // no-warning
+}
+
+void
+ok24(void) {
+ if(pthread_mutex_destroy(&mtx1) != 0) // no-warning
+ pthread_mutex_lock(&mtx1); // no-warning
+}
+
+void
+ok25(void) {
+ if(pthread_mutex_destroy(&mtx1) != 0) // no-warning
+ pthread_mutex_unlock(&mtx1); // no-warning
+}
+
+void
+ok26(void) {
+ pthread_mutex_unlock(&mtx1); // no-warning
+ if(pthread_mutex_destroy(&mtx1) != 0) // no-warning
+ pthread_mutex_lock(&mtx1); // no-warning
+}
+
+void
+ok27(void) {
+ pthread_mutex_unlock(&mtx1); // no-warning
+ if(pthread_mutex_destroy(&mtx1) != 0) // no-warning
+ pthread_mutex_lock(&mtx1); // no-warning
+ else
+ pthread_mutex_init(&mtx1, NULL); // no-warning
+}
+
+void
+ok28() {
+ if(pthread_mutex_destroy(&mtx1)!=0) { // no-warning
+ pthread_mutex_lock(&mtx1); // no-warning
+ pthread_mutex_unlock(&mtx1); // no-warning
+ pthread_mutex_destroy(&mtx1); // no-warning
+ }
+}
+
void
bad1(void)
@@ -392,3 +435,56 @@
pthread_mutex_unlock(&mtx1); // no-warning
pthread_mutex_init(&mtx1, NULL); // expected-warning{{This lock has already been initialized}}
}
+
+void
+bad27(void)
+{
+ pthread_mutex_unlock(&mtx1); // no-warning
+ int ret = pthread_mutex_destroy(&mtx1); // no-warning
+ if(ret != 0) // no-warning
+ pthread_mutex_lock(&mtx1); // no-warning
+ else
+ pthread_mutex_unlock(&mtx1); // expected-warning{{This lock has already been destroyed}}
+}
+
+void
+bad28(void)
+{
+ pthread_mutex_unlock(&mtx1); // no-warning
+ int ret = pthread_mutex_destroy(&mtx1); // no-warning
+ if(ret != 0) // no-warning
+ pthread_mutex_lock(&mtx1); // no-warning
+ else
+ pthread_mutex_lock(&mtx1); // expected-warning{{This lock has already been destroyed}}
+}
+
+void
+bad29()
+{
+ pthread_mutex_lock(&mtx1); // no-warning
+ pthread_mutex_unlock(&mtx1); // no-warning
+ if(pthread_mutex_destroy(&mtx1) != 0) // no-warning
+ pthread_mutex_init(&mtx1, NULL); // expected-warning{{This lock has already been initialized}}
+ else
+ pthread_mutex_init(&mtx1, NULL); // no-warning
+}
+
+void
+bad30()
+{
+ pthread_mutex_lock(&mtx1); // no-warning
+ pthread_mutex_unlock(&mtx1); // no-warning
+ if(pthread_mutex_destroy(&mtx1) != 0) // no-warning
+ pthread_mutex_init(&mtx1, NULL); // expected-warning{{This lock has already been initialized}}
+ else
+ pthread_mutex_destroy(&mtx1); // expected-warning{{This lock has already been destroyed}}
+}
+
+void
+bad31()
+{
+ int ret = pthread_mutex_destroy(&mtx1); // no-warning
+ pthread_mutex_lock(&mtx1); // expected-warning{{This lock has already been destroyed}}
+ if(ret != 0)
+ pthread_mutex_lock(&mtx1);
+}
Index: lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
+++ lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
@@ -25,58 +25,79 @@
namespace {
struct LockState {
- enum Kind { Destroyed, Locked, Unlocked } K;
+ enum Kind {
+Destroyed,
+Locked,
+Unlocked,
+UntouchedAndPossiblyDestroyed,
+UnlockedAndPossiblyDestroyed
+ } K;
private:
LockState(Kind K) : K(K) {}
public:
static LockState getLocked() { return LockState(Locked); }
static LockState getUnlocked() { return LockState(Unlocked); }
static LockState getDestroyed() { return LockState(Destroyed); }
-
- bool operator==(const LockState &X) const {
-return K == X.K;
+ static LockState getUntouchedAndPossiblyDestroyed() {
+return LockState(UntouchedAndPossiblyDestroyed);
+ }
+ static LockState getUnlockedAndPossiblyDestroyed() {
+return LockState(UnlockedAndPossiblyDestroyed);
}
+ bool operator==(const LockState &X) const { return K == X.K; }
+
bool isLocked() const { return K == Locked; }
bool isUnlocked() const { return K == Unlocked; }
bool isDestroyed() const { return K == Destroyed; }
-
- void Profile(llvm::FoldingSetNodeID &ID) const {
-ID.AddInteger(K);
+ bool isUntouchedAndPossiblyDestroyed() const {
+return K == UntouchedAndPossiblyDestroyed;
}
+ bool isUnlockedAndPossiblyDestroyed() const {
+return K == UnlockedAndPossiblyDestroyed;
+ }
+
+ void Profile(llvm
[PATCH] D32449: Modifying PthreadLockChecker.cpp to reduce false positives.
malhar1995 added inline comments. Comment at: lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp:143-146 +if(lstate->isSchrodingerUntouched()) + state = state->remove(lockR); +else if(lstate->isSchrodingerUnlocked()) + state = state->set(lockR, LockState::getUnlocked()); NoQ wrote: > I think we can be certain that the lock is in one of these states, and assert > that. We can be certain that the lock state will be either of the two only if I add the following statement before returning from this function. ``` state = state->remove(lockR); ``` If I don't add the above statement, a return value symbol for the region specified by lockR will still be in DestroyRetVal and it may have an actual lock state (locked, unlocked or destroyed). Repository: rL LLVM https://reviews.llvm.org/D32449 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32449: Modifying PthreadLockChecker.cpp to reduce false positives.
malhar1995 marked 2 inline comments as done. malhar1995 added a comment. In https://reviews.llvm.org/D32449#760141, @NoQ wrote: > Thanks! Your code looks very clear now, and it seems correct to me. > > One last thing we definitely should do here would be add regression tests for > the new functionality. I guess you already have your tests, otherwise you > wouldn't know if your code works, so you'd just need to append them to the > patch, somewhere at the bottom of `test/Analysis/pthreadlock.c`, and make > sure that `make -j4 check-clang-analysis` passes. Ideally, we should cover as > many branches as possible. > > A few ideas of what to test (you might have thought about most of them > already, and i expect them to actually work by just looking at what your code > accomplishes): > > - What we can/cannot do with the mutex in the failed-to-be-destroyed state, > depending on the state of the mutex before destruction was attempted. > - What we can/cannot do with the mutex in each of the "Schrodinger" states - > in particular, do we display the double-destroy warning in such cases? > - How return-symbol death races against resolving success of the destroy > operation: what if the programmer first tries to destroy mutex, then uses the > mutex, then checks the return value? > - Are you sure we cannot `assert(lstate)` on line 137? - a test could be > added that would cause such assertion to fail if someone tries to impose it. > > Apart from that, i guess it'd be good to use more informative variable > names in a few places (see inline comments), and fix the formatting, i.e. > spaces and line breaks (should be easy with `clang-format`). Also you > shouldn't add the `.DS_Store` files :) And then we'd accept and commit this > patch. Dear Dr. Artem, Thank you so much for such a detailed review. I'll work on addressing these comments ASAP and reach out to you in case I have any queries. Regards, Malhar Thakkar Repository: rL LLVM https://reviews.llvm.org/D32449 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32449: Modifying PthreadLockChecker.cpp to reduce false positives.
malhar1995 updated this revision to Diff 99388.
malhar1995 marked an inline comment as done.
malhar1995 added a comment.
Cleaned up the previous patch.
Added checking of LockState before initializing a mutex as well.
Added separate branches of execution for PthreadSemantics and XNUSemantics.
Added assert in case of checkDeadSymbols as existence in DestroyRetVal ensures
existence in LockMap.
Repository:
rL LLVM
https://reviews.llvm.org/D32449
Files:
.DS_Store
lib/.DS_Store
lib/StaticAnalyzer/.DS_Store
lib/StaticAnalyzer/Checkers/.DS_Store
lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
test/.DS_Store
test/Analysis/.DS_Store
Index: lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
+++ lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
@@ -25,30 +25,34 @@
namespace {
struct LockState {
- enum Kind { Destroyed, Locked, Unlocked } K;
+ enum Kind { Destroyed, Locked, Unlocked, SchrodingerUntouched, SchrodingerUnlocked } K;
private:
LockState(Kind K) : K(K) {}
public:
static LockState getLocked() { return LockState(Locked); }
static LockState getUnlocked() { return LockState(Unlocked); }
static LockState getDestroyed() { return LockState(Destroyed); }
+ static LockState getSchrodingerUntouched() { return LockState(SchrodingerUntouched); }
+ static LockState getSchrodingerUnlocked() { return LockState(SchrodingerUnlocked); }
bool operator==(const LockState &X) const {
return K == X.K;
}
bool isLocked() const { return K == Locked; }
bool isUnlocked() const { return K == Unlocked; }
bool isDestroyed() const { return K == Destroyed; }
+ bool isSchrodingerUntouched() const { return K == SchrodingerUntouched; }
+ bool isSchrodingerUnlocked() const { return K == SchrodingerUnlocked; }
void Profile(llvm::FoldingSetNodeID &ID) const {
ID.AddInteger(K);
}
};
-class PthreadLockChecker : public Checker< check::PostStmt > {
+class PthreadLockChecker : public Checker< check::PostStmt, check::DeadSymbols > {
mutable std::unique_ptr BT_doublelock;
mutable std::unique_ptr BT_doubleunlock;
mutable std::unique_ptr BT_destroylock;
@@ -61,21 +65,24 @@
};
public:
void checkPostStmt(const CallExpr *CE, CheckerContext &C) const;
+ void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
void AcquireLock(CheckerContext &C, const CallExpr *CE, SVal lock,
bool isTryLock, enum LockingSemantics semantics) const;
void ReleaseLock(CheckerContext &C, const CallExpr *CE, SVal lock) const;
- void DestroyLock(CheckerContext &C, const CallExpr *CE, SVal Lock) const;
+ void DestroyLock(CheckerContext &C, const CallExpr *CE, SVal Lock, enum LockingSemantics semantics) const;
void InitLock(CheckerContext &C, const CallExpr *CE, SVal Lock) const;
void reportUseDestroyedBug(CheckerContext &C, const CallExpr *CE) const;
+ ProgramStateRef setAppropriateLockState(ProgramStateRef state, const MemRegion* lockR, const SymbolRef* sym, bool fromCheckDeadSymbols) const;
};
} // end anonymous namespace
// GDM Entry for tracking lock state.
REGISTER_LIST_WITH_PROGRAMSTATE(LockSet, const MemRegion *)
REGISTER_MAP_WITH_PROGRAMSTATE(LockMap, const MemRegion *, LockState)
+REGISTER_MAP_WITH_PROGRAMSTATE(DestroyRetVal, const MemRegion *, SymbolRef)
void PthreadLockChecker::checkPostStmt(const CallExpr *CE,
CheckerContext &C) const {
@@ -113,22 +120,50 @@
FName == "lck_mtx_unlock" ||
FName == "lck_rw_done")
ReleaseLock(C, CE, state->getSVal(CE->getArg(0), LCtx));
- else if (FName == "pthread_mutex_destroy" ||
- FName == "lck_mtx_destroy")
-DestroyLock(C, CE, state->getSVal(CE->getArg(0), LCtx));
+ else if (FName == "pthread_mutex_destroy")
+DestroyLock(C, CE, state->getSVal(CE->getArg(0), LCtx), PthreadSemantics);
+ else if (FName == "lck_mtx_destroy")
+DestroyLock(C, CE, state->getSVal(CE->getArg(0), LCtx), XNUSemantics);
else if (FName == "pthread_mutex_init")
InitLock(C, CE, state->getSVal(CE->getArg(0), LCtx));
}
+ProgramStateRef PthreadLockChecker::setAppropriateLockState(ProgramStateRef state, const MemRegion* lockR, const SymbolRef* sym, bool fromCheckDeadSymbols) const {
+ const LockState* lstate = state->get(lockR);
+ // Existence in DestroyRetVal ensures existence in LockMap.
+ if(fromCheckDeadSymbols)
+assert(lstate);
+ else{
+if(!lstate)
+ return state;
+ }
+ ConstraintManager &CMgr = state->getConstraintManager();
+ ConditionTruthVal retZero = CMgr.isNull(state, *sym);
+ if(retZero.isConstrainedFalse()){
+if(lstate->isSchrodingerUntouched())
+ state = state->remove(lockR);
+else if(lstate->isSchrodingerUnlocked())
+ state = state->set(lockR, LockState::getUnlocked());
+ }
+ else{
+if(lstate->isSchrodingerUntouch
[PATCH] D32449: Modifying PthreadLockChecker.cpp to reduce false positives.
malhar1995 updated this revision to Diff 99179.
malhar1995 added a comment.
Added context.
Also, I removed the inclusion of iostream and also added the repetitive code to
the function setAppropriateLockState.
Currently working on finding various corner cases and invariants.
Repository:
rL LLVM
https://reviews.llvm.org/D32449
Files:
.DS_Store
lib/.DS_Store
lib/StaticAnalyzer/.DS_Store
lib/StaticAnalyzer/Checkers/.DS_Store
lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
Index: lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
+++ lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
@@ -25,30 +25,34 @@
namespace {
struct LockState {
- enum Kind { Destroyed, Locked, Unlocked } K;
+ enum Kind { Destroyed, Locked, Unlocked, SchrodingerLocked, SchrodingerUnlocked } K;
private:
LockState(Kind K) : K(K) {}
public:
static LockState getLocked() { return LockState(Locked); }
static LockState getUnlocked() { return LockState(Unlocked); }
static LockState getDestroyed() { return LockState(Destroyed); }
+ static LockState getSchrodingerLocked() { return LockState(SchrodingerLocked); }
+ static LockState getSchrodingerUnlocked() { return LockState(SchrodingerUnlocked); }
bool operator==(const LockState &X) const {
return K == X.K;
}
bool isLocked() const { return K == Locked; }
bool isUnlocked() const { return K == Unlocked; }
bool isDestroyed() const { return K == Destroyed; }
+ bool isSchrodingerLocked() const { return K == SchrodingerLocked; }
+ bool isSchrodingerUnlocked() const { return K == SchrodingerUnlocked; }
void Profile(llvm::FoldingSetNodeID &ID) const {
ID.AddInteger(K);
}
};
-class PthreadLockChecker : public Checker< check::PostStmt > {
+class PthreadLockChecker : public Checker< check::PostStmt, check::DeadSymbols > {
mutable std::unique_ptr BT_doublelock;
mutable std::unique_ptr BT_doubleunlock;
mutable std::unique_ptr BT_destroylock;
@@ -61,21 +65,24 @@
};
public:
void checkPostStmt(const CallExpr *CE, CheckerContext &C) const;
+ void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
void AcquireLock(CheckerContext &C, const CallExpr *CE, SVal lock,
bool isTryLock, enum LockingSemantics semantics) const;
void ReleaseLock(CheckerContext &C, const CallExpr *CE, SVal lock) const;
void DestroyLock(CheckerContext &C, const CallExpr *CE, SVal Lock) const;
void InitLock(CheckerContext &C, const CallExpr *CE, SVal Lock) const;
void reportUseDestroyedBug(CheckerContext &C, const CallExpr *CE) const;
+ ProgramStateRef setAppropriateLockState(ProgramStateRef state, const MemRegion* lockR) const;
};
} // end anonymous namespace
// GDM Entry for tracking lock state.
REGISTER_LIST_WITH_PROGRAMSTATE(LockSet, const MemRegion *)
REGISTER_MAP_WITH_PROGRAMSTATE(LockMap, const MemRegion *, LockState)
+REGISTER_MAP_WITH_PROGRAMSTATE(DestroyRetVal, const MemRegion *, SymbolRef)
void PthreadLockChecker::checkPostStmt(const CallExpr *CE,
CheckerContext &C) const {
@@ -120,16 +127,40 @@
InitLock(C, CE, state->getSVal(CE->getArg(0), LCtx));
}
+ProgramStateRef PthreadLockChecker::setAppropriateLockState(ProgramStateRef state, const MemRegion* lockR) const {
+ const SymbolRef* sym = state->get(lockR);
+ if(sym){
+const LockState* lstate = state->get(lockR);
+if(lstate){
+ ConstraintManager &CMgr = state->getConstraintManager();
+ ConditionTruthVal retZero = CMgr.isNull(state, *sym);
+ if(retZero.isConstrainedFalse()){
+if(lstate->isSchrodingerLocked())
+ state = state->set(lockR, LockState::getLocked());
+else if(lstate->isSchrodingerUnlocked())
+ state = state->set(lockR, LockState::getUnlocked());
+ }
+ else{
+if(!lstate || lstate->isSchrodingerLocked() || lstate->isSchrodingerUnlocked())
+ state = state->set(lockR, LockState::getDestroyed());
+ }
+}
+ }
+ return state;
+}
+
void PthreadLockChecker::AcquireLock(CheckerContext &C, const CallExpr *CE,
SVal lock, bool isTryLock,
enum LockingSemantics semantics) const {
const MemRegion *lockR = lock.getAsRegion();
if (!lockR)
-return;
+return;
ProgramStateRef state = C.getState();
+ state = setAppropriateLockState(state, lockR);
+
SVal X = state->getSVal(CE, C.getLocationContext());
if (X.isUnknownOrUndef())
return;
@@ -198,6 +229,8 @@
ProgramStateRef state = C.getState();
+ state = setAppropriateLockState(state, lockR);
+
if (const LockState *LState = state->get(lockR)) {
if (LState->isUnlocked()) {
if (!BT_doubleunlock)
@@ -253,9 +286,32 @@
ProgramStateRef State = C.getState();
+ State = setAppropriateL
[PATCH] D32449: Modifying PthreadLockChecker.cpp to reduce false positives.
malhar1995 created this revision.
I am currently working on to avoid false positives which currently occur as the
return values of mutex functions like pthread_mutex_destroy() are not taken
into consideration.
The precise description of the bug can be found here:
https://bugs.llvm.org/show_bug.cgi?id=32455
Dr. Devin and Dr. Artem have been guiding me to fix PthreadLockChecker to avoid
such false positives. The patch I'm attaching is not 100% correct and hence I
need your advice to proceed further.
Thank you.
Repository:
rL LLVM
https://reviews.llvm.org/D32449
Files:
lib/.DS_Store
lib/StaticAnalyzer/.DS_Store
lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
Index: lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
+++ lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
@@ -18,6 +18,7 @@
#include "clang/StaticAnalyzer/Core/CheckerManager.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include
using namespace clang;
using namespace ento;
@@ -25,7 +26,7 @@
namespace {
struct LockState {
- enum Kind { Destroyed, Locked, Unlocked } K;
+ enum Kind { Destroyed, Locked, Unlocked, SchrodingerLocked, SchrodingerUnlocked } K;
private:
LockState(Kind K) : K(K) {}
@@ -34,6 +35,8 @@
static LockState getLocked() { return LockState(Locked); }
static LockState getUnlocked() { return LockState(Unlocked); }
static LockState getDestroyed() { return LockState(Destroyed); }
+ static LockState getSchrodingerLocked() { return LockState(SchrodingerLocked); }
+ static LockState getSchrodingerUnlocked() { return LockState(SchrodingerUnlocked); }
bool operator==(const LockState &X) const {
return K == X.K;
@@ -42,13 +45,15 @@
bool isLocked() const { return K == Locked; }
bool isUnlocked() const { return K == Unlocked; }
bool isDestroyed() const { return K == Destroyed; }
+ bool isSchrodingerLocked() const { return K == SchrodingerLocked; }
+ bool isSchrodingerUnlocked() const { return K == SchrodingerUnlocked; }
void Profile(llvm::FoldingSetNodeID &ID) const {
ID.AddInteger(K);
}
};
-class PthreadLockChecker : public Checker< check::PostStmt > {
+class PthreadLockChecker : public Checker< check::PostStmt, check::DeadSymbols > {
mutable std::unique_ptr BT_doublelock;
mutable std::unique_ptr BT_doubleunlock;
mutable std::unique_ptr BT_destroylock;
@@ -61,6 +66,7 @@
};
public:
void checkPostStmt(const CallExpr *CE, CheckerContext &C) const;
+ void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
void AcquireLock(CheckerContext &C, const CallExpr *CE, SVal lock,
bool isTryLock, enum LockingSemantics semantics) const;
@@ -76,6 +82,7 @@
REGISTER_LIST_WITH_PROGRAMSTATE(LockSet, const MemRegion *)
REGISTER_MAP_WITH_PROGRAMSTATE(LockMap, const MemRegion *, LockState)
+REGISTER_MAP_WITH_PROGRAMSTATE(DestroyRetVal, const MemRegion *, SymbolRef)
void PthreadLockChecker::checkPostStmt(const CallExpr *CE,
CheckerContext &C) const {
@@ -126,10 +133,30 @@
const MemRegion *lockR = lock.getAsRegion();
if (!lockR)
-return;
+return;
ProgramStateRef state = C.getState();
+ // CHECK THIS
+ const SymbolRef* sym = state->get(lockR);
+ if(sym){
+const LockState* lstate = state->get(lockR);
+if(lstate){
+ ConstraintManager &CMgr = state->getConstraintManager();
+ ConditionTruthVal retZero = CMgr.isNull(state, *sym);
+ if(retZero.isConstrainedFalse()){
+if(lstate->isSchrodingerLocked())
+ state = state->set(lockR, LockState::getLocked());
+else if(lstate->isSchrodingerUnlocked())
+ state = state->set(lockR, LockState::getUnlocked());
+ }
+ else{
+if(!lstate || lstate->isSchrodingerLocked() || lstate->isSchrodingerUnlocked())
+ state = state->set(lockR, LockState::getDestroyed());
+ }
+}
+ }
+
SVal X = state->getSVal(CE, C.getLocationContext());
if (X.isUnknownOrUndef())
return;
@@ -198,6 +225,26 @@
ProgramStateRef state = C.getState();
+ // CHECK THIS
+ const SymbolRef* sym = state->get(lockR);
+ if(sym){
+const LockState* lstate = state->get(lockR);
+if(lstate){
+ ConstraintManager &CMgr = state->getConstraintManager();
+ ConditionTruthVal retZero = CMgr.isNull(state, *sym);
+ if(retZero.isConstrainedFalse()){
+if(lstate->isSchrodingerLocked())
+ state = state->set(lockR, LockState::getLocked());
+else if(lstate->isSchrodingerUnlocked())
+ state = state->set(lockR, LockState::getUnlocked());
+ }
+ else{
+if(!lstate || lstate->isSchrodingerLocked() || lstate->isSchrodingerUnlocked())
+ state = state->set(lockR, LockState::getDest
