[PATCH] D61161: [analyzer] RetainCount: Add a suppression for functions that follow "the Matching rule".

[Artem Dergachev via Phabricator via cfe-commits]

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);
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D61161.196756.patch
Type: text/x-patch
Size: 3350 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190425/96891eff/attachment.bin>