[PATCH] D57860: [analyzer] Validate checker option names and values

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

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL361011: [analyzer] Validate checker option names and values 
(authored by Szelethus, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D57860?vs=190992&id=25#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57860/new/

https://reviews.llvm.org/D57860

Files:
  cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td
  cfe/trunk/include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  cfe/trunk/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  cfe/trunk/test/Analysis/checker-plugins.c
  cfe/trunk/test/Analysis/invalid-checker-option.c

Index: cfe/trunk/include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
===
--- cfe/trunk/include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
+++ cfe/trunk/include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
@@ -107,6 +107,17 @@
   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;
 }
   };
 
@@ -150,6 +161,12 @@
   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,
 bool IsHidden)
Index: cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td
@@ -307,6 +307,8 @@
 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">;
 
Index: cfe/trunk/test/Analysis/checker-plugins.c
===
--- cfe/trunk/test/Analysis/checker-plugins.c
+++ cfe/trunk/test/Analysis/checker-plugins.c
@@ -62,3 +62,35 @@
 // RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-CHECKER-OPTION-TRUE
 
 // CHECK-CHECKER-OPTION-TRUE: example.MyChecker:ExampleOption = true
+
+// RUN: not %clang_analyze_cc1 -verify %s \
+// RUN:   -load %llvmshlibdir/CheckerOptionHandlingAnalyzerPlugin%pluginext\
+// RUN:   -analyzer-checker=example.MyChecker \
+// RUN:   -analyzer-config example.MyChecker:Example=true \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-NON-EXISTENT-CHECKER-OPTION
+
+// CHECK-NON-EXISTENT-CHECKER-OPTION: (frontend): checker 'example.MyChecker'
+// CHECK-NON-EXISTENT-CHECKER-OPTION-SAME: has no option called 'Example'
+
+// RUN: not %clang_analyze_cc1 -verify %s \
+// RUN:   -load %llvmshlibdir/CheckerOptionHandlingAnalyzerPlugin%pluginext\
+// RUN:   -analyzer-checker=example.MyChecker \
+// RUN:   -analyzer-config-compatibility-mode=true \
+// RUN:   -analyzer-config example.MyChecker:Example=true
+
+
+// RUN: not %clang_analyze_cc1 -verify %s \
+// RUN:   -load %llvmshlibdir/CheckerOptionHandlingAnalyzerPlugin%pluginext\
+// RUN:   -analyzer-checker=example.MyChecker \
+// RUN:   -analyzer-config example.MyChecker:ExampleOption=example \
+// 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: 'example.MyChecker:ExampleOption', that
+// CHECK-INVALID-BOOL-VALUE-SAME: expects a boolean value
+
+// RUN: not %clang_analyze_cc1 -verify %s \
+// RUN:   -load %llvmshlibdir/CheckerOptionHandlingAnalyzerPlugin%pluginext\
+// RUN:   -analyzer-checker=example.MyChecker \
+// RUN:   -analyzer-config-compatibility-mode=true \
+// RUN:   -analyzer-config example.MyChecker:ExampleOption=example
Inde

[PATCH] D57860: [analyzer] Validate checker option names and values

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57860/new/

https://reviews.llvm.org/D57860



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57860: [analyzer] Validate checker option names and values

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

Szelethus added a comment.

Ping^4


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57860/new/

https://reviews.llvm.org/D57860



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57860: [analyzer] Validate checker option names and values

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

Szelethus added a comment.

Ping^3


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57860/new/

https://reviews.llvm.org/D57860



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57860: [analyzer] Validate checker option names and values

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

Szelethus added a comment.

Ping^2


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57860/new/

https://reviews.llvm.org/D57860



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57860: [analyzer] Validate checker option names and values

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

Szelethus added a comment.

Ping.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57860/new/

https://reviews.llvm.org/D57860



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57860: [analyzer] Validate checker option names and values

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

Szelethus added a comment.

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 
:

> 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 frequent contributor, I 
would personally strongly prefer the analyzer to just terminate and not waste 
my time when I mess up an invocation.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57860/new/

https://reviews.llvm.org/D57860



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57860: [analyzer] Validate checker option names and values

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57860/new/

https://reviews.llvm.org/D57860



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57860: [analyzer] Validate checker option names and values

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin requested changes to this revision.
dcoughlin added a comment.
This revision now requires changes to proceed.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57860/new/

https://reviews.llvm.org/D57860



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57860: [analyzer] Validate checker option names and values

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

Szelethus updated this revision to Diff 190992.
Szelethus added a comment.

Add a test case for checker plugins.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57860/new/

https://reviews.llvm.org/D57860

Files:
  include/clang/Basic/DiagnosticDriverKinds.td
  include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  test/Analysis/checker-plugins.c
  test/Analysis/invalid-checker-option.c

Index: test/Analysis/invalid-checker-option.c
===
--- test/Analysis/invalid-checker-option.c
+++ test/Analysis/invalid-checker-option.c
@@ -14,6 +14,63 @@
 // 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() {}
Index: test/Analysis/checker-plugins.c
===
--- test/Analysis/checker-plugins.c
+++ test/Analysis/checker-plugins.c
@@ -62,3 +62,35 @@
 // RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-CHECKER-OPTION-TRUE
 
 // CHECK-CHECKER-OPTION-TRUE: example.MyChecker:ExampleOption = true
+
+// RUN: not %clang_analyze_cc1 -verify %s \
+// RUN:   -load %llvmshlibdir/OptionHandlingAnalyzerPlugin%pluginext\
+// RUN:   -analyzer-checker=example.MyChecker \
+// RUN:   -analyzer-config example.MyChecker:Example=true \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-NON-EXISTENT-CHECKER-OPTION
+
+// CHECK-NON-EXISTENT-CHECKER-OPTION: (frontend): checker 'example.MyChecker'
+// CHECK-NON-EXISTENT-CHECKER-OPTION-SAME: has no option called 'Example'
+
+// RUN: not %clang_analyze_cc1 -verify %s \
+// RUN:   -load %llvmshlibdir/OptionHandlingAnalyzerPlugin%pluginext\
+// RUN:   -analyzer-checker=example.MyChecker \
+// RUN:   -analyzer-config-compatibility-mode=true \
+// RUN:   -analyzer-config example.MyChecker:Example=true
+
+
+// RUN: not %clang_analyze_cc1 -verify %s \
+// RUN:   -load %llvmshlibdir/OptionHandlingAnalyzerPlugin%pluginext\
+// RUN:   -analyzer-checker=example.MyChecker \
+// RUN:   -analyzer-config example.MyChecker:ExampleOption=example \
+// 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: 'example.MyChecker:ExampleOption', that
+// CHECK-INVALID-BOOL-VALUE-SAME: expects a boolean value
+
+// RUN: not %clang_analyze_cc1 -verify %s \
+// RUN:   -load %llvmshlibdir/OptionHandlingAnalyzerPlugin%pluginext\
+// RUN:   -analyzer-checker=example.MyCheck

[PATCH] D57860: [analyzer] Validate checker option names and values

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

Szelethus updated this revision to Diff 190981.
Szelethus added a comment.

Rebase.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57860/new/

https://reviews.llvm.org/D57860

Files:
  include/clang/Basic/DiagnosticDriverKinds.td
  include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  test/Analysis/invalid-checker-option.c

Index: test/Analysis/invalid-checker-option.c
===
--- test/Analysis/invalid-checker-option.c
+++ test/Analysis/invalid-checker-option.c
@@ -14,6 +14,63 @@
 // 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() {}
Index: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
===
--- lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
+++ lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
@@ -310,18 +310,61 @@
   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)
+<< FullOpt

[PATCH] D57860: [analyzer] Validate checker option names and values

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

Szelethus updated this revision to Diff 190630.
Szelethus added a comment.

Add a testcase for packages.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57860/new/

https://reviews.llvm.org/D57860

Files:
  include/clang/Basic/DiagnosticDriverKinds.td
  include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  test/Analysis/invalid-checker-option.c

Index: test/Analysis/invalid-checker-option.c
===
--- test/Analysis/invalid-checker-option.c
+++ test/Analysis/invalid-checker-option.c
@@ -14,6 +14,63 @@
 // 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() {}
Index: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
===
--- lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
+++ lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
@@ -315,6 +315,50 @@
   CheckerIt->Dependencies.emplace_back(&*DependencyIt);
 }
 
+/// 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 OptionType, StringRef FullName,
+  StringRef OptionName, StringRef DefaultValue,
+  AnalyzerOptions &AnOpts,
+  DiagnosticsEngine &Diags) {
+
+  std::string FullOption = (FullName + ":" + OptionName).str();
+
+  auto It = AnOpts.Config.insert({FullOption, DefaultValue});
+
+  // 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 (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 (OptionType == "int") {
+int Tmp;
+bool HasFailed = SuppliedValue.getAsInteger(0, Tmp);
+if (HasFailed) {
+  if (AnOpts.ShouldEmitErrorsOnInvalidConfigValue) {
+Diags.Report(diag::err_analyzer_checker_option_invalid_input)
+

[PATCH] D57860: [analyzer] Validate checker option names and values

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

Szelethus updated this revision to Diff 190629.
Szelethus marked 6 inline comments as done.
Szelethus added a comment.

Fixes according to inline comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57860/new/

https://reviews.llvm.org/D57860

Files:
  include/clang/Basic/DiagnosticDriverKinds.td
  include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  test/Analysis/invalid-checker-option.c

Index: test/Analysis/invalid-checker-option.c
===
--- test/Analysis/invalid-checker-option.c
+++ test/Analysis/invalid-checker-option.c
@@ -14,6 +14,53 @@
 // 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
+
 // expected-no-diagnostics
 
 int main() {}
Index: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
===
--- lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
+++ lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
@@ -315,6 +315,50 @@
   CheckerIt->Dependencies.emplace_back(&*DependencyIt);
 }
 
+/// 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 OptionType, StringRef FullName,
+  StringRef OptionName, StringRef DefaultValue,
+  AnalyzerOptions &AnOpts,
+  DiagnosticsEngine &Diags) {
+
+  std::string FullOption = (FullName + ":" + OptionName).str();
+
+  auto It = AnOpts.Config.insert({FullOption, DefaultValue});
+
+  // 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 (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 (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;
+  }
+}
+
 void CheckerRegistry::addCheckerOption(StringRef OptionType,
StringRef FullName,
StringRef OptionName,
@@ -329,7 +373,8 @@
   CheckerIt->CmdLineOptions.emplace_back(
   OptionType, OptionName, DefaultValStr, Descrip

[PATCH] D57860: [analyzer] Validate checker option names and values

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57860/new/

https://reviews.llvm.org/D57860



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57860: [analyzer] Validate checker option names and values

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added inline comments.



Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:325
+  std::string FullOption =
+  (llvm::Twine() + FullName + ":" + OptionName).str();
+

whisperity wrote:
> baloghadamsoftware wrote:
> > Is this the most efficient way to concatenate `StringRef`s?
> > @baloghadamsoftware 
> > Is this the most efficient way to concatenate `StringRef`s?
> 
> `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()`.
The constructor `Twine(const StringRef &Str)` is implicit so `(FullName + ':' + 
OptionName).str()` results the same and is more readable.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57860/new/

https://reviews.llvm.org/D57860



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57860: [analyzer] Validate checker option names and values

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:325
+  std::string FullOption =
+  (llvm::Twine() + FullName + ":" + OptionName).str();
+

baloghadamsoftware wrote:
> Is this the most efficient way to concatenate `StringRef`s?
> @baloghadamsoftware 
> Is this the most efficient way to concatenate `StringRef`s?

`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()`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57860/new/

https://reviews.llvm.org/D57860



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57860: [analyzer] Validate checker option names and values

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added inline comments.



Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:325
+  std::string FullOption =
+  (llvm::Twine() + FullName + ":" + OptionName).str();
+

Is this the most efficient way to concatenate `StringRef`s?



Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:443
+StringRef SuppliedChecker(Config.first().substr(0, Pos));
+StringRef SuppliedOption(Config.first().drop_front(Pos + 1));
 

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57860/new/

https://reviews.llvm.org/D57860



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57860: [analyzer] Validate checker option names and values

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

Szelethus marked 2 inline comments as done.
Szelethus added inline comments.



Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:340
+  if (OptionType == "bool") {
+if (SuppliedValue != "true" && SuppliedValue != "false") {
+  if (AnOpts.ShouldEmitErrorsOnInvalidConfigValue) {

whisperity wrote:
> 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?
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57860/new/

https://reviews.llvm.org/D57860



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57860: [analyzer] Validate checker option names and values

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:329
+
+  // Insertation was successful -- CmdLineOption's constructor will validate
+  // whether values received from plugins or TableGen files are correct.

Insertion



Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:340
+  if (OptionType == "bool") {
+if (SuppliedValue != "true" && SuppliedValue != "false") {
+  if (AnOpts.ShouldEmitErrorsOnInvalidConfigValue) {

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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57860/new/

https://reviews.llvm.org/D57860



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57860: [analyzer] Validate checker option names and values

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

Szelethus updated this revision to Diff 190023.
Szelethus added a comment.
Herald added subscribers: Charusso, jdoerfert.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57860/new/

https://reviews.llvm.org/D57860

Files:
  include/clang/Basic/DiagnosticDriverKinds.td
  include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  test/Analysis/invalid-checker-option.c

Index: test/Analysis/invalid-checker-option.c
===
--- test/Analysis/invalid-checker-option.c
+++ test/Analysis/invalid-checker-option.c
@@ -14,6 +14,53 @@
 // 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
+
 // expected-no-diagnostics
 
 int main() {}
Index: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
===
--- lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
+++ lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
@@ -314,6 +314,51 @@
   CheckerIt->Dependencies.emplace_back(&*DependencyIt);
 }
 
+/// 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 OptionType, StringRef FullName,
+  StringRef OptionName, StringRef DefaultValue,
+  AnalyzerOptions &AnOpts,
+  DiagnosticsEngine &Diags) {
+
+  std::string FullOption =
+  (llvm::Twine() + FullName + ":" + OptionName).str();
+
+  auto It = AnOpts.Config.insert({FullOption, DefaultValue});
+
+  // Insertation was successful -- CmdLineOption's constructor will validate
+  // whether values received from plugins or TableGen files are correct.
+  if (It.second)
+return;
+
+  // Insertation 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 (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 (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;
+  }
+}
+
 void CheckerRegistry::addCheckerOption(StringRef OptionType,
StringRef CheckerFullName,
   

[PATCH] D57860: [analyzer] Validate checker option names and values

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

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

Whoa!!

*celebrates the graphomania epidemic*


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57860/new/

https://reviews.llvm.org/D57860



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57860: [analyzer] Validate checker option names and values

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

Szelethus created this revision.
Szelethus added reviewers: NoQ, george.karpenkov, xazax.hun, rnkovacs, 
baloghadamsoftware.
Herald added subscribers: cfe-commits, gamesh411, dkrupp, donat.nagy, 
mikhail.ramalho, a.sidorin, szepet, whisperity.
Herald added a project: clang.

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`.


Repository:
  rC Clang

https://reviews.llvm.org/D57860

Files:
  include/clang/Basic/DiagnosticDriverKinds.td
  lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  test/Analysis/invalid-checker-option.c

Index: test/Analysis/invalid-checker-option.c
===
--- test/Analysis/invalid-checker-option.c
+++ test/Analysis/invalid-checker-option.c
@@ -14,6 +14,53 @@
 // 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
+
 // expected-no-diagnostics
 
 int main() {}
Index: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
===
--- lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
+++ lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
@@ -365,6 +365,55 @@
   }
 }
 
+static void validateOptionNameAndValue(
+const CheckerRegistry::CmdLineOptionList &OptionList,
+StringRef SuppliedChecker,
+StringRef SuppliedOption,
+StringRef SuppliedValue,
+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);
+
+  // In this case we know that Supplied* point to different parts of the same
+  // string.
+  StringRef FullOption(SuppliedChecker.data(),
+   SuppliedChecker.size() + SuppliedOption.size() + 1);
+
+  if (OptionIt == OptionList.end()) {
+Diags.Report(diag::err_analyzer_checker_option_unknown)
+<< SuppliedChecker << SuppliedOption;
+return;
+  }
+
+  if (OptionIt->OptionType == "bool") {
+if (SuppliedValue != "true" && SuppliedValue != "false") {
+  Diags.Report(diag::err_analyzer_checker_option_invalid_input)
+  << FullOption << "a boolean";
+