[analyzer] Emit an error rather than assert on invalid checker option input

Asserting on invalid input isn't very nice, hence the patch to emit an error
instead.

This is the first of many patches to overhaul the way we handle checker options.

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

llvm-svn: 355704

14 files changed, 107 insertions, 32 deletions

diff --git a/clang/include/clang/Basic/DiagnosticDriverKinds.td b/clang/include/clang/Basic/DiagnosticDriverKinds.td
index 3ebd931f48e..c822380a021 100644
--- a/clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ b/clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -303,6 +303,8 @@ def err_analyzer_config_multiple_values : Error<
 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_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">;
diff --git a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
index b4de26d7929..612286ba8b1 100644
--- a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
+++ b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
@@ -136,6 +136,12 @@ public:
   AnalyzerOptions &getAnalyzerOptions() { return AOptions; }
   ASTContext &getASTContext() { return Context; }
 
+  /// Emits an error through a DiagnosticsEngine about an invalid user supplied
+  /// checker option value.
+  void reportInvalidCheckerOptionValue(const CheckerBase *C,
+                                       StringRef OptionName,
+                                       StringRef ExpectedValueDesc);
+
   using CheckerRef = CheckerBase *;
   using CheckerTag = const void *;
   using CheckerDtor = CheckerFn<void ()>;
diff --git a/clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp b/clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
index 6be66d20afc..a7ca814c8f9 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
@@ -1086,14 +1086,10 @@ bool ObjCDeallocChecker::isNibLoadedIvarWithoutRetain(
 }
 
 void ento::registerObjCDeallocChecker(CheckerManager &Mgr) {
-  const LangOptions &LangOpts = Mgr.getLangOpts();
-  // These checker only makes sense under MRR.
-  if (LangOpts.getGC() == LangOptions::GCOnly || LangOpts.ObjCAutoRefCount)
-    return;
-
   Mgr.registerChecker<ObjCDeallocChecker>();
 }
 
 bool ento::shouldRegisterObjCDeallocChecker(const LangOptions &LO) {
-  return true;
+  // These checker only makes sense under MRR.
+  return LO.getGC() != LangOptions::GCOnly && !LO.ObjCAutoRefCount;
 }
diff --git a/clang/lib/StaticAnalyzer/Checkers/CloneChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CloneChecker.cpp
index 84018c5c39b..11a33e50bf5 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CloneChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/CloneChecker.cpp
@@ -27,6 +27,13 @@ using namespace ento;
 namespace {
 class CloneChecker
     : public Checker<check::ASTCodeBody, check::EndOfTranslationUnit> {
+public:
+  // Checker options.
+  int MinComplexity;
+  bool ReportNormalClones;
+  StringRef IgnoredFilesPattern;
+
+private:
   mutable CloneDetector Detector;
   mutable std::unique_ptr<BugType> BT_Exact, BT_Suspicious;
 
@@ -62,19 +69,6 @@ void CloneChecker::checkEndOfTranslationUnit(const TranslationUnitDecl *TU,
   // At this point, every statement in the translation unit has been analyzed by
   // the CloneDetector. The only thing left to do is to report the found clones.
 
-  int MinComplexity = Mgr.getAnalyzerOptions().getCheckerIntegerOption(
-      this, "MinimumCloneComplexity", 50);
-  assert(MinComplexity >= 0);
-
-  bool ReportSuspiciousClones = Mgr.getAnalyzerOptions()
-    .getCheckerBooleanOption(this, "ReportSuspiciousClones", true);
-
-  bool ReportNormalClones = Mgr.getAnalyzerOptions().getCheckerBooleanOption(
-      this, "ReportNormalClones", true);
-
-  StringRef IgnoredFilesPattern = Mgr.getAnalyzerOptions()
-    .getCheckerStringOption(this, "IgnoredFilesPattern", "");
-
   // Let the CloneDetector create a list of clones from all the analyzed
   // statements. We don't filter for matching variable patterns at this point
   // because reportSuspiciousClones() wants to search them for errors.
@@ -86,8 +80,7 @@ void CloneChecker::checkEndOfTranslationUnit(const TranslationUnitDecl *TU,
       MinComplexityConstraint(MinComplexity),
       RecursiveCloneTypeIIVerifyConstraint(), OnlyLargestCloneConstraint());
 
-  if (ReportSuspiciousClones)
-    reportSuspiciousClones(BR, Mgr, AllCloneGroups);
+  reportSuspiciousClones(BR, Mgr, AllCloneGroups);
 
   // We are done for this translation unit unless we also need to report normal
   // clones.
@@ -199,7 +192,20 @@ void CloneChecker::reportSuspiciousClones(
 //===----------------------------------------------------------------------===//
 
 void ento::registerCloneChecker(CheckerManager &Mgr) {
-  Mgr.registerChecker<CloneChecker>();
+  auto *Checker = Mgr.registerChecker<CloneChecker>();
+
+  Checker->MinComplexity = Mgr.getAnalyzerOptions().getCheckerIntegerOption(
+      Checker, "MinimumCloneComplexity", 50);
+
+  if (Checker->MinComplexity < 0)
+    Mgr.reportInvalidCheckerOptionValue(
+        Checker, "MinimumCloneComplexity", "a non-negative value");
+
+  Checker->ReportNormalClones = Mgr.getAnalyzerOptions().getCheckerBooleanOption(
+      Checker, "ReportNormalClones", true);
+
+  Checker->IgnoredFilesPattern = Mgr.getAnalyzerOptions()
+    .getCheckerStringOption(Checker, "IgnoredFilesPattern", "");
 }
 
 bool ento::shouldRegisterCloneChecker(const LangOptions &LO) {
diff --git a/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp
index eab3fa3af95..545f310a51b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp
@@ -13,6 +13,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "clang/AST/ExprCXX.h"
+#include "clang/Driver/DriverDiagnostic.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
@@ -186,13 +187,17 @@ private:
   AggressivenessKind Aggressiveness;
 
 public:
-  void setAggressiveness(StringRef Str) {
+  void setAggressiveness(StringRef Str, CheckerManager &Mgr) {
     Aggressiveness =
         llvm::StringSwitch<AggressivenessKind>(Str)
             .Case("KnownsOnly", AK_KnownsOnly)
             .Case("KnownsAndLocals", AK_KnownsAndLocals)
             .Case("All", AK_All)
-            .Default(AK_KnownsAndLocals); // A sane default.
+            .Default(AK_Invalid);
+
+    if (Aggressiveness == AK_Invalid)
+      Mgr.reportInvalidCheckerOptionValue(this, "WarnOn",
+          "either \"KnownsOnly\", \"KnownsAndLocals\" or \"All\" string value");
   };
 
 private:
@@ -735,7 +740,8 @@ 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", ""));
+      mgr.getAnalyzerOptions().getCheckerStringOption(chk, "WarnOn",
+                                                      "KnownsAndLocals"), mgr);
 }
 
 bool ento::shouldRegisterMoveChecker(const LangOptions &LO) {
diff --git a/clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
index df5b46ebd2e..abc90986f40 100644
--- a/clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
@@ -16,6 +16,7 @@
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/RecordLayout.h"
 #include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/Driver/DriverDiagnostic.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
@@ -348,8 +349,9 @@ void ento::registerPaddingChecker(CheckerManager &Mgr) {
   auto *Checker = Mgr.registerChecker<PaddingChecker>();
   Checker->AllowedPad = Mgr.getAnalyzerOptions()
           .getCheckerIntegerOption(Checker, "AllowedPad", 24);
-  assert(Checker->AllowedPad >= 0 &&
-         "AllowedPad option should be non-negative");
+  if (Checker->AllowedPad < 0)
+    Mgr.reportInvalidCheckerOptionValue(
+        Checker, "AllowedPad", "a non-negative value");
 }
 
 bool ento::shouldRegisterPaddingChecker(const LangOptions &LO) {
diff --git a/clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
index 53da456fd53..187e868fba0 100644
--- a/clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
@@ -20,6 +20,7 @@
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "UninitializedObject.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Driver/DriverDiagnostic.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
@@ -618,10 +619,16 @@ void ento::registerUninitializedObjectChecker(CheckerManager &Mgr) {
       Chk, "CheckPointeeInitialization", /*DefaultVal*/ false);
   ChOpts.IgnoredRecordsWithFieldPattern =
       AnOpts.getCheckerStringOption(Chk, "IgnoreRecordsWithField",
-                                    /*DefaultVal*/ "");
+                                    /*DefaultVal*/ "\"\"");
   ChOpts.IgnoreGuardedFields =
       AnOpts.getCheckerBooleanOption(Chk, "IgnoreGuardedFields",
                                      /*DefaultVal*/ false);
+
+  std::string ErrorMsg;
+  if (!llvm::Regex(ChOpts.IgnoredRecordsWithFieldPattern).isValid(ErrorMsg))
+    Mgr.reportInvalidCheckerOptionValue(Chk, "IgnoreRecordsWithField",
+        "a valid regex, building failed with error message "
+        "\"" + ErrorMsg + "\"");
 }
 
 bool ento::shouldRegisterUninitializedObjectChecker(const LangOptions &LO) {
diff --git a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
index c874ed2ddcd..53d872021af 100644
--- a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
@@ -15,6 +15,7 @@
 #include "clang/AST/Stmt.h"
 #include "clang/Analysis/ProgramPoint.h"
 #include "clang/Basic/LLVM.h"
+#include "clang/Driver/DriverDiagnostic.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
@@ -58,6 +59,15 @@ void CheckerManager::finishedCheckerRegistration() {
 #endif
 }
 
+void CheckerManager::reportInvalidCheckerOptionValue(
+    const CheckerBase *C, StringRef OptionName, StringRef ExpectedValueDesc) {
+
+  Context.getDiagnostics()
+      .Report(diag::err_analyzer_checker_option_invalid_input)
+          << (llvm::Twine() + C->getTagDescription() + ":" + OptionName).str()
+          << ExpectedValueDesc;
+}
+
 //===----------------------------------------------------------------------===//
 // Functions for running checkers for AST traversing..
 //===----------------------------------------------------------------------===//
diff --git a/clang/test/Analysis/copypaste/suspicious-clones.cpp b/clang/test/Analysis/copypaste/suspicious-clones.cpp
index ae29b0e16d1..61eb45a37a0 100644
--- a/clang/test/Analysis/copypaste/suspicious-clones.cpp
+++ b/clang/test/Analysis/copypaste/suspicious-clones.cpp
@@ -1,4 +1,7 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.clone.CloneChecker -analyzer-config alpha.clone.CloneChecker:ReportSuspiciousClones=true  -analyzer-config alpha.clone.CloneChecker:ReportNormalClones=false -analyzer-config alpha.clone.CloneChecker:MinimumCloneComplexity=10 -verify %s
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=alpha.clone.CloneChecker \
+// RUN:   -analyzer-config alpha.clone.CloneChecker:ReportNormalClones=false \
+// RUN:   -analyzer-config alpha.clone.CloneChecker:MinimumCloneComplexity=10
 
 // Tests finding a suspicious clone that references local variables.
 
diff --git a/clang/test/Analysis/cxx-uninitialized-object-unionlike-constructs.cpp b/clang/test/Analysis/cxx-uninitialized-object-unionlike-constructs.cpp
index dc52afd9019..069f960b16d 100644
--- a/clang/test/Analysis/cxx-uninitialized-object-unionlike-constructs.cpp
+++ b/clang/test/Analysis/cxx-uninitialized-object-unionlike-constructs.cpp
@@ -3,6 +3,20 @@
 // RUN:   -analyzer-config alpha.cplusplus.UninitializedObject:IgnoreRecordsWithField="[Tt]ag|[Kk]ind" \
 // RUN:   -std=c++11 -verify  %s
 
+// RUN: not %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.cplusplus.UninitializedObject \
+// RUN:   -analyzer-config \
+// RUN:     alpha.cplusplus.UninitializedObject:IgnoreRecordsWithField="([)]" \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-UNINIT-INVALID-REGEX
+
+// CHECK-UNINIT-INVALID-REGEX: (frontend): invalid input for checker option
+// CHECK-UNINIT-INVALID-REGEX-SAME: 'alpha.cplusplus.UninitializedObject:IgnoreRecordsWithField',
+// CHECK-UNINIT-INVALID-REGEX-SAME: that expects a valid regex, building failed
+// CHECK-UNINIT-INVALID-REGEX-SAME: with error message "parentheses not
+// CHECK-UNINIT-INVALID-REGEX-SAME: balanced"
+
+
 // expected-no-diagnostics
 
 // Both type and name contains "kind".
diff --git a/clang/test/Analysis/outofbound.c b/clang/test/Analysis/outofbound.c
index c4af1fbb901..60190b4bc35 100644
--- a/clang/test/Analysis/outofbound.c
+++ b/clang/test/Analysis/outofbound.c
@@ -2,7 +2,7 @@
 // RUN:   -analyzer-checker=core \
 // RUN:   -analyzer-checker=unix \
 // RUN:   -analyzer-checker=alpha.security.ArrayBound \
-// RUN:   -analyzer-config unix:Optimistic=true
+// RUN:   -analyzer-config unix.DynamicMemoryModeling:Optimistic=true
 
 typedef __typeof(sizeof(int)) size_t;
 void *malloc(size_t);
diff --git a/clang/test/Analysis/padding_c.c b/clang/test/Analysis/padding_c.c
index f4178f54579..9e216a923e7 100644
--- a/clang/test/Analysis/padding_c.c
+++ b/clang/test/Analysis/padding_c.c
@@ -1,4 +1,16 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=optin.performance -analyzer-config optin.performance.Padding:AllowedPad=2 -verify %s
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=optin.performance \
+// RUN:   -analyzer-config optin.performance.Padding:AllowedPad=2
+
+// RUN: not %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=optin.performance.Padding \
+// RUN:   -analyzer-config optin.performance.Padding:AllowedPad=-10 \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-PAD-NEGATIVE-VALUE
+
+// CHECK-PAD-NEGATIVE-VALUE: (frontend): invalid input for checker option
+// CHECK-PAD-NEGATIVE-VALUE-SAME: 'optin.performance.Padding:AllowedPad', that
+// CHECK-PAD-NEGATIVE-VALUE-SAME: expects a non-negative value
 
 #if __has_include(<stdalign.h>)
 #include <stdalign.h>
diff --git a/clang/test/Analysis/undef-buffers.c b/clang/test/Analysis/undef-buffers.c
index 10a46c64f69..70db8eb6e55 100644
--- a/clang/test/Analysis/undef-buffers.c
+++ b/clang/test/Analysis/undef-buffers.c
@@ -2,7 +2,7 @@
 // RUN:   -analyzer-checker=core \
 // RUN:   -analyzer-checker=unix \
 // RUN:   -analyzer-checker=core.uninitialized \
-// RUN:   -analyzer-config unix:Optimistic=true
+// RUN:   -analyzer-config unix.DynamicMemoryModeling:Optimistic=true
 
 typedef __typeof(sizeof(int)) size_t;
 void *malloc(size_t);
diff --git a/clang/test/Analysis/use-after-move.cpp b/clang/test/Analysis/use-after-move.cpp
index 435976c5a7c..837f4c732e9 100644
--- a/clang/test/Analysis/use-after-move.cpp
+++ b/clang/test/Analysis/use-after-move.cpp
@@ -27,6 +27,17 @@
 // RUN:  -analyzer-config cplusplus.Move:WarnOn=All -DAGGRESSIVE\
 // RUN:  -analyzer-checker debug.ExprInspection
 
+// RUN: not %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=cplusplus.Move \
+// RUN:   -analyzer-config cplusplus.Move:WarnOn="a bunch of things" \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-MOVE-INVALID-VALUE
+
+// CHECK-MOVE-INVALID-VALUE: (frontend): invalid input for checker option
+// CHECK-MOVE-INVALID-VALUE-SAME: 'cplusplus.Move:WarnOn', that expects either
+// CHECK-MOVE-INVALID-VALUE-SAME: "KnownsOnly", "KnownsAndLocals" or "All"
+// CHECK-MOVE-INVALID-VALUE-SAME: string value
+
 #include "Inputs/system-header-simulator-cxx.h"
 
 void clang_analyzer_warnIfReached();