[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

274

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/Basic/DiagnosticDriverKinds.td

 def err_drv_optimization_remark_pattern : Error<
   "%0 in '%1'">;
 
+def warn_analyzer_config_incorrect_checker_option_section_count : Warning<
+  "analyzer-config option '%0' should have three sections but has %1; ignoring">,
+  InGroup<UnusedCommandLineArgument>;
+def warn_analyzer_config_incorrect_checker_option_empty : Warning<
+  "analyzer-config option '%0' should have first non-empty option; ignoring">,
+  InGroup<UnusedCommandLineArgument>;
 def warn_O4_is_O3 : Warning<"-O4 is equivalent to -O3">, InGroup<Deprecated>;
 def warn_drv_optimization_value : Warning<"optimization level '%0' is unsupported; using '%1%2' instead">,
   InGroup<InvalidCommandLineArgument>;

include/clang/Driver/CC1Options.td

 def analyzer_disable_checker_EQ : Joined<["-"], "analyzer-disable-checker=">,
   Alias<analyzer_disable_checker>;
 
+def analyzer_checker_option : Separate<["-"], "analyzer-checker-option">,
+  HelpText<"Specify checker-specific settings as \"CheckerName:CheckerOption:Value\"">;
+def analyzer_checker_option_EQ : Joined<["-"], "analyzer-checker-option=">,
+  Alias<analyzer_checker_option>;
+
 def analyzer_checker_help : Flag<["-"], "analyzer-checker-help">,
   HelpText<"Display the list of analyzer checkers that are available">;
 

include/clang/StaticAnalyzer/Core/AnalyzerOptions.h

   AnalysisPurgeMode AnalysisPurgeOpt;
   
   std::string AnalyzeSpecificFunction;
-  
+  typedef llvm::StringMap<llvm::StringMap<llvm::StringRef> > CheckerOptionMap;
+  CheckerOptionMap CheckerSpecificOptions;
+
   /// \brief The maximum number of times the analyzer visits a block.
   unsigned maxBlockVisitOnPath;
   

   /// This is controlled by the 'max-nodes' config option.
   unsigned getMaxNodesPerTopLevelFunction();
 
+  /// Returns selected string option for selected checker
+  /// or default value if this option is not specified
+  const StringRef &getCheckerOption(const StringRef &CheckerName,
+                                    const StringRef &OptionName,
+                                    const StringRef &DefaultValue) const;
+
 public:
   AnalyzerOptions() :
     AnalysisStoreOpt(RegionStoreModel),

lib/Frontend/CompilerInvocation.cpp

     for (unsigned i = 0, e = checkers.size(); i != e; ++i)
       Opts.CheckersControlList.push_back(std::make_pair(checkers[i], enable));
   }
-  
+  for (arg_iterator it = Args.filtered_begin(OPT_analyzer_checker_option),
+         ie = Args.filtered_end(); it != ie; ++it) {
+    const Arg *A = *it;
+    A->claim();
+    // Checker option should look like "checkerName:optionName:optionValue"
+    StringRef optionList = A->getValue();
+    SmallVector<StringRef, 3> options;
+    optionList.split(options, ":");
+    if (options.size() != 3) {
+      Diags.Report(SourceLocation(),
+              diag::warn_analyzer_config_incorrect_checker_option_section_count)
+          << optionList
+          << StringRef(llvm::APInt(8, options.size()).toString(10, false));
+      continue;
+    }
+    if (options[0] == "") {
+      Diags.Report(SourceLocation(),
+                   diag::warn_analyzer_config_incorrect_checker_option_empty)
+          << optionList;
+      continue;
+    }
+
+    llvm::StringMap<StringRef> &CheckerOpts =
+        Opts.CheckerSpecificOptions[options[0]];
+    CheckerOpts[options[1]] = options[2];
+  }
+
   // Go through the analyzer configuration options.
   for (arg_iterator it = Args.filtered_begin(OPT_analyzer_config),
        ie = Args.filtered_end(); it != ie; ++it) {

lib/StaticAnalyzer/Core/AnalyzerOptions.cpp

   return MaxNodesPerTopLevelFunction.getValue();
 }
 
+const StringRef &
+AnalyzerOptions::getCheckerOption(const StringRef &CheckerName,
+                                  const StringRef &OptionName,
+                                  const StringRef &DefaultValue) const {
+  CheckerOptionMap::const_iterator CheckerOpts =
+      CheckerSpecificOptions.find(CheckerName);
+  if (CheckerOpts == CheckerSpecificOptions.end())
+    return DefaultValue;
+  llvm::StringMap<llvm::StringRef>::const_iterator Option =
+      CheckerOpts->second.find(OptionName);
+  if (Option == CheckerOpts->second.end())
+    return DefaultValue;
+  return Option->second;
+}
+
 bool AnalyzerOptions::shouldSynthesizeBodies() {
   return getBooleanOption("faux-bodies", true);
 }