[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

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

diff --git a/include/clang/Basic/DiagnosticDriverKinds.td b/include/clang/Basic/DiagnosticDriverKinds.td
index 3ebd931f48e859b5acc4e43be020545fc1229030..c822380a02160ed051a05b2895dfdd44be814e95 100644 (file)
--- a/include/clang/Basic/DiagnosticDriverKinds.td
+++ b/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/include/clang/StaticAnalyzer/Core/CheckerManager.h b/include/clang/StaticAnalyzer/Core/CheckerManager.h
index b4de26d792911f4f53e1b9e0990d48147fa4482c..612286ba8b1ff60d7f06f7d64ef32bef635f81d0 100644 (file)
--- a/include/clang/StaticAnalyzer/Core/CheckerManager.h
+++ b/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/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp b/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
index 6be66d20afc25415007ce91bcf6a9ddd9a092685..a7ca814c8f9672881cb9492ae3597e7030fce14c 100644 (file)
--- a/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
+++ b/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/lib/StaticAnalyzer/Checkers/CloneChecker.cpp b/lib/StaticAnalyzer/Checkers/CloneChecker.cpp
index 84018c5c39bd9a6d179d60789a1521e3dd9dc523..11a33e50bf5f5fac6dcc43afbdfd044dd9599f8c 100644 (file)
--- a/lib/StaticAnalyzer/Checkers/CloneChecker.cpp
+++ b/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/lib/StaticAnalyzer/Checkers/MoveChecker.cpp b/lib/StaticAnalyzer/Checkers/MoveChecker.cpp
index eab3fa3af958553df503f6fc89b750060678193d..545f310a51bf0d9d1bf72f01bb4ae721462d50b2 100644 (file)
--- a/lib/StaticAnalyzer/Checkers/MoveChecker.cpp
+++ b/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/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp b/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
index df5b46ebd2e53825842bce8166659698903afdd1..abc90986f4020cf17220b8ce4ae4a3954f0aae0c 100644 (file)
--- a/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
+++ b/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/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp b/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
index 53da456fd5375cb1a5ee96a9f7a1695e98fec71b..187e868fba0dc654504777d6f6872232858db744 100644 (file)
--- a/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
+++ b/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/lib/StaticAnalyzer/Core/CheckerManager.cpp b/lib/StaticAnalyzer/Core/CheckerManager.cpp
index c874ed2ddcd978e43fb0ad301d5ef7c267701b31..53d872021af5b93c03985792afc28b2eb3ddf6f9 100644 (file)
--- a/lib/StaticAnalyzer/Core/CheckerManager.cpp
+++ b/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/test/Analysis/copypaste/suspicious-clones.cpp b/test/Analysis/copypaste/suspicious-clones.cpp
index ae29b0e16d191f29036053767291218f26b836db..61eb45a37a07e6f658794b4c155fef723e4264e2 100644 (file)
--- a/test/Analysis/copypaste/suspicious-clones.cpp
+++ b/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/test/Analysis/cxx-uninitialized-object-unionlike-constructs.cpp b/test/Analysis/cxx-uninitialized-object-unionlike-constructs.cpp
index dc52afd90195014238504e1ca6bc5f03a42d89b2..069f960b16deb9383bddc3aaafbc027e29758cb7 100644 (file)
--- a/test/Analysis/cxx-uninitialized-object-unionlike-constructs.cpp
+++ b/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/test/Analysis/outofbound.c b/test/Analysis/outofbound.c
index c4af1fbb901f0eb4183eef91ce5c62390aff4cc6..60190b4bc35544ade7d13b546d4a972526abbdd2 100644 (file)
--- a/test/Analysis/outofbound.c
+++ b/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/test/Analysis/padding_c.c b/test/Analysis/padding_c.c
index f4178f545791ad5e7275ff89ad11fba244bcbb7b..9e216a923e707123c631a6db69494ee306515288 100644 (file)
--- a/test/Analysis/padding_c.c
+++ b/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/test/Analysis/undef-buffers.c b/test/Analysis/undef-buffers.c
index 10a46c64f698bbcaea0fcf3ff08613123c9c29f5..70db8eb6e551697042e5c08c3ed5d133efc79861 100644 (file)
--- a/test/Analysis/undef-buffers.c
+++ b/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/test/Analysis/use-after-move.cpp b/test/Analysis/use-after-move.cpp
index 435976c5a7c2962cb600a73c5da2ab4a050180d4..837f4c732e9e708122dd747b4b0a52482c091d18 100644 (file)
--- a/test/Analysis/use-after-move.cpp
+++ b/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();