[analyzer] Validate checker option names and values

[Szelethus]

Summary

Validate whether the option exists, and also whether the supplied value is of the correct type.

This is the final part of the checker option refactoring. I guess we could arrange the AnalyzerOptions related changes to 5 "chapters":

1. Reimplement -analyzer-config options
2. Fix the checker naming bug by reimplementing checker dependencies
3. Reimplement checker options
4. (already in the works!) Document the frontend of the analyzer, sneak peak here.
5. (soon™) Make AnalyzerOptions const everywhere after the analyzer is initialized, make AnalyzerOptions::ConfigTable private, reimplement debug.ConfigDumper.

[NoQ]

(already in the works!) Document the frontend of the analyzer, sneak peak here.

Whoa!!

*celebrates the graphomania epidemic*

[Szelethus]

Refactored the entire patch, NFC compared to the earlier diff. This was needed as in a followup revision, much of the code would be rewritten.

[whisperity]

lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp

325

Insertion

336

Does this mean that we no longer can give "1" and "0" for boolean checker options? Or was that Tidy who allowed such in the first place?

[Szelethus]

lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp

336

Well yea, "true" or "false" is the only input we accept (and have ever accepted) for boolean configs. I have nothing against adding 1 and 0 too, but let that be a topic of another revision.

[baloghadamsoftware]

lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp

321

Is this the most efficient way to concatenate StringRefs?

461

What about Config.first().split(":")?

[whisperity]

lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp

321

@baloghadamsoftware
Is this the most efficient way to concatenate StringRefs?

Twine is specifically designed for that, yes.
You can't evade the final "copy", although most likely there'll be a RVO taking place.

However, the *first* Twine could be initialised from the FullName variable, as opposed to empty node.
And you could use single characters as characters, as opposed to strings, i.e. (Twine(FullName) + ':' + OptionName).str().

[baloghadamsoftware]

lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp

321

The constructor Twine(const StringRef &Str) is implicit so (FullName + ':' + OptionName).str() results the same and is more readable.

[xazax.hun]

Did we test all the codepaths including the package level configs? If not, please add some package level config option related tests.
Otherwise the patch looks good to me after all the comments are resolved.

[Szelethus]

Fixes according to inline comments.

[Szelethus]

Add a testcase for packages.

[Szelethus]

Rebase.

[Szelethus]

Add a test case for checker plugins.

[dcoughlin]

I think it would be better if the default for compatibility mode were 'true'. That way this change will be backwards compatible and clients who want to enforce stricter checking could enable it. Setting compatibility mode to be true in the driver is not sufficient since many build systems call the frontend directly.

[baloghadamsoftware]

In D57860#1432728, @dcoughlin wrote:

I think it would be better if the default for compatibility mode were 'true'. That way this change will be backwards compatible and clients who want to enforce stricter checking could enable it. Setting compatibility mode to be true in the driver is not sufficient since many build systems call the frontend directly.

How long must compatibility be kept?

[Szelethus]

In D57860#1432728, @dcoughlin wrote:

I think it would be better if the default for compatibility mode were 'true'. That way this change will be backwards compatible and clients who want to enforce stricter checking could enable it. Setting compatibility mode to be true in the driver is not sufficient since many build systems call the frontend directly.

Well, this is how 8.0.0. landed, so for better or for worse, changing it back wouldn't help on those clients much -- the question is now that whether there's a point in changing this yet back again. I would argue *strongly* on the side that it is not, because, according to the FAQ (which is aimed at end users):

Frontend-only options are intended to be used only by clang developers. Users should not run clang -cc1 directly, because -cc1 options are not guaranteed to be stable.

Of course, developers could also just invoke the analyzer with compatibility mode disabled, but thats I think a chore, and easy to forget -- it would greatly diminish the value of these patches, and as a maintainer, I would personally strongly prefer the analyzer to just terminate and not waste my time when I mess up an invocation.

[Szelethus]

Ping.

[Szelethus]

Ping^2

[Szelethus]

Ping^3

[Szelethus]

Ping^4

[NoQ]

In D57860#1432728, @dcoughlin wrote:

Setting compatibility mode to be true in the driver is not sufficient since many build systems call the frontend directly.

This doesn't seem to be the case; all build systems i'm aware of would pick up the driver flag.

We've seen really bad problems due to lack of compatibility in the past, but this time i don't foresee any, as we've already tested this for non-checker flags. Let's land this and i'll ask for a revert if it ever causes any problems.

Note that even if we revert the driver hack and keep compatibility mode on by default, we can still disable compatibility mode in LIT substitutions (i.e., in %clang_analyze_cc1), so that we still had checking in our tests and were sure that they test something.

include/clang/Basic/DiagnosticDriverKinds.td

 
 def err_analyzer_config_no_value : Error<
   "analyzer-config option '%0' has a key but no value">;
 def err_analyzer_config_multiple_values : Error<
   "analyzer-config option '%0' should contain only one '='">;
 def err_analyzer_config_invalid_input : Error<
   "invalid input for analyzer-config option '%0', that expects %1 value">;
 def err_analyzer_config_unknown : Error<"unknown analyzer-config '%0'">;
+def err_analyzer_checker_option_unknown : Error<
+  "checker '%0' has no option called '%1'">;
 def err_analyzer_checker_option_invalid_input : Error<
   "invalid input for checker option '%0', that expects %1">;
 
 def err_drv_invalid_hvx_length : Error<
   "-mhvx-length is not supported without a -mhvx/-mhvx= flag">;
 def warn_drv_vectorize_needs_hvx : Warning<
   "auto-vectorization requires HVX, use -mhvx to enable it">,
   InGroup<OptionIgnored>;

include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h

struct CmdLineOption {
     CmdLineOption(StringRef OptionType, StringRef OptionName,
                   StringRef DefaultValStr, StringRef Description)
         : OptionType(OptionType), OptionName(OptionName),
           DefaultValStr(DefaultValStr), Description(Description) {
 
       assert((OptionType == "bool" || OptionType == "string" ||
               OptionType == "int") &&
              "Unknown command line option type!");
+
+      assert((OptionType != "bool" ||
+              (DefaultValStr == "true" || DefaultValStr == "false")) &&
+             "Invalid value for boolean command line option! Maybe incorrect "
+             "parameters to the addCheckerOption or addPackageOption method?");
+
+      int Tmp;
+      assert((OptionType != "int" || !DefaultValStr.getAsInteger(0, Tmp)) &&
+             "Invalid value for integer command line option! Maybe incorrect "
+             "parameters to the addCheckerOption or addPackageOption method?");
+      (void)Tmp;
     }
   };
 
   using CmdLineOptionList = llvm::SmallVector<CmdLineOption, 0>;
 
   struct CheckerInfo;
 
   using CheckerInfoList = std::vector<CheckerInfo>;
struct CheckerInfo {
     bool isEnabled(const LangOptions &LO) const {
       return State == StateFromCmdLine::State_Enabled && ShouldRegister(LO);
     }
 
     bool isDisabled(const LangOptions &LO) const {
       return State == StateFromCmdLine::State_Disabled && ShouldRegister(LO);
     }
 
+    // Since each checker must have a different full name, we can identify
+    // CheckerInfo objects by them.
+    bool operator==(const CheckerInfo &Rhs) const {
+      return FullName == Rhs.FullName;
+    }
+
     CheckerInfo(InitializationFunction Fn, ShouldRegisterFunction sfn,
                 StringRef Name, StringRef Desc, StringRef DocsUri)
         : Initialize(Fn), ShouldRegister(sfn), FullName(Name), Desc(Desc),
           DocumentationUri(DocsUri) {}
 
     // Used for lower_bound.
     explicit CheckerInfo(StringRef FullName) : FullName(FullName) {}
   };

lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp

void CheckerRegistry::resolveDependencies() {
 
   Dependencies.clear();
 }
 
 void CheckerRegistry::addDependency(StringRef FullName, StringRef Dependency) {
   Dependencies.emplace_back(FullName, Dependency);
 }
 
+/// Insert the checker/package option to AnalyzerOptions' config table, and
+/// validate it, if the user supplied it on the command line.
+static void insertAndValidate(StringRef FullName,
+                              const CheckerRegistry::CmdLineOption &Option,
+                              AnalyzerOptions &AnOpts,
+                              DiagnosticsEngine &Diags) {
+
+  std::string FullOption = (FullName + ":" + Option.OptionName).str();
+
+  auto It = AnOpts.Config.insert({FullOption, Option.DefaultValStr});
+
+  // Insertation was successful -- CmdLineOption's constructor will validate
+  // whether values received from plugins or TableGen files are correct.
+  if (It.second)
+    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.
+
+  StringRef SuppliedValue = It.first->getValue();
+
+  if (Option.OptionType == "bool") {
+    if (SuppliedValue != "true" && SuppliedValue != "false") {
+      if (AnOpts.ShouldEmitErrorsOnInvalidConfigValue) {
+        Diags.Report(diag::err_analyzer_checker_option_invalid_input)
+            << FullOption << "a boolean value";
+      }
+    }
+    return;
+  }
+
+  if (Option.OptionType == "int") {
+    int Tmp;
+    bool HasFailed = SuppliedValue.getAsInteger(0, Tmp);
+    if (HasFailed) {
+      if (AnOpts.ShouldEmitErrorsOnInvalidConfigValue) {
+        Diags.Report(diag::err_analyzer_checker_option_invalid_input)
+            << FullOption << "an integer value";
+      }
+    }
+    return;
+  }
+}
+
 template <class T>
 static void
 insertOptionToCollection(StringRef FullName, T &Collection,
                          const CheckerRegistry::CmdLineOption &&Option,
-                         AnalyzerOptions &AnOpts) {
+                         AnalyzerOptions &AnOpts, DiagnosticsEngine &Diags) {
   auto It = binaryFind(Collection, FullName);
   assert(It != Collection.end() &&
          "Failed to find the checker while attempting to add a command line "
          "option to it!");
 
-  AnOpts.Config.insert(
-      {(FullName + ":" + Option.OptionName).str(), Option.DefaultValStr});
+  insertAndValidate(FullName, Option, AnOpts, Diags);
 
   It->CmdLineOptions.emplace_back(std::move(Option));
 }
 
 void CheckerRegistry::resolveCheckerAndPackageOptions() {
   for (const std::pair<StringRef, CmdLineOption> &CheckerOptEntry :
        CheckerOptions) {
     insertOptionToCollection(CheckerOptEntry.first, Checkers,
-                             std::move(CheckerOptEntry.second), AnOpts);
+                             std::move(CheckerOptEntry.second), AnOpts, Diags);
   }
   CheckerOptions.clear();
 
   for (const std::pair<StringRef, CmdLineOption> &PackageOptEntry :
        PackageOptions) {
-    insertOptionToCollection(PackageOptEntry.first, Checkers,
-                             std::move(PackageOptEntry.second), AnOpts);
+    insertOptionToCollection(PackageOptEntry.first, Packages,
+                             std::move(PackageOptEntry.second), AnOpts, Diags);
   }
   PackageOptions.clear();
 }
 
 void CheckerRegistry::addPackage(StringRef FullName) {
   Packages.emplace_back(PackageInfo(FullName));
 }
 
void CheckerRegistry::initializeManager(CheckerManager &CheckerMgr) const {
 
   // Initialize the CheckerManager with all enabled checkers.
   for (const auto *Checker : enabledCheckers) {
     CheckerMgr.setCurrentCheckName(CheckName(Checker->FullName));
     Checker->Initialize(CheckerMgr);
   }
 }
 
+static void
+isOptionContainedIn(const CheckerRegistry::CmdLineOptionList &OptionList,
+                    StringRef SuppliedChecker, StringRef SuppliedOption,
+                    const AnalyzerOptions &AnOpts, DiagnosticsEngine &Diags) {
+
+  if (!AnOpts.ShouldEmitErrorsOnInvalidConfigValue)
+    return;
+
+  using CmdLineOption = CheckerRegistry::CmdLineOption;
+
+  auto SameOptName = [SuppliedOption](const CmdLineOption &Opt) {
+    return Opt.OptionName == SuppliedOption;
+  };
+
+  auto OptionIt = llvm::find_if(OptionList, SameOptName);
+
+  if (OptionIt == OptionList.end()) {
+    Diags.Report(diag::err_analyzer_checker_option_unknown)
+        << SuppliedChecker << SuppliedOption;
+    return;
+  }
+}
+
 void CheckerRegistry::validateCheckerOptions() const {
   for (const auto &Config : AnOpts.Config) {
-    size_t Pos = Config.getKey().find(':');
-    if (Pos == StringRef::npos)
+
+    StringRef SuppliedChecker;
+    StringRef SuppliedOption;
+    std::tie(SuppliedChecker, SuppliedOption) = Config.getKey().split(':');
+
+    if (SuppliedOption.empty())
       continue;
 
-    bool HasChecker = false;
-    StringRef CheckerName = Config.getKey().substr(0, Pos);
-    for (const auto &Checker : Checkers) {
-      if (Checker.FullName.startswith(CheckerName) &&
-          (Checker.FullName.size() == Pos || Checker.FullName[Pos] == '.')) {
-        HasChecker = true;
-        break;
-      }
+    // AnalyzerOptions' config table contains the user input, so it entry could
+    // look like this:
+    //
+    //   cor:NoFalsePositives=true
+    //
+    // Since lower_bound would look for the first element *not less* than "cor",
+    // it would return with an iterator to the first checker in the core, so we
+    // we really have to use find here, which uses operator==.
+    auto CheckerIt = llvm::find(Checkers, CheckerInfo(SuppliedChecker));
+    if (CheckerIt != Checkers.end()) {
+      isOptionContainedIn(CheckerIt->CmdLineOptions, SuppliedChecker,
+                          SuppliedOption, AnOpts, Diags);
+      continue;
+
+    auto PackageIt = llvm::find(Packages, PackageInfo(SuppliedChecker));
+    if (PackageIt != Packages.end()) {
+      isOptionContainedIn(PackageIt->CmdLineOptions, SuppliedChecker,
+                          SuppliedOption, AnOpts, Diags);
+      continue;
     }
-    if (!HasChecker)
-      Diags.Report(diag::err_unknown_analyzer_checker) << CheckerName;
+
+    Diags.Report(diag::err_unknown_analyzer_checker) << SuppliedChecker;
   }
 }
 
 void CheckerRegistry::printCheckerWithDescList(raw_ostream &Out,
                                                size_t MaxNameChars) const {
   // FIXME: Print available packages.
 
   Out << "CHECKERS:\n";

test/Analysis/invalid-checker-option.c

 // RUN: not %clang_analyze_cc1 -verify %s \
 // RUN:   -analyzer-checker=core \
 // RUN:   -analyzer-config RetainOneTwoThree:CheckOSObject=false \
 // RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-NON-EXISTENT-CHECKER
 
 // Note that non-existent packages and checkers were always reported.
 
 // RUN: not %clang_analyze_cc1 -verify %s \
 // RUN:   -analyzer-checker=core \
 // RUN:   -analyzer-config-compatibility-mode=true \
 // RUN:   -analyzer-config RetainOneTwoThree:CheckOSObject=false \
 // RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-NON-EXISTENT-CHECKER
 
 // CHECK-NON-EXISTENT-CHECKER: (frontend): no analyzer checkers or packages
 // CHECK-NON-EXISTENT-CHECKER-SAME: are associated with 'RetainOneTwoThree'
 
+
+// Every other error should be avoidable in compatiblity mode.
+
+
+// RUN: not %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config debug.AnalysisOrder:Everything=false \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-NON-EXISTENT-CHECKER-OPTION
+
+// CHECK-NON-EXISTENT-CHECKER-OPTION: (frontend): checker 'debug.AnalysisOrder'
+// CHECK-NON-EXISTENT-CHECKER-OPTION-SAME: has no option called 'Everything'
+
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config-compatibility-mode=true \
+// RUN:   -analyzer-config debug.AnalysisOrder:Everything=false
+
+
+// RUN: not %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config debug.AnalysisOrder:*=nothankyou \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-INVALID-BOOL-VALUE
+
+// CHECK-INVALID-BOOL-VALUE: (frontend): invalid input for checker option
+// CHECK-INVALID-BOOL-VALUE-SAME: 'debug.AnalysisOrder:*', that expects a
+// CHECK-INVALID-BOOL-VALUE-SAME: boolean value
+
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config-compatibility-mode=true \
+// RUN:   -analyzer-config debug.AnalysisOrder:*=nothankyou
+
+
+// RUN: not %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config optin.performance.Padding:AllowedPad=surpriseme \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-INVALID-INT-VALUE
+
+// CHECK-INVALID-INT-VALUE: (frontend): invalid input for checker option
+// CHECK-INVALID-INT-VALUE-SAME: 'optin.performance.Padding:AllowedPad', that
+// CHECK-INVALID-INT-VALUE-SAME: expects an integer value
+
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config-compatibility-mode=true \
+// RUN:   -analyzer-config optin.performance.Padding:AllowedPad=surpriseme
+
+
+// RUN: not %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config nullability:NoDiagnoseCallsToSystemHeaders=sure \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-PACKAGE-VALUE
+
+// CHECK-PACKAGE-VALUE: (frontend): invalid input for checker option
+// CHECK-PACKAGE-VALUE-SAME: 'nullability:NoDiagnoseCallsToSystemHeaders', that
+// CHECK-PACKAGE-VALUE-SAME: expects a boolean value
+
 // expected-no-diagnostics
 
 int main() {}