[analyzer] Prefer returns values to out-params in CheckerRegistry.cpp

[Szelethus]

Summary

This bugged me for a long time, so it's time to put an end to it: collectCheckers was cryptic and hard to understand. This is done by

• Renaming collectCheckers to getEnabledCheckers
• Changing the functionality to acquire all enabled checkers, rather then collect checkers for a specific CheckerOptInfo (for example, collecting all checkers for { "core", true }, which meant enabling all checkers from the core package, which was an unnecessary complication).
• Removing CheckerOptInfo, instead of storing whether the option was claimed via a field, we handle errors immediately, as getEnabledCheckers can now access a DiagnosticsEngine. Realize that the remaining information it stored is directly accessible through AnalyzerOptions.CheckerControlList.
• Fix a test with -analyzer-disable-checker -verify accidentally left in.

[Szelethus]

Also, remove diags::note_suggest_disabling_all_checkers.

[MTC]

I'm totally fine with this patch personally. However I am not familiar with this part, so can't give substantial help :).

lib/StaticAnalyzer/Core/CheckerRegistry.cpp

49–91

I just curious about the reason why you move the CheckerInfoSet from the parameter passing by reference to the return value, keep the number of parameter at a lower value? Or make the code at the caller cite cleaner?

[Szelethus]

Thanks for taking a look! ^-^

lib/StaticAnalyzer/Core/CheckerRegistry.cpp

49–91

Both, actually. From what I've seen, out-params usually lead to much harder to comprehend code.

[Szelethus]

Might as well make it a method.

[george.karpenkov]

include/clang/Basic/DiagnosticFrontendKinds.td

177 ↗

(On Diff #173717)

@NoQ what does this option even do? Can you think of some legitimate uses?

[george.karpenkov]

Also, remove diags::note_suggest_disabling_all_checkers.

Isn't that a separate revision? Given that removing existing options is often questionable, I'd much rather see this patch separated.

[Szelethus]

In D54401#1295832, @george.karpenkov wrote:

Also, remove diags::note_suggest_disabling_all_checkers.

Isn't that a separate revision? Given that removing existing options is often questionable, I'd much rather see this patch separated.

This isn't an option, just a note :)

[NoQ]

include/clang/Basic/DiagnosticFrontendKinds.td

177 ↗

(On Diff #173717)

rC216763 is the commit in which it was added and the commit message sounds useful. I never understood how exactly are people using it, but it sounds as if they are trying to skip analysis of certain files by appending a flag that disables checkers to the build(?!) command of those files.

[Szelethus]

include/clang/Basic/DiagnosticFrontendKinds.td

177 ↗

(On Diff #173717)

Okay, let's move this to another revision then.

[Szelethus]

Restored the discussed note.

[Szelethus]

Ping

[Szelethus]

Ping^2

[NoQ]

The code looks good, but i vaguely remember that we might accidentally break clang-tidy integration that uses this API to enable/disable specific checkers via -checks=-analyzer-....

*summons @alexfh*

[Szelethus]

*advanced summoning*

[alexfh]

In D54401#1315354, @NoQ wrote:

The code looks good, but i vaguely remember that we might accidentally break clang-tidy integration that uses this API to enable/disable specific checkers via -checks=-analyzer-....

*summons @alexfh*

It's hard to tell without trying. Could you build and test clang-tools-extra with this patch? There should be a test for the relevant functionality in clang-tidy.

[Szelethus]

In D54401#1318282, @alexfh wrote:

In D54401#1315354, @NoQ wrote:

The code looks good, but i vaguely remember that we might accidentally break clang-tidy integration that uses this API to enable/disable specific checkers via -checks=-analyzer-....

*summons @alexfh*

It's hard to tell without trying. Could you build and test clang-tools-extra with this patch? There should be a test for the relevant functionality in clang-tidy.

It compiles without problem, and check-all doesn't produce a single clang-tools-extra failure. Yay!

[alexfh]

In D54401#1320772, @Szelethus wrote:

In D54401#1318282, @alexfh wrote:

In D54401#1315354, @NoQ wrote:

The code looks good, but i vaguely remember that we might accidentally break clang-tidy integration that uses this API to enable/disable specific checkers via -checks=-analyzer-....

*summons @alexfh*

It's hard to tell without trying. Could you build and test clang-tools-extra with this patch? There should be a test for the relevant functionality in clang-tidy.

It compiles without problem, and check-all doesn't produce a single clang-tools-extra failure. Yay!

Then I have no concerns.

[Szelethus]

Thanks! I'll commit in a couple days to give some breathing room for anyone who'd object.

[Szelethus]

Commited in rC349274.

include/clang/StaticAnalyzer/Core/CheckerRegistry.h

struct CheckerInfo {
     StringRef FullName;
     StringRef Desc;
 
     CheckerInfo(InitializationFunction fn, StringRef name, StringRef desc)
         : Initialize(fn), FullName(name), Desc(desc) {}
   };
 
   using CheckerInfoList = std::vector<CheckerInfo>;
+  using CheckerInfoSet = llvm::SetVector<const CheckerRegistry::CheckerInfo *>;
 
 private:
   template <typename T>
   static void initializeManager(CheckerManager &mgr) {
     mgr.registerChecker<T>();
   }
 
 public:
public:
 
   /// Check if every option corresponds to a specific checker or package.
   void validateCheckerOptions(const AnalyzerOptions &opts,
                               DiagnosticsEngine &diags) 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 AnalyzerOptions &opts) const;
+  void printList(raw_ostream &out, const AnalyzerOptions &opts,
+                 DiagnosticsEngine &diags) const;
 
 private:
+  CheckerInfoSet getEnabledCheckers(const AnalyzerOptions &Opts,
+                                    DiagnosticsEngine &diags) const;
+
   mutable CheckerInfoList Checkers;
   mutable llvm::StringMap<size_t> Packages;
 };
 
 } // namespace ento
 
 } // namespace clang
 
 #endif // LLVM_CLANG_STATICANALYZER_CORE_CHECKERREGISTRY_H

include/clang/StaticAnalyzer/Frontend/FrontendActions.h

std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
                                                  StringRef InFile) override;
 
 private:
   llvm::StringMap<Stmt *> &Bodies;
 };
 
 void printCheckerHelp(raw_ostream &OS, ArrayRef<std::string> plugins);
 void printEnabledCheckerList(raw_ostream &OS, ArrayRef<std::string> plugins,
-                             const AnalyzerOptions &opts);
+                             const AnalyzerOptions &opts,
+                             DiagnosticsEngine &diags);
 void printAnalyzerConfigList(raw_ostream &OS);
 
 } // end GR namespace
 
 } // end namespace clang
 
 #endif

lib/FrontendTool/ExecuteCompilerInvocation.cpp

if (Clang->getAnalyzerOpts()->ShowCheckerHelp) {
     ento::printCheckerHelp(llvm::outs(), Clang->getFrontendOpts().Plugins);
     return true;
   }
 
   // Honor -analyzer-list-enabled-checkers.
   if (Clang->getAnalyzerOpts()->ShowEnabledCheckerList) {
     ento::printEnabledCheckerList(llvm::outs(),
                                   Clang->getFrontendOpts().Plugins,
-                                  *Clang->getAnalyzerOpts());
+                                  *Clang->getAnalyzerOpts(),
+                                  Clang->getDiagnostics());
   }
 
   // Honor -analyzer-config-help.
   if (Clang->getAnalyzerOpts()->ShowConfigOptionsList) {
     ento::printAnalyzerConfigList(llvm::outs());
     return true;
   }
 #endif

lib/StaticAnalyzer/Core/CheckerRegistry.cpp

 //===- CheckerRegistry.cpp - Maintains all available checkers -------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/StaticAnalyzer/Core/CheckerRegistry.h"
 #include "clang/Basic/Diagnostic.h"
-#include "clang/Basic/LLVM.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
-#include <cstddef>
-#include <tuple>
 
 using namespace clang;
 using namespace ento;
 
-static const char PackageSeparator = '.';
-
-using CheckerInfoSet = llvm::SetVector<const CheckerRegistry::CheckerInfo *>;
-
-namespace {
-/// Represents a request to include or exclude a checker or package from a
-/// specific analysis run.
-///
-/// \sa CheckerRegistry::initializeManager
-class CheckerOptInfo {
-  StringRef Name;
-  bool Enable;
-  bool Claimed;
-
-public:
-  CheckerOptInfo(StringRef name, bool enable)
-    : Name(name), Enable(enable), Claimed(false) { }
-
-  StringRef getName() const { return Name; }
-  bool isEnabled() const { return Enable; }
-  bool isDisabled() const { return !isEnabled(); }
-
-  bool isClaimed() const { return Claimed; }
-  bool isUnclaimed() const { return !isClaimed(); }
-  void claim() { Claimed = true; }
-};
-
-} // end of anonymous namespace
-
-static SmallVector<CheckerOptInfo, 8>
-getCheckerOptList(const AnalyzerOptions &opts) {
-  SmallVector<CheckerOptInfo, 8> checkerOpts;
-  for (unsigned i = 0, e = opts.CheckersControlList.size(); i != e; ++i) {
-    const std::pair<std::string, bool> &opt = opts.CheckersControlList[i];
-    checkerOpts.push_back(CheckerOptInfo(opt.first, opt.second));
-  }
-  return checkerOpts;
-}
+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;
 
   // 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;
 }
 
-/// Collects the checkers for the supplied \p opt option into \p collected.
-static void collectCheckers(const CheckerRegistry::CheckerInfoList &checkers,
-                            const llvm::StringMap<size_t> &packageSizes,
-                            CheckerOptInfo &opt, CheckerInfoSet &collected) {
-  // Use a binary search to find the possible start of the package.
-  CheckerRegistry::CheckerInfo packageInfo(nullptr, opt.getName(), "");
-  auto end = checkers.cend();
-  auto i = std::lower_bound(checkers.cbegin(), end, packageInfo, checkerNameLT);
-
-  // If we didn't even find a possible package, give up.
-  if (i == end)
-    return;
-
-  // If what we found doesn't actually start the package, give up.
-  if (!isInPackage(*i, opt.getName()))
-    return;
+CheckerRegistry::CheckerInfoSet CheckerRegistry::getEnabledCheckers(
+                                              const AnalyzerOptions &Opts,
+                                              DiagnosticsEngine &diags) 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();
 
-  // There is at least one checker in the package; claim the option.
-  opt.claim();
+  for (const std::pair<std::string, bool> &opt : Opts.CheckersControlList) {
+    // Use a binary search to find the possible start of the package.
+    CheckerRegistry::CheckerInfo packageInfo(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.
+    // 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 =
-    packageSizes.find(opt.getName());
-  if (packageSize != packageSizes.end())
+      Packages.find(opt.first);
+    if (packageSize != Packages.end())
       size = packageSize->getValue();
 
     // Step through all the checkers in the package.
-  for (auto checkEnd = i+size; i != checkEnd; ++i)
-    if (opt.isEnabled())
-      collected.insert(&*i);
+    for (auto lastRelatedChecker = firstRelatedChecker+size;
+         firstRelatedChecker != lastRelatedChecker; ++firstRelatedChecker)
+      if (opt.second)
+        enabledCheckers.insert(&*firstRelatedChecker);
       else
-      collected.remove(&*i);
+        enabledCheckers.remove(&*firstRelatedChecker);
+  }
+
+  return enabledCheckers;
 }
 
 void CheckerRegistry::addChecker(InitializationFunction fn, StringRef name,
                                  StringRef desc) {
   Checkers.push_back(CheckerInfo(fn, name, desc));
 
   // 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 AnalyzerOptions &Opts,
                                         DiagnosticsEngine &diags) const {
   // Sort checkers for efficient collection.
   llvm::sort(Checkers, checkerNameLT);
 
-  llvm::SmallVector<CheckerOptInfo, 8> checkerOpts = getCheckerOptList(Opts);
   // Collect checkers enabled by the options.
-  CheckerInfoSet enabledCheckers;
-  for (auto &i : checkerOpts)
-    collectCheckers(Checkers, Packages, i, enabledCheckers);
+  CheckerInfoSet enabledCheckers = getEnabledCheckers(Opts, diags);
 
   // Initialize the CheckerManager with all enabled checkers.
-  for (const auto *i :enabledCheckers) {
+  for (const auto *i : enabledCheckers) {
     checkerMgr.setCurrentCheckName(CheckName(i->FullName));
     i->Initialize(checkerMgr);
   }
-
-  for (unsigned i = 0, e = checkerOpts.size(); i != e; ++i) {
-    if (checkerOpts[i].isUnclaimed()) {
-      diags.Report(diag::err_unknown_analyzer_checker)
-          << checkerOpts[i].getName();
-      diags.Report(diag::note_suggest_disabling_all_checkers);
-    }
-
-  }
 }
 
 void CheckerRegistry::validateCheckerOptions(const AnalyzerOptions &opts,
                                              DiagnosticsEngine &diags) const {
   for (const auto &config : opts.Config) {
     size_t pos = config.getKey().find(':');
     if (pos == StringRef::npos)
       continue;
for (const auto &i : Checkers) {
     }
     out.indent(pad + 2) << i.Desc;
 
     out << '\n';
   }
 }
 
 void CheckerRegistry::printList(raw_ostream &out,
-                                const AnalyzerOptions &opts) const {
+                                const AnalyzerOptions &opts,
+                                DiagnosticsEngine &diags) const {
+  // Sort checkers for efficient collection.
   llvm::sort(Checkers, checkerNameLT);
 
-  llvm::SmallVector<CheckerOptInfo, 8> checkerOpts = getCheckerOptList(opts);
   // Collect checkers enabled by the options.
-  CheckerInfoSet enabledCheckers;
-  for (auto &i : checkerOpts)
-    collectCheckers(Checkers, Packages, i, enabledCheckers);
+  CheckerInfoSet enabledCheckers = getEnabledCheckers(opts, diags);
 
   for (const auto *i : enabledCheckers)
     out << i->FullName << '\n';
 }

lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp

void ento::printCheckerHelp(raw_ostream &out, ArrayRef<std::string> plugins) {
   out << "OVERVIEW: Clang Static Analyzer Checkers List\n\n";
   out << "USAGE: -analyzer-checker <CHECKER or PACKAGE,...>\n\n";
 
   ClangCheckerRegistry(plugins).printHelp(out);
 }
 
 void ento::printEnabledCheckerList(raw_ostream &out,
                                    ArrayRef<std::string> plugins,
-                                   const AnalyzerOptions &opts) {
+                                   const AnalyzerOptions &opts,
+                                   DiagnosticsEngine &diags) {
   out << "OVERVIEW: Clang Static Analyzer Enabled Checkers List\n\n";
 
-  ClangCheckerRegistry(plugins).printList(out, opts);
+  ClangCheckerRegistry(plugins).printList(out, opts, diags);
 }
 
 void ento::printAnalyzerConfigList(raw_ostream &out) {
   out << "OVERVIEW: Clang Static Analyzer -analyzer-config Option List\n\n";
   out << "USAGE: clang -cc1 [CLANG_OPTIONS] -analyzer-config "
                                         "<OPTION1=VALUE,OPTION2=VALUE,...>\n\n";
   out << "       clang -cc1 [CLANG_OPTIONS] -analyzer-config OPTION1=VALUE, "
                                       "-analyzer-config OPTION2=VALUE, ...\n\n";

test/Analysis/disable-all-checks.c

-// RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-store=region -analyzer-disable-all-checks -verify %s
-// RUN: %clang_analyze_cc1 -analyzer-disable-all-checks -analyzer-checker=core -analyzer-store=region -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-store=region \
+// RUN:   -analyzer-disable-all-checks -verify %s
+//
+// RUN: %clang_analyze_cc1 -analyzer-disable-all-checks -analyzer-checker=core \
+// RUN:   -analyzer-store=region -verify %s
+//
 // RUN: %clang_analyze_cc1 -analyzer-disable-all-checks -verify %s
-// RUN: not %clang_analyze_cc1 -analyzer-checker=core -analyzer-store=region -analyzer-disable-checker -verify %s 2>&1 | FileCheck %s
+//
+// RUN: not %clang_analyze_cc1 -analyzer-checker=core -analyzer-store=region \
+// RUN:   -analyzer-disable-checker non.existant.Checker -verify %s 2>&1 \
+// RUN:   | FileCheck %s
+//
 // expected-no-diagnostics
 
+// CHECK: no analyzer checkers are associated with 'non.existant.Checker'
 // CHECK: use -analyzer-disable-all-checks to disable all static analyzer checkers
 int buggy() {
   int x = 0;
   return 5/x; // no warning
 }