Skip to content

Commit

Permalink
[analyzer][NFC] Reimplement checker options
Browse files Browse the repository at this point in the history 
TL;DR:

* 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 and package option to Checkers.td.
* 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

Detailed description:

In the last couple months, a lot of effort was put into tightening the
analyzer's command line interface. The main issue is that it's spectacularly
easy to mess up a lenghty enough invocation of the analyzer, and the user was
given no warnings or errors at all in that case.

We can divide the effort of resolving this into several chapters:

* Non-checker analyzer configurations:
    Gather every analyzer configuration into a dedicated file. Emit errors for
    non-existent configurations or incorrect values. Be able to list these
    configurations. Tighten AnalyzerOptions interface to disallow making such
    a mistake in the future.

* Fix the "Checker Naming Bug" by reimplementing checker dependencies:
    When cplusplus.InnerPointer was enabled, it implicitly registered
    unix.Malloc, which implicitly registered some sort of a modeling checker
    from the CStringChecker family. This resulted in all of these checker
    objects recieving the name "cplusplus.InnerPointer", making AnalyzerOptions
    asking for the wrong checker options from the command line:
      cplusplus.InnerPointer:Optimisic
    istead of
      unix.Malloc:Optimistic.
    This was resolved by making CheckerRegistry responsible for checker
    dependency handling, instead of checkers themselves.

* Checker options: (this patch included!)
    Same as the first item, but for checkers.

(+ minor fixes here and there, and everything else that is yet to come)

There were several issues regarding checker options, that non-checker
configurations didn't suffer from: checker plugins are loaded runtime, and they
could add new checkers and new options, meaning that unlike for non-checker
configurations, we can't collect every checker option purely by generating code.
Also, as seen from the "Checker Naming Bug" issue raised above, they are very
rarely used in practice, and all sorts of skeletons fell out of the closet while
working on this project.

They were extremely problematic for users as well, purely because of how long
they were. Consider the following monster of a checker option:

  alpha.cplusplus.UninitializedObject:CheckPointeeInitialization=false

While we were able to verify whether the checker itself (the part before the
colon) existed, any errors past that point were unreported, easily resulting
in 7+ hours of analyses going to waste.

This patch, similarly to how dependencies were reimplemented, uses TableGen to
register checker options into Checkers.td, so that Checkers.inc now contains
entries for both checker and package options. Using the preprocessor,
Checkers.inc is converted into code in CheckerRegistry, adding every builtin
(checkers and packages that have an entry in the Checkers.td file) checker and
package option to the registry. The new addPackageOption and addCheckerOption
functions expose the same functionality to statically-linked non-builtin and
plugin checkers and packages as well.

Emitting errors for incorrect user input, being able to list these options, and
some other functionalies will land in later patches.

Differential Revision: https://reviews.llvm.org/D57855

llvm-svn: 358752
  • Loading branch information
Kristof Umann committed Apr 19, 2019
1 parent a64fcb6 commit b4788b2
Show file tree
Hide file tree
Showing 9 changed files with 555 additions and 36 deletions.
2 changes: 1 addition & 1 deletion clang/include/clang/Basic/DiagnosticCommonKinds.td
View file
Expand Up @@ -292,7 +292,7 @@ def err_omp_more_one_clause : Error<

// Static Analyzer Core
def err_unknown_analyzer_checker : Error<
"no analyzer checkers are associated with '%0'">;
"no analyzer checkers or packages are associated with '%0'">;
def note_suggest_disabling_all_checkers : Note<
"use -analyzer-disable-all-checks to disable all static analyzer checkers">;
}
53 changes: 46 additions & 7 deletions clang/include/clang/StaticAnalyzer/Checkers/CheckerBase.td
View file
Expand Up @@ -10,14 +10,45 @@
//
//===----------------------------------------------------------------------===//

/// Describes a checker or package option type. This is important for validating
/// user supplied inputs.
/// New option types can be added by modifying this enum. Note that this
/// requires changes in the TableGen emitter file ClangSACheckersEmitter.cpp.
class CmdLineOptionTypeEnum<bits<2> val> {
bits<2> Type = val;
}
def Integer : CmdLineOptionTypeEnum<0>;
def String : CmdLineOptionTypeEnum<1>;
def Boolean : CmdLineOptionTypeEnum<2>;

class Type<CmdLineOptionTypeEnum val> {
bits<2> Type = val.Type;
}

/// Describes an option for a checker or a package.
class CmdLineOption<CmdLineOptionTypeEnum type, string cmdFlag, string desc,
string defaultVal> {
bits<2> Type = type.Type;
string CmdFlag = cmdFlag;
string Desc = desc;
string DefaultVal = defaultVal;
}

/// Describes a list of package options.
class PackageOptions<list<CmdLineOption> opts> {
list<CmdLineOption> PackageOptions = opts;
}

/// Describes a package. Every checker is a part of a package, for example,
/// 'NullDereference' is part of the 'core' package, hence it's full name is
/// 'core.NullDereference'.
/// Example:
/// def Core : Package<"core">;
class Package<string name> {
string PackageName = name;
Package ParentPackage;
string PackageName = name;
// This field is optional.
list<CmdLineOption> PackageOptions;
Package ParentPackage;
}

/// Describes a 'super' package that holds another package inside it. This is
Expand Down Expand Up @@ -52,11 +83,19 @@ class Documentation<DocumentationEnum val> {
/// def DereferenceChecker : Checker<"NullDereference">,
/// HelpText<"Check for dereferences of null pointers">;
class Checker<string name = ""> {
string CheckerName = name;
string HelpText;
list<Checker> Dependencies;
bits<2> Documentation;
Package ParentPackage;
string CheckerName = name;
string HelpText;
// This field is optional.
list<CmdLineOption> CheckerOptions;
// This field is optional.
list<Checker> Dependencies;
bits<2> Documentation;
Package ParentPackage;
}

/// Describes a list of checker options.
class CheckerOptions<list<CmdLineOption> opts> {
list<CmdLineOption> CheckerOptions = opts;
}

/// Describes dependencies in between checkers. For example, InnerPointerChecker
Expand Down
225 changes: 219 additions & 6 deletions clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
View file
Expand Up @@ -42,7 +42,17 @@ def OptIn : Package<"optin">;
// development, but unwanted for developers who target only a single platform.
def PortabilityOptIn : Package<"portability">, ParentPackage<OptIn>;

def Nullability : Package<"nullability">;
def Nullability : Package<"nullability">,
PackageOptions<[
CmdLineOption<Boolean,
"NoDiagnoseCallsToSystemHeaders",
"Suppresses warnings for violating nullability annotations "
"of system header functions. This is useful if you are "
"concerned with your custom nullability annotations more "
"than with following nullability specifications of system "
"header functions.",
"false">
]>;

def Cplusplus : Package<"cplusplus">;
def CplusplusAlpha : Package<"cplusplus">, ParentPackage<Alpha>;
Expand Down Expand Up @@ -371,6 +381,14 @@ 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<[
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">
]>,
Dependencies<[CStringModeling]>,
Documentation<NotDocumented>;

Expand Down Expand Up @@ -447,7 +465,28 @@ def CXXSelfAssignmentChecker : Checker<"SelfAssignment">,
Documentation<NotDocumented>;

def MoveChecker: Checker<"Move">,
HelpText<"Find use-after-move bugs in C++">,
HelpText<"Find use-after-move bugs in C++">,
CheckerOptions<[
CmdLineOption<String,
"WarnOn",
"In non-aggressive mode, only warn on use-after-move of "
"local variables (or local rvalue references) and of STL "
"objects. The former is possible because local variables (or "
"local rvalue references) are not tempting their user to "
"re-use the storage. The latter is possible because STL "
"objects are known to end up in a valid but unspecified "
"state after the move and their state-reset methods are also "
"known, which allows us to predict precisely when "
"use-after-move is invalid. Some STL objects are known to "
"conform to additional contracts after move, so they are not "
"tracked. However, smart pointers specifically are tracked "
"because we can perform extra checking over them. In "
"aggressive mode, warn on any use-after-move because the "
"user has intentionally asked us to completely eliminate "
"use-after-move in his code. Values: \"KnownsOnly\", "
"\"KnownsAndLocals\", \"All\".",
"KnownsAndLocals">
]>,
Documentation<HasDocumentation>;

} // end: "cplusplus"
Expand All @@ -456,6 +495,12 @@ let ParentPackage = CplusplusOptIn in {

def VirtualCallChecker : Checker<"VirtualCall">,
HelpText<"Check virtual function calls during construction or destruction">,
CheckerOptions<[
CmdLineOption<Boolean,
"PureOnly",
"Whether to only report calls to pure virtual methods.",
"false">
]>,
Documentation<HasDocumentation>;

} // end: "optin.cplusplus"
Expand Down Expand Up @@ -492,7 +537,40 @@ def MismatchedIteratorChecker : Checker<"MismatchedIterator">,
Documentation<HasAlphaDocumentation>;

def UninitializedObjectChecker: Checker<"UninitializedObject">,
HelpText<"Reports uninitialized fields after object construction">,
HelpText<"Reports uninitialized fields after object construction">,
CheckerOptions<[
CmdLineOption<Boolean,
"Pedantic",
"If set to false, the checker won't emit warnings "
"for objects that don't have at least one initialized "
"field.",
"false">,
CmdLineOption<Boolean,
"NotesAsWarnings",
"If set to true, the checker will emit a warning "
"for each uninitalized field, as opposed to emitting one "
"warning per constructor call, and listing the uninitialized "
"fields that belongs to it in notes.",
"false">,
CmdLineOption<Boolean,
"CheckPointeeInitialization",
"If set to false, the checker will not analyze "
"the pointee of pointer/reference fields, and will only "
"check whether the object itself is initialized.",
"false">,
CmdLineOption<String,
"IgnoreRecordsWithField",
"If supplied, the checker will not analyze "
"structures that have a field with a name or type name that "
"matches the given pattern.",
"\"\"">,
CmdLineOption<Boolean,
"IgnoreGuardedFields",
"If set to true, the checker will analyze _syntactically_ "
"whether the found uninitialized object is used without a "
"preceding assert call. Defaults to false.",
"false">
]>,
Documentation<HasAlphaDocumentation>;

} // end: "alpha.cplusplus"
Expand Down Expand Up @@ -554,6 +632,13 @@ let ParentPackage = Performance in {

def PaddingChecker : Checker<"Padding">,
HelpText<"Check for excessively padded structs.">,
CheckerOptions<[
CmdLineOption<Integer,
"AllowedPad",
"Reports are only generated if the excessive padding exceeds "
"'AllowedPad' in bytes.",
"24">
]>,
Documentation<NotDocumented>;

} // end: "padding"
Expand Down Expand Up @@ -662,11 +747,18 @@ def MallocOverflowSecurityChecker : Checker<"MallocOverflow">,
HelpText<"Check for overflows in the arguments to malloc()">,
Documentation<HasAlphaDocumentation>;

// Operating systems specific PROT_READ/PROT_WRITE values is not implemented,
// the defaults are correct for several common operating systems though,
// but may need to be overridden via the related analyzer-config flags.
def MmapWriteExecChecker : Checker<"MmapWriteExec">,
HelpText<"Warn on mmap() calls that are both writable and executable">,
CheckerOptions<[
CmdLineOption<Integer,
"MmapProtExec",
"Specifies the value of PROT_EXEC",
"0x04">,
CmdLineOption<Integer,
"MmapProtRead",
"Specifies the value of PROT_READ",
"0x01">
]>,
Documentation<HasAlphaDocumentation>;

} // end "alpha.security"
Expand Down Expand Up @@ -704,6 +796,14 @@ def NSOrCFErrorDerefChecker : Checker<"NSOrCFErrorDerefChecker">,
def NumberObjectConversionChecker : Checker<"NumberObjectConversion">,
HelpText<"Check for erroneous conversions of objects representing numbers "
"into numbers">,
CheckerOptions<[
CmdLineOption<Boolean,
"Pedantic",
"Enables detection of more conversion patterns (which are "
"most likely more harmless, and therefore are more likely to "
"produce false positives).",
"false">
]>,
Documentation<NotDocumented>;

def MacOSXAPIChecker : Checker<"API">,
Expand Down Expand Up @@ -795,6 +895,23 @@ def NSErrorChecker : Checker<"NSError">,

def RetainCountChecker : Checker<"RetainCount">,
HelpText<"Check for leaks and improper reference count management">,
CheckerOptions<[
CmdLineOption<Boolean,
"CheckOSObject",
"Find violations of retain-release rules applied to XNU "
"OSObject instances. By default, the checker only checks "
"retain-release rules for Objective-C NSObject instances "
"and CoreFoundation objects.",
"true">,
CmdLineOption<Boolean,
"TrackNSCFStartParam",
"Check not only that the code follows retain-release rules "
"with respect to objects it allocates or borrows from "
"elsewhere, but also that it fulfills its own retain count "
"specification with respect to objects that it receives as "
"arguments.",
"false">
]>,
Dependencies<[RetainCountBase]>,
Documentation<HasDocumentation>;

Expand Down Expand Up @@ -903,6 +1020,17 @@ let ParentPackage = LocalizabilityOptIn in {
def NonLocalizedStringChecker : Checker<"NonLocalizedStringChecker">,
HelpText<"Warns about uses of non-localized NSStrings passed to UI methods "
"expecting localized NSStrings">,
CheckerOptions<[
CmdLineOption<Boolean,
"AggressiveReport",
"Marks a string being returned by any call as localized if "
"it is in LocStringFunctions (LSF) or the function is "
"annotated. Otherwise, we mark it as NonLocalized "
"(Aggressive) or NonLocalized only if it is not backed by a "
"SymRegion (Non-Aggressive), basically leaving only string "
"literals as NonLocalized.",
"false">
]>,
Documentation<HasDocumentation>;

def EmptyLocalizationContextChecker :
Expand Down Expand Up @@ -961,6 +1089,72 @@ let ParentPackage = Debug in {

def AnalysisOrderChecker : Checker<"AnalysisOrder">,
HelpText<"Print callbacks that are called during analysis in order">,
CheckerOptions<[
CmdLineOption<Boolean,
"PreStmtCastExpr",
"",
"false">,
CmdLineOption<Boolean,
"PostStmtCastExpr",
"",
"false">,
CmdLineOption<Boolean,
"PreStmtArraySubscriptExpr",
"",
"false">,
CmdLineOption<Boolean,
"PostStmtArraySubscriptExpr",
"",
"false">,
CmdLineOption<Boolean,
"PreStmtCXXNewExpr",
"",
"false">,
CmdLineOption<Boolean,
"PostStmtCXXNewExpr",
"",
"false">,
CmdLineOption<Boolean,
"PreStmtOffsetOfExpr",
"",
"false">,
CmdLineOption<Boolean,
"PostStmtOffsetOfExpr",
"",
"false">,
CmdLineOption<Boolean,
"PreCall",
"",
"false">,
CmdLineOption<Boolean,
"PostCall",
"",
"false">,
CmdLineOption<Boolean,
"EndFunction",
"",
"false">,
CmdLineOption<Boolean,
"NewAllocator",
"",
"false">,
CmdLineOption<Boolean,
"Bind",
"",
"false">,
CmdLineOption<Boolean,
"LiveSymbols",
"",
"false">,
CmdLineOption<Boolean,
"RegionChanges",
"",
"false">,
CmdLineOption<Boolean,
"*",
"Enables all callbacks.",
"false">
]>,
Documentation<NotDocumented>;

def DominatorsTreeDumper : Checker<"DumpDominators">,
Expand Down Expand Up @@ -1034,6 +1228,25 @@ let ParentPackage = CloneDetectionAlpha in {

def CloneChecker : Checker<"CloneChecker">,
HelpText<"Reports similar pieces of code.">,
CheckerOptions<[
CmdLineOption<Integer,
"MinimumCloneComplexity",
"Ensures that every clone has at least the given complexity. "
"Complexity is here defined as the total amount of children "
"of a statement. This constraint assumes the first statement "
"in the group is representative for all other statements in "
"the group in terms of complexity.",
"50">,
CmdLineOption<Boolean,
"ReportNormalClones",
"Report all clones, even less suspicious ones.",
"true">,
CmdLineOption<String,
"IgnoredFilesPattern",
"If supplied, the checker wont analyze files with a filename "
"that matches the given pattern.",
"\"\"">
]>,
Documentation<HasAlphaDocumentation>;

} // end "clone"
Expand Down

0 comments on commit b4788b2

Please sign in to comment.