Page MenuHomePhabricator

[Proposal] [Analyzer] Individual options for checkers
AbandonedPublic

Authored by a.sidorin on May 30 2014, 6:50 AM.

Details

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?

Diff Detail

Event Timeline

a.sidorin updated this revision to Diff 9958.May 30 2014, 6:50 AM
a.sidorin retitled this revision from to [Proposal] [Analyzer] Individual options for checkers.
a.sidorin updated this object.
a.sidorin edited the test plan for this revision. (Show Details)
a.sidorin added a reviewer: jordan_rose.
a.sidorin added a subscriber: Unknown Object (MLST).

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 updated this revision to Diff 12582.Aug 16 2014, 6:48 AM
a.sidorin edited edge metadata.

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

krememek edited edge metadata.Aug 16 2014, 11:03 AM

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

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 added inline comments.Aug 19 2014, 2:52 PM
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'.

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

a.sidorin updated this revision to Diff 12691.Aug 20 2014, 12:34 AM
a.sidorin edited edge metadata.

Add doxygen comments, replace pass method for StringRefs.

zaks.anna edited edge metadata.Sep 26 2014, 5:59 PM

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 updated this revision to Diff 14945.Oct 15 2014, 9:02 AM
a.sidorin edited edge metadata.

This is how MallocChecker looks with options used.

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?

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

Thanks Alexei! I've replied.

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.

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 added a comment.EditedFeb 18 2015, 11:32 AM

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 <ganna@apple.com> 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.
"

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 abandoned this revision.Sep 16 2015, 1:24 AM

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