Page MenuHomePhabricator

[analyzer] Validate checker option names and values
Needs RevisionPublic

Authored by Szelethus on Feb 6 2019, 4:01 PM.

Details

Summary

Validate whether the option exists, and also whether the supplied value is of the correct type.

This is the final part of the checker option refactoring. I guess we could arrange the AnalyzerOptions related changes to 5 "chapters":

  1. Reimplement -analyzer-config options
  2. Fix the checker naming bug by reimplementing checker dependencies
  3. Reimplement checker options
  4. (already in the works!) Document the frontend of the analyzer, sneak peak here.
  5. (soon™) Make AnalyzerOptions const everywhere after the analyzer is initialized, make AnalyzerOptions::ConfigTable private, reimplement debug.ConfigDumper.

Diff Detail

Event Timeline

Szelethus created this revision.Feb 6 2019, 4:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2019, 4:01 PM
NoQ accepted this revision.Feb 8 2019, 6:29 PM

(already in the works!) Document the frontend of the analyzer, sneak peak here.

Whoa!!

*celebrates the graphomania epidemic*

This revision is now accepted and ready to land.Feb 8 2019, 6:29 PM
Szelethus updated this revision to Diff 190023.Mar 10 2019, 2:14 PM

Refactored the entire patch, NFC compared to the earlier diff. This was needed as in a followup revision, much of the code would be rewritten.

whisperity added inline comments.Mar 12 2019, 3:32 AM
lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
317

Insertion

328

Does this mean that we no longer can give "1" and "0" for boolean checker options? Or was that Tidy who allowed such in the first place?

Szelethus marked 2 inline comments as done.Mar 12 2019, 4:36 AM
Szelethus added inline comments.
lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
328

Well yea, "true" or "false" is the only input we accept (and have ever accepted) for boolean configs. I have nothing against adding 1 and 0 too, but let that be a topic of another revision.

lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
313

Is this the most efficient way to concatenate StringRefs?

460

What about Config.first().split(":")?

whisperity added inline comments.Mar 12 2019, 6:21 AM
lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
313

@baloghadamsoftware
Is this the most efficient way to concatenate StringRefs?

Twine is specifically designed for that, yes.
You can't evade the final "copy", although most likely there'll be a RVO taking place.

However, the *first* Twine could be initialised from the FullName variable, as opposed to empty node.
And you could use single characters as characters, as opposed to strings, i.e. (Twine(FullName) + ':' + OptionName).str().

lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
313

The constructor Twine(const StringRef &Str) is implicit so (FullName + ':' + OptionName).str() results the same and is more readable.

Did we test all the codepaths including the package level configs? If not, please add some package level config option related tests.
Otherwise the patch looks good to me after all the comments are resolved.

Szelethus updated this revision to Diff 190629.Mar 14 2019, 7:47 AM
Szelethus marked 6 inline comments as done.

Fixes according to inline comments.

Szelethus updated this revision to Diff 190630.Mar 14 2019, 7:54 AM

Add a testcase for packages.

Add a test case for checker plugins.

dcoughlin requested changes to this revision.Mar 17 2019, 8:29 PM

I think it would be better if the default for compatibility mode were 'true'. That way this change will be backwards compatible and clients who want to enforce stricter checking could enable it. Setting compatibility mode to be true in the driver is not sufficient since many build systems call the frontend directly.

This revision now requires changes to proceed.Mar 17 2019, 8:29 PM

I think it would be better if the default for compatibility mode were 'true'. That way this change will be backwards compatible and clients who want to enforce stricter checking could enable it. Setting compatibility mode to be true in the driver is not sufficient since many build systems call the frontend directly.

How long must compatibility be kept?

Szelethus added a comment.EditedMar 23 2019, 3:41 AM

I think it would be better if the default for compatibility mode were 'true'. That way this change will be backwards compatible and clients who want to enforce stricter checking could enable it. Setting compatibility mode to be true in the driver is not sufficient since many build systems call the frontend directly.

Well, this is how 8.0.0. landed, so for better or for worse, changing it back wouldn't help on those clients much -- the question is now that whether there's a point in changing this yet back again. I would argue *strongly* on the side that it is not, because, according to the FAQ (which is aimed at end users):

Frontend-only options are intended to be used only by clang developers. Users should not run clang -cc1 directly, because -cc1 options are not guaranteed to be stable.

Of course, developers could also just invoke the analyzer with compatibility mode disabled, but thats I think a chore, and easy to forget -- it would greatly diminish the value of these patches, and as a maintainer, I would personally strongly prefer the analyzer to just terminate and not waste my time when I mess up an invocation.