[PATCH] D59461: [analyzer] Fix an assertion failure if plugins added dependencies

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus created this revision.
Szelethus added reviewers: NoQ, xazax.hun, baloghadamsoftware, rnkovacs.
Szelethus added a project: clang.
Herald added subscribers: cfe-commits, Charusso, gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, szepet, whisperity.
Szelethus added a parent revision: D59459: [analyzer][NFC] Prefer binary searches in CheckerRegistry.

Ideally, there is no reason behind not being able to depend on checkers that come from a different plugin (or on builtin checkers) -- however, this is only possible if all checkers are added to the registry before resolving checker dependencies. Since I used a binary search in my `addDependency` method, this also resulted in an assertion failure (due to `CheckerRegistry::Checkers` not being sorted), since the function used by plugins to register their checkers (`clang_registerCheckers`) calls `addDependency`.

This patch resolves this issue by only noting which dependencies have to established when `addDependency` is called, and resolves them at a later stage when no more checkers are added to the registry.


Repository:
  rC Clang

https://reviews.llvm.org/D59461

Files:
  include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  test/Analysis/checker-dependencies.c


Index: test/Analysis/checker-dependencies.c
===================================================================
--- test/Analysis/checker-dependencies.c
+++ test/Analysis/checker-dependencies.c
@@ -1,3 +1,20 @@
 // RUN: %clang_analyze_cc1 %s \
 // RUN:   -analyzer-checker=core \
 // RUN:   -analyzer-checker=nullability.NullReturnedFromNonnull
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=osx.cocoa.RetainCount \
+// RUN:   -analyzer-list-enabled-checkers \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-IMPLICITLY-ENABLED
+
+// CHECK-IMPLICITLY-ENABLED: osx.cocoa.RetainCountBase
+// CHECK-IMPLICITLY-ENABLED: osx.cocoa.RetainCount
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=osx.cocoa.RetainCount \
+// RUN:   -analyzer-disable-checker=osx.cocoa.RetainCountBase \
+// RUN:   -analyzer-list-enabled-checkers \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-IMPLICITLY-DISABLED
+
+// CHECK-IMPLICITLY-DISABLED-NOT: osx.cocoa.RetainCountBase
+// CHECK-IMPLICITLY-DISABLED-NOT: osx.cocoa.RetainCount
Index: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
===================================================================
--- lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
+++ lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
@@ -180,6 +180,8 @@
 #undef CHECKER_DEPENDENCY
 #undef GET_CHECKER_DEPENDENCIES
 
+  resolveDependencies();
+
   // Parse '-analyzer-checker' and '-analyzer-disable-checker' options from the
   // command line.
   for (const std::pair<std::string, bool> &Opt : AnOpts.CheckersControlList) {
@@ -281,17 +283,25 @@
   }
 }
 
-void CheckerRegistry::addDependency(StringRef FullName, StringRef Dependency) {
-  auto CheckerIt = binaryFind(Checkers, FullName);
-  assert(CheckerIt != Checkers.end() &&
-         "Failed to find the checker while attempting to set up its "
-         "dependencies!");
+void CheckerRegistry::resolveDependencies() {
+  for (const std::pair<StringRef, StringRef> &Entry : Dependencies) {
+    auto CheckerIt = binaryFind(Checkers, Entry.first);
+    assert(CheckerIt != Checkers.end() &&
+           "Failed to find the checker while attempting to set up its "
+           "dependencies!");
 
-  auto DependencyIt = binaryFind(Checkers, Dependency);
-  assert(DependencyIt != Checkers.end() &&
-         "Failed to find the dependency of a checker!");
+    auto DependencyIt = binaryFind(Checkers, Entry.second);
+    assert(DependencyIt != Checkers.end() &&
+           "Failed to find the dependency of a checker!");
+
+    CheckerIt->Dependencies.emplace_back(&*DependencyIt);
+  }
 
-  CheckerIt->Dependencies.emplace_back(&*DependencyIt);
+  Dependencies.clear();
+}
+
+void CheckerRegistry::addDependency(StringRef FullName, StringRef Dependency) {
+  Dependencies.emplace_back(FullName, Dependency);
 }
 
 void CheckerRegistry::initializeManager(CheckerManager &CheckerMgr) const {
Index: include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
===================================================================
--- include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
+++ include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
@@ -195,6 +195,12 @@
   CheckerInfoList Checkers;
   llvm::StringMap<size_t> PackageSizes;
 
+  /// Contains all (Dependendent checker, Dependency) pairs. We need this, as
+  /// we'll resolve dependencies after all checkers were added first.
+  llvm::SmallVector<std::pair<StringRef, StringRef>, 0> Dependencies;
+
+  void resolveDependencies();
+
   DiagnosticsEngine &Diags;
   AnalyzerOptions &AnOpts;
   const LangOptions &LangOpts;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D59461.190976.patch
Type: text/x-patch
Size: 3578 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190316/6bef161a/attachment.bin>