[analyzer] Add CheckerManager::getChecker, make sure that a registry function registers no more than 1 checker

[Szelethus]

Summary

Title says it all, this is pretty important. This is the last patch needed to ensure that the checker naming bug won't arise again.

[Szelethus]

Actually, ensuring that a registry function registers no more than one checker can be pulled off very easily -- the change required is so little, I added it to this patch. With that, the checker name bug is "officially" fixed.

[NoQ]

Aha, ok, so what's the final procedure now to register a checker that's artificially split in two? Something like this, right?

def CommonModel : Checker<"Common">,
  HelpText<"Doesn't emit warnings but models common stuff.">;

def SubChecker : Checker<"Sub">,
  HelpText<"Emits common warnings for the sub-stuff.">,
  Dependencies<[CommonModel]>;

void registerCommonModel(CheckerManager &Mgr) {
  Mgr.registerChecker<CommonModel>();
}

void registerSubChecker(CheckerManager &Mgr) {
  CommonModel *Model = Mgr.getChecker<CommonModel>();
  Model->EnableSubChecker = true;
}

This looks quite usable to me.

test/Analysis/free.c

1

When cleaning this stuff up, please feel free to drop -analyzer-store=region entirely (same for other options that allow only one value).

[Szelethus]

In D55429#1324035, @NoQ wrote:

Aha, ok, so what's the final procedure now to register a checker that's artificially split in two? Something like this, right?

def CommonModel : Checker<"Common">,
  HelpText<"Doesn't emit warnings but models common stuff.">;

def SubChecker : Checker<"Sub">,
  HelpText<"Emits common warnings for the sub-stuff.">,
  Dependencies<[CommonModel]>;

void registerCommonModel(CheckerManager &Mgr) {
  Mgr.registerChecker<CommonModel>();
}

void registerSubChecker(CheckerManager &Mgr) {
  CommonModel *Model = Mgr.getChecker<CommonModel>();
  Model->EnableSubChecker = true;
}

This looks quite usable to me.

Correct! But since I spent so much time with these files, and I don't expect maaajor changes to them in the foreseeable future, I'll take the time to properly document how the frontend of the analyzer (especially how checker registration) works. Maybe with the new sphinx format if it goes through by then, but any format is better than none.

[Szelethus]

Rebase.

include/clang/StaticAnalyzer/Core/CheckerManager.h

public:
 
   void setCurrentCheckName(CheckName name) { CurrentCheckName = name; }
   CheckName getCurrentCheckName() const { return CurrentCheckName; }
 
   bool hasPathSensitiveCheckers() const;
 
   void finishedCheckerRegistration();
 
+  unsigned getRegisteredCheckerCount() { return CheckerDtors.size(); }
+
   const LangOptions &getLangOpts() const { return LangOpts; }
   AnalyzerOptions &getAnalyzerOptions() { return AOptions; }
   ASTContext &getASTContext() { return Context; }
 
   using CheckerRef = CheckerBase *;
   using CheckerTag = const void *;
   using CheckerDtor = CheckerFn<void ()>;
 
 //===----------------------------------------------------------------------===//
-// registerChecker
+// Checker registration.
 //===----------------------------------------------------------------------===//
 
   /// Used to register checkers.
   /// All arguments are automatically passed through to the checker
   /// constructor.
   ///
   /// \returns a pointer to the checker object.
   template <typename CHECKER, typename... AT>
   CHECKER *registerChecker(AT &&... Args) {
     CheckerTag tag = getTag<CHECKER>();
     CheckerRef &ref = CheckerTags[tag];
-    if (ref)
-      return static_cast<CHECKER *>(ref); // already registered.
+    assert(!ref && "Checker already registered, use getChecker!");
 
     CHECKER *checker = new CHECKER(std::forward<AT>(Args)...);
     checker->Name = CurrentCheckName;
     CheckerDtors.push_back(CheckerDtor(checker, destruct<CHECKER>));
     CHECKER::_register(checker, *this);
     ref = checker;
     return checker;
   }
 
+  template <typename CHECKER>
+  CHECKER *getChecker() {
+    CheckerTag tag = getTag<CHECKER>();
+    assert(CheckerTags.count(tag) != 0 &&
+           "Requested checker is not registered! Maybe you should add it as a "
+           "dependency in Checkers.td?");
+    return static_cast<CHECKER *>(CheckerTags[tag]);
+  }
+
 //===----------------------------------------------------------------------===//
-// Functions for running checkers for AST traversing..
+// Functions for running checkers for AST traversing.
 //===----------------------------------------------------------------------===//
 
   /// Run checkers handling Decls.
   void runCheckersOnASTDecl(const Decl *D, AnalysisManager& mgr,
                             BugReporter &BR);
 
   /// Run checkers handling Decls containing a Stmt body.
   void runCheckersOnASTBody(const Decl *D, AnalysisManager& mgr,

lib/StaticAnalyzer/Checkers/CStringChecker.cpp

 }
 
 bool ento::shouldRegisterCStringModeling(const LangOptions &LO) {
   return true;
 }
 
 #define REGISTER_CHECKER(name)                                                 \
   void ento::register##name(CheckerManager &mgr) {                             \
-    CStringChecker *checker = mgr.registerChecker<CStringChecker>();           \
+    CStringChecker *checker = mgr.getChecker<CStringChecker>();                \
     checker->Filter.Check##name = true;                                        \
     checker->Filter.CheckName##name = mgr.getCurrentCheckName();               \
   }                                                                            \
                                                                                \
   bool ento::shouldRegister##name(const LangOptions &LO) {                     \
     return true;                                                               \
   }
 
   REGISTER_CHECKER(CStringNullArg)
   REGISTER_CHECKER(CStringOutOfBounds)
   REGISTER_CHECKER(CStringBufferOverlap)
 REGISTER_CHECKER(CStringNotNullTerm)

lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp

void ento::registerCallAndMessageChecker(CheckerManager &mgr) {
   mgr.registerChecker<CallAndMessageChecker>();
 }
 
 bool ento::shouldRegisterCallAndMessageChecker(const LangOptions &LO) {
   return true;
 }
 
 void ento::registerCallAndMessageUnInitRefArg(CheckerManager &mgr) {
-  CallAndMessageChecker *Checker =
-      mgr.registerChecker<CallAndMessageChecker>();
+  CallAndMessageChecker *Checker = mgr.getChecker<CallAndMessageChecker>();
   Checker->Check_CallAndMessageUnInitRefArg = true;
   Checker->CheckName_CallAndMessageUnInitRefArg = mgr.getCurrentCheckName();
 }
 
 bool ento::shouldRegisterCallAndMessageUnInitRefArg(const LangOptions &LO) {
   return true;
 }

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

 }
 
 bool ento::shouldRegisterSecuritySyntaxChecker(const LangOptions &LO) {
   return true;
 }
 
 #define REGISTER_CHECKER(name)                                                 \
   void ento::register##name(CheckerManager &mgr) {                             \
-    SecuritySyntaxChecker *checker =                                           \
-        mgr.registerChecker<SecuritySyntaxChecker>();                          \
+    SecuritySyntaxChecker *checker =  mgr.getChecker<SecuritySyntaxChecker>(); \
     checker->filter.check_##name = true;                                       \
     checker->filter.checkName_##name = mgr.getCurrentCheckName();              \
   }                                                                            \
                                                                                \
   bool ento::shouldRegister##name(const LangOptions &LO) {                     \
     return true;                                                               \
   }
 

lib/StaticAnalyzer/Checkers/DirectIvarAssignment.cpp

 }
 
 bool ento::shouldRegisterDirectIvarAssignment(const LangOptions &LO) {
   return true;
 }
 
 void ento::registerDirectIvarAssignmentForAnnotatedFunctions(
     CheckerManager &mgr) {
-  mgr.registerChecker<DirectIvarAssignment>()->ShouldSkipMethod = &AttrFilter;
+  mgr.getChecker<DirectIvarAssignment>()->ShouldSkipMethod = &AttrFilter;
 }
 
 bool ento::shouldRegisterDirectIvarAssignmentForAnnotatedFunctions(
                                                         const LangOptions &LO) {
   return true;
 }

lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp

DynamicTypePropagation::GenericsBugVisitor::VisitNode(const ExplodedNode *N,
   PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
                              N->getLocationContext());
   return std::make_shared<PathDiagnosticEventPiece>(Pos, OS.str(), true,
                                                     nullptr);
 }
 
 /// Register checkers.
 void ento::registerObjCGenericsChecker(CheckerManager &mgr) {
-  DynamicTypePropagation *checker =
-      mgr.registerChecker<DynamicTypePropagation>();
+  DynamicTypePropagation *checker = mgr.getChecker<DynamicTypePropagation>();
   checker->CheckGenerics = true;
 }
 
 bool ento::shouldRegisterObjCGenericsChecker(const LangOptions &LO) {
   return true;
 }
 
 void ento::registerDynamicTypePropagation(CheckerManager &mgr) {
   mgr.registerChecker<DynamicTypePropagation>();
 }
 
 bool ento::shouldRegisterDynamicTypePropagation(const LangOptions &LO) {
   return true;
 }

lib/StaticAnalyzer/Checkers/IteratorChecker.cpp

 }
 
 bool ento::shouldRegisterIteratorModeling(const LangOptions &LO) {
   return true;
 }
 
 #define REGISTER_CHECKER(name)                                                 \
   void ento::register##name(CheckerManager &Mgr) {                             \
-    auto *checker = Mgr.registerChecker<IteratorChecker>();                    \
+    auto *checker = Mgr.getChecker<IteratorChecker>();                         \
     checker->ChecksEnabled[IteratorChecker::CK_##name] = true;                 \
     checker->CheckNames[IteratorChecker::CK_##name] =                          \
         Mgr.getCurrentCheckName();                                             \
   }                                                                            \
                                                                                \
   bool ento::shouldRegister##name(const LangOptions &LO) {                     \
     return true;                                                               \
   }
 
 REGISTER_CHECKER(IteratorRangeChecker)
 REGISTER_CHECKER(MismatchedIteratorChecker)
 REGISTER_CHECKER(InvalidatedIteratorChecker)

lib/StaticAnalyzer/Checkers/IvarInvalidationChecker.cpp

 
 bool ento::shouldRegisterIvarInvalidationModeling(const LangOptions &LO) {
   return true;
 }
 
 #define REGISTER_CHECKER(name)                                                 \
   void ento::register##name(CheckerManager &mgr) {                             \
     IvarInvalidationChecker *checker =                                         \
-        mgr.registerChecker<IvarInvalidationChecker>();                        \
+        mgr.getChecker<IvarInvalidationChecker>();                             \
     checker->Filter.check_##name = true;                                       \
     checker->Filter.checkName_##name = mgr.getCurrentCheckName();              \
   }                                                                            \
                                                                                \
   bool ento::shouldRegister##name(const LangOptions &LO) {                     \
     return true;                                                               \
   }
 
 REGISTER_CHECKER(InstanceVariableInvalidation)
 REGISTER_CHECKER(MissingInvalidationMethod)

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

 
 } // end namespace allocation_state
 } // end namespace ento
 } // end namespace clang
 
 // Intended to be used in InnerPointerChecker to register the part of
 // MallocChecker connected to it.
 void ento::registerInnerPointerCheckerAux(CheckerManager &mgr) {
-    MallocChecker *checker = mgr.registerChecker<MallocChecker>();
-    checker->IsOptimistic = mgr.getAnalyzerOptions().getCheckerBooleanOption(
-        "Optimistic", false, checker);
-    checker->ChecksEnabled[MallocChecker::CK_InnerPointerChecker] = true;
-    checker->CheckNames[MallocChecker::CK_InnerPointerChecker] =
-        mgr.getCurrentCheckName();
+  MallocChecker *checker = mgr.getChecker<MallocChecker>();
 }
 
 void ento::registerDynamicMemoryModeling(CheckerManager &mgr) {
-  mgr.registerChecker<MallocChecker>();
+  auto *checker = mgr.registerChecker<MallocChecker>();
+  checker->IsOptimistic = mgr.getAnalyzerOptions().getCheckerBooleanOption(
+        "Optimistic", false, checker);
 }
 
 bool ento::shouldRegisterDynamicMemoryModeling(const LangOptions &LO) {
   return true;
 }
 
 #define REGISTER_CHECKER(name)                                                 \
   void ento::register##name(CheckerManager &mgr) {                             \
-    MallocChecker *checker = mgr.registerChecker<MallocChecker>();             \
-    checker->IsOptimistic = mgr.getAnalyzerOptions().getCheckerBooleanOption(  \
-        "Optimistic", false, checker);                                         \
+    MallocChecker *checker = mgr.getChecker<MallocChecker>();                  \
     checker->ChecksEnabled[MallocChecker::CK_##name] = true;                   \
     checker->CheckNames[MallocChecker::CK_##name] = mgr.getCurrentCheckName(); \
   }                                                                            \
                                                                                \
   bool ento::shouldRegister##name(const LangOptions &LO) {                     \
     return true;                                                               \
   }
 
 REGISTER_CHECKER(MallocChecker)
 REGISTER_CHECKER(NewDeleteChecker)
 REGISTER_CHECKER(NewDeleteLeaksChecker)
 REGISTER_CHECKER(MismatchedDeallocatorChecker)

lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp

 }
 
 bool ento::shouldRegisterNSOrCFErrorDerefChecker(const LangOptions &LO) {
   return true;
 }
 
 void ento::registerNSErrorChecker(CheckerManager &mgr) {
   mgr.registerChecker<NSErrorMethodChecker>();
-  NSOrCFErrorDerefChecker *checker =
-      mgr.registerChecker<NSOrCFErrorDerefChecker>();
+  NSOrCFErrorDerefChecker *checker = mgr.getChecker<NSOrCFErrorDerefChecker>();
   checker->ShouldCheckNSError = true;
 }
 
 bool ento::shouldRegisterNSErrorChecker(const LangOptions &LO) {
   return true;
 }
 
 void ento::registerCFErrorChecker(CheckerManager &mgr) {
   mgr.registerChecker<CFErrorFunctionChecker>();
-  NSOrCFErrorDerefChecker *checker =
-      mgr.registerChecker<NSOrCFErrorDerefChecker>();
+  NSOrCFErrorDerefChecker *checker = mgr.getChecker<NSOrCFErrorDerefChecker>();
   checker->ShouldCheckCFError = true;
 }
 
 bool ento::shouldRegisterCFErrorChecker(const LangOptions &LO) {
   return true;
 }

lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

 }
 
 bool ento::shouldRegisterNullabilityBase(const LangOptions &LO) {
   return true;
 }
 
 #define REGISTER_CHECKER(name, trackingRequired)                               \
   void ento::register##name##Checker(CheckerManager &mgr) {                    \
-    NullabilityChecker *checker = mgr.registerChecker<NullabilityChecker>();   \
+    NullabilityChecker *checker = mgr.getChecker<NullabilityChecker>();        \
     checker->Filter.Check##name = true;                                        \
     checker->Filter.CheckName##name = mgr.getCurrentCheckName();               \
     checker->NeedTracking = checker->NeedTracking || trackingRequired;         \
     checker->NoDiagnoseCallsToSystemHeaders =                                  \
         checker->NoDiagnoseCallsToSystemHeaders ||                             \
         mgr.getAnalyzerOptions().getCheckerBooleanOption(                      \
                       "NoDiagnoseCallsToSystemHeaders", false, checker, true); \
   }                                                                            \

lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp

 void ento::registerStackAddrEscapeBase(CheckerManager &mgr) {
   mgr.registerChecker<StackAddrEscapeChecker>();
 }
 
 bool ento::shouldRegisterStackAddrEscapeBase(const LangOptions &LO) {
   return true;
 }
 
-#define REGISTER_CHECKER(name) \
-  void ento::register##name(CheckerManager &Mgr) { \
-    StackAddrEscapeChecker *Chk = \
-        Mgr.registerChecker<StackAddrEscapeChecker>(); \
-    Chk->ChecksEnabled[StackAddrEscapeChecker::CK_##name] = true; \
+#define REGISTER_CHECKER(name)                                                 \
+  void ento::register##name(CheckerManager &Mgr) {                             \
+    StackAddrEscapeChecker *Chk =                                              \
+        Mgr.getChecker<StackAddrEscapeChecker>();                              \
+    Chk->ChecksEnabled[StackAddrEscapeChecker::CK_##name] = true;              \
   }                                                                            \
                                                                                \
   bool ento::shouldRegister##name(const LangOptions &LO) {                     \
     return true;                                                               \
   }
 
 REGISTER_CHECKER(StackAddrEscapeChecker)
 REGISTER_CHECKER(StackAddrAsyncEscapeChecker)

lib/StaticAnalyzer/Checkers/ValistChecker.cpp

 }
 
 bool ento::shouldRegisterValistBase(const LangOptions &LO) {
   return true;
 }
 
 #define REGISTER_CHECKER(name)                                                 \
   void ento::register##name##Checker(CheckerManager &mgr) {                    \
-    ValistChecker *checker = mgr.registerChecker<ValistChecker>();             \
+    ValistChecker *checker = mgr.getChecker<ValistChecker>();                  \
     checker->ChecksEnabled[ValistChecker::CK_##name] = true;                   \
     checker->CheckNames[ValistChecker::CK_##name] = mgr.getCurrentCheckName(); \
   }                                                                            \
                                                                                \
   bool ento::shouldRegister##name##Checker(const LangOptions &LO) {            \
     return true;                                                               \
   }
 
 REGISTER_CHECKER(Uninitialized)
 REGISTER_CHECKER(Unterminated)
 REGISTER_CHECKER(CopyToSelf)

lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp

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))
           continue;
 
         // Recursively enable it's dependencies.
         llvm::Optional<CheckerInfoSet> deps =
             collectDependencies(*firstRelatedChecker, LangOpts);
 
         if (!deps) {

 
 void CheckerRegistry::initializeManager(CheckerManager &checkerMgr,
                                         const AnalyzerOptions &Opts) const {
   // Collect checkers enabled by the options.
   CheckerInfoSet enabledCheckers = getEnabledCheckers(Opts);
 
   // Initialize the CheckerManager with all enabled checkers.
   for (const auto *i : enabledCheckers) {
+    unsigned CurrentEnabledCheckerCount =
+        checkerMgr.getRegisteredCheckerCount();
+
     checkerMgr.setCurrentCheckName(CheckName(i->FullName));
     i->Initialize(checkerMgr);
+
+    assert(checkerMgr.getRegisteredCheckerCount() -
+           CurrentEnabledCheckerCount <= 1 &&
+           "A registry function shouldn't register more than one checker!");
+    (void)CurrentEnabledCheckerCount;
   }
 }
 
 void CheckerRegistry::validateCheckerOptions(
                                             const AnalyzerOptions &opts) const {
   for (const auto &config : opts.Config) {
     size_t pos = config.getKey().find(':');
     if (pos == StringRef::npos)

test/Analysis/analyzer-checker-config.c

 // RUN: not %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc -analyzer-config unix.mallo:Optimistic=true 2>&1 | FileCheck %s
 // RUN: not %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc -analyzer-config uni:Optimistic=true 2>&1 | FileCheck %s
 // RUN: not %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc -analyzer-config uni.:Optimistic=true 2>&1 | FileCheck %s
 // RUN: not %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc -analyzer-config ..:Optimistic=true 2>&1 | FileCheck %s
 // RUN: not %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc -analyzer-config unix.:Optimistic=true 2>&1 | FileCheck %s
 // RUN: not %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc -analyzer-config unrelated:Optimistic=true 2>&1 | FileCheck %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc -analyzer-config unix.Malloc:Optimistic=true
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc -analyzer-config unix.DynamicMemoryModeling:Optimistic=true
 
 // Just to test clang is working.
 # foo
 
 // CHECK: error:

test/Analysis/free.c

-// RUN: %clang_analyze_cc1 -analyzer-store=region -analyzer-checker=core,unix.Malloc -fblocks -verify %s
-// RUN: %clang_analyze_cc1 -analyzer-store=region -analyzer-checker=core,unix.Malloc -fblocks -verify -analyzer-config unix.Malloc:Optimistic=true %s
+// RUN: %clang_analyze_cc1 -fblocks -verify %s -analyzer-store=region \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=unix.Malloc
+//
+// RUN: %clang_analyze_cc1 -fblocks -verify %s -analyzer-store=region \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=unix.Malloc \
+// RUN:   -analyzer-config unix.DynamicMemoryModeling:Optimistic=true
 typedef __typeof(sizeof(int)) size_t;
 void free(void *);
 void *alloca(size_t);
 
 void t1 () {
   int a[] = { 1 };
   free(a); // expected-warning {{Argument to free() is the address of the local variable 'a', which is not memory allocated by malloc()}}
 }

test/Analysis/outofbound.c

-// RUN: %clang_analyze_cc1 -Wno-array-bounds -analyzer-checker=core,unix,alpha.security.ArrayBound -analyzer-store=region -verify -analyzer-config unix:Optimistic=true %s
+// RUN: %clang_analyze_cc1 -Wno-array-bounds -analyzer-store=region -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=unix \
+// RUN:   -analyzer-checker=alpha.security.ArrayBound \
+// RUN:   -analyzer-config unix:Optimistic=true
 
 typedef __typeof(sizeof(int)) size_t;
 void *malloc(size_t);
 void *calloc(size_t, size_t);
 
 char f1() {
   char* s = "abcd";
   char c = s[4]; // no-warning

test/Analysis/undef-buffers.c

-// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix,core.uninitialized -analyzer-store=region -verify -analyzer-config unix:Optimistic=true %s
+// RUN: %clang_analyze_cc1 -analyzer-store=region -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=unix \
+// RUN:   -analyzer-checker=core.uninitialized \
+// RUN:   -analyzer-config unix:Optimistic=true
+
 typedef __typeof(sizeof(int)) size_t;
 void *malloc(size_t);
 void free(void *);
 
 char stackBased1 () {
   char buf[2];
   buf[0] = 'a';
   return buf[1]; // expected-warning{{Undefined}}