[Analyzer] More granular special casing in RetainCountChecker

[george.karpenkov]

Summary

Take into account return type of the function as well, as otherwise we have unexpected collisions with kernel-land function with the same name.

[dcoughlin]

Looks good to me with the (minor) inline comments addressed.

lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp

1089 ↗

(On Diff #117462)

__CFDictionary is technically an implementation detail. It is probably better to use RetTy.getAsString() == "CFMutableDictionaryRef"

test/Analysis/retain-release.mm

471 ↗

(On Diff #117462)

Please add a // no-warning comment to this line to let future maintainers know that it is specifically not supposed to warn.

It is also probably worth explicitly extending the comment on IOBSDNameMatching() to say that we don't want to warn unless the return type is CFMutableDictionaryRef.

[NoQ]

Yep, looks good.

lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp

1066–1068 ↗

(On Diff #117462)

A bit fancier and a common pattern here:

if (const IdentifierInfo *II = RetTy.getBaseTypeIdentifier()) {
  RetTyName = II->getName();
}

[george.karpenkov]

Adhered to review comments.

[timshen]

Hi,

I see a failure on retain-release.m with this patch. Revert it for now.

• TEST 'Clang :: Analysis/retain-release.m' FAILED ****

Script:

rm -f /usr/local/google/home/timshen/src/llvm-build/tools/clang/test/Analysis/Output/retain-release.m.tmp.objc.plist /usr/local/google/home/timshen/src/llvm-build/tools/clang/test/Analysis/Output/retain-release.m.tmp.objcpp.plist
/usr/local/google/home/timshen/src/llvm-build/bin/clang -cc1 -internal-isystem /usr/local/google/home/timshen/src/llvm-build/lib/clang/6.0.0/include -nostdsysteminc -analyze -analyzer-constraints=range -triple x86_64-apple-darwin10 -analyzer-checker=core,osx.coreFoundation.CFRetainRelease,osx.cocoa.ClassRelease,osx.cocoa.RetainCount -analyzer-store=region -fblocks -verify -Wno-objc-root-class /usr/local/google/home/timshen/src/llvm-project/clang/test/Analysis/retain-release.m -analyzer-output=plist -o /usr/local/google/home/timshen/src/llvm-build/tools/clang/test/Analysis/Output/retain-release.m.tmp.objc.plist

/usr/local/google/home/timshen/src/llvm-build/bin/clang -cc1 -internal-isystem /usr/local/google/home/timshen/src/llvm-build/lib/clang/6.0.0/include -nostdsysteminc -analyze -analyzer-constraints=range -triple x86_64-apple-darwin10 -analyzer-checker=core,osx.coreFoundation.CFRetainRelease,osx.cocoa.ClassRelease,osx.cocoa.RetainCount -analyzer-store=region -fblocks -verify -x objective-c++ -std=gnu++98 -Wno-objc-root-class /usr/local/google/home/timshen/src/llvm-project/clang/test/Analysis/retain-release.m -analyzer-output=plist -o /usr/local/google/home/timshen/src/llvm-build/tools/clang/test/Analysis/Output/retain-release.m.tmp.objcpp.plist

Exit Code: 1

Command Output (stderr):

error: 'warning' diagnostics expected but not seen:

File /usr/local/google/home/timshen/src/llvm-project/clang/test/Analysis/retain-release.m Line 974: leak
File /usr/local/google/home/timshen/src/llvm-project/clang/test/Analysis/retain-release.m Line 978: leak
File /usr/local/google/home/timshen/src/llvm-project/clang/test/Analysis/retain-release.m Line 982: leak
File /usr/local/google/home/timshen/src/llvm-project/clang/test/Analysis/retain-release.m Line 997: leak
File /usr/local/google/home/timshen/src/llvm-project/clang/test/Analysis/retain-release.m Line 1002: leak

5 errors generated.

---

cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp

do {
     StringRef FName = II->getName();
 
     // Strip away preceding '_'.  Doing this here will effect all the checks
     // down below.
     FName = FName.substr(FName.find_first_not_of('_'));
 
     // Inspect the result type.
     QualType RetTy = FT->getReturnType();
+    StringRef RetTyName = RetTy.getAsString();
 
     // FIXME: This should all be refactored into a chain of "summary lookup"
     //  filters.
     assert(ScratchArgs.isEmpty());
 
     if (FName == "pthread_create" || FName == "pthread_setspecific") {
       // Part of: <rdar://problem/7299394> and <rdar://problem/11282706>.
       // This will be addressed better with IPA.
       S = getPersistentStopSummary();
     } else if (FName == "NSMakeCollectable") {
       // Handle: id NSMakeCollectable(CFTypeRef)
       S = (RetTy->isObjCIdType())
           ? getUnarySummary(FT, cfmakecollectable)
           : getPersistentStopSummary();
       // The headers on OS X 10.8 use cf_consumed/ns_returns_retained,
       // but we can fully model NSMakeCollectable ourselves.
       AllowAnnotations = false;
     } else if (FName == "CFPlugInInstanceCreate") {
       S = getPersistentSummary(RetEffect::MakeNoRet());
-    } else if (FName == "IOBSDNameMatching" ||
+    } else if (FName == "IORegistryEntrySearchCFProperty"
+        || (RetTyName == "CFMutableDictionaryRef" && (
+               FName == "IOBSDNameMatching" ||
                FName == "IOServiceMatching" ||
                FName == "IOServiceNameMatching" ||
-               FName == "IORegistryEntrySearchCFProperty" ||
                FName == "IORegistryEntryIDMatching" ||
-               FName == "IOOpenFirmwarePathMatching") {
+               FName == "IOOpenFirmwarePathMatching"
+            ))) {
       // Part of <rdar://problem/6961230>. (IOKit)
       // This should be addressed using a API table.
       S = getPersistentSummary(RetEffect::MakeOwned(RetEffect::CF),
                                DoNothing, DoNothing);
     } else if (FName == "IOServiceGetMatchingService" ||
                FName == "IOServiceGetMatchingServices") {
       // FIXES: <rdar://problem/6326900>
       // This should be addressed using a API table.  This strcmp is also

cfe/trunk/test/Analysis/retain-release.mm

void  radar13722286::PrepareBitmap() {
   if (mStr.get() != 0) {
     Holder<const struct __CFString> str1;
     mStr.reset( CFStringCreateCopy( 0, str1.get() ) ); //expected-warning {{Potential leak of an object}}
   }
 }
 
 // rdar://34210609
 void _() { _(); }; // no-warning
+
+// Do not assume that IOBSDNameMatching increments a reference counter,
+// unless return type is CFMutableDictionaryRef.
+void* IOBSDNameMatching();
+void rdar33832412() {
+  void* x = IOBSDNameMatching(); // no-warning
+}