NoQ created this revision.
NoQ added a reviewer: dcoughlin.
Herald added subscribers: cfe-commits, Charusso, dkrupp, donat.nagy, Szelethus,
mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.
Herald added a project: clang.
In the OSObject universe there appears to be another slightly popular contract,
apart from "create" and "get", which is "matching". It optionally consumes a
"table" parameter and if a table is passed, it fills in the table and returns
it at +0; otherwise, it creates a new table, fills it in and returns it at +1.
For now suppress false positives by doing a conservative escape on all
functions that end with "Matching", which is the naming convention that seems
to be followed by all such methods. Let's see if we'll ever get to actually
supporting these semantics.
Repository:
rC Clang
https://reviews.llvm.org/D61161
Files:
clang/lib/Analysis/RetainSummaryManager.cpp
clang/test/Analysis/osobject-retain-release.cpp
Index: clang/test/Analysis/osobject-retain-release.cpp
===================================================================
--- clang/test/Analysis/osobject-retain-release.cpp
+++ clang/test/Analysis/osobject-retain-release.cpp
@@ -679,3 +679,20 @@
obj->release();
obj->release();
}
+
+OSObject *somethingMatching(OSObject *table = 0);
+OSObject *testSuppressionForMethodsEndingWithMatching(OSObject *table = 0) {
+ // This probably just passes table through. We should probably not make
+ // ptr1 definitely equal to table, but we should not warn about leaks.
+ OSObject *ptr1 = somethingMatching(table); // no-warning
+
+ // FIXME: This, however, should follow the Create Rule regardless.
+ // We should warn about the leak here.
+ OSObject *ptr2 = somethingMatching(); // no-warning
+
+ if (!table)
+ table = OSTypeAlloc(OSArray);
+ // This method itself ends with "Matching"! Do not warn when we're
+ // returning from it at +0.
+ return table; // no-warning
+}
Index: clang/lib/Analysis/RetainSummaryManager.cpp
===================================================================
--- clang/lib/Analysis/RetainSummaryManager.cpp
+++ clang/lib/Analysis/RetainSummaryManager.cpp
@@ -228,29 +228,35 @@
const RetainSummary *
RetainSummaryManager::getSummaryForOSObject(const FunctionDecl *FD,
StringRef FName, QualType RetTy) {
+ assert(TrackOSObjects &&
+ "Requesting a summary for an OSObject but OSObjects are not tracked");
+
if (RetTy->isPointerType()) {
const CXXRecordDecl *PD = RetTy->getPointeeType()->getAsCXXRecordDecl();
if (PD && isOSObjectSubclass(PD)) {
- if (const IdentifierInfo *II = FD->getIdentifier()) {
- StringRef FuncName = II->getName();
- if (isOSObjectDynamicCast(FuncName) || isOSObjectThisCast(FuncName))
- return getDefaultSummary();
-
- // All objects returned with functions *not* starting with
- // get, or iterators, are returned at +1.
- if ((!FuncName.startswith("get") && !FuncName.startswith("Get")) ||
- isOSIteratorSubclass(PD)) {
- return getOSSummaryCreateRule(FD);
- } else {
- return getOSSummaryGetRule(FD);
- }
+ if (isOSObjectDynamicCast(FName) || isOSObjectThisCast(FName))
+ return getDefaultSummary();
+ // TODO: Add support for the slightly common *Matching(table) idiom.
+ // Cf. IOService::nameMatching() etc. - these function have an unusual
+ // contract of returning at +0 or +1 depending on their last argument.
+ if (FName.endswith("Matching")) {
+ return getPersistentStopSummary();
+ }
+
+ // All objects returned with functions *not* starting with 'get',
+ // or iterators, are returned at +1.
+ if ((!FName.startswith("get") && !FName.startswith("Get")) ||
+ isOSIteratorSubclass(PD)) {
+ return getOSSummaryCreateRule(FD);
+ } else {
+ return getOSSummaryGetRule(FD);
}
}
}
if (const auto *MD = dyn_cast<CXXMethodDecl>(FD)) {
const CXXRecordDecl *Parent = MD->getParent();
- if (TrackOSObjects && Parent && isOSObjectSubclass(Parent)) {
+ if (Parent && isOSObjectSubclass(Parent)) {
if (FName == "release" || FName == "taggedRelease")
return getOSSummaryReleaseRule(FD);
Index: clang/test/Analysis/osobject-retain-release.cpp
===================================================================
--- clang/test/Analysis/osobject-retain-release.cpp
+++ clang/test/Analysis/osobject-retain-release.cpp
@@ -679,3 +679,20 @@
obj->release();
obj->release();
}
+
+OSObject *somethingMatching(OSObject *table = 0);
+OSObject *testSuppressionForMethodsEndingWithMatching(OSObject *table = 0) {
+ // This probably just passes table through. We should probably not make
+ // ptr1 definitely equal to table, but we should not warn about leaks.
+ OSObject *ptr1 = somethingMatching(table); // no-warning
+
+ // FIXME: This, however, should follow the Create Rule regardless.
+ // We should warn about the leak here.
+ OSObject *ptr2 = somethingMatching(); // no-warning
+
+ if (!table)
+ table = OSTypeAlloc(OSArray);
+ // This method itself ends with "Matching"! Do not warn when we're
+ // returning from it at +0.
+ return table; // no-warning
+}
Index: clang/lib/Analysis/RetainSummaryManager.cpp
===================================================================
--- clang/lib/Analysis/RetainSummaryManager.cpp
+++ clang/lib/Analysis/RetainSummaryManager.cpp
@@ -228,29 +228,35 @@
const RetainSummary *
RetainSummaryManager::getSummaryForOSObject(const FunctionDecl *FD,
StringRef FName, QualType RetTy) {
+ assert(TrackOSObjects &&
+ "Requesting a summary for an OSObject but OSObjects are not tracked");
+
if (RetTy->isPointerType()) {
const CXXRecordDecl *PD = RetTy->getPointeeType()->getAsCXXRecordDecl();
if (PD && isOSObjectSubclass(PD)) {
- if (const IdentifierInfo *II = FD->getIdentifier()) {
- StringRef FuncName = II->getName();
- if (isOSObjectDynamicCast(FuncName) || isOSObjectThisCast(FuncName))
- return getDefaultSummary();
-
- // All objects returned with functions *not* starting with
- // get, or iterators, are returned at +1.
- if ((!FuncName.startswith("get") && !FuncName.startswith("Get")) ||
- isOSIteratorSubclass(PD)) {
- return getOSSummaryCreateRule(FD);
- } else {
- return getOSSummaryGetRule(FD);
- }
+ if (isOSObjectDynamicCast(FName) || isOSObjectThisCast(FName))
+ return getDefaultSummary();
+ // TODO: Add support for the slightly common *Matching(table) idiom.
+ // Cf. IOService::nameMatching() etc. - these function have an unusual
+ // contract of returning at +0 or +1 depending on their last argument.
+ if (FName.endswith("Matching")) {
+ return getPersistentStopSummary();
+ }
+
+ // All objects returned with functions *not* starting with 'get',
+ // or iterators, are returned at +1.
+ if ((!FName.startswith("get") && !FName.startswith("Get")) ||
+ isOSIteratorSubclass(PD)) {
+ return getOSSummaryCreateRule(FD);
+ } else {
+ return getOSSummaryGetRule(FD);
}
}
}
if (const auto *MD = dyn_cast<CXXMethodDecl>(FD)) {
const CXXRecordDecl *Parent = MD->getParent();
- if (TrackOSObjects && Parent && isOSObjectSubclass(Parent)) {
+ if (Parent && isOSObjectSubclass(Parent)) {
if (FName == "release" || FName == "taggedRelease")
return getOSSummaryReleaseRule(FD);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
