Page MenuHomePhabricator

[analyzer][NFC] Reimplement checker options
ClosedPublic

Authored by Szelethus on Feb 6 2019, 3:22 PM.

Details

Summary

Here we go guys! This patch is I believe the most important since I started working on this AnalyzerOptions refactoring effort.

It's been a long standing issue that checker options were deep hidden within the implementation, not even necessarily on the bottom of the checker files, in some cases they were totally undocumented, and the possibility of user error due to them being so long was enormous, while at the same time giving no warnings or errors at all.

This patch, similarly to how dependencies were reimplemented, uses TableGen to register checker options, adds them to CheckerRegistry while at the same time exposing the same functionality to non-generated statically linked and plugin checkers.

In detail:

  • Add checker and package options to the TableGen files
    • Added a new class called CmdLineOption, and both Package and Checker recieved a list<CmdLineOption> field.
    • Added every existing checker option to Checkers.td. I'm a little unsure about some, so take a look I guess!
  • The CheckerRegistry class
    • Received some comments to most of it's inline classes
    • Received the CmdLineOption and PackageInfo inline classes, a list of CmdLineOption was added to CheckerInfo and PackageInfo
    • Added addCheckerOption and addPackageOption
    • Added a new field called Packages, used in addPackageOptions, filled up in addPackage

Diff Detail

Repository
rC Clang

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2019, 3:22 PM
Szelethus updated this revision to Diff 185861.Feb 7 2019, 1:29 PM
Szelethus edited the summary of this revision. (Show Details)

Moved all AnalyzerOptions related changes to a separate revision.

Szelethus added a reviewer: aaron.ballman.EditedFeb 7 2019, 1:46 PM

Aaron, you recently changed these file too, if you have the time, I'll happily listen to your feedback :)

Also, I was thinking that maybe we could just std::move CheckerRegisrty::Checkers and CheckerRegisrty::Packages to AnalyzerOptions in CheckerRegisrty's destructor, which will finally enable us to access all registered checkers, and a bunch of other things (like whether they are enabled, what options they have, etc).

We have examples/analyzer-plugin. I would prefer to add an example option to the example plugin so people do see how to do this when they are registering a checker from a plugin.

include/clang/StaticAnalyzer/Checkers/Checkers.td
48

Did you want to add this option to nullability? I believe it belongs to Malloc Checker.

49

In is a possible typo? If or When?

50

Do we need the? Also, I think the checker also do not know the allocating functions. I think it might be better to describe the opposite. Optimistic means that the checker assumes all the allocating, deallocating functions are annotated.

385

It would be great if there were a way to define options once and reuse them.

Szelethus marked an inline comment as done.Feb 11 2019, 5:12 AM

We have examples/analyzer-plugin. I would prefer to add an example option to the example plugin so people do see how to do this when they are registering a checker from a plugin.

Not only that, but one for dependencies would be great too, thanks!

include/clang/StaticAnalyzer/Checkers/Checkers.td
48

Yup, shouldn't be here. I vaguely remembered (incorrectly) that a test file contained a nullability:Optimistic option, but I can't find (=grep) any evidence of that. Nice catch!

Szelethus updated this revision to Diff 186350.Feb 11 2019, 3:18 PM
  • Removed the accidentally added nullability:Optimistic entry,
  • Reworded unix.DynamicMemoryModeling:Optimistic.
Szelethus marked 5 inline comments as done.Feb 11 2019, 3:21 PM
Szelethus added inline comments.
include/clang/StaticAnalyzer/Checkers/Checkers.td
385

I think it's possible with something similar:

def OptimisticOption :
  CmdLineOption<Boolean,
                "Optimistic",
                "If set to true, the checker assumes that all the "
                "allocating and deallocating functions are annotated with "
                "ownership_holds, ownership_takes and ownership_returns.",
                "false">;

def DynamicMemoryModeling: Checker<"DynamicMemoryModeling">,
  HelpText<"The base of several malloc() related checkers. On it's own it "
           "emits no reports, but adds valuable information to the analysis "
           "when enabled.">,
  CheckerOptions<[
    OptimisticOption
  ]>,
  Dependencies<[CStringModeling]>,
  Documentation<NotDocumented>;
aaron.ballman added inline comments.Feb 13 2019, 6:55 AM
include/clang/StaticAnalyzer/Checkers/CheckerBase.td
13–14

Might want to add a comment mentioning that additional enumerators require changes to the tablegen code.

include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
169

Make this explicit?

250–253

If we're changing these, can you fix up the parameter names for our coding conventions?

lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
310

explicit?

321

Naming conventions.

364

emplace_back() instead?

utils/TableGen/ClangSACheckersEmitter.cpp
95

There should be come comments here marrying this up with the .td file.

Also, the formatting here looks somewhat off (the case statement seem to be indented too far).

174

Naming conventions.

256

Naming conventions.

261

Can remove some of the extra vertical whitespace.

Szelethus marked an inline comment as done.Feb 13 2019, 7:16 AM

Since my changes are very invasive towards CheckerRegistry in general, I might clang-format the whole thing, and make the code follow the coding guidelines. Thanks for the review!

I guess removing some of the checker option descriptions from lib/StataicAnalyzer/Checkers/ would be nice too, to easy on maintenance. One could always just use -analyzer-checker-option-help whenever in doubt.

Szelethus updated this revision to Diff 189088.Mar 3 2019, 9:03 AM
  • Capitalized some variable names
  • Added more comments
  • Preferring binary searches to linear searches wherever possible
Szelethus marked 9 inline comments as done.Mar 3 2019, 9:06 AM
Szelethus updated this revision to Diff 189090.Mar 3 2019, 9:39 AM
Szelethus marked an inline comment as done.
Szelethus updated this revision to Diff 189105.Mar 3 2019, 4:48 PM

Remembered that we can't use use llvm::binary_search, as we'll need the iterators in a later patch. Should've rebased all my branches before updating :^)

aaron.ballman accepted this revision.Mar 6 2019, 8:58 AM

LGTM aside from some minor nits, but you should wait for someone more well-versed in the static analyzer to sign off before committing.

include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
158

Make this explicit?

lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
404

llvm::any_of() instead of find()? Same below.

This revision is now accepted and ready to land.Mar 6 2019, 8:58 AM
Szelethus updated this revision to Diff 190022.Mar 10 2019, 2:05 PM
Szelethus marked an inline comment as done.

Adding explicit to CheckerInfo's constructor.

Szelethus marked 2 inline comments as done.Mar 10 2019, 2:06 PM
Szelethus added inline comments.
lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
404

Will need that iterator in a followup patch.

baloghadamsoftware requested changes to this revision.Mar 11 2019, 7:46 AM
baloghadamsoftware added inline comments.
include/clang/StaticAnalyzer/Checkers/CheckerBase.td
49

Please add a comment that this field is optional.

88

Same as above.

include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
156

Are we able now to distinguish checkers of the same short names but in different packages? In earlier versions the short name had to be different to avoid conflicts.

272

Cannot we store the size of the Package in the Package itself?

lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
48

Could we maybe use std::less<CheckerOrPackageInfo>?

66

I would better keep the original message here. It has a more formal style.

175

We always have packages first. Please swap these lines.

308

Typo: it's -> its.

404

Please explain the problem with std::lower_bound(). I do not see why we cannot use it (followed by a check whether we found an element equal to the searched element, not a greater one). I cannot see any excuse to use linear search instead of binary one in a sorted collection.

This revision now requires changes to proceed.Mar 11 2019, 7:46 AM
Szelethus updated this revision to Diff 190223.Mar 12 2019, 2:23 AM
Szelethus marked 13 inline comments as done.

Fixes according to reviewer comments.

include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
156

Full names contain the packages as well, like "alpha.cplusplus.IteratorModeling". We don't really care about short names (which refers to the checker's name, right?) in CheckerRegistry.

272

Yup, this is a mess, but I only renamed this field. I sat on this for a while, thinking about how Packages and PackageSizes could be merged, and figured that it just isn't worth the development time to refactor, and would just further bloat this patch.

lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
48

Nope, CheckerOrPackageInfo doesn't implement operator<.

whisperity added inline comments.Mar 12 2019, 3:10 AM
include/clang/StaticAnalyzer/Checkers/Checkers.td
49

Let's put at least a FIXME here that the documentation for this option was missing.

911

Same here for missing documentation/description for optstring.

lib/Frontend/CompilerInvocation.cpp
425

What is this trying to do?

Szelethus marked 3 inline comments as done.Mar 14 2019, 7:25 AM

Let's put at least a FIXME here that the documentation for this option was missing.

I'd prefer to just simply fix this. @NoQ, could you help us fill in the gaps? We need a desctiption for

  • nullability:NoDiagnoseCallsToSystemHeaders
  • osx.cocoa.RetainCount:CheckOSObject
  • osx.cocoa.RetainCount:leak-diagnostics-reference-allocation
  • osx.cocoa.RetainCount:TrackNSCFStartParam
lib/Frontend/CompilerInvocation.cpp
425

Oh right, this is why (mind you, we had a checker option that was hexadecimal, but apparently never used, as it caused an assertation failure):

template<typename T >
std::enable_if<std::numeric_limits<T>::is_signed, bool>::type
llvm::StringRef::getAsInteger(unsigned Radix, T &Result) const

Parse the current string as an integer of the specified radix.

If Radix is specified as zero, this does radix autosensing using extended C rules: 0 is octal, 0x is hex, 0b is binary.

If the string is invalid or if only a subset of the string is valid, this returns true to signify the error. The string is considered erroneous if empty or if it overflows T.

Szelethus planned changes to this revision.Mar 15 2019, 10:14 AM
Szelethus marked 3 inline comments as done.

Alright, I'll de-clutter this patch a bit.

Szelethus edited the summary of this revision. (Show Details)
  • Moved every non-related change to smaller patches, this should ease a lot on reviewers.
  • Now processing options once all checkers are added to the registry. This is important, because I use binary searches to find the checkers and packages that need to be modified -- if a plugin however called CheckerRegistry::add*Option, it would've cause an assertion failure.

We have examples/analyzer-plugin. I would prefer to add an example option to the example plugin so people do see how to do this when they are registering a checker from a plugin.

Coming in a separate patch later!

Aside from little comment it seems OK.

lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
195

Everywhere 1. package 2. checker, please!

This revision is now accepted and ready to land.Mar 18 2019, 7:47 AM
Szelethus retitled this revision from [analyzer] Reimplement checker options to [analyzer][NFC] Reimplement checker options.Mar 20 2019, 4:21 AM
This revision was automatically updated to reflect the committed changes.