[Proposal] [Analyzer] Individual options for checkers

[a.sidorin]

Summary

This patch allows to set up options for checkers. These option may include verbosity, search strategy and others. As I understand, the only way to pass options to checker is to create new checker kinds (for example, MallocPessimistic, MallocOptimistic). This patch allows passing options to checker via '-analyzer-checker-option' argument.
Example: -analyzer-checker-option Malloc:Strategy:Optimistic
Checkers can retrieve options with AnalyzerOptions::getCheckerOption(...) method.
What is your opinion?

[jordan_rose]

Definitely needs a look from Ted or Anna as well. In theory the existing infrastructure can already be used for this, so adding a new flag seems questionable to me:

-analyzer-option unix.Malloc:Strategy=optimistic

Having a helper function that constructs the full name from the checker name and option name seems fine, but doesn't seem like more than a convenience. The real convenience would be adding a helper method on CheckerContext to access options (taking either the combined name or a name that gets built up from the checker and the option).

I'm more interested in coming up with a naming schema here, and specifically I think we should be using the full name of the checker (including the package part) in prefixing options, not just the short name. This also makes more sense if we eventually have options that apply to several checkers—the option prefix could be their common package.

[a.sidorin]

Use analyzer-config command line option instead of a separate option to retrieve checker options.

[krememek]

Hi Aleksei,

Can you explain at a high level what functionality change this patch is implementing? I'd like to understand the motivation a bit more.

Thanks,
Ted

[a.sidorin]

Hello Ted,

This patch (its current version) implements a helper function to retrieve individual options for checkers. Currently the only way to set options for checkers is a registration of a special checker. I tried to implement a more convenient way to do this. For example,

checker->setCheckEnabled(Expr::SET_VolatileRead, getBooleanOption("EmitOnVolatileRead", true, checker));

instead of registration of a new checker (Malloc and CString checkers have even macros to do this). It is only a small convenience and doesn't break any existing functionality.
This should also work for groups of checkers. For example, ... -analyzer-config unix=some_option may be specified to set behaviour for all checkers in unix group.

[krememek]

include/clang/StaticAnalyzer/Core/AnalyzerOptions.h

251

Please add a doxygen comment for this method.

It's not clear if 'CheckerName' needs to be completely "qualified" with the package name.

lib/StaticAnalyzer/Core/AnalyzerOptions.cpp

104–105

Is there a reason to use StringRef&? Why not just use StringRef? This makes it look like we need to modify the StringRef value itself. StringRef is a very lightweight value that we can pass around. There's no need to use '&' or 'const'.

[krememek]

Thanks Aleksei. That explanation helps quite a bit. I've responded in Phabricator with some comments.

[a.sidorin]

Add doxygen comments, replace pass method for StringRefs.

[zaks.anna]

Aleksei,

Thanks for working on this!

I've added a small comment above.

The main problem with this patch is that it is lacking test cases. Is it possible to use it for the cases in CString checker and/or MallocChecker you've mentioned above?

Anna.

include/clang/StaticAnalyzer/Core/AnalyzerOptions.h

257

Please, add a comment about CheckerBase parameter in all the changed public methods.

[a.sidorin]

This is how MallocChecker looks with options used.

[zaks.anna]

Hi Aleksei,

We have discussed the patch and summarized the comments (from Jordan, Ted, and me) on this thread: "[cfe-dev] Static Analyzer Checker Options Proposal". This patch is very close, but does not address some of the points raised in the proposal. Would you be interested in discussing the proposal further?

[a.sidorin]

Hello Anna,

I leaved a comment in that thread (I don't know why they aren't showing
in the main thread).
Please see
http://lists.cs.uiuc.edu/pipermail/cfe-dev/2014-October/039572.html for
this comment and Gabor Kozar's reply.

14.11.2014 02:38, Anna Zaks пишет:

Hi Aleksei,

We have discussed the patch and summarized the comments (from Jordan, Ted, and me) on this thread: "[cfe-dev] Static Analyzer Checker Options Proposal". This patch is very close, but does not address some of the points raised in the proposal. Would you be interested in discussing the proposal further?

http://reviews.llvm.org/D3967

[zaks.anna]

Thanks Alexei! I've replied.

[xazax.hun]

Hi Aleksei!

What is the state of this patch? Do you still working on it? I am very interested in this feature.

One minor note though: there are some hard limit on some platforms on the length of the command line (as far as I know it's about 8k characters on windows. source: http://support.microsoft.com/kb/830473). Command line also contains include paths and several other valuable information. At Ericsson we can have command lines that are several thousand characters long when invoking the static analyzer. Adding configuration options to the command line might result in hitting the character limit. In the long run I think it would be better to support configuration files.

[a.sidorin]

Hello Gábor,

I was thinking that a discussion is dead. It seems like I have fixed all
the issues that were pointed with this review, but there were no
replies. So, I was thinking that nobody needs this functionality.
You can try this patch and leave your opinion about it if you have any
suggestions.
About long command lines... AFAIR command line is a common way to pass
parameters for all parts of Clang, including CSA. Anyway, there are only
few checkers that may support this option now so it cannot add a lot to
the command line currently.

Hi Aleksei!

What is the state of this patch? Do you still working on it? I am very interested in this feature.

One minor note though: there are some hard limit on some platforms on the length of the command line (as far as I know it's about 8k characters on windows. source: http://support.microsoft.com/kb/830473). Command line also contains include paths and several other valuable information. At Ericsson we can have command lines that are several thousand characters long when invoking the static analyzer. Adding configuration options to the command line might result in hitting the character limit. In the long run I think it would be better to support configuration files.

http://reviews.llvm.org/D3967

EMAIL PREFERENCES

http://reviews.llvm.org/settings/panel/emailpreferences/

[zaks.anna]

Aleksei,

There must have been some miscommunication. I've replied the following on the thread discussing the "Static Analyzer Checker Options Proposal". As far as I can tell, your implementation differs to what was proposed. I have not received any reply to that. We are interested in this functionality, but do not agree with some aspects of this patch.

"
On Nov 14, 2014, at 11:24 AM, Anna Zaks <[e-mail]> wrote:

Hi Aleksei,

I believe that with your implementation, it's assumed that the checker always wants to inherit the options of the parent package (it’s not opt in).

Also, since you don’t support query by checker/package name, there is no ability for the checker to query the package options.

Anna.
"

[xazax.hun]

Hi Aleksei,

According to Anna's comment, there are only two things left:

• Adding opt-in for inheritance and turning it off by default.
• Make it possible to query options for the package. There is one nontrivial design decision though. Should it be possible to query the options of another package (e.g.: should the Malloc checker be able to query the options of the osx package)?

Are you going to do these changes? If you need any help feel free to ping me. If you do not want to finish this patch I am volunteering to do it.

lib/StaticAnalyzer/Core/AnalyzerOptions.cpp

114

nit: Local variables should start with upper case letters.

[a.sidorin]

Abandon this change since the patch based on this is committed in http://reviews.llvm.org/D7905

include/clang/StaticAnalyzer/Core/AnalyzerOptions.h

 class DiagnosticsEngine;
 class Preprocessor;
 class LangOptions;
+namespace ento {
+class CheckerBase;
+}
 
 /// Analysis - Set of available source code analyses.
 enum Analyses {

   /// \sa getMaxNodesPerTopLevelFunction
   Optional<unsigned> MaxNodesPerTopLevelFunction;
 
+  StringRef getCheckerOption(const StringRef &CheckerName,
+                             const StringRef &OptionName,
+                             const StringRef &Default) const;
+
 public:
   /// Interprets an option's string value as a boolean.
   ///
   /// Accepts the strings "true" and "false".
   /// If an option value is not provided, returns the given \p DefaultVal.
-  bool getBooleanOption(StringRef Name, bool DefaultVal);
+  bool getBooleanOption(StringRef Name, bool DefaultVal,
+                        const ento::CheckerBase *C = nullptr);
 
   /// Variant that accepts a Optional value to cache the result.
-  bool getBooleanOption(Optional<bool> &V, StringRef Name, bool DefaultVal);
+  bool getBooleanOption(Optional<bool> &V, StringRef Name, bool DefaultVal,
+                        const ento::CheckerBase *C = nullptr);
 
   /// Interprets an option's string value as an integer value.
-  int getOptionAsInteger(StringRef Name, int DefaultVal);
+  int getOptionAsInteger(StringRef Name, int DefaultVal,
+                         const ento::CheckerBase *C = nullptr);
 
   /// \brief Retrieves and sets the UserMode. This is a high-level option,
   /// which is used to set other low-level options. It is not accessible

lib/StaticAnalyzer/Core/AnalyzerOptions.cpp

 //===----------------------------------------------------------------------===//
 
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace clang;
+using namespace ento;
 using namespace llvm;
 
 AnalyzerOptions::UserModeKind AnalyzerOptions::getUserMode() {

 
 static StringRef toString(bool b) { return b ? "true" : "false"; }
 
-bool AnalyzerOptions::getBooleanOption(StringRef Name, bool DefaultVal) {
+StringRef AnalyzerOptions::getCheckerOption(const StringRef &CheckerName,
+                                            const StringRef &OptionName,
+                                            const StringRef &Default) const {
+  std::string Name = CheckerName;
+  std::string Suffix = ":" + OptionName.str();
+  // Search for a category option if option for checker is not specified
+  ConfigTable::const_iterator e = Config.end();
+  while (!Name.empty()) {
+    ConfigTable::const_iterator i = Config.find(Name + Suffix);
+    if (i != e)
+      return StringRef(i->getValue());
+    size_t pos = Name.rfind('.');
+    if (pos == std::string::npos)
+      return Default;
+    Name = Name.substr(0, pos);
+  }
+  return Default;
+}
+
+bool AnalyzerOptions::getBooleanOption(StringRef Name, bool DefaultVal,
+                                       const CheckerBase *C) {
   // FIXME: We should emit a warning here if the value is something other than
   // "true", "false", or the empty string (meaning the default value),
   // but the AnalyzerOptions doesn't have access to a diagnostic engine.
-  StringRef V(Config.GetOrCreateValue(Name, toString(DefaultVal)).getValue());
+  StringRef Default = toString(DefaultVal);
+  StringRef V = C ? getCheckerOption(C->getTagDescription(), Name, Default)
+                  : StringRef(Config.GetOrCreateValue(Name,Default).getValue());
   return llvm::StringSwitch<bool>(V)
       .Case("true", true)
       .Case("false", false)

 }
 
 bool AnalyzerOptions::getBooleanOption(Optional<bool> &V, StringRef Name,
-                                       bool DefaultVal) {
+                                       bool DefaultVal, const CheckerBase *C) {
   if (!V.hasValue())
-    V = getBooleanOption(Name, DefaultVal);
+    V = getBooleanOption(Name, DefaultVal, C);
   return V.getValue();
 }
 

                           /* Default = */ false);
 }
 
-int AnalyzerOptions::getOptionAsInteger(StringRef Name, int DefaultVal) {
+int AnalyzerOptions::getOptionAsInteger(StringRef Name, int DefaultVal,
+                                        const CheckerBase *C) {
   SmallString<10> StrBuf;
   llvm::raw_svector_ostream OS(StrBuf);
   OS << DefaultVal;
   
-  StringRef V(Config.GetOrCreateValue(Name, OS.str()).getValue());
+  StringRef V = C ? getCheckerOption(C->getTagDescription(), Name, OS.str())
+                  : StringRef(Config.GetOrCreateValue(Name,OS.str()).getValue());
   int Res = DefaultVal;
   bool b = V.getAsInteger(10, Res);
   assert(!b && "analyzer-config option should be numeric");