[tooling] Implement determinsitic ordering of CompilationDatabasePlugins

[njames93]

Summary

Right now plugins appear to be registed in an undefined order based on what the linker decides to link in first.
This is undesireable as if there are multiple potential databases in a directory we can't specify which one should be treated as the defacto.

To address this there is a virtual getPriority method in the CompilationDatabasePlugin.
Plugins will then be sorted based on that(lowest first) and then search for a CompilationDatabase in the specified directory.

JSON database will now load first before Fixed database but there is plenty of room to insert plugins that would take priority over JSON, like ninja or make files.

Addresses https://github.com/clangd/clangd/issues/578

[nridge]

Thanks for working on this!

clang/lib/Tooling/CompilationDatabase.cpp

73

A side effect of this change is that every plugin will get instantiated, not just the ones for which we attempt a load.

Perhaps we should store a pointer to the entry in the vector here, and instantiate() in the second loop below?

[njames93]

clang/lib/Tooling/CompilationDatabase.cpp

73

The original setup was to instantiate every plugin until we find one that is able to load from the directory.
As for your proposed solution, that wouldn't work as we can't get the priority without instantiating the plugin.
The current registry system also doesn't lend itself nicely.
I'm not too familiar, but it could be possible to specialise SimpleRegistryEntry for CompilationDatabasePlugin.

[nridge]

Not sure if I'm authorized to approve changes to libTooling, but this LGTM!

clang/lib/Tooling/CompilationDatabase.cpp

73

that wouldn't work as we can't get the priority without instantiating the plugin

Right, of course, I overlooked that.

Anyways, it looks like instantiating the plugin just calls its constructor and the constructors of both existing plugins are no-ops, so it shouldn't be a concern.

[njames93]

I'm hesitant to land this right away, want to see what happens with D91454 and its related mailing list. In my mind thats a much nicer way to fix this issue.

clang/include/clang/Tooling/CompilationDatabasePluginRegistry.h

 public:
   virtual ~CompilationDatabasePlugin();
 
   /// Loads a compilation database from a build directory.
   ///
   /// \see CompilationDatabase::loadFromDirectory().
   virtual std::unique_ptr<CompilationDatabase>
   loadFromDirectory(StringRef Directory, std::string &ErrorMessage) = 0;
+
+  /// Used to sort the plugins before attempting to load. Plugins with a lower
+  /// value here will run over a directory first.
+  virtual unsigned getPriority() const = 0;
 };
 
 using CompilationDatabasePluginRegistry =
     llvm::Registry<CompilationDatabasePlugin>;
 
 } // namespace tooling
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLING_COMPILATIONDATABASEPLUGINREGISTRY_H

clang/lib/Tooling/CompilationDatabase.cpp

 LLVM_INSTANTIATE_REGISTRY(CompilationDatabasePluginRegistry)
 
 CompilationDatabase::~CompilationDatabase() = default;
 
 std::unique_ptr<CompilationDatabase>
 CompilationDatabase::loadFromDirectory(StringRef BuildDirectory,
                                        std::string &ErrorMessage) {
   llvm::raw_string_ostream ErrorStream(ErrorMessage);
+  using NameAndPlugin =
+      std::pair<StringRef, std::unique_ptr<CompilationDatabasePlugin>>;
+  llvm::SmallVector<NameAndPlugin, 4> Plugins;
+
   for (const CompilationDatabasePluginRegistry::entry &Database :
        CompilationDatabasePluginRegistry::entries()) {
+    Plugins.emplace_back(Database.getName(), Database.instantiate());
+  }
+  llvm::sort(Plugins, [](const NameAndPlugin &LHS, const NameAndPlugin &RHS) {
+    assert(LHS.second->getPriority() != RHS.second->getPriority() &&
+           "Undeterministic priority");
+    return LHS.second->getPriority() < RHS.second->getPriority();
+  });
+  for (auto &Plugin : Plugins) {
     std::string DatabaseErrorMessage;
-    std::unique_ptr<CompilationDatabasePlugin> Plugin(Database.instantiate());
     if (std::unique_ptr<CompilationDatabase> DB =
-            Plugin->loadFromDirectory(BuildDirectory, DatabaseErrorMessage))
+            Plugin.second->loadFromDirectory(BuildDirectory,
+                                             DatabaseErrorMessage))
       return DB;
-    ErrorStream << Database.getName() << ": " << DatabaseErrorMessage << "\n";
+    ErrorStream << Plugin.first << ": " << DatabaseErrorMessage << "\n";
   }
   return nullptr;
 }
 
 static std::unique_ptr<CompilationDatabase>
 findCompilationDatabaseFromDirectory(StringRef Directory,
                                      std::string &ErrorMessage) {
   std::stringstream ErrorStream;

 
 class FixedCompilationDatabasePlugin : public CompilationDatabasePlugin {
   std::unique_ptr<CompilationDatabase>
   loadFromDirectory(StringRef Directory, std::string &ErrorMessage) override {
     SmallString<1024> DatabasePath(Directory);
     llvm::sys::path::append(DatabasePath, "compile_flags.txt");
     return FixedCompilationDatabase::loadFromFile(DatabasePath, ErrorMessage);
   }
+
+  unsigned int getPriority() const override {
+    // Give this a high value as it should be the fallback and any other Plugin
+    // should be used first.
+    return 100;
+  }
 };
 
 } // namespace
 
 static CompilationDatabasePluginRegistry::Add<FixedCompilationDatabasePlugin>
 X("fixed-compilation-database", "Reads plain-text flags file");
 
 namespace clang {

clang/lib/Tooling/JSONCompilationDatabase.cpp

loadFromDirectory(StringRef Directory, std::string &ErrorMessage) override {
     llvm::sys::path::append(JSONDatabasePath, "compile_commands.json");
     auto Base = JSONCompilationDatabase::loadFromFile(
         JSONDatabasePath, ErrorMessage, JSONCommandLineSyntax::AutoDetect);
     return Base ? inferTargetAndDriverMode(
                       inferMissingCompileCommands(expandResponseFiles(
                           std::move(Base), llvm::vfs::getRealFileSystem())))
                 : nullptr;
   }
+
+  unsigned int getPriority() const override {
+    // Pretty arbitrary. However we want to leave room for database sources
+    // which could have a higher priority, like directly supporting ninja or
+    // make files.
+    return 50;
+  }
 };
 
 } // namespace
 
 // Register the JSONCompilationDatabasePlugin with the
 // CompilationDatabasePluginRegistry using this statically initialized variable.
 static CompilationDatabasePluginRegistry::Add<JSONCompilationDatabasePlugin>
 X("json-compilation-database", "Reads JSON formatted compilation databases");