This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Validate checker option names and values
ClosedPublic

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
329

Insertion

340

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
340

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
325

Is this the most efficient way to concatenate StringRefs?

443

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

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

@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
325

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.

Szelethus requested review of this revision.Apr 30 2019, 4:32 AM
NoQ accepted this revision.May 10 2019, 1:52 PM

Setting compatibility mode to be true in the driver is not sufficient since many build systems call the frontend directly.

This doesn't seem to be the case; all build systems i'm aware of would pick up the driver flag.

We've seen really bad problems due to lack of compatibility in the past, but this time i don't foresee any, as we've already tested this for non-checker flags. Let's land this and i'll ask for a revert if it ever causes any problems.

Note that even if we revert the driver hack and keep compatibility mode on by default, we can still disable compatibility mode in LIT substitutions (i.e., in %clang_analyze_cc1), so that we still had checking in our tests and were sure that they test something.

This revision was not accepted when it landed; it landed in state Needs Review.May 17 2019, 2:51 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2019, 2:51 AM