[analyzer] Insert checker options into AnalyzerOption::ConfigTable

[Szelethus]

Summary

The more entries we have in AnalyzerOptions::ConfigTable, the more helpful debug.ConfigDumper is. With this patch, I'm pretty confident that it'll now emit the entire state of the analyzer, minus the frontend flags.

It would be nice to reserve the config table specifically to checker options only, as storing the regular analyzer configs is kinda redundant.

[Szelethus]

lib/StaticAnalyzer/Core/AnalyzerOptions.cpp

183 ↗

(On Diff #185853)

alpha.security.MmapWriteExec:MmapProtRead expects it's option to be of a hexadecimal value -- as of now, using that option crashes.

From the doxygen:

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.

[xazax.hun]

LG!

[Szelethus]

Hmm, supplying the default value for AnalyzerOptions::getChecker*Option is not only redundant with this patch, but also extremely bug-prone. I should remove them too in the same breath.

[Szelethus]

Remove the default value parameters from getChecker*Option, as they are no longer necessary. Note the changes to the unit test file: since the current thinking is that all options must've been validated by CheckerRegistry, if any irregularities are detected within those methods, we no longer return default values or even dream of a peaceful termination, as it clearly indicates a programming error, hence the removal of some test cases as asserts can't be tested.

[Szelethus]

Uhh, we still need to restore the default value -- I might end up splitting this patch a little further.

[Szelethus]

Removed every piece of code not directly related to inserting checker option to the config table, moving it to a new patch. This essentially restores the revision to it's earliest state, except that it's even simpler: no changes are made to AnalyzerOptions.

[baloghadamsoftware]

lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp

332 ↗

(On Diff #190021)

Twine(CheckerFullName) + ":" + OptionName. However, since the constructor Twine(const StringRef &Str) is implicit, CheckerFullName + ":" + OptionName results the same and is more readable.

354 ↗

(On Diff #190021)

Same as above.

[Szelethus]

Rebase.

[Szelethus]

Add a test case for checker plugins.

[dcoughlin]

This looks good to me. It is great to see this tested!

Did you consider separating the checker options from the non-checker options in the config dumper output? That would probably be easier to read.

[baloghadamsoftware]

LGTM!

[whisperity]

I think this is good. Patch still marked as Needs review for some reason. 😦 Can we look up this blocking review thing? Perhaps this could be marked ready to roll once the dependency patch is ironed out.

lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp

332 ↗

(On Diff #190021)

This comment is Done.

[NoQ]

lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp

314 ↗

(On Diff #190991)

Mmm, const rvalue reference? Is this even a thing?

331 ↗

(On Diff #190991)

Wait, how did you manage to move from a const &?

This type doesn't look very move-friendly in general.

test/Analysis/analyzer-config.c

1 ↗

(On Diff #190991)

If i understand correctly, you should be able to simplify this line dramatically.

[Szelethus]

I committed but changed the moves to copies. Singel CheckerRegisrty::CmdLineOption is only filled with StringRefs, it shouldn't really matter.

lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp

314 ↗

(On Diff #190991)

Apparently :D

cfe/trunk/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp

 int AnalyzerOptions::getCheckerIntegerOption(StringRef CheckerName,
                                              StringRef OptionName,
                                              int DefaultVal,
                                              bool SearchInParents ) const {
   int Ret = DefaultVal;
   bool HasFailed = getCheckerStringOption(CheckerName, OptionName,
                                           std::to_string(DefaultVal),
                                           SearchInParents)
-                     .getAsInteger(10, Ret);
+                     .getAsInteger(0, Ret);
   assert(!HasFailed && "analyzer-config option should be numeric");
   (void)HasFailed;
   return Ret;
 }
 
 int AnalyzerOptions::getCheckerIntegerOption(const ento::CheckerBase *C,
                                              StringRef OptionName,
                                              int DefaultVal,
                                              bool SearchInParents ) const {
   return getCheckerIntegerOption(
              C->getTagDescription(), OptionName, DefaultVal, SearchInParents);
 }

cfe/trunk/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp

 
 void CheckerRegistry::addDependency(StringRef FullName, StringRef Dependency) {
   Dependencies.emplace_back(FullName, Dependency);
 }
 
 template <class T>
 static void
 insertOptionToCollection(StringRef FullName, T &Collection,
-                         const CheckerRegistry::CmdLineOption &&Option) {
+                         const CheckerRegistry::CmdLineOption &Option,
+                         AnalyzerOptions &AnOpts) {
   auto It = binaryFind(Collection, FullName);
   assert(It != Collection.end() &&
          "Failed to find the checker while attempting to add a command line "
          "option to it!");
 
-  It->CmdLineOptions.emplace_back(std::move(Option));
+  AnOpts.Config.insert(
+      {(FullName + ":" + Option.OptionName).str(), Option.DefaultValStr});
+
+  It->CmdLineOptions.emplace_back(Option);
 }
 
 void CheckerRegistry::resolveCheckerAndPackageOptions() {
   for (const std::pair<StringRef, CmdLineOption> &CheckerOptEntry :
        CheckerOptions) {
     insertOptionToCollection(CheckerOptEntry.first, Checkers,
-                             std::move(CheckerOptEntry.second));
+                             CheckerOptEntry.second, AnOpts);
   }
   CheckerOptions.clear();
 
   for (const std::pair<StringRef, CmdLineOption> &PackageOptEntry :
        PackageOptions) {
     insertOptionToCollection(PackageOptEntry.first, Checkers,
-                             std::move(PackageOptEntry.second));
+                             PackageOptEntry.second, AnOpts);
   }
   PackageOptions.clear();
 }
 
 void CheckerRegistry::addPackage(StringRef FullName) {
   Packages.emplace_back(PackageInfo(FullName));
 }
 

cfe/trunk/test/Analysis/analyzer-config.c

-// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 %s -o /dev/null -analyzer-checker=core,osx.cocoa,debug.ConfigDumper -analyzer-max-loop 34 > %t 2>&1
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ConfigDumper > %t 2>&1
 // RUN: FileCheck --input-file=%t %s --match-full-lines
 
 // CHECK: [config]
 // CHECK-NEXT: aggressive-binary-operation-simplification = false
+// CHECK-NEXT: alpha.clone.CloneChecker:IgnoredFilesPattern = ""
+// CHECK-NEXT: alpha.clone.CloneChecker:MinimumCloneComplexity = 50
+// CHECK-NEXT: alpha.clone.CloneChecker:ReportNormalClones = true
+// CHECK-NEXT: alpha.security.MmapWriteExec:MmapProtExec = 0x04
+// CHECK-NEXT: alpha.security.MmapWriteExec:MmapProtRead = 0x01
 // CHECK-NEXT: avoid-suppressing-null-argument-paths = false
 // CHECK-NEXT: c++-allocator-inlining = true
 // CHECK-NEXT: c++-container-inlining = false
 // CHECK-NEXT: c++-inlining = destructors
 // CHECK-NEXT: c++-shared_ptr-inlining = false
 // CHECK-NEXT: c++-stdlib-inlining = true
 // CHECK-NEXT: c++-temp-dtor-inlining = true
 // CHECK-NEXT: c++-template-inlining = true
 // CHECK-NEXT: cfg-conditional-static-initializers = true
 // CHECK-NEXT: cfg-implicit-dtors = true
 // CHECK-NEXT: cfg-lifetime = false
 // CHECK-NEXT: cfg-loopexit = false
 // CHECK-NEXT: cfg-rich-constructors = true
 // CHECK-NEXT: cfg-scopes = false
 // CHECK-NEXT: cfg-temporary-dtors = true
+// CHECK-NEXT: cplusplus.Move:WarnOn = KnownsAndLocals
 // CHECK-NEXT: crosscheck-with-z3 = false
 // CHECK-NEXT: ctu-dir = ""
 // CHECK-NEXT: ctu-index-name = externalDefMap.txt
+// CHECK-NEXT: debug.AnalysisOrder:* = false
+// CHECK-NEXT: debug.AnalysisOrder:Bind = false
+// CHECK-NEXT: debug.AnalysisOrder:EndFunction = false
+// CHECK-NEXT: debug.AnalysisOrder:LiveSymbols = false
+// CHECK-NEXT: debug.AnalysisOrder:NewAllocator = false
+// CHECK-NEXT: debug.AnalysisOrder:PostCall = false
+// CHECK-NEXT: debug.AnalysisOrder:PostStmtArraySubscriptExpr = false
+// CHECK-NEXT: debug.AnalysisOrder:PostStmtCXXNewExpr = false
+// CHECK-NEXT: debug.AnalysisOrder:PostStmtCastExpr = false
+// CHECK-NEXT: debug.AnalysisOrder:PostStmtOffsetOfExpr = false
+// CHECK-NEXT: debug.AnalysisOrder:PreCall = false
+// CHECK-NEXT: debug.AnalysisOrder:PreStmtArraySubscriptExpr = false
+// CHECK-NEXT: debug.AnalysisOrder:PreStmtCXXNewExpr = false
+// CHECK-NEXT: debug.AnalysisOrder:PreStmtCastExpr = false
+// CHECK-NEXT: debug.AnalysisOrder:PreStmtOffsetOfExpr = false
+// CHECK-NEXT: debug.AnalysisOrder:RegionChanges = false
 // CHECK-NEXT: display-ctu-progress = false
 // CHECK-NEXT: eagerly-assume = true
 // CHECK-NEXT: elide-constructors = true
 // CHECK-NEXT: expand-macros = false
 // CHECK-NEXT: experimental-enable-naive-ctu-analysis = false
 // CHECK-NEXT: exploration_strategy = unexplored_first_queue
 // CHECK-NEXT: faux-bodies = true
 // CHECK-NEXT: graph-trim-interval = 1000
 // CHECK-NEXT: inline-lambdas = true
 // CHECK-NEXT: ipa = dynamic-bifurcate
 // CHECK-NEXT: ipa-always-inline-size = 3
 // CHECK-NEXT: max-inlinable-size = 100
 // CHECK-NEXT: max-nodes = 225000
 // CHECK-NEXT: max-symbol-complexity = 35
 // CHECK-NEXT: max-times-inline-large = 32
 // CHECK-NEXT: min-cfg-size-treat-functions-as-large = 14
 // CHECK-NEXT: mode = deep
 // CHECK-NEXT: model-path = ""
 // CHECK-NEXT: notes-as-events = false
+// CHECK-NEXT: nullability:NoDiagnoseCallsToSystemHeaders = false
 // CHECK-NEXT: objc-inlining = true
+// CHECK-NEXT: optin.cplusplus.UninitializedObject:CheckPointeeInitialization = false
+// CHECK-NEXT: optin.cplusplus.UninitializedObject:IgnoreGuardedFields = false
+// CHECK-NEXT: optin.cplusplus.UninitializedObject:IgnoreRecordsWithField = ""
+// CHECK-NEXT: optin.cplusplus.UninitializedObject:NotesAsWarnings = false
+// CHECK-NEXT: optin.cplusplus.UninitializedObject:Pedantic = false
+// CHECK-NEXT: optin.cplusplus.VirtualCall:PureOnly = false
+// CHECK-NEXT: optin.osx.cocoa.localizability.NonLocalizedStringChecker:AggressiveReport = false
+// CHECK-NEXT: optin.performance.Padding:AllowedPad = 24
+// CHECK-NEXT: osx.NumberObjectConversion:Pedantic = false
+// CHECK-NEXT: osx.cocoa.RetainCount:CheckOSObject = true
+// CHECK-NEXT: osx.cocoa.RetainCount:TrackNSCFStartParam = false
 // CHECK-NEXT: prune-paths = true
 // CHECK-NEXT: region-store-small-struct-limit = 2
 // CHECK-NEXT: report-in-main-source-file = false
 // CHECK-NEXT: serialize-stats = false
 // CHECK-NEXT: stable-report-filename = false
 // CHECK-NEXT: suppress-c++-stdlib = true
 // CHECK-NEXT: suppress-inlined-defensive-checks = true
 // CHECK-NEXT: suppress-null-return-paths = true
+// CHECK-NEXT: unix.DynamicMemoryModeling:Optimistic = false
 // CHECK-NEXT: unroll-loops = false
 // CHECK-NEXT: widen-loops = false
 // CHECK-NEXT: [stats]
-// CHECK-NEXT: num-entries = 49
+// CHECK-NEXT: num-entries = 84

cfe/trunk/test/Analysis/checker-plugins.c

 
 // RUN: %clang_analyze_cc1 %s \
 // RUN:   -load %llvmshlibdir/CheckerOptionHandlingAnalyzerPlugin%pluginext\
 // RUN:   -analyzer-checker=example.MyChecker \
 // RUN:   -analyzer-config example.MyChecker:ExampleOption=true \
 // RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-CHECKER-OPTION-OUTPUT-TRUE
 
 // CHECK-CHECKER-OPTION-OUTPUT-TRUE: Example option is set to true
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -load %llvmshlibdir/CheckerOptionHandlingAnalyzerPlugin%pluginext\
+// RUN:   -analyzer-checker=example.MyChecker \
+// RUN:   -analyzer-checker=debug.ConfigDumper \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-CHECKER-OPTION
+
+// CHECK-CHECKER-OPTION: example.MyChecker:ExampleOption = false
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -load %llvmshlibdir/CheckerOptionHandlingAnalyzerPlugin%pluginext\
+// RUN:   -analyzer-checker=example.MyChecker \
+// RUN:   -analyzer-checker=debug.ConfigDumper \
+// RUN:   -analyzer-config example.MyChecker:ExampleOption=true \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-CHECKER-OPTION-TRUE
+
+// CHECK-CHECKER-OPTION-TRUE: example.MyChecker:ExampleOption = true