Page MenuHomePhabricator

[analyzer] Restrict AnalyzerOptions' interface so that non-checker objects have to be registered
ClosedPublic

Authored by Szelethus on Oct 21 2018, 2:58 PM.

Details

Summary

One of the reasons why AnalyzerOptions is so chaotic is that options can be retrieved from the command line whenever and wherever. This allowed for some options to be forgotten for a looooooong time. Have you ever heard of "region-store-small-struct-limit"? In order to prevent this in the future, I'm proposing to restrict AnalyzerOptions' interface so that only checker options can be retrieved without special getters. I would like to make every option be accessible only through a getter, but checkers from plugins are a thing, so I'll have to figure something out for that.

This also forces developers who'd like to add a new option to register it properly in the .def file.

This is done by

  • making the third checker pointer parameter non-optional, and checked by an assert to be non-null.
  • I added new, but private non-checkers option initializers, meant only for internal use,
  • Renamed these methods accordingly (mind the consistent name for once with getBooleanOption!):
    • getOptionAsString -> getCheckerStringOption,
    • getOptionAsInteger -> getCheckerIntegerOption
  • The 3 functions meant for initializing data members (with the not very descriptive getBooleanOption, getOptionAsString and getOptionAsUInt names) were renamed to be overloads of the getAndInitOption function name.
  • All options were in some way retrieved via getCheckerOption. I removed it, and moved the logic to getStringOption and getCheckerStringOption. This did cause some code duplication, but that's the only way I could do it, now that checker and non-checker options are separated. Note that the non-checker version inserts the new option to the ConfigTable with the default value, but the checker version only attempts to find already existing entries. This is how it always worked, but this is clunky and I might end reworking that too, so we can eventually get a ConfigTable that contains the entire configuration of the analyzer.

Diff Detail

Repository
rL LLVM

Event Timeline

Szelethus created this revision.Oct 21 2018, 2:58 PM
Szelethus edited the summary of this revision. (Show Details)Oct 21 2018, 3:08 PM
Szelethus edited the summary of this revision. (Show Details)Oct 21 2018, 3:53 PM

Overall looks good if the community agrees with the directions. Some comments inline.

include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
243 ↗(On Diff #170356)

If you want the devs to maintain an explicit getter for each analyzer option rather than making this one public at some point, please document expectation this somewhere.

lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
157 ↗(On Diff #170356)

Can this assert be triggered using a bad invocation of the analyzer? I wonder if it is a good idea to use asserts to validate user input. Maybe it would be better to generate a warning and return the default value?

205 ↗(On Diff #170356)

Same as above.

OK, so the overall direction makes sense: unregistered options are restricted to checkers, and for others, an explicit getter must be maintained.
(though I would also prefer if even checkers could pre-register their options somehow)

@NoQ does this make sense to you?

include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
243 ↗(On Diff #170356)

+1

(though I would also prefer if even checkers could pre-register their options somehow)

Good news, I've also successfully modified the tblgen file Checkers.td to be able to register checker options (it took waaaay more effort and I thought it'd take), so a list of non-plugin checker options should only be a patch away. Still struggling with the plugin part, but I'm making progress.

lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
157 ↗(On Diff #170356)

This is the current state of things, but I intend to change it later in a followup patch. I am not even sure what the thinking was behind this solution myself.

Szelethus updated this revision to Diff 171060.EditedOct 25 2018, 4:19 AM
Szelethus edited the summary of this revision. (Show Details)

This diff has numerous changes to the last, but it's all "cosmetic", the actual logic is untouched.

This was done to ease on followup patches.

  • Added documentation as @xazax.hun suggested
  • Removed getIntegerOption, getBooleanOption, which I realized were redundant with getOption. Left getStringOption in, which is now a private function.
  • getOption, used internally to initialize config fields, was renamed to initOption and it's function changed accordingly (is now void).
  • Moved the generated getter functions inline.
  • (woohoo!) Realize that checker options do not modify the state of AnalyzerOptions, make them const.
Szelethus marked 2 inline comments as done.Oct 25 2018, 4:21 AM
Szelethus marked 3 inline comments as done.

Changing asserts to warnings will be delivered in a followup patch.

Szelethus edited the summary of this revision. (Show Details)Oct 25 2018, 4:28 AM

A polite ping :)

NoQ added inline comments.Nov 1 2018, 9:10 PM
include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
146–149 ↗(On Diff #171060)

Nono, don't add more -cc1 flags :)

lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
172 ↗(On Diff #171060)

I guess @xazax.hun's comment also applies to this assert.

Szelethus added inline comments.Nov 2 2018, 4:21 AM
include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
146–149 ↗(On Diff #171060)

Code review is there to stop adding unnecessary -cc1 flags. Are we sure we wouldn't even like to document it? I myself will add at least 2 more -cc1 flags in the future (-analyzer-config-help, -analyzer-checker-option-help), that undoubtedly belong there.

lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
172 ↗(On Diff #171060)

Which is sadly only going to be addressed in followup patches :/ Note that I merely moved code around in this patch, but have organized things in a way that emitting proper diagnostics will be super easy going forward. Since analyzer configs are mainly meant for developers, I think this is acceptable before I fix it (in any case, it has always been like this).

NoQ accepted this revision.Nov 2 2018, 11:51 AM
NoQ added inline comments.
include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
146–149 ↗(On Diff #171060)

But these flags wouldn't define new analyzer options(?)

lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
172 ↗(On Diff #171060)

Hmm, right. I guess @xazax.hun's comment also applies to all other asserts all over this code.

Szelethus added inline comments.Nov 2 2018, 12:05 PM
include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
146–149 ↗(On Diff #171060)

That is correct, but we do store similar cc1 flags here, because they belong to the the analyzer.

Although, hm, some of those would be more fitting as -analyzer-config flags, but I don't see how I could pull that off in a backward compatible way.

NoQ added inline comments.Nov 2 2018, 12:10 PM
include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
146–149 ↗(On Diff #171060)

Those were there before AnalyzerOptions were invented. That said, @george.karpenkov just made an experiment with converting -analyzer-eagerly-assume into -analyzer-config eagerly-assume (D51251) and it seems to have went well. For feature flags that were not really ever supposed to have been appended by GUIs, it is enough to just make sure that the correct behavior is enabled by default; there's no need to maintain backwards compatibility.

Szelethus added inline comments.Nov 2 2018, 12:55 PM
include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
146–149 ↗(On Diff #171060)

Hmm, maybe I could add that to my TODO list :)

I think the comment should still mention cc1 flags, but with an addition:

Add a -cc1 only if your flag is related to the Static Analyzer, but doesn't actually change the actual analysis.

NoQ added inline comments.Nov 2 2018, 12:58 PM
include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
146–149 ↗(On Diff #171060)

Sounds good! Or something like that: "TODO: Some of these options are controlled by raw frontend flags. These should be eventually converted into -analyzer-config flags. New analyzer options should not be implemented as frontend flags. Frontend flags still make sense for things that do not affect the actual analysis."

Szelethus added inline comments.Nov 2 2018, 1:02 PM
include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
146–149 ↗(On Diff #171060)

Will do that! Aaaafter I'm confident enough that the previous patches cause no more trouble.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 4 2018, 7:54 PM
This revision was automatically updated to reflect the committed changes.