[clang-tidy] **Prototype** use AllTUsExecutors

[hokein]

Summary

Only works on StandaloneToolExecutor, crashes happen when running
AllTUsExecutors with multithreads (I believe it is because
ClangTidyContext is not threadsafe).

// Restrict thread to 1.
./bin/clang-tidy -executor=all-TUs -filter=".*/extra/*" -checks="-*,readability-braces-around-statements" -execute-concurrency=1 -export-fixes="/tmp/fixes.yaml" .

[JonasToth]

Very interesting. As I am not very familiar with all the internals I do have a few questions :)

Right now notes seem not be closely attached to their related warning. But within the there is a check, that a note is emitted after an error. Would it make sense to group these errors and notes together?
How would the dedup happen for static-analyzer diagnostics? Right now its only from the source-location, but as mentionend in the other review there might be many paths that lead to the same diagnostics, with the difference in the notes.

Did you check with TSAN if you get some hints on what could be the problem on parallel execution? Would you do the synchronization within ClangTidyContext or are there other places as well that lead to race-conditions?

clang-tidy/ClangTidy.cpp

566

is this deduplication, or the other place (or both? and if yes why at two places?)

clang-tidy/ClangTidyDiagnosticConsumer.cpp

652

Or this dedup?

clang-tidy/ClangTidy.h

 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDY_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDY_H
 
 #include "ClangTidyDiagnosticConsumer.h"
 #include "ClangTidyOptions.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Tooling/Execution.h"
 #include "clang/Tooling/Refactoring.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/raw_ostream.h"
 #include <memory>
 #include <type_traits>
 #include <vector>
 
 namespace clang {

 ///
 /// \param EnableCheckProfile If provided, it enables check profile collection
 /// in MatchFinder, and will contain the result of the profile.
 /// \param StoreCheckProfile If provided, and EnableCheckProfile is true,
 /// the profile will not be output to stderr, but will instead be stored
 /// as a JSON file in the specified directory.
 std::vector<ClangTidyError>
 runClangTidy(clang::tidy::ClangTidyContext &Context,
-             const tooling::CompilationDatabase &Compilations,
-             ArrayRef<std::string> InputFiles,
+             std::unique_ptr<tooling::ToolExecutor> Executor,
+             // const tooling::CompilationDatabase &Compilations,
+             //  ArrayRef<std::string> InputFiles,
              llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS,
              bool EnableCheckProfile = false,
              llvm::StringRef StoreCheckProfile = StringRef());
 
 // FIXME: This interface will need to be significantly extended to be useful.
 // FIXME: Implement confidence levels for displaying/fixing errors.
 //
 /// \brief Displays the found \p Errors to the users. If \p Fix is true, \p

clang-tidy/ClangTidy.cpp

getCheckOptions(const ClangTidyOptions &Options,
   clang::tidy::ClangTidyContext Context(
       llvm::make_unique<DefaultOptionsProvider>(ClangTidyGlobalOptions(),
                                                 Options),
       AllowEnablingAnalyzerAlphaCheckers);
   ClangTidyASTConsumerFactory Factory(Context);
   return Factory.getCheckOptions();
 }
 
+// FIXME: support FS overlay.
+// FIXME: make ClangTidyContext thread safe.
 std::vector<ClangTidyError>
 runClangTidy(clang::tidy::ClangTidyContext &Context,
-             const CompilationDatabase &Compilations,
-             ArrayRef<std::string> InputFiles,
+             std::unique_ptr<tooling::ToolExecutor> Executor,
              llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS,
              bool EnableCheckProfile, llvm::StringRef StoreCheckProfile) {
-  ClangTool Tool(Compilations, InputFiles,
-                 std::make_shared<PCHContainerOperations>(), BaseFS);
-
   // Add extra arguments passed by the clang-tidy command-line.
   ArgumentsAdjuster PerFileExtraArgumentsInserter =
       [&Context](const CommandLineArguments &Args, StringRef Filename) {
         ClangTidyOptions Opts = Context.getOptionsForFile(Filename);
         CommandLineArguments AdjustedArgs = Args;
         if (Opts.ExtraArgsBefore) {
           auto I = AdjustedArgs.begin();
           if (I != AdjustedArgs.end() && !StringRef(*I).startswith("-"))
ArgumentsAdjuster PluginArgumentsRemover =
               Args[I + 2] == "-Xclang") {
             I += 3;
           } else
             AdjustedArgs.push_back(Args[I]);
         }
         return AdjustedArgs;
       };
 
-  Tool.appendArgumentsAdjuster(PerFileExtraArgumentsInserter);
-  Tool.appendArgumentsAdjuster(PluginArgumentsRemover);
   Context.setEnableProfiling(EnableCheckProfile);
   Context.setProfileStoragePrefix(StoreCheckProfile);
 
-  ClangTidyDiagnosticConsumer DiagConsumer(Context);
-
-  Tool.setDiagnosticConsumer(&DiagConsumer);
-
   class ActionFactory : public FrontendActionFactory {
   public:
-    ActionFactory(ClangTidyContext &Context) : ConsumerFactory(Context) {}
-    FrontendAction *create() override { return new Action(&ConsumerFactory); }
+    ActionFactory(ClangTidyContext &Context,
+                  std::vector<tidy::ClangTidyError> &Results)
+        : ConsumerFactory(Context), Context(Context), Results(Results) {}
+
+    FrontendAction *create() override {
+      return new Action(&ConsumerFactory, Context,
+                        [&](std::vector<tidy::ClangTidyError> Errors) {
+                          Results.insert(Results.end(), Errors.begin(),
+                                         Errors.end());
+                        });
+    }
+
+    ~ActionFactory() {
+      // Deduplicate, and emit the final results.
+      std::sort(Results.begin(), Results.end());
+      Results.erase(std::unique(Results.begin(), Results.end()), Results.end());
+    }
 
     bool runInvocation(std::shared_ptr<CompilerInvocation> Invocation,
                        FileManager *Files,
                        std::shared_ptr<PCHContainerOperations> PCHContainerOps,
                        DiagnosticConsumer *DiagConsumer) override {
       // Explicitly set ProgramAction to RunAnalysis to make the preprocessor
       // define __clang_analyzer__ macro. The frontend analyzer action will not
       // be called here.
       Invocation->getFrontendOpts().ProgramAction = frontend::RunAnalysis;
       return FrontendActionFactory::runInvocation(
           Invocation, Files, PCHContainerOps, DiagConsumer);
     }
 
   private:
     class Action : public ASTFrontendAction {
     public:
-      Action(ClangTidyASTConsumerFactory *Factory) : Factory(Factory) {}
+      Action(ClangTidyASTConsumerFactory *Factory, ClangTidyContext &Context,
+             std::function<void(std::vector<tidy::ClangTidyError>)>
+                 CTErrorsCallback)
+          : Factory(Factory), CTDiagConsumer(Context),
+            CTErrorsCallback(CTErrorsCallback) {}
       std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &Compiler,
                                                      StringRef File) override {
         return Factory->CreateASTConsumer(Compiler, File);
       }
 
+      bool BeginInvocation(CompilerInstance &CI) override {
+        // Inject clang-tidy diagnostic consumer to store all diagnostics.
+        // FIXME: at this point, we miss diagnostics (e.g. unknown argument)
+        // that emitted during a CompilerInstance is created from compile
+        // command arguments.
+        CI.getDiagnostics().setClient(&CTDiagConsumer, false);
+        return true;
+      }
+
+      // Emits all collected diagnostics.
+      // We can't emit at EndSourceFileAction, because EndSourceFileAction will
+      // not be called when the compliation of the file should be aborted.
+      // clang-tidy wants all diagnostics, including errors.
+      ~Action() { CTErrorsCallback(CTDiagConsumer.take()); }
+
     private:
       ClangTidyASTConsumerFactory *Factory;
+      ClangTidyDiagnosticConsumer CTDiagConsumer;
+      std::function<void(std::vector<tidy::ClangTidyError>)> CTErrorsCallback;
     };
 
     ClangTidyASTConsumerFactory ConsumerFactory;
+    ClangTidyContext &Context;
+    std::vector<tidy::ClangTidyError> &Results;
   };
 
-  ActionFactory Factory(Context);
-  Tool.run(&Factory);
-  return DiagConsumer.take();
+  ArgumentsAdjuster CTArgumentAjuster =
+      combineAdjusters(std::move(PerFileExtraArgumentsInserter),
+                       std::move(PluginArgumentsRemover));
+  std::vector<tidy::ClangTidyError> Results;
+  auto Factory = llvm::make_unique<ActionFactory>(Context, Results);
+  auto Err = Executor->execute(std::move(Factory), CTArgumentAjuster);
+  if (Err) {
+    // Consume this error, as all errors will be reported by clang-tidy.
+    llvm::consumeError(std::move(Err));
+  }
+  return Results;
 }
 
 void handleErrors(llvm::ArrayRef<ClangTidyError> Errors,
                   ClangTidyContext &Context, bool Fix,
                   unsigned &WarningsAsErrorsCount,
                   llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS) {
   ErrorReporter Reporter(Context, Fix, BaseFS);
   llvm::vfs::FileSystem &FileSystem =

clang-tidy/ClangTidyDiagnosticConsumer.h

 ///
 /// FIXME: Make Diagnostics flexible enough to support this directly.
 struct ClangTidyError : tooling::Diagnostic {
   ClangTidyError(StringRef CheckName, Level DiagLevel, StringRef BuildDirectory,
                  bool IsWarningAsError);
 
   bool IsWarningAsError;
 };
+inline bool operator<(const ClangTidyError &LHS, const ClangTidyError &RHS) {
+  const tooling::DiagnosticMessage &M1 = LHS.Message;
+  const tooling::DiagnosticMessage &M2 = RHS.Message;
+
+  return std::tie(M1.FilePath, M1.FileOffset, M1.Message) <
+         std::tie(M2.FilePath, M2.FileOffset, M2.Message);
+};
+inline bool operator==(const ClangTidyError &LHS, const ClangTidyError &RHS) {
+  const tooling::DiagnosticMessage &M1 = LHS.Message;
+  const tooling::DiagnosticMessage &M2 = RHS.Message;
+
+  return std::tie(M1.FilePath, M1.FileOffset, M1.Message) ==
+         std::tie(M2.FilePath, M2.FileOffset, M2.Message);
+}
 
 /// \brief Read-only set of strings represented as a list of positive and
 /// negative globs. Positive globs add all matched strings to the set, negative
 /// globs remove them in the order of appearance in the list.
 class GlobList {
 public:
   /// \brief \p GlobList is a comma-separated list of globs (only '*'
   /// metacharacter is supported) with optional '-' prefix to denote exclusion.

clang-tidy/ClangTidyDiagnosticConsumer.cpp

for (unsigned I = 0; I < Errors.size(); ++I) {
     if (!Apply[I]) {
       Errors[I].Fix.clear();
       Errors[I].Notes.emplace_back(
           "this fix will not be applied because it overlaps with another fix");
     }
   }
 }
 
-namespace {
-struct LessClangTidyError {
-  bool operator()(const ClangTidyError &LHS, const ClangTidyError &RHS) const {
-    const tooling::DiagnosticMessage &M1 = LHS.Message;
-    const tooling::DiagnosticMessage &M2 = RHS.Message;
-
-    return std::tie(M1.FilePath, M1.FileOffset, M1.Message) <
-           std::tie(M2.FilePath, M2.FileOffset, M2.Message);
-  }
-};
-struct EqualClangTidyError {
-  bool operator()(const ClangTidyError &LHS, const ClangTidyError &RHS) const {
-    LessClangTidyError Less;
-    return !Less(LHS, RHS) && !Less(RHS, LHS);
-  }
-};
-} // end anonymous namespace
-
 std::vector<ClangTidyError> ClangTidyDiagnosticConsumer::take() {
   finalizeLastError();
 
-  std::sort(Errors.begin(), Errors.end(), LessClangTidyError());
-  Errors.erase(std::unique(Errors.begin(), Errors.end(), EqualClangTidyError()),
-               Errors.end());
+  std::sort(Errors.begin(), Errors.end());
+  Errors.erase(std::unique(Errors.begin(), Errors.end()), Errors.end());
   if (RemoveIncompatibleErrors)
     removeIncompatibleErrors();
   return std::move(Errors);
 }

clang-tidy/tool/ClangTidyMain.cpp

 ///    http://clang.llvm.org/docs/HowToSetupToolingForLLVM.html
 ///  for details on setting it up with LLVM source tree.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "clang/Config/config.h"
 #include "clang/Tooling/CommonOptionsParser.h"
+#include "clang/Tooling/Execution.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/TargetSelect.h"
 
 using namespace clang::ast_matchers;
 using namespace clang::driver;
 using namespace clang::tooling;
 using namespace llvm;
 
llvm::errs() << "Error: invalid virtual filesystem overlay file '"
                  << OverlayFile << "'.\n";
     return nullptr;
   }
   OverlayFS->pushOverlay(FS);
   return OverlayFS;
 }
 
 static int clangTidyMain(int argc, const char **argv) {
-  CommonOptionsParser OptionsParser(argc, argv, ClangTidyCategory,
-                                    cl::ZeroOrMore);
+  // CommonOptionsParser OptionsParser(argc, argv, ClangTidyCategory,
+  //                                   cl::ZeroOrMore);
+  auto Executor = clang::tooling::createExecutorFromCommandLineArgs(
+      argc, argv, ClangTidyCategory);
+  if (!Executor) {
+    llvm::errs() << llvm::toString(Executor.takeError()) << "\n";
+    return 1;
+  }
   llvm::IntrusiveRefCntPtr<vfs::FileSystem> BaseFS(
       VfsOverlay.empty() ? vfs::getRealFileSystem()
                          : getVfsOverlayFromFile(VfsOverlay));
   if (!BaseFS)
     return 1;
 
   auto OwningOptionsProvider = createOptionsProvider(BaseFS);
   auto *OptionsProvider = OwningOptionsProvider.get();
if (std::error_code EC = llvm::sys::fs::make_absolute(AbsolutePath)) {
                    << EC.message() << "\n";
     }
     return AbsolutePath;
   };
 
   SmallString<256> ProfilePrefix = MakeAbsolute(StoreCheckProfile);
 
   StringRef FileName("dummy");
-  auto PathList = OptionsParser.getSourcePathList();
-  if (!PathList.empty()) {
-    FileName = PathList.front();
-  }
+  // auto PathList = OptionsParser.getSourcePathList();
+  // if (!PathList.empty()) {
+  //   FileName = PathList.front();
+  // }
 
   SmallString<256> FilePath = MakeAbsolute(FileName);
 
   ClangTidyOptions EffectiveOptions = OptionsProvider->getOptions(FilePath);
   std::vector<std::string> EnabledChecks =
       getCheckNames(EffectiveOptions, AllowEnablingAnalyzerAlphaCheckers);
 
   if (ExplainConfig) {
static int clangTidyMain(int argc, const char **argv) {
   }
 
   if (EnabledChecks.empty()) {
     llvm::errs() << "Error: no checks enabled.\n";
     llvm::cl::PrintHelpMessage(/*Hidden=*/false, /*Categorized=*/true);
     return 1;
   }
 
-  if (PathList.empty()) {
-    llvm::errs() << "Error: no input files specified.\n";
-    llvm::cl::PrintHelpMessage(/*Hidden=*/false, /*Categorized=*/true);
-    return 1;
-  }
+  // if (PathList.empty()) {
+  //   llvm::errs() << "Error: no input files specified.\n";
+  //   llvm::cl::PrintHelpMessage(/*Hidden=*/false, /*Categorized=*/true);
+  //   return 1;
+  // }
 
   llvm::InitializeAllTargetInfos();
   llvm::InitializeAllTargetMCs();
   llvm::InitializeAllAsmParsers();
 
   ClangTidyContext Context(std::move(OwningOptionsProvider),
                            AllowEnablingAnalyzerAlphaCheckers);
-  std::vector<ClangTidyError> Errors =
-      runClangTidy(Context, OptionsParser.getCompilations(), PathList, BaseFS,
-                   EnableCheckProfile, ProfilePrefix);
+  std::vector<ClangTidyError> Errors = runClangTidy(
+      Context, std::move(*Executor), BaseFS, EnableCheckProfile, ProfilePrefix);
   bool FoundErrors = llvm::find_if(Errors, [](const ClangTidyError &E) {
                        return E.DiagLevel == ClangTidyError::Error;
                      }) != Errors.end();
 
   const bool DisableFixes = Fix && FoundErrors && !FixErrors;
 
   unsigned WErrorCount = 0;