[analyzer][NFC] Keep track of whether enabling a checker was explictly specified in command line arguments

[Szelethus]

Summary

One of the bugs that I left in in my dependency patch D54438 was luckily discovered thanks to @george.karpenkov commiting a patch a couple days before I attempted to commit: Namely, the rigid structure of how the user can enable/disable checkers wasn't respected, and chaos ensued when a dependency was disabled, but the checker that was depending on it was enabled.

I added a new enum to CheckerInfo, so we can easily track whether the check is explicitly enabled, explicitly disabled, or isn't specified in this regard. Checkers belonging in the latter category may be implicitly enabled through dependencies in the followup patch. I also made sure that this is done within CheckerRegisty's constructor, leading to very significant simplifications in its query-like methods.

[Szelethus]

Remove mutable specifiers from all fields of CheckerRegistry.

[NoQ]

Cool!

I guess we still need to think how exactly do we want to resolve conflicts between dependencies.

include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h

152 ↗

(On Diff #182724)

Indeed! And, which is probably even more important, to scan-build's help (assuming that the respective flags of scan-build do actually work in a similar manner).

lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp

143 ↗

(On Diff #182724)

I learned a new word today :) :)

[Szelethus]

lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp

143 ↗

(On Diff #182724)

Wish it came from me, but I just moved it around... put a smile on my face every time I came across it too though :)

[Szelethus]

Oops, I ran into the issue of check-clang-analysis not actually running out unit tests. I decided not to revert this patch and just commit the fix: rC352284. Any objections?

cfe/trunk/include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h

CheckerRegistry(ArrayRef<std::string> plugins, DiagnosticsEngine &diags,
                   AnalyzerOptions &AnOpts, const LangOptions &LangOpts);
 
   /// Initialization functions perform any necessary setup for a checker.
   /// They should include a call to CheckerManager::registerChecker.
   using InitializationFunction = void (*)(CheckerManager &);
   using ShouldRegisterFunction = bool (*)(const LangOptions &);
 
   struct CheckerInfo {
+    enum class StateFromCmdLine {
+      // This checker wasn't explicitly enabled or disabled.
+      State_Unspecified,
+      // This checker was explicitly disabled.
+      State_Disabled,
+      // This checker was explicitly enabled.
+      State_Enabled
+    };
+
     InitializationFunction Initialize;
     ShouldRegisterFunction ShouldRegister;
     StringRef FullName;
     StringRef Desc;
     StringRef DocumentationUri;
+    StateFromCmdLine State = StateFromCmdLine::State_Unspecified;
+
+    bool isEnabled(const LangOptions &LO) const {
+      return State == StateFromCmdLine::State_Enabled && ShouldRegister(LO);
+    }
 
     CheckerInfo(InitializationFunction Fn, ShouldRegisterFunction sfn,
                 StringRef Name, StringRef Desc, StringRef DocsUri)
         : Initialize(Fn), ShouldRegister(sfn), FullName(Name), Desc(Desc),
           DocumentationUri(DocsUri) {}
   };
 
+  using StateFromCmdLine = CheckerInfo::StateFromCmdLine;
   using CheckerInfoList = std::vector<CheckerInfo>;
+  using CheckerInfoListRange = llvm::iterator_range<CheckerInfoList::iterator>;
   using CheckerInfoSet = llvm::SetVector<const CheckerRegistry::CheckerInfo *>;
 
 private:
   template <typename T>
   static void initializeManager(CheckerManager &mgr) {
     mgr.registerChecker<T>();
   }
 
public:
   template <class T>
   void addChecker(StringRef FullName, StringRef Desc, StringRef DocsUri) {
     // Avoid MSVC's Compiler Error C2276:
     // http://msdn.microsoft.com/en-us/library/850cstw1(v=VS.80).aspx
     addChecker(&CheckerRegistry::initializeManager<T>,
                &CheckerRegistry::returnTrue<T>, FullName, Desc, DocsUri);
   }
 
+  // FIXME: This *really* should be added to the frontend flag descriptions.
   /// Initializes a CheckerManager by calling the initialization functions for
   /// all checkers specified by the given CheckerOptInfo list. The order of this
   /// list is significant; later options can be used to reverse earlier ones.
   /// This can be used to exclude certain checkers in an included package.
   void initializeManager(CheckerManager &mgr) const;
 
   /// Check if every option corresponds to a specific checker or package.
   void validateCheckerOptions() const;
 
   /// Prints the name and description of all checkers in this registry.
   /// This output is not intended to be machine-parseable.
   void printHelp(raw_ostream &out, size_t maxNameChars = 30) const;
   void printList(raw_ostream &out) const;
 
 private:
   CheckerInfoSet getEnabledCheckers() const;
 
-  mutable CheckerInfoList Checkers;
-  mutable llvm::StringMap<size_t> Packages;
+  /// Return an iterator range of mutable CheckerInfos \p CmdLineArg applies to.
+  /// For example, it'll return the checkers for the core package, if
+  /// \p CmdLineArg is "core".
+  CheckerInfoListRange getMutableCheckersForCmdLineArg(StringRef CmdLineArg);
+
+  CheckerInfoList Checkers;
+  llvm::StringMap<size_t> Packages;
 
   DiagnosticsEngine &Diags;
   AnalyzerOptions &AnOpts;
   const LangOptions &LangOpts;
 };
 
 } // namespace ento
 
 } // namespace clang
 
 #endif // LLVM_CLANG_STATICANALYZER_CORE_CHECKERREGISTRY_H

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

static bool isCompatibleAPIVersion(const char *versionString) {
   if (!versionString)
     return false;
 
   // For now, none of the static analyzer API is considered stable.
   // Versions must match exactly.
   return strcmp(versionString, CLANG_ANALYZER_API_VERSION_STRING) == 0;
 }
 
+static bool checkerNameLT(const CheckerRegistry::CheckerInfo &a,
+                          const CheckerRegistry::CheckerInfo &b) {
+  return a.FullName < b.FullName;
+}
+
+static constexpr char PackageSeparator = '.';
+
+static bool isInPackage(const CheckerRegistry::CheckerInfo &checker,
+                        StringRef packageName) {
+  // Does the checker's full name have the package as a prefix?
+  if (!checker.FullName.startswith(packageName))
+    return false;
+
+  // Is the package actually just the name of a specific checker?
+  if (checker.FullName.size() == packageName.size())
+    return true;
+
+  // Is the checker in the package (or a subpackage)?
+  if (checker.FullName[packageName.size()] == PackageSeparator)
+    return true;
+
+  return false;
+}
+
+CheckerRegistry::CheckerInfoListRange
+CheckerRegistry::getMutableCheckersForCmdLineArg(StringRef CmdLineArg) {
+
+  assert(std::is_sorted(Checkers.begin(), Checkers.end(), checkerNameLT) &&
+         "In order to efficiently gather checkers, this function expects them "
+         "to be already sorted!");
+
+  // Use a binary search to find the possible start of the package.
+  CheckerRegistry::CheckerInfo
+      packageInfo(nullptr, nullptr, CmdLineArg, "", "");
+  auto it = std::lower_bound(Checkers.begin(), Checkers.end(),
+                             packageInfo, checkerNameLT);
+
+  if (!isInPackage(*it, CmdLineArg))
+    return { Checkers.end(), Checkers.end() };
+
+  // See how large the package is.
+  // If the package doesn't exist, assume the option refers to a single
+  // checker.
+  size_t size = 1;
+  llvm::StringMap<size_t>::const_iterator packageSize =
+      Packages.find(CmdLineArg);
+
+  if (packageSize != Packages.end())
+    size = packageSize->getValue();
+
+  return { it, it + size };
+}
+
 CheckerRegistry::CheckerRegistry(ArrayRef<std::string> plugins,
                                  DiagnosticsEngine &diags,
                                  AnalyzerOptions &AnOpts,
                                  const LangOptions &LangOpts)
   : Diags(diags), AnOpts(AnOpts), LangOpts(LangOpts) {
 
+  // Register builtin checkers.
 #define GET_CHECKERS
 #define CHECKER(FULLNAME, CLASS, HELPTEXT, DOC_URI)                            \
   addChecker(register##CLASS, shouldRegister##CLASS, FULLNAME, HELPTEXT,       \
              DOC_URI);
 #include "clang/StaticAnalyzer/Checkers/Checkers.inc"
 #undef CHECKER
 #undef GET_CHECKERS
 
+  // Register checkers from plugins.
   for (ArrayRef<std::string>::iterator i = plugins.begin(), e = plugins.end();
        i != e; ++i) {
     // Get access to the plugin.
     std::string err;
     DynamicLibrary lib = DynamicLibrary::getPermanentLibrary(i->c_str(), &err);
     if (!lib.isValid()) {
       diags.Report(diag::err_fe_unable_to_load_plugin) << *i << err;
       continue;
for (ArrayRef<std::string>::iterator i = plugins.begin(), e = plugins.end();
 
     // Register its checkers.
     RegisterCheckersFn registerPluginCheckers =
       (RegisterCheckersFn) (intptr_t) lib.getAddressOfSymbol(
                                                       "clang_registerCheckers");
     if (registerPluginCheckers)
       registerPluginCheckers(*this);
   }
-}
-
-static constexpr char PackageSeparator = '.';
 
-static bool checkerNameLT(const CheckerRegistry::CheckerInfo &a,
-                          const CheckerRegistry::CheckerInfo &b) {
-  return a.FullName < b.FullName;
-}
-
-static bool isInPackage(const CheckerRegistry::CheckerInfo &checker,
-                        StringRef packageName) {
-  // Does the checker's full name have the package as a prefix?
-  if (!checker.FullName.startswith(packageName))
-    return false;
+  // Sort checkers for efficient collection.
+  // FIXME: Alphabetical sort puts 'experimental' in the middle.
+  // Would it be better to name it '~experimental' or something else
+  // that's ASCIIbetically last?
+  llvm::sort(Checkers, checkerNameLT);
 
-  // Is the package actually just the name of a specific checker?
-  if (checker.FullName.size() == packageName.size())
-    return true;
+  // Parse '-analyzer-checker' and '-analyzer-disable-checker' options from the
+  // command line.
+  for (const std::pair<std::string, bool> &opt : AnOpts.CheckersControlList) {
+    CheckerInfoListRange checkersForCmdLineArg =
+                                     getMutableCheckersForCmdLineArg(opt.first);
 
-  // Is the checker in the package (or a subpackage)?
-  if (checker.FullName[packageName.size()] == PackageSeparator)
-    return true;
+    if (checkersForCmdLineArg.begin() == checkersForCmdLineArg.end()) {
+      Diags.Report(diag::err_unknown_analyzer_checker) << opt.first;
+      Diags.Report(diag::note_suggest_disabling_all_checkers);
+    }
 
-  return false;
+    for (CheckerInfo &checker : checkersForCmdLineArg) {
+      checker.State = opt.second ? StateFromCmdLine::State_Enabled :
+                                   StateFromCmdLine::State_Disabled;
+    }
+  }
 }
 
 CheckerRegistry::CheckerInfoSet CheckerRegistry::getEnabledCheckers() const {
 
-  assert(std::is_sorted(Checkers.begin(), Checkers.end(), checkerNameLT) &&
-         "In order to efficiently gather checkers, this function expects them "
-         "to be already sorted!");
-
   CheckerInfoSet enabledCheckers;
-  const auto end = Checkers.cend();
 
-  for (const std::pair<std::string, bool> &opt : AnOpts.CheckersControlList) {
-    // Use a binary search to find the possible start of the package.
-    CheckerRegistry::CheckerInfo
-        packageInfo(nullptr, nullptr, opt.first, "", "");
-    auto firstRelatedChecker =
-      std::lower_bound(Checkers.cbegin(), end, packageInfo, checkerNameLT);
-
-    if (firstRelatedChecker == end ||
-        !isInPackage(*firstRelatedChecker, opt.first)) {
-      Diags.Report(diag::err_unknown_analyzer_checker) << opt.first;
-      Diags.Report(diag::note_suggest_disabling_all_checkers);
-      return {};
-    }
-
-    // See how large the package is.
-    // If the package doesn't exist, assume the option refers to a single
-    // checker.
-    size_t size = 1;
-    llvm::StringMap<size_t>::const_iterator packageSize =
-      Packages.find(opt.first);
-    if (packageSize != Packages.end())
-      size = packageSize->getValue();
-
-    // Step through all the checkers in the package.
-    for (auto lastRelatedChecker = firstRelatedChecker+size;
-         firstRelatedChecker != lastRelatedChecker; ++firstRelatedChecker)
-      if (opt.second) {
-        if (firstRelatedChecker->ShouldRegister(LangOpts))
-          enabledCheckers.insert(&*firstRelatedChecker);
-      } else {
-        enabledCheckers.remove(&*firstRelatedChecker);
-      }
+  for (const CheckerInfo &checker : Checkers) {
+    if (checker.isEnabled(LangOpts))
+      enabledCheckers.insert(&checker);
   }
 
   return enabledCheckers;
 }
 
 void CheckerRegistry::addChecker(InitializationFunction Rfn,
                                  ShouldRegisterFunction Sfn, StringRef Name,
                                  StringRef Desc, StringRef DocsUri) {
   Checkers.emplace_back(Rfn, Sfn, Name, Desc, DocsUri);
 
   // Record the presence of the checker in its packages.
   StringRef packageName, leafName;
   std::tie(packageName, leafName) = Name.rsplit(PackageSeparator);
   while (!leafName.empty()) {
     Packages[packageName] += 1;
     std::tie(packageName, leafName) = packageName.rsplit(PackageSeparator);
   }
 }
 
 void CheckerRegistry::initializeManager(CheckerManager &checkerMgr) const {
-  // Sort checkers for efficient collection.
-  llvm::sort(Checkers, checkerNameLT);
 
   // Collect checkers enabled by the options.
   CheckerInfoSet enabledCheckers = getEnabledCheckers();
 
   // Initialize the CheckerManager with all enabled checkers.
   for (const auto *i : enabledCheckers) {
     checkerMgr.setCurrentCheckName(CheckName(i->FullName));
     i->Initialize(checkerMgr);
for (const auto &config : AnOpts.Config) {
     }
     if (!hasChecker)
       Diags.Report(diag::err_unknown_analyzer_checker) << checkerName;
   }
 }
 
 void CheckerRegistry::printHelp(raw_ostream &out,
                                 size_t maxNameChars) const {
-  // FIXME: Alphabetical sort puts 'experimental' in the middle.
-  // Would it be better to name it '~experimental' or something else
-  // that's ASCIIbetically last?
-  llvm::sort(Checkers, checkerNameLT);
-
   // FIXME: Print available packages.
 
   out << "CHECKERS:\n";
 
   // Find the maximum option length.
   size_t optionFieldWidth = 0;
   for (const auto &i : Checkers) {
     // Limit the amount of padding we are willing to give up for alignment.
for (const auto &i : Checkers) {
     }
     out.indent(pad + 2) << i.Desc;
 
     out << '\n';
   }
 }
 
 void CheckerRegistry::printList(raw_ostream &out) const {
-  // Sort checkers for efficient collection.
-  llvm::sort(Checkers, checkerNameLT);
-
   // Collect checkers enabled by the options.
   CheckerInfoSet enabledCheckers = getEnabledCheckers();
 
   for (const auto *i : enabledCheckers)
     out << i->FullName << '\n';
 }