[PATCH] D53280: [analyzer] Emit a warning for unknown -analyzer-config options

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

Szelethus planned changes to this revision.
Szelethus added a comment.

Since I've dug myself quite deep into the refactoring effort, I realized that 
there are more elegant ways of achieving this, should I hammer 
`AnalyzerOptions` for long enough. I'll probably change everything but the 
revision title.


https://reviews.llvm.org/D53280



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

[PATCH] D53280: [analyzer] Emit a warning for unknown -analyzer-config options

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

xazax.hun added a comment.

I agree with NoQ. Forward and backward compatibility might be important for 
CodeChecker as well.
But I wonder if it make sense to have analyzer-config compatibility mode on a 
per config basis?
E.g., if we have two configs:

- One did not exist in earlier clang versions, but a tool (like CodeChecker) 
would like to pass this to the analyzer without doing a version check first. 
Passing this in a compatibility mode makes sense. This could be the regural 
`-analyzer-config`
- The second option also did not exist in earlier clang versions, but we do not 
want to support those versions. In the case passing this config in a more 
strict mode makes sense. This could be something like `-analyzer-config-strict`.

Or we could choose the deafults the other way around.

What do you think? Does it make sense to have the compatibility mode on a per 
config bases or too much code for too little gain?




Comment at: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:323
+  std::vector getConfigOptionList() const {
+return {
+#define ANALYZER_OPTION(TYPE, NAME, CMDFLAG, DESC) \

I wonder if it is worth to store this in a static object, have it sorted and 
use binary search for the warning. If we do not expect to have a lot more 
options in the future I am fine with the current solution.


https://reviews.llvm.org/D53280



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

[PATCH] D53280: [analyzer] Emit a warning for unknown -analyzer-config options

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.
Herald added a subscriber: dkrupp.

I guess we should consider the idea in 
http://lists.llvm.org/pipermail/cfe-dev/2018-October/059864.html




Comment at: lib/Frontend/CompilerInvocation.cpp:343
   }
+  // Check whether this really is a valid -analyzer-condfig option.
+  // TODO: We should check whether all options are valid or not, but for

typo: "config"


https://reviews.llvm.org/D53280



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

[PATCH] D53280: [analyzer] Emit a warning for unknown -analyzer-config options

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

Szelethus updated this revision to Diff 169693.

https://reviews.llvm.org/D53280

Files:
  include/clang/Basic/DiagnosticDriverKinds.td
  include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  lib/Frontend/CompilerInvocation.cpp


Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -314,6 +314,8 @@
   Opts.CheckersControlList.emplace_back(checker, enable);
   }
 
+  const std::vector RegisteredOptions = Opts.getConfigOptionList();
+
   // Go through the analyzer configuration options.
   for (const auto *A : Args.filtered(OPT_analyzer_config)) {
 A->claim();
@@ -338,6 +340,14 @@
 Success = false;
 break;
   }
+  // Check whether this really is a valid -analyzer-condfig option.
+  // TODO: We should check whether all options are valid or not, but for
+  // now, skip checker options.
+  if (key.count(':') == 0) {
+if (llvm::find(RegisteredOptions, key) == RegisteredOptions.end())
+  Diags.Report(diag::warn_analyzer_config_unknown_config) << key;
+  }
+
   Opts.Config[key] = val;
 }
   }
Index: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
===
--- include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
+++ include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
@@ -319,6 +319,15 @@
   template 
   T getDefaultValForUserMode(T ShallowVal, T DeepVal);
 
+  std::vector getConfigOptionList() const {
+return {
+#define ANALYZER_OPTION(TYPE, NAME, CMDFLAG, DESC) \
+  CMDFLAG,
+#include "clang/StaticAnalyzer/Core/AnalyzerOptions.def"
+#undef ANALYZER_OPTION
+};
+  }
+
 #define ANALYZER_OPTION_WITH_FN(TYPE, NAME, CMDFLAG, DESC, DEFAULT_VAL, \
 CREATE_FN)  \
   TYPE CREATE_FN();
Index: include/clang/Basic/DiagnosticDriverKinds.td
===
--- include/clang/Basic/DiagnosticDriverKinds.td
+++ include/clang/Basic/DiagnosticDriverKinds.td
@@ -291,6 +291,9 @@
   "analyzer-config option '%0' has a key but no value">;
 def err_analyzer_config_multiple_values : Error<
   "analyzer-config option '%0' should contain only one '='">;
+def warn_analyzer_config_unknown_config : Warning<
+  "unkown -analyzer-config option '%0'">,
+  InGroup;
 
 def err_drv_invalid_hvx_length : Error<
   "-mhvx-length is not supported without a -mhvx/-mhvx= flag">;


Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -314,6 +314,8 @@
   Opts.CheckersControlList.emplace_back(checker, enable);
   }
 
+  const std::vector RegisteredOptions = Opts.getConfigOptionList();
+
   // Go through the analyzer configuration options.
   for (const auto *A : Args.filtered(OPT_analyzer_config)) {
 A->claim();
@@ -338,6 +340,14 @@
 Success = false;
 break;
   }
+  // Check whether this really is a valid -analyzer-condfig option.
+  // TODO: We should check whether all options are valid or not, but for
+  // now, skip checker options.
+  if (key.count(':') == 0) {
+if (llvm::find(RegisteredOptions, key) == RegisteredOptions.end())
+  Diags.Report(diag::warn_analyzer_config_unknown_config) << key;
+  }
+
   Opts.Config[key] = val;
 }
   }
Index: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
===
--- include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
+++ include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
@@ -319,6 +319,15 @@
   template 
   T getDefaultValForUserMode(T ShallowVal, T DeepVal);
 
+  std::vector getConfigOptionList() const {
+return {
+#define ANALYZER_OPTION(TYPE, NAME, CMDFLAG, DESC) \
+  CMDFLAG,
+#include "clang/StaticAnalyzer/Core/AnalyzerOptions.def"
+#undef ANALYZER_OPTION
+};
+  }
+
 #define ANALYZER_OPTION_WITH_FN(TYPE, NAME, CMDFLAG, DESC, DEFAULT_VAL, \
 CREATE_FN)  \
   TYPE CREATE_FN();
Index: include/clang/Basic/DiagnosticDriverKinds.td
===
--- include/clang/Basic/DiagnosticDriverKinds.td
+++ include/clang/Basic/DiagnosticDriverKinds.td
@@ -291,6 +291,9 @@
   "analyzer-config option '%0' has a key but no value">;
 def err_analyzer_config_multiple_values : Error<
   "analyzer-config option '%0' should contain only one '='">;
+def warn_analyzer_config_unknown_config : Warning<
+  "unkown -analyzer-config option '%0'">,
+  InGroup;
 
 def err_drv_invalid_hvx_length : Error<
   "-mhvx-length is not supported without a -mhvx/-mhvx= flag">;
___
cfe-commits mailing list
cfe-commits@lists.llvm.o

[PATCH] D53280: [analyzer] Emit a warning for unknown -analyzer-config options

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

Szelethus added a comment.

@whisperity @xazax.hun A worry of mine is shared libraries, for example, we've 
got an array of Ericsson-specific checkers that we load run-time. Do we support 
(or should we) support acquiring non-checker `-analyzer-config` options?


Repository:
  rC Clang

https://reviews.llvm.org/D53280



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

[PATCH] D53280: [analyzer] Emit a warning for unknown -analyzer-config options

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

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

I'm in the process of refactoring AnalyzerOptions. The main motivation behind 
here is to emit warnings if an invalid -analyzer-config option is given from 
the command line, and be able to list them all.

In this patch, I'll implement the warning.


Repository:
  rC Clang

https://reviews.llvm.org/D53280

Files:
  include/clang/Basic/DiagnosticDriverKinds.td
  include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  lib/Frontend/CompilerInvocation.cpp


Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -314,6 +314,8 @@
   Opts.CheckersControlList.emplace_back(checker, enable);
   }
 
+  std::vector RegisteredOptions = Opts.getConfigOptionList();
+
   // Go through the analyzer configuration options.
   for (const auto *A : Args.filtered(OPT_analyzer_config)) {
 A->claim();
@@ -338,6 +340,13 @@
 Success = false;
 break;
   }
+  // Check whether this really is a valid -analyzer-condfig option.
+  // TODO: We should check whether all options are valid or not, but for
+  // now, skip checker options.
+  if (key.count(':') == 0) {
+if (llvm::find(RegisteredOptions, key) == RegisteredOptions.end())
+  Diags.Report(diag::warn_analyzer_config_unknown_config) << key;
+  }
   Opts.Config[key] = val;
 }
   }
Index: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
===
--- include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
+++ include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
@@ -320,6 +320,15 @@
   template 
   T getDefaultValForUserMode(T ShallowVal, T DeepVal);
 
+  std::vector getConfigOptionList() const {
+return {
+#define ANALYZER_OPTION(TYPE, NAME, CMDFLAG, DESC, DEFAULT_VAL) \
+  CMDFLAG,
+#include "clang/StaticAnalyzer/Core/AnalyzerOptions.def"
+#undef ANALYZER_OPTION
+};
+  }
+
 #define ANALYZER_OPTION_WITH_FN(TYPE, NAME, CMDFLAG, DESC, DEFAULT_VAL, \
 CREATE_FN)  \
   TYPE CREATE_FN();
Index: include/clang/Basic/DiagnosticDriverKinds.td
===
--- include/clang/Basic/DiagnosticDriverKinds.td
+++ include/clang/Basic/DiagnosticDriverKinds.td
@@ -291,6 +291,9 @@
   "analyzer-config option '%0' has a key but no value">;
 def err_analyzer_config_multiple_values : Error<
   "analyzer-config option '%0' should contain only one '='">;
+def warn_analyzer_config_unknown_config : Warning<
+  "unkown -analyzer-config option '%0'">,
+  InGroup;
 
 def err_drv_invalid_hvx_length : Error<
   "-mhvx-length is not supported without a -mhvx/-mhvx= flag">;


Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -314,6 +314,8 @@
   Opts.CheckersControlList.emplace_back(checker, enable);
   }
 
+  std::vector RegisteredOptions = Opts.getConfigOptionList();
+
   // Go through the analyzer configuration options.
   for (const auto *A : Args.filtered(OPT_analyzer_config)) {
 A->claim();
@@ -338,6 +340,13 @@
 Success = false;
 break;
   }
+  // Check whether this really is a valid -analyzer-condfig option.
+  // TODO: We should check whether all options are valid or not, but for
+  // now, skip checker options.
+  if (key.count(':') == 0) {
+if (llvm::find(RegisteredOptions, key) == RegisteredOptions.end())
+  Diags.Report(diag::warn_analyzer_config_unknown_config) << key;
+  }
   Opts.Config[key] = val;
 }
   }
Index: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
===
--- include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
+++ include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
@@ -320,6 +320,15 @@
   template 
   T getDefaultValForUserMode(T ShallowVal, T DeepVal);
 
+  std::vector getConfigOptionList() const {
+return {
+#define ANALYZER_OPTION(TYPE, NAME, CMDFLAG, DESC, DEFAULT_VAL) \
+  CMDFLAG,
+#include "clang/StaticAnalyzer/Core/AnalyzerOptions.def"
+#undef ANALYZER_OPTION
+};
+  }
+
 #define ANALYZER_OPTION_WITH_FN(TYPE, NAME, CMDFLAG, DESC, DEFAULT_VAL, \
 CREATE_FN)  \
   TYPE CREATE_FN();
Index: include/clang/Basic/DiagnosticDriverKinds.td
===
--- include/clang/Basic/DiagnosticDriverKinds.td
+++ include/clang/Basic/DiagnosticDriverKinds.td
@@ -291,6 +291,9 @@
   "analyzer-config option '%0' has a