[PATCH] D38877: [analyzer] RetainCount: Accept "safe" CFRetain wrappers.

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL315736: [analyzer] RetainCount: Ignore annotations on user-made CFRetain wrappers. (authored by dergachev).

Changed prior to commit:
  https://reviews.llvm.org/D38877?vs=118891&id=118953#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D38877

Files:
  cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
  cfe/trunk/test/Analysis/retain-release-safe.c


Index: cfe/trunk/test/Analysis/retain-release-safe.c
===================================================================
--- cfe/trunk/test/Analysis/retain-release-safe.c
+++ cfe/trunk/test/Analysis/retain-release-safe.c
@@ -0,0 +1,72 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,osx.coreFoundation.CFRetainRelease,osx.cocoa.RetainCount -verify %s
+
+#pragma clang arc_cf_code_audited begin
+typedef const void * CFTypeRef;
+extern CFTypeRef CFRetain(CFTypeRef cf);
+extern void CFRelease(CFTypeRef cf);
+#pragma clang arc_cf_code_audited end
+
+#define CF_RETURNS_RETAINED __attribute__((cf_returns_retained))
+#define CF_CONSUMED __attribute__((cf_consumed))
+
+extern CFTypeRef CFCreate() CF_RETURNS_RETAINED;
+
+// A "safe" variant of CFRetain that doesn't crash when a null pointer is
+// retained. This is often defined by users in a similar manner. The
+// CF_RETURNS_RETAINED annotation is misleading here, because the function
+// is not supposed to return an object with a +1 retain count. Instead, it
+// is supposed to return an object with +(N+1) retain count, where N is
+// the original retain count of 'cf'. However, there is no good annotation
+// to use in this case, and it is pointless to provide such annotation
+// because the only use cases would be CFRetain and SafeCFRetain.
+// So instead we teach the analyzer to be able to accept such code
+// and ignore the misplaced annotation.
+CFTypeRef SafeCFRetain(CFTypeRef cf) CF_RETURNS_RETAINED {
+  if (cf) {
+    return CFRetain(cf);
+  }
+  return cf;
+}
+
+// A "safe" variant of CFRelease that doesn't crash when a null pointer is
+// released. The CF_CONSUMED annotation seems reasonable here.
+void SafeCFRelease(CFTypeRef CF_CONSUMED cf) {
+  if (cf)
+    CFRelease(cf); // no-warning (when inlined)
+}
+
+void escape(CFTypeRef cf);
+
+void makeSureTestsWork() {
+  CFTypeRef cf = CFCreate();
+  CFRelease(cf);
+  CFRelease(cf); // expected-warning{{Reference-counted object is used after it is released}}
+}
+
+// Make sure we understand that the second SafeCFRetain doesn't return an
+// object with +1 retain count, which we won't be able to release twice.
+void falseOverrelease(CFTypeRef cf) {
+  SafeCFRetain(cf);
+  SafeCFRetain(cf);
+  SafeCFRelease(cf);
+  SafeCFRelease(cf); // no-warning after inlining this.
+}
+
+// Regular CFRelease() should behave similarly.
+void sameWithNormalRelease(CFTypeRef cf) {
+  SafeCFRetain(cf);
+  SafeCFRetain(cf);
+  CFRelease(cf);
+  CFRelease(cf); // no-warning
+}
+
+// Make sure we understand that the second SafeCFRetain doesn't return an
+// object with +1 retain count, which would no longer be owned by us after
+// it escapes to escape() and released once.
+void falseReleaseNotOwned(CFTypeRef cf) {
+  SafeCFRetain(cf);
+  SafeCFRetain(cf);
+  escape(cf);
+  SafeCFRelease(cf);
+  SafeCFRelease(cf); // no-warning after inlining this.
+}
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
===================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
@@ -1170,6 +1170,11 @@
       if (cocoa::isRefType(RetTy, "CF", FName)) {
         if (isRetain(FD, FName)) {
           S = getUnarySummary(FT, cfretain);
+          // CFRetain isn't supposed to be annotated. However, this may as well
+          // be a user-made "safe" CFRetain function that is incorrectly
+          // annotated as cf_returns_retained due to lack of better options.
+          // We want to ignore such annotation.
+          AllowAnnotations = false;
         } else if (isAutorelease(FD, FName)) {
           S = getUnarySummary(FT, cfautorelease);
           // The headers use cf_consumed, but we can fully model CFAutorelease


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D38877.118953.patch
Type: text/x-patch
Size: 3803 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20171013/ac68af16/attachment.bin>