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

2019-04-11 Thread 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

2019-03-23 Thread 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

2019-03-18 Thread 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

2019-03-17 Thread 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

2019-03-16 Thread 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:   

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

2019-03-16 Thread 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 ,
+  AnalyzerOptions ,
+  DiagnosticsEngine ) {
+
+  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 

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

2019-03-14 Thread 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 ,
+  DiagnosticsEngine ) {
+
+  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

2019-03-14 Thread 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 ,
+  DiagnosticsEngine ) {
+
+  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, Description);
 
-  

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

2019-03-14 Thread 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

2019-03-12 Thread 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 )` 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

2019-03-12 Thread 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

2019-03-12 Thread 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

2019-03-12 Thread 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

2019-03-12 Thread 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

2019-03-10 Thread 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 ,
+  DiagnosticsEngine ) {
+
+  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

2019-02-08 Thread 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

2019-02-06 Thread 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 ,
+StringRef SuppliedChecker,
+StringRef SuppliedOption,
+StringRef SuppliedValue,
+const AnalyzerOptions ,
+DiagnosticsEngine ) {
+
+  if (!AnOpts.ShouldEmitErrorsOnInvalidConfigValue)
+return;
+
+  using CmdLineOption = CheckerRegistry::CmdLineOption;
+
+  auto SameOptName = [SuppliedOption](const CmdLineOption ) {
+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";
+  return;
+}
+  }
+