Page MenuHomePhabricator

[Analyzer] Individual options for checkers #2
ClosedPublic

Authored by xazax.hun on Feb 26 2015, 5:06 AM.

Details

Summary

This is a revised version of the original proposal that can be found here: http://reviews.llvm.org/D3967
The discussions on the mailing lists: http://lists.cs.uiuc.edu/pipermail/cfe-dev/2014-October/039552.html

Changes since the original proposal:

  • Option inheritance is now op-in
  • Ability to query package options
  • Updated to recent trunk
  • Minor coding style fixes
  • Replaced some std::string with StringRefs where appropriate

This patch may lack some regression tests but I wanted to get some input on the changes, documentation, API before writing those.

Diff Detail

Repository
rL LLVM

Event Timeline

xazax.hun updated this revision to Diff 20746.Feb 26 2015, 5:06 AM
xazax.hun retitled this revision from to [Analyzer] Individual options for checkers #2.
xazax.hun updated this object.
xazax.hun edited the test plan for this revision. (Show Details)
xazax.hun added a subscriber: Unknown Object (MLST).
zaks.anna edited edge metadata.Feb 26 2015, 1:50 PM

I'd also have "Optimistic" be an option of malloc checker, not unix package since there are no other optimistic/pessimistic checkers in unix. The only reason to do that would be testing, but maybe we could test in other ways.

Also, It would be good to have this extra checking, but can come as port of a later commit:
"
To avoid ambiguities with regular options, we should enforce the following:

  1. <Option Name> should be an identifier
  2. Checker names should be identifiers.
  3. Package names should be identifiers joined with '.’.
  4. <Full Checker Name> has the same form as package names.

"

Thank you!
Anna.

include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
191 ↗(On Diff #20746)

We were trying to stay away from inheritance of options not to imply that the checkers will automatically inherit options. See the discussion on the proposal thread:
http://lists.cs.uiuc.edu/pipermail/cfe-dev/2014-October/039576.html

291 ↗(On Diff #20746)

I'd simplify this and have it take a book instead of the Kind.

I agree that the Optimistic option for the malloc checker should not be inherited.

About the testing:
I think the most appropriate way to test this feature would be to write some unit tests for AnalyzerOptions class. However there are no unittests for the static analyzer yet. Should I create some as part of this patch?

About the validation:
Right now there is no way for a checker to specify the package or checker name when querying an option. The getCheckerOption function is private (the config table is public tough). The checkers can use getBooleanOption with a pointer to the checker, this means filling the package name and checker name is automatic. With this API a checker can not query the options of an unrelated package and there is no way to misspell the package or the checker name. Right now there is no validation for the option name. Should global option names for the static analyzer be validated as well?

include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
191 ↗(On Diff #20746)

I think the inheritance is explicit in this case, the checker have to specify it when querying the option. Should I change something here (e.g. remove the inherited versions or modify their semantics) or you only intended to link the discussion for other reviewers?

291 ↗(On Diff #20746)

Could you elaborate on that?

xazax.hun updated this revision to Diff 20841.Feb 27 2015, 3:17 AM
xazax.hun edited edge metadata.

Changes since the last version:

  • Added validation for the command line option parsing of checker options.
  • The Optimistic option for the Malloc checker is no longer inherited.
a.sidorin edited edge metadata.Feb 27 2015, 10:05 AM

Thank you for your work Gabor. I'm sorry for the inconvenience but I was a little busy these days to continue this work. i didn't find any issues in this patch except a small nit.

a.sidorin added inline comments.Feb 27 2015, 10:08 AM
include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
303 ↗(On Diff #20841)

wether => whether :) And the same on the next line.

Right now there is no way for a checker to specify the package or checker name when querying an option. The getCheckerOption function is private
(the config table is public tough). The checkers can use getBooleanOption with a pointer to the checker, this means filling the package name and >checker name is automatic. With this API a checker can not query the options of an unrelated package and there is no way to misspell the package or
the checker name.

The model I was going for was simpler. A checker would be allowed to query options that are set on itself or a given package. For that, we would want to expose the string based APIs to the checkers. However, this solution is good as well and does have a benefit of being less error prone. The only downside that I see with this solution is that a checker will not be able to search options of other checkers/packages. Maybe it's fine to disallow that. What do you think?

I would like to stay away from mentioning "inheritance" in these APIs. Here is a summary of the thread where this was discussed (on the "Static Analyzer Checker Options Proposal" thread):

Anna (initial proposal):
"However, there will be no inheritance (i.e. the setting 'unix:Optimistic' is entirely distinct from the setting ‘unix.Malloc:Optimistic’)."
Gabor Kozar:
"I think inheritance like that could be useful in some situations."
Anna:
"The main reason is that we currently do not have a need for it and allowing inheritance requires a design for it. For example, what happens when a package adds an option? How would the checkers access it? If we were to allow dynamically loaded checkers how would they inherit it? What happens if a checker overrides a package option?"
Gabor Kozar:
"I would expect such options to be inherited, i.e. if I set 'unix:Optimistic', then I expect this to be visible everywhere in the 'unix' package. Why are disallowing this?"
Anna:
"The package options will be visible to checkers; specifically, checkers could query the package options. For example, MallocChecker could call getOption(Optimistic, "unix") to check if the option has been set on the package. "
Gabor Kozar:
"How about the getOption() function getting a boolean parameter specifying whether to allow "inherited" options, with the default being true?

getOption("unix.stuff.moo.FooChecker", "MyOption", true) would then be equivalent to trying to all of these:
unix.stuff.moo.FooChecker:MyOption
unix.stuff.moo:MyOption
unix.stuff:MyOption
unix:MyOption
I think this behavior would be clear and straight-forward."
Anna:
"This looks good to me. I was mainly thinking/talking about implicit inheritance. Improving the way checkers can query options is a definite plus."

Given the above, the name "OptionKind" is confusing. (It implies that options have kinds, but we don't specify/enforce option kinds anywhere. We don't have a notion of an inherited option that will be automatically applied to all checkers in that package.) It's not an option kind but rather a search mode requested by the checker. It could be as easy as just a bool - search for the option in all parent packages or only search in this checker. In this patch, you allow many more ways of searching (OptionKind), which is fine if well documented and tested. However, if no one needs to all of these, implementing them could be an overkill at this point. It also complicated the API. We also need to document what happens when an option is overriden. The more kinds we have, the more complex the rule will be.

I think the most appropriate way to test this feature would be to write some unit tests for AnalyzerOptions class. However there are no
unittests for the static analyzer yet. Should I create some as part of this patch?

If you volunteer to do this, that would be awesome! There is no good way of testing these now.

xazax.hun updated this revision to Diff 20998.Mar 2 2015, 4:35 AM
xazax.hun edited edge metadata.

Changes since the last revision:

  • Updated to current head
  • Replaced option kind with a bool value
  • Added a unit test

The model I was going for was simpler. A checker would be allowed to query options that are set on itself or a given package. For that, we would want to expose the string based APIs to the checkers. However, this solution is good as well and does have a benefit of being less error prone. The only downside that I see with this solution is that a checker will not be able to search options of other checkers/packages. Maybe it's fine to disallow that. What do you think?

Right now the only way to query a package option is to query an option that is not specified for a given checker. I think that is not a problem, because that way every package options can be overridden for a specific checker. I think that the lack of API to query an option of a specific package is not a big limitation either. Checkers that need common knowledge of an option tend to be end up in the same package anyways. However, when such API is required, it can be added.

I would like to stay away from mentioning "inheritance" in these APIs.

Agreed.

Thank you for your work Gabor. I'm sorry for the inconvenience but I was a little busy these days to continue this work. i didn't find any issues in this patch except a small nit.

No problem, thank you for the original patch :)

Could you include more context with the patch? See: http://llvm.org/docs/Phabricator.html

Looks like we don't have a public API for options with string values.

Thanks!
Anna.

include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
275 ↗(On Diff #20998)

Extra "//".
Nit: Please, make sure all comments have proper punctuation. (A period is missing here and in a few other places.)

288 ↗(On Diff #20998)

This is a bit confusing.

311 ↗(On Diff #20998)

This might be more clear: "The optional checker parameter that can be used to restrict the search to the options of this particular checker."

lib/Frontend/CompilerInvocation.cpp
140 ↗(On Diff #20998)

Shouldn't we use an existing clang check to check if substrings are identifiers?

lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
110 ↗(On Diff #20998)

The comment needs to move and be rephrased (there is no inheritance and checkers are organized in packages, not categories).

unittests/Analysis/AnalyzerOptionsTest.cpp
47 ↗(On Diff #20998)

I think we need more tests. Ex: overridden options, options set only in a package.

unittests/CMakeLists.txt
16 ↗(On Diff #20998)

"Analysis" is too vague if we only plan to use this for the clang static analyzer.

xazax.hun updated this revision to Diff 21192.Mar 4 2015, 5:20 AM

Thank you for the review.

Changes since last revision:

  • Improved the comments as you recommended.
  • Added a public function to query options as string values and appropriate test cases.
  • Added some more test cases to package and checker options.
  • Added more context to the diff.
  • Updated to current head.

Regarding the options validation on the frontend:
Indeed, it would be nicer to use a clang method to check if a string is an identifier however I did not found a nice way to do that. The only way I came up with was to create a raw lexer and lex the option string. If you consider that better than using a RegEx I will alter the patch accordingly.

Regarding the options validation on the frontend:
Indeed, it would be nicer to use a clang method to check if a string is an identifier however I did not found a >nice way to do that. The only way I came up with was to create a raw lexer and lex the option string. If you >consider that better than using a RegEx I will alter the patch accordingly.

It would also be good to check that the names of the checkers and packages are identifiers and that the option is a fully qualified checker name followed by the option name... Let's create a separate patch for validation and check if it can be done more elegantly.

The rest of the patch LGTM and can land. Thank you for working on this!

This revision was automatically updated to reflect the committed changes.

Commited in r231266.

Thank you Aleksei for the original patch. Thank you Anna for your help and review.