[clangd] Use ToolExecutor to write the global-symbol-builder tool.

[ioeric]

Summary

This enables more execution modes like standalone and Mapreduce-style execution.

See also D41729

[hokein]

The implementation looks good, but to see whether sam has high-level comments on this.

clangd/index/SymbolYAML.cpp

140 ↗

(On Diff #128623)

The function could be simplified by using the SymbolToYAML below.

std::string Str;
for (const Symbol& S : Symbols) {
   Str += SymbolToYAML(S);
}

145 ↗

(On Diff #128623)

might be use Symbol as parameter type.

clangd/index/SymbolYAML.h

30 ↗

(On Diff #128623)

might be also mention The YAML result is safe to concatenate.

[ioeric]

Address review comments.

clangd/index/SymbolYAML.cpp

140 ↗

(On Diff #128623)

This saves two lines of code but would regress the performance, so I am inclined to the current approach.

[sammccall]

This makes SymbolCollector (production code) depend on YAML (explicitly experimental), as well as on Tooling interfaces.
At least we should invert this by making the thing passed to SymbolCollector a function<void(const Symbol&)> or so.

Moreover, it's unfortunate we'd need to change SymbolCollector at all. Currently it's a nice abstraction that does one thing (AST -> SymbolSlab), now we're making it do two things. It can't be for scalability reasons - there's no problem fitting all the symbols from a TU in memory. So why do we need to do this?
I don't really understand the ToolExecutor interfaces well, but ISTM overriding SymbolIndexActionFactory::runInvocation to flush the symbols in the slab out to the ExecutionContext would allow you to use SymbolCollector unchanged?

[ioeric]

Address review comment: move all ToolExecutor and YAML-specific logics out of SymbolCollector.

[ioeric]

In D41730#970704, @sammccall wrote:

This makes SymbolCollector (production code) depend on YAML (explicitly experimental), as well as on Tooling interfaces.
At least we should invert this by making the thing passed to SymbolCollector a function<void(const Symbol&)> or so.

Thanks for the catch! I completely missed these perspectives...

Moreover, it's unfortunate we'd need to change SymbolCollector at all. Currently it's a nice abstraction that does one thing (AST -> SymbolSlab), now we're making it do two things. It can't be for scalability reasons - there's no problem fitting all the symbols from a TU in memory. So why do we need to do this?
I don't really understand the ToolExecutor interfaces well, but ISTM overriding SymbolIndexActionFactory::runInvocation to flush the symbols in the slab out to the ExecutionContext would allow you to use SymbolCollector unchanged?

WrapperFrontendAction is helpful here. All changes have been moved into the tool now. PTAL

clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp

 // production code.
 //
 //===---------------------------------------------------------------------===//
 
 #include "index/Index.h"
 #include "index/SymbolCollector.h"
 #include "index/SymbolYAML.h"
 #include "clang/Frontend/FrontendActions.h"
-#include "clang/Index/IndexingAction.h"
 #include "clang/Index/IndexDataConsumer.h"
+#include "clang/Index/IndexingAction.h"
 #include "clang/Tooling/CommonOptionsParser.h"
+#include "clang/Tooling/Execution.h"
 #include "clang/Tooling/Tooling.h"
-#include "clang/Tooling/CommonOptionsParser.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/MemoryBuffer.h"
-#include "llvm/Support/Signals.h"
 #include "llvm/Support/Path.h"
-#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Signals.h"
 #include "llvm/Support/ThreadPool.h"
 
 using namespace llvm;
 using namespace clang::tooling;
 using clang::clangd::SymbolSlab;
 
 namespace clang {
 namespace clangd {
 
 class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
 public:
-  SymbolIndexActionFactory() = default;
+  SymbolIndexActionFactory(tooling::ExecutionContext *Ctx) : Ctx(Ctx) {}
 
   clang::FrontendAction *create() override {
+    // Wraps the index action and reports collected symbols to the execution
+    // context at the end of each translation unit.
+    class WrappedIndexAction : public WrapperFrontendAction {
+    public:
+      WrappedIndexAction(std::shared_ptr<SymbolCollector> C,
+                         const index::IndexingOptions &Opts,
+                         tooling::ExecutionContext *Ctx)
+          : WrapperFrontendAction(
+                index::createIndexingAction(C, Opts, nullptr)),
+            Ctx(Ctx), Collector(C) {}
+
+      void EndSourceFileAction() override {
+        WrapperFrontendAction::EndSourceFileAction();
+
+        auto Symbols = Collector->takeSymbols();
+        for (const auto &Sym : Symbols) {
+          std::string IDStr;
+          llvm::raw_string_ostream OS(IDStr);
+          OS << Sym.ID;
+          Ctx->reportResult(OS.str(), SymbolToYAML(Sym));
+        }
+      }
+
+    private:
+      tooling::ExecutionContext *Ctx;
+      std::shared_ptr<SymbolCollector> Collector;
+    };
+
     index::IndexingOptions IndexOpts;
     IndexOpts.SystemSymbolFilter =
         index::IndexingOptions::SystemSymbolFilterKind::All;
     IndexOpts.IndexFunctionLocals = false;
-    Collector = std::make_shared<SymbolCollector>();
-    return index::createIndexingAction(Collector, IndexOpts, nullptr).release();
+    return new WrappedIndexAction(std::make_shared<SymbolCollector>(),
+                                  IndexOpts, Ctx);
   }
 
-  std::shared_ptr<SymbolCollector> Collector;
+  tooling::ExecutionContext *Ctx;
 };
 
 } // namespace clangd
 } // namespace clang
 
 int main(int argc, const char **argv) {
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
 
   const char* Overview =
       "This is an **experimental** tool to generate YAML-format "
       "project-wide symbols for clangd (global code completion). It would be "
       "changed and deprecated eventually. Don't use it in production code!";
-  CommonOptionsParser OptionsParser(argc, argv, cl::GeneralCategory,
-                                    /*Overview=*/Overview);
+  auto Executor = clang::tooling::createExecutorFromCommandLineArgs(
+      argc, argv, cl::GeneralCategory, Overview);
 
-  // No compilation database found, fallback to single TU analysis, this is
-  // mainly for debugging purpose:
-  //   global-symbol-buidler /tmp/t.cc -- -std=c++11.
-  if (OptionsParser.getCompilations().getAllFiles().empty()) {
-    llvm::errs() << "No compilation database found, processing individual "
-                    "files with flags from command-line\n.";
-    ClangTool Tool(OptionsParser.getCompilations(),
-                   OptionsParser.getSourcePathList());
-    clang::clangd::SymbolIndexActionFactory IndexAction;
-    Tool.run(&IndexAction);
-    llvm::outs() << SymbolToYAML(IndexAction.Collector->takeSymbols());
-    return 0;
+  if (!Executor) {
+    llvm::errs() << llvm::toString(Executor.takeError()) << "\n";
+    return 1;
   }
 
-  // Found compilation database, we iterate all TUs from database to get all
-  // symbols, and then merge them into a single SymbolSlab.
-  SymbolSlab::Builder GlobalSymbols;
-  std::mutex SymbolMutex;
-  auto AddSymbols = [&](const SymbolSlab& NewSymbols) {
-    // Synchronize set accesses.
-    std::unique_lock<std::mutex> LockGuard(SymbolMutex);
-    for (auto Sym : NewSymbols) {
-      // FIXME: Better handling the overlap symbols, currently we overwrite it
-      // with the latest one, but we always want to good declarations (class
-      // definitions, instead of forward declarations).
-      GlobalSymbols.insert(Sym);
-    }
-  };
-
-  {
-    llvm::ThreadPool Pool;
-    for (auto& file : OptionsParser.getCompilations().getAllFiles()) {
-      Pool.async([&OptionsParser, &AddSymbols](llvm::StringRef Path) {
-        ClangTool Tool(OptionsParser.getCompilations(), {Path});
-        clang::clangd::SymbolIndexActionFactory IndexAction;
-        Tool.run(&IndexAction);
-        AddSymbols(IndexAction.Collector->takeSymbols());
-      }, file);
-    }
+  auto Err = Executor->get()->execute(
+      llvm::make_unique<clang::clangd::SymbolIndexActionFactory>(
+          Executor->get()->getExecutionContext()));
+  if (Err) {
+    llvm::errs() << llvm::toString(std::move(Err)) << "\n";
   }
 
-  llvm::outs() << SymbolToYAML(std::move(GlobalSymbols).build());
+  Executor->get()->getToolResults()->forEachResult(
+      [](llvm::StringRef, llvm::StringRef Value) { llvm::outs() << Value; });
   return 0;
 }

clang-tools-extra/trunk/clangd/index/SymbolYAML.h

 #include "llvm/Support/Error.h"
 
 namespace clang {
 namespace clangd {
 
 // Read symbols from a YAML-format string.
 SymbolSlab SymbolFromYAML(llvm::StringRef YAMLContent);
 
+// Convert a single symbol to YAML-format string.
+// The YAML result is safe to concatenate.
+std::string SymbolToYAML(Symbol Sym);
+
 // Convert symbols to a YAML-format string.
 // The YAML result is safe to concatenate if you have multiple symbol slabs.
-std::string SymbolToYAML(const SymbolSlab& Symbols);
+std::string SymbolsToYAML(const SymbolSlab& Symbols);
 
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_SYMBOL_FROM_YAML_H

clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp

SymbolSlab SymbolFromYAML(llvm::StringRef YAMLContent) {
   llvm::yaml::Input Yin(YAMLContent);
   Yin >> S;
   SymbolSlab::Builder Syms;
   for (auto& Sym : S)
     Syms.insert(Sym);
   return std::move(Syms).build();
 }
 
-std::string SymbolToYAML(const SymbolSlab& Symbols) {
+std::string SymbolsToYAML(const SymbolSlab& Symbols) {
   std::string Str;
   llvm::raw_string_ostream OS(Str);
   llvm::yaml::Output Yout(OS);
   for (Symbol S : Symbols) // copy: Yout<< requires mutability.
     Yout<< S;
   return OS.str();
 }
 
+std::string SymbolToYAML(Symbol Sym) {
+  std::string Str;
+  llvm::raw_string_ostream OS(Str);
+  llvm::yaml::Output Yout(OS);
+  Yout << Sym;
+  return OS.str();
+}
+
 } // namespace clangd
 } // namespace clang

clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp

)";
   auto Symbols1 = SymbolFromYAML(YAML1);
   EXPECT_THAT(Symbols1,
               UnorderedElementsAre(QName("clang::Foo1")));
   auto Symbols2 = SymbolFromYAML(YAML2);
   EXPECT_THAT(Symbols2,
               UnorderedElementsAre(QName("clang::Foo2")));
 
   std::string ConcatenatedYAML =
-      SymbolToYAML(Symbols1) + SymbolToYAML(Symbols2);
+      SymbolsToYAML(Symbols1) + SymbolsToYAML(Symbols2);
   auto ConcatenatedSymbols = SymbolFromYAML(ConcatenatedYAML);
   EXPECT_THAT(ConcatenatedSymbols,
               UnorderedElementsAre(QName("clang::Foo1"),
                                    QName("clang::Foo2")));
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang