[PATCH] D78097: [analyzer][RetainCount] Remove the CheckOSObject option

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus created this revision.
Szelethus added reviewers: NoQ, vsavchenko, martong, baloghadamsoftware, xazax.hun, balazske, dcoughlin.
Szelethus added a project: clang.
Herald added subscribers: cfe-commits, ASDenysPetrov, steakhal, Charusso, gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, whisperity.

As per http://lists.llvm.org/pipermail/cfe-dev/2019-August/063215.html, lets get rid of this option.

It presents 2 issues that have bugged me for years now:

- `OSObject` is NOT a `bool`ean option. It in fact has 3 states:
  - `osx.OSObjectRetainCount` is enabled but `OSObject` it set to false: RetainCount regards the option as disabled.
  - `osx.OSObjectRetainCount` is enabled and `OSObject` it set to true: RetainCount regards the option as enabled.
  - `osx.OSObjectRetainCount` is disabled: RetainCount regards the option as disabled.
- The hack involves directly modifying `AnalyzerOptions::ConfigTable`, which shouldn't even be public in the first place.

This still isn't really ideal, because it would be better to preserve the option and remove the checker (we want visible checkers to be associated with diagnostics, and hidden options like this one to be associated with changing how the modeling is done), but backwards compatibility is an issue.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78097

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/test-separate-retaincount.cpp


Index: clang/test/Analysis/test-separate-retaincount.cpp
===================================================================
--- clang/test/Analysis/test-separate-retaincount.cpp
+++ clang/test/Analysis/test-separate-retaincount.cpp
@@ -5,10 +5,6 @@
 // RUN: %clang_analyze_cc1 -std=c++14 -DNO_OS_OBJECT -verify %s \
 // RUN:   -analyzer-checker=core,osx \
 // RUN:   -analyzer-disable-checker osx.OSObjectRetainCount
-//
-// RUN: %clang_analyze_cc1 -std=c++14 -DNO_OS_OBJECT -verify %s \
-// RUN:   -analyzer-checker=core,osx \
-// RUN:   -analyzer-config "osx.cocoa.RetainCount:CheckOSObject=false"
 
 #include "os_object_base.h"
 
Index: clang/test/Analysis/analyzer-config.c
===================================================================
--- clang/test/Analysis/analyzer-config.c
+++ clang/test/Analysis/analyzer-config.c
@@ -89,7 +89,6 @@
 // CHECK-NEXT: optin.osx.cocoa.localizability.NonLocalizedStringChecker:AggressiveReport = false
 // CHECK-NEXT: optin.performance.Padding:AllowedPad = 24
 // CHECK-NEXT: osx.NumberObjectConversion:Pedantic = false
-// CHECK-NEXT: osx.cocoa.RetainCount:CheckOSObject = true
 // CHECK-NEXT: osx.cocoa.RetainCount:TrackNSCFStartParam = false
 // CHECK-NEXT: prune-paths = true
 // CHECK-NEXT: region-store-small-struct-limit = 2
@@ -106,4 +105,4 @@
 // CHECK-NEXT: unroll-loops = false
 // CHECK-NEXT: widen-loops = false
 // CHECK-NEXT: [stats]
-// CHECK-NEXT: num-entries = 103
+// CHECK-NEXT: num-entries = 102
Index: clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
@@ -1481,26 +1481,11 @@
   return true;
 }
 
-// FIXME: remove this, hack for backwards compatibility:
-// it should be possible to enable the NS/CF retain count checker as
-// osx.cocoa.RetainCount, and it should be possible to disable
-// osx.OSObjectRetainCount using osx.cocoa.RetainCount:CheckOSObject=false.
-static bool getOption(const AnalyzerOptions &Options,
-                      StringRef Postfix,
-                      StringRef Value) {
-  auto I = Options.Config.find(
-    (StringRef("osx.cocoa.RetainCount:") + Postfix).str());
-  if (I != Options.Config.end())
-    return I->getValue() == Value;
-  return false;
-}
-
 void ento::registerRetainCountChecker(CheckerManager &Mgr) {
   auto *Chk = Mgr.getChecker<RetainCountChecker>();
   Chk->TrackObjCAndCFObjects = true;
-  Chk->TrackNSCFStartParam = getOption(Mgr.getAnalyzerOptions(),
-                                       "TrackNSCFStartParam",
-                                       "true");
+  Chk->TrackNSCFStartParam = Mgr.getAnalyzerOptions().getCheckerBooleanOption(
+      Mgr.getCurrentCheckerName(), "TrackNSCFStartParam");
 }
 
 bool ento::shouldRegisterRetainCountChecker(const CheckerManager &mgr) {
@@ -1509,10 +1494,7 @@
 
 void ento::registerOSObjectRetainCountChecker(CheckerManager &Mgr) {
   auto *Chk = Mgr.getChecker<RetainCountChecker>();
-  if (!getOption(Mgr.getAnalyzerOptions(),
-                 "CheckOSObject",
-                 "false"))
-    Chk->TrackOSObjects = true;
+  Chk->TrackOSObjects = true;
 }
 
 bool ento::shouldRegisterOSObjectRetainCountChecker(const CheckerManager &mgr) {
Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
===================================================================
--- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1027,15 +1027,6 @@
 def RetainCountChecker : Checker<"RetainCount">,
   HelpText<"Check for leaks and improper reference count management">,
   CheckerOptions<[
-    CmdLineOption<Boolean,
-                  "CheckOSObject",
-                  "Find violations of retain-release rules applied to XNU "
-                  "OSObject instances. By default, the checker only checks "
-                  "retain-release rules for Objective-C NSObject instances "
-                  "and CoreFoundation objects.",
-                  "true",
-                  InAlpha,
-                  Hide>,
     CmdLineOption<Boolean,
                   "TrackNSCFStartParam",
                   "Check not only that the code follows retain-release rules "


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D78097.257272.patch
Type: text/x-patch
Size: 4362 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200414/1ee7fdaa/attachment-0001.bin>