[analyzer] Remove the default value arg from getChecker*Option

Since D57922, the config table contains every checker option, and it's default
value, so having it as an argument for getChecker*Option is redundant.

By the time any of the getChecker*Option function is called, we verified the
value in CheckerRegistry (after D57860), so we can confidently assert here, as
any irregularities detected at this point must be a programmer error. However,
in compatibility mode, verification won't happen, so the default value must be
restored.

This implies something else, other than adding removing one more potential point
of failure -- debug.ConfigDumper will always contain valid values for
checker/package options!

Differential Revision: https://reviews.llvm.org/D59195


git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@361042 91177308-0d34-0410-b5e6-96231b3b80d8

include/clang/StaticAnalyzer/Core/AnalyzerOptions.h

@@ -281,18 +281,14 @@ public:
   /// Checker options are retrieved in the following format:
   /// `-analyzer-config CheckerName:OptionName=Value.
   /// @param [in] OptionName Name for option to retrieve.
-  /// @param [in] DefaultVal Default value returned if no such option was
-  /// specified.
   /// @param [in] SearchInParents If set to true and the searched option was not
   /// specified for the given checker the options for the parent packages will
   /// be searched as well. The inner packages take precedence over the outer
   /// ones.
   bool getCheckerBooleanOption(StringRef CheckerName, StringRef OptionName,
-                               bool DefaultVal,
                                bool SearchInParents = false) const;
 
   bool getCheckerBooleanOption(const ento::CheckerBase *C, StringRef OptionName,
-                               bool DefaultVal,
                                bool SearchInParents = false) const;
 
   /// Interprets an option's string value as an integer value.
@@ -305,18 +301,14 @@ public:
   /// Checker options are retrieved in the following format:
   /// `-analyzer-config CheckerName:OptionName=Value.
   /// @param [in] OptionName Name for option to retrieve.
-  /// @param [in] DefaultVal Default value returned if no such option was
-  /// specified.
   /// @param [in] SearchInParents If set to true and the searched option was not
   /// specified for the given checker the options for the parent packages will
   /// be searched as well. The inner packages take precedence over the outer
   /// ones.
   int getCheckerIntegerOption(StringRef CheckerName, StringRef OptionName,
-                              int DefaultVal,
                               bool SearchInParents = false) const;
 
   int getCheckerIntegerOption(const ento::CheckerBase *C, StringRef OptionName,
-                              int DefaultVal,
                               bool SearchInParents = false) const;
 
   /// Query an option's string value.
@@ -329,18 +321,15 @@ public:
   /// Checker options are retrieved in the following format:
   /// `-analyzer-config CheckerName:OptionName=Value.
   /// @param [in] OptionName Name for option to retrieve.
-  /// @param [in] DefaultVal Default value returned if no such option was
-  /// specified.
   /// @param [in] SearchInParents If set to true and the searched option was not
   /// specified for the given checker the options for the parent packages will
   /// be searched as well. The inner packages take precedence over the outer
   /// ones.
   StringRef getCheckerStringOption(StringRef CheckerName, StringRef OptionName,
-                                   StringRef DefaultVal,
                                    bool SearchInParents = false) const;
 
   StringRef getCheckerStringOption(const ento::CheckerBase *C,
-                                   StringRef OptionName, StringRef DefaultVal,
+                                   StringRef OptionName,
                                    bool SearchInParents = false) const;
 
   /// Retrieves and sets the UserMode. This is a high-level option,

lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp

@@ -44,8 +44,8 @@ class AnalysisOrderChecker
                      check::LiveSymbols> {
 
   bool isCallbackEnabled(AnalyzerOptions &Opts, StringRef CallbackName) const {
-    return Opts.getCheckerBooleanOption(this, "*", false) ||
-        Opts.getCheckerBooleanOption(this, CallbackName, false);
+    return Opts.getCheckerBooleanOption(this, "*") ||
+           Opts.getCheckerBooleanOption(this, CallbackName);
   }
 
   bool isCallbackEnabled(CheckerContext &C, StringRef CallbackName) const {

lib/StaticAnalyzer/Checkers/CloneChecker.cpp

@@ -195,17 +195,17 @@ void ento::registerCloneChecker(CheckerManager &Mgr) {
   auto *Checker = Mgr.registerChecker<CloneChecker>();
 
   Checker->MinComplexity = Mgr.getAnalyzerOptions().getCheckerIntegerOption(
-      Checker, "MinimumCloneComplexity", 50);
+      Checker, "MinimumCloneComplexity");
 
   if (Checker->MinComplexity < 0)
     Mgr.reportInvalidCheckerOptionValue(
         Checker, "MinimumCloneComplexity", "a non-negative value");
 
   Checker->ReportNormalClones = Mgr.getAnalyzerOptions().getCheckerBooleanOption(
-      Checker, "ReportNormalClones", true);
+      Checker, "ReportNormalClones");
 
   Checker->IgnoredFilesPattern = Mgr.getAnalyzerOptions()
-    .getCheckerStringOption(Checker, "IgnoredFilesPattern", "");
+    .getCheckerStringOption(Checker, "IgnoredFilesPattern");
 }
 
 bool ento::shouldRegisterCloneChecker(const LangOptions &LO) {

lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp

@@ -1398,7 +1398,7 @@ void ento::registerNonLocalizedStringChecker(CheckerManager &mgr) {
       mgr.registerChecker<NonLocalizedStringChecker>();
   checker->IsAggressive =
       mgr.getAnalyzerOptions().getCheckerBooleanOption(
-          checker, "AggressiveReport", false);
+          checker, "AggressiveReport");
 }
 
 bool ento::shouldRegisterNonLocalizedStringChecker(const LangOptions &LO) {

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

@@ -3098,7 +3098,7 @@ void ento::registerInnerPointerCheckerAux(CheckerManager &mgr) {
 void ento::registerDynamicMemoryModeling(CheckerManager &mgr) {
   auto *checker = mgr.registerChecker<MallocChecker>();
   checker->IsOptimistic = mgr.getAnalyzerOptions().getCheckerBooleanOption(
-                                                  checker, "Optimistic", false);
+                                                         checker, "Optimistic");
 }
 
 bool ento::shouldRegisterDynamicMemoryModeling(const LangOptions &LO) {

lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp

@@ -82,10 +82,10 @@ void ento::registerMmapWriteExecChecker(CheckerManager &mgr) {
       mgr.registerChecker<MmapWriteExecChecker>();
   Mwec->ProtExecOv =
     mgr.getAnalyzerOptions()
-      .getCheckerIntegerOption(Mwec, "MmapProtExec", 0x04);
+      .getCheckerIntegerOption(Mwec, "MmapProtExec");
   Mwec->ProtReadOv =
     mgr.getAnalyzerOptions()
-      .getCheckerIntegerOption(Mwec, "MmapProtRead", 0x01);
+      .getCheckerIntegerOption(Mwec, "MmapProtRead");
 }
 
 bool ento::shouldRegisterMmapWriteExecChecker(const LangOptions &LO) {

lib/StaticAnalyzer/Checkers/MoveChecker.cpp

@@ -752,8 +752,7 @@ void MoveChecker::printState(raw_ostream &Out, ProgramStateRef State,
 void ento::registerMoveChecker(CheckerManager &mgr) {
   MoveChecker *chk = mgr.registerChecker<MoveChecker>();
   chk->setAggressiveness(
-      mgr.getAnalyzerOptions().getCheckerStringOption(chk, "WarnOn",
-                                                      "KnownsAndLocals"), mgr);
+      mgr.getAnalyzerOptions().getCheckerStringOption(chk, "WarnOn"), mgr);
 }
 
 bool ento::shouldRegisterMoveChecker(const LangOptions &LO) {

lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

@@ -1208,7 +1208,7 @@ bool ento::shouldRegisterNullabilityBase(const LangOptions &LO) {
     checker->NoDiagnoseCallsToSystemHeaders =                                  \
         checker->NoDiagnoseCallsToSystemHeaders ||                             \
         mgr.getAnalyzerOptions().getCheckerBooleanOption(                      \
-                      checker, "NoDiagnoseCallsToSystemHeaders", false, true); \
+                      checker, "NoDiagnoseCallsToSystemHeaders", true);        \
   }                                                                            \
                                                                                \
   bool ento::shouldRegister##name##Checker(const LangOptions &LO) {            \

lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp

@@ -346,7 +346,7 @@ void ento::registerNumberObjectConversionChecker(CheckerManager &Mgr) {
   NumberObjectConversionChecker *Chk =
       Mgr.registerChecker<NumberObjectConversionChecker>();
   Chk->Pedantic =
-      Mgr.getAnalyzerOptions().getCheckerBooleanOption(Chk, "Pedantic", false);
+      Mgr.getAnalyzerOptions().getCheckerBooleanOption(Chk, "Pedantic");
 }
 
 bool ento::shouldRegisterNumberObjectConversionChecker(const LangOptions &LO) {

lib/StaticAnalyzer/Checkers/PaddingChecker.cpp

@@ -348,7 +348,7 @@ public:
 void ento::registerPaddingChecker(CheckerManager &Mgr) {
   auto *Checker = Mgr.registerChecker<PaddingChecker>();
   Checker->AllowedPad = Mgr.getAnalyzerOptions()
-          .getCheckerIntegerOption(Checker, "AllowedPad", 24);
+          .getCheckerIntegerOption(Checker, "AllowedPad");
   if (Checker->AllowedPad < 0)
     Mgr.reportInvalidCheckerOptionValue(
         Checker, "AllowedPad", "a non-negative value");

lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp

@@ -611,18 +611,15 @@ void ento::registerUninitializedObjectChecker(CheckerManager &Mgr) {
   AnalyzerOptions &AnOpts = Mgr.getAnalyzerOptions();
   UninitObjCheckerOptions &ChOpts = Chk->Opts;
 
-  ChOpts.IsPedantic =
-      AnOpts.getCheckerBooleanOption(Chk, "Pedantic", /*DefaultVal*/ false);
+  ChOpts.IsPedantic = AnOpts.getCheckerBooleanOption(Chk, "Pedantic");
   ChOpts.ShouldConvertNotesToWarnings = AnOpts.getCheckerBooleanOption(
-      Chk, "NotesAsWarnings", /*DefaultVal*/ false);
+      Chk, "NotesAsWarnings");
   ChOpts.CheckPointeeInitialization = AnOpts.getCheckerBooleanOption(
-      Chk, "CheckPointeeInitialization", /*DefaultVal*/ false);
+      Chk, "CheckPointeeInitialization");
   ChOpts.IgnoredRecordsWithFieldPattern =
-      AnOpts.getCheckerStringOption(Chk, "IgnoreRecordsWithField",
-                                    /*DefaultVal*/ "\"\"");
+      AnOpts.getCheckerStringOption(Chk, "IgnoreRecordsWithField");
   ChOpts.IgnoreGuardedFields =
-      AnOpts.getCheckerBooleanOption(Chk, "IgnoreGuardedFields",
-                                     /*DefaultVal*/ false);
+      AnOpts.getCheckerBooleanOption(Chk, "IgnoreGuardedFields");
 
   std::string ErrorMsg;
   if (!llvm::Regex(ChOpts.IgnoredRecordsWithFieldPattern).isValid(ErrorMsg))

lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

@@ -279,8 +279,7 @@ void ento::registerVirtualCallChecker(CheckerManager &mgr) {
   VirtualCallChecker *checker = mgr.registerChecker<VirtualCallChecker>();
 
   checker->IsPureOnly =
-      mgr.getAnalyzerOptions().getCheckerBooleanOption(
-                                                    checker, "PureOnly", false);
+      mgr.getAnalyzerOptions().getCheckerBooleanOption(checker, "PureOnly");
 }
 
 bool ento::shouldRegisterVirtualCallChecker(const LangOptions &LO) {

lib/StaticAnalyzer/Core/AnalyzerOptions.cpp

@@ -103,8 +103,7 @@ AnalyzerOptions::mayInlineCXXMemberFunction(
 
 StringRef AnalyzerOptions::getCheckerStringOption(StringRef CheckerName,
                                                   StringRef OptionName,
-                                                  StringRef DefaultVal,
-                                                  bool SearchInParents ) const {
+                                                  bool SearchInParents) const {
   assert(!CheckerName.empty() &&
          "Empty checker name! Make sure the checker object (including it's "
          "bases!) if fully initialized before calling this function!");
@@ -117,62 +116,66 @@ StringRef AnalyzerOptions::getCheckerStringOption(StringRef CheckerName,
       return StringRef(I->getValue());
     size_t Pos = CheckerName.rfind('.');
     if (Pos == StringRef::npos)
-      return DefaultVal;
+      break;
+
     CheckerName = CheckerName.substr(0, Pos);
   } while (!CheckerName.empty() && SearchInParents);
-  return DefaultVal;
+
+  llvm_unreachable("Unknown checker option! Did you call getChecker*Option "
+                   "with incorrect parameters? User input must've been "
+                   "verified by CheckerRegistry.");
+
+  return "";
 }
 
 StringRef AnalyzerOptions::getCheckerStringOption(const ento::CheckerBase *C,
                                                   StringRef OptionName,
-                                                  StringRef DefaultVal,
-                                                  bool SearchInParents ) const {
+                                                  bool SearchInParents) const {
   return getCheckerStringOption(
-             C->getTagDescription(), OptionName, DefaultVal, SearchInParents);
+                           C->getTagDescription(), OptionName, SearchInParents);
 }
 
 bool AnalyzerOptions::getCheckerBooleanOption(StringRef CheckerName,
                                               StringRef OptionName,
-                                              bool DefaultVal,
-                                              bool SearchInParents ) const {
-  // FIXME: We should emit a warning here if the value is something other than
-  // "true", "false", or the empty string (meaning the default value),
-  // but the AnalyzerOptions doesn't have access to a diagnostic engine.
-  return llvm::StringSwitch<bool>(
+                                              bool SearchInParents) const {
+  auto Ret = llvm::StringSwitch<llvm::Optional<bool>>(
       getCheckerStringOption(CheckerName, OptionName,
-                             DefaultVal ? "true" : "false",
                              SearchInParents))
       .Case("true", true)
       .Case("false", false)
-      .Default(DefaultVal);
+      .Default(None);
+
+  assert(Ret &&
+         "This option should be either 'true' or 'false', and should've been "
+         "validated by CheckerRegistry!");
+
+  return *Ret;
 }
 
 bool AnalyzerOptions::getCheckerBooleanOption(const ento::CheckerBase *C,
                                               StringRef OptionName,
-                                              bool DefaultVal,
-                                              bool SearchInParents ) const {
+                                              bool SearchInParents) const {
   return getCheckerBooleanOption(
-             C->getTagDescription(), OptionName, DefaultVal, SearchInParents);
+             C->getTagDescription(), OptionName, SearchInParents);
 }
 
 int AnalyzerOptions::getCheckerIntegerOption(StringRef CheckerName,
                                              StringRef OptionName,
-                                             int DefaultVal,
-                                             bool SearchInParents ) const {
-  int Ret = DefaultVal;
+                                             bool SearchInParents) const {
+  int Ret = 0;
   bool HasFailed = getCheckerStringOption(CheckerName, OptionName,
-                                          std::to_string(DefaultVal),
                                           SearchInParents)
                      .getAsInteger(0, Ret);
-  assert(!HasFailed && "analyzer-config option should be numeric");
+  assert(!HasFailed &&
+         "This option should be numeric, and should've been validated by "
+         "CheckerRegistry!");
   (void)HasFailed;
   return Ret;
 }
 
 int AnalyzerOptions::getCheckerIntegerOption(const ento::CheckerBase *C,
                                              StringRef OptionName,
-                                             int DefaultVal,
-                                             bool SearchInParents ) const {
+                                             bool SearchInParents) const {
   return getCheckerIntegerOption(
-             C->getTagDescription(), OptionName, DefaultVal, SearchInParents);
+                           C->getTagDescription(), OptionName, SearchInParents);
 }

lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp

@@ -324,7 +324,9 @@ static void insertAndValidate(StringRef FullName,
     return;
 
   // Insertion failed, the user supplied this package/checker option on the
-  // command line. If the supplied value is invalid, we'll emit an error.
+  // command line. If the supplied value is invalid, we'll restore the option
+  // to it's default value, and if we're in non-compatibility mode, we'll also
+  // emit an error.
 
   StringRef SuppliedValue = It.first->getValue();
 
@@ -334,6 +336,8 @@ static void insertAndValidate(StringRef FullName,
         Diags.Report(diag::err_analyzer_checker_option_invalid_input)
             << FullOption << "a boolean value";
       }
+
+      It.first->setValue(Option.DefaultValStr);
     }
     return;
   }
@@ -346,6 +350,8 @@ static void insertAndValidate(StringRef FullName,
         Diags.Report(diag::err_analyzer_checker_option_invalid_input)
             << FullOption << "an integer value";
       }
+
+      It.first->setValue(Option.DefaultValStr);
     }
     return;
   }

test/Analysis/checker-plugins.c

@@ -94,3 +94,13 @@ void caller() {
 // RUN:   -analyzer-checker=example.MyChecker \
 // RUN:   -analyzer-config-compatibility-mode=true \
 // RUN:   -analyzer-config example.MyChecker:ExampleOption=example
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -load %llvmshlibdir/CheckerOptionHandlingAnalyzerPlugin%pluginext\
+// RUN:   -analyzer-checker=example.MyChecker \
+// RUN:   -analyzer-checker=debug.ConfigDumper \
+// RUN:   -analyzer-config-compatibility-mode=true \
+// RUN:   -analyzer-config example.MyChecker:ExampleOption=example \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-CORRECTED-BOOL-VALUE
+
+// CHECK-CORRECTED-BOOL-VALUE: example.MyChecker:ExampleOption = false

test/Analysis/invalid-checker-option.c

@@ -15,6 +15,27 @@
 // CHECK-NON-EXISTENT-CHECKER-SAME: are associated with 'RetainOneTwoThree'
 
 
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=debug.ConfigDumper \
+// RUN:   -analyzer-checker=debug.AnalysisOrder \
+// RUN:   -analyzer-config-compatibility-mode=true \
+// RUN:   -analyzer-config debug.AnalysisOrder:*=yesplease \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-CORRECTED-BOOL-VALUE
+
+// CHECK-CORRECTED-BOOL-VALUE: debug.AnalysisOrder:* = false
+//
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=debug.ConfigDumper \
+// RUN:   -analyzer-checker=optin.performance.Padding \
+// RUN:   -analyzer-config-compatibility-mode=true \
+// RUN:   -analyzer-config optin.performance.Padding:AllowedPad=surpriseme \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-CORRECTED-INT-VALUE
+
+// CHECK-CORRECTED-INT-VALUE: optin.performance.Padding:AllowedPad = 24
+
+
 // Every other error should be avoidable in compatiblity mode.
 
 

test/Analysis/plugins/CheckerOptionHandling/CheckerOptionHandling.cpp

@@ -15,7 +15,7 @@ void registerMyChecker(CheckerManager &Mgr) {
   MyChecker *Checker = Mgr.registerChecker<MyChecker>();
   llvm::outs() << "Example option is set to "
                << (Mgr.getAnalyzerOptions().getCheckerBooleanOption(
-                       Checker, "ExampleOption", false)
+                       Checker, "ExampleOption")
                        ? "true"
                        : "false")
                << '\n';

unittests/StaticAnalyzer/AnalyzerOptionsTest.cpp

@@ -51,25 +51,24 @@ TEST(StaticAnalyzerOptions, SearchInParentPackageTests) {
   // CheckerTwo one has Option specified as true. It should read true regardless
   // of search mode.
   CheckerOneMock CheckerOne;
-  EXPECT_TRUE(Opts.getCheckerBooleanOption(&CheckerOne, "Option", false));
+  EXPECT_TRUE(Opts.getCheckerBooleanOption(&CheckerOne, "Option"));
   // The package option is overridden with a checker option.
-  EXPECT_TRUE(Opts.getCheckerBooleanOption(&CheckerOne, "Option", false,
-                                           true));
+  EXPECT_TRUE(Opts.getCheckerBooleanOption(&CheckerOne, "Option", true));
   // The Outer package option is overridden by the Inner package option. No
   // package option is specified.
-  EXPECT_TRUE(Opts.getCheckerBooleanOption(&CheckerOne, "Option2", false,
-                                           true));
-  // No package option is specified and search in packages is turned off. The
-  // default value should be returned.
-  EXPECT_FALSE(Opts.getCheckerBooleanOption(&CheckerOne, "Option2", false));
   EXPECT_TRUE(Opts.getCheckerBooleanOption(&CheckerOne, "Option2", true));
+  // No package option is specified and search in packages is turned off. We
+  // should assert here, but we can't test that.
+  //Opts.getCheckerBooleanOption(&CheckerOne, "Option2");
+  //Opts.getCheckerBooleanOption(&CheckerOne, "Option2");
 
-  // Checker true has no option specified. It should get the default value when
-  // search in parents turned off and false when search in parents turned on.
+  // Checker true has no option specified. It should get false when search in
+  // parents turned on.
   CheckerTwoMock CheckerTwo;
-  EXPECT_FALSE(Opts.getCheckerBooleanOption(&CheckerTwo, "Option", false));
-  EXPECT_TRUE(Opts.getCheckerBooleanOption(&CheckerTwo, "Option", true));
-  EXPECT_FALSE(Opts.getCheckerBooleanOption(&CheckerTwo, "Option", true, true));
+  EXPECT_FALSE(Opts.getCheckerBooleanOption(&CheckerTwo, "Option", true));
+  // In any other case, we should assert, that we cannot test unfortunately.
+  //Opts.getCheckerBooleanOption(&CheckerTwo, "Option");
+  //Opts.getCheckerBooleanOption(&CheckerTwo, "Option");
 }
 
 TEST(StaticAnalyzerOptions, StringOptions) {
@@ -84,16 +83,14 @@ TEST(StaticAnalyzerOptions, StringOptions) {
 
   CheckerOneMock CheckerOne;
   EXPECT_TRUE("StringValue" ==
-            Opts.getCheckerStringOption(&CheckerOne, "Option", "DefaultValue"));
-  EXPECT_TRUE("DefaultValue" ==
-           Opts.getCheckerStringOption(&CheckerOne, "Option2", "DefaultValue"));
+            Opts.getCheckerStringOption(&CheckerOne, "Option"));
 }
 
 TEST(StaticAnalyzerOptions, SubCheckerOptions) {
   AnalyzerOptions Opts;
   Opts.Config["Outer.Inner.CheckerOne:Option"] = "StringValue";
   EXPECT_TRUE("StringValue" == Opts.getCheckerStringOption(
-        "Outer.Inner.CheckerOne", "Option", "DefaultValue"));
+        "Outer.Inner.CheckerOne", "Option"));
 }
 
 } // end namespace ento