[clangd] Fix background index not triggering on windows due to case mismatch.

[sammccall]

Summary

This isn't a general fix to all paths where we assume case-sensitivity, it's
a minimally-invasive fix targeting the llvm 9 branch.

I don't have a good automated test for this :-\ I have manually verified that it fixes the issue on my windows machine.

The plan would be to cherrypick this to llvm 9, then later back this out and replace it with a real (testable) fix.

[sammccall]

Sorry, this has a hideous bug that rebroadcasts CDBs over and over. Working on a fix...

clang-tools-extra/clangd/GlobalCompilationDatabase.cpp

195 ↗

(On Diff #211887)

this line is incorrect because the key should be lowercase

[sammccall]

Fix bug where CDB wasn't being marked as broadcast due to case differences.
Having getCDBLocked() return CachedCDB seems much cleaner to me.

[hokein]

clang-tools-extra/clangd/GlobalCompilationDatabase.cpp

168 ↗

(On Diff #211921)

nit: set default nullptr.

210 ↗

(On Diff #211921)

should we use the pathEqual here too?

[sammccall]

clang-tools-extra/clangd/GlobalCompilationDatabase.cpp

168 ↗

(On Diff #211921)

Done, and also checked for it.

210 ↗

(On Diff #211921)

no, if we have a CompileCommandsDir then we'll only search in that exact directory name, and so the result should match exactly

clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.h

public:
   llvm::Optional<tooling::CompileCommand>
   getCompileCommand(PathRef File) const override;
 
   /// Returns the path to first directory containing a compilation database in
   /// \p File's parents.
   llvm::Optional<ProjectInfo> getProjectInfo(PathRef File) const override;
 
 private:
-  std::pair<tooling::CompilationDatabase *, /*SentBroadcast*/ bool>
-  getCDBInDirLocked(PathRef File) const;
+  /// Caches compilation databases loaded from directories.
+  struct CachedCDB {
+    std::string Path; // Not case-folded.
+    std::unique_ptr<clang::tooling::CompilationDatabase> CDB = nullptr;
+    bool SentBroadcast = false;
+  };
+  CachedCDB &getCDBInDirLocked(PathRef File) const;
 
   struct CDBLookupRequest {
     PathRef FileName;
     // Whether this lookup should trigger discovery of the CDB found.
     bool ShouldBroadcast = false;
   };
   struct CDBLookupResult {
     tooling::CompilationDatabase *CDB = nullptr;
     ProjectInfo PI;
   };
   llvm::Optional<CDBLookupResult> lookupCDB(CDBLookupRequest Request) const;
 
   // Performs broadcast on governed files.
   void broadcastCDB(CDBLookupResult Res) const;
 
   mutable std::mutex Mutex;
-  /// Caches compilation databases loaded from directories(keys are
-  /// directories).
-  struct CachedCDB {
-    std::unique_ptr<clang::tooling::CompilationDatabase> CDB = nullptr;
-    bool SentBroadcast = false;
-  };
+  // Keyed by possibly-case-folded directory path.
   mutable llvm::StringMap<CachedCDB> CompilationDatabases;
 
   /// Used for command argument pointing to folder where compile_commands.json
   /// is located.
   llvm::Optional<Path> CompileCommandsDir;
 };
 
 /// Extracts system include search path from drivers matching QueryDriverGlobs

clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp

DirectoryBasedGlobalCompilationDatabase::getCompileCommand(PathRef File) const {
 
   auto Candidates = Res->CDB->getCompileCommands(File);
   if (!Candidates.empty())
     return std::move(Candidates.front());
 
   return None;
 }
 
-std::pair<tooling::CompilationDatabase *, /*SentBroadcast*/ bool>
+// For platforms where paths are case-insensitive (but case-preserving),
+// we need to do case-insensitive comparisons and use lowercase keys.
+// FIXME: Make Path a real class with desired semantics instead.
+//        This class is not the only place this problem exists.
+// FIXME: Mac filesystems default to case-insensitive, but may be sensitive.
+
+static std::string maybeCaseFoldPath(PathRef Path) {
+#if defined(_WIN32) || defined(__APPLE__)
+  return Path.lower();
+#else
+  return Path;
+#endif
+}
+
+static bool pathEqual(PathRef A, PathRef B) {
+#if defined(_WIN32) || defined(__APPLE__)
+  return A.equals_lower(B);
+#else
+  return A == B;
+#endif
+}
+
+DirectoryBasedGlobalCompilationDatabase::CachedCDB &
 DirectoryBasedGlobalCompilationDatabase::getCDBInDirLocked(PathRef Dir) const {
   // FIXME(ibiryukov): Invalidate cached compilation databases on changes
-  auto CachedIt = CompilationDatabases.find(Dir);
-  if (CachedIt != CompilationDatabases.end())
-    return {CachedIt->second.CDB.get(), CachedIt->second.SentBroadcast};
+  // FIXME(sammccall): this function hot, avoid copying key when hitting cache.
+  auto Key = maybeCaseFoldPath(Dir);
+  auto R = CompilationDatabases.try_emplace(Key);
+  if (R.second) { // Cache miss, try to load CDB.
+    CachedCDB &Entry = R.first->second;
     std::string Error = "";
-
-  CachedCDB Entry;
     Entry.CDB = tooling::CompilationDatabase::loadFromDirectory(Dir, Error);
-  auto Result = Entry.CDB.get();
-  CompilationDatabases[Dir] = std::move(Entry);
-
-  return {Result, false};
+    Entry.Path = Dir;
+  }
+  return R.first->second;
 }
 
 llvm::Optional<DirectoryBasedGlobalCompilationDatabase::CDBLookupResult>
 DirectoryBasedGlobalCompilationDatabase::lookupCDB(
     CDBLookupRequest Request) const {
   assert(llvm::sys::path::is_absolute(Request.FileName) &&
          "path must be absolute");
 
+  bool ShouldBroadcast = false;
   CDBLookupResult Result;
-  bool SentBroadcast = false;
 
   {
     std::lock_guard<std::mutex> Lock(Mutex);
+    CachedCDB *Entry = nullptr;
     if (CompileCommandsDir) {
-      std::tie(Result.CDB, SentBroadcast) =
-          getCDBInDirLocked(*CompileCommandsDir);
-      Result.PI.SourceRoot = *CompileCommandsDir;
+      Entry = &getCDBInDirLocked(*CompileCommandsDir);
     } else {
       // Traverse the canonical version to prevent false positives. i.e.:
       // src/build/../a.cc can detect a CDB in /src/build if not canonicalized.
+      // FIXME(sammccall): this loop is hot, use a union-find-like structure.
       actOnAllParentDirectories(removeDots(Request.FileName),
-                                [this, &SentBroadcast, &Result](PathRef Path) {
-                                  std::tie(Result.CDB, SentBroadcast) =
-                                      getCDBInDirLocked(Path);
-                                  Result.PI.SourceRoot = Path;
-                                  return Result.CDB != nullptr;
+                                [&](PathRef Path) {
+                                  Entry = &getCDBInDirLocked(Path);
+                                  return Entry->CDB != nullptr;
                                 });
     }
 
-    if (!Result.CDB)
+    if (!Entry || !Entry->CDB)
       return llvm::None;
 
     // Mark CDB as broadcasted to make sure discovery is performed once.
-    if (Request.ShouldBroadcast && !SentBroadcast)
-      CompilationDatabases[Result.PI.SourceRoot].SentBroadcast = true;
+    if (Request.ShouldBroadcast && !Entry->SentBroadcast) {
+      Entry->SentBroadcast = true;
+      ShouldBroadcast = true;
+    }
+
+    Result.CDB = Entry->CDB.get();
+    Result.PI.SourceRoot = Entry->Path;
   }
 
   // FIXME: Maybe make the following part async, since this can block retrieval
   // of compile commands.
-  if (Request.ShouldBroadcast && !SentBroadcast)
+  if (ShouldBroadcast)
     broadcastCDB(Result);
   return Result;
 }
 
 void DirectoryBasedGlobalCompilationDatabase::broadcastCDB(
     CDBLookupResult Result) const {
   assert(Result.CDB && "Trying to broadcast an invalid CDB!");
 
llvm::StringMap<bool> DirectoryHasCDB;
     // Uniquify all parent directories of all files.
     for (llvm::StringRef File : AllFiles) {
       actOnAllParentDirectories(File, [&](PathRef Path) {
         auto It = DirectoryHasCDB.try_emplace(Path);
         // Already seen this path, and all of its parents.
         if (!It.second)
           return true;
 
-        auto Res = getCDBInDirLocked(Path);
-        It.first->second = Res.first != nullptr;
-        return Path == Result.PI.SourceRoot;
+        CachedCDB &Entry = getCDBInDirLocked(Path);
+        It.first->second = Entry.CDB != nullptr;
+        return pathEqual(Path, Result.PI.SourceRoot);
       });
     }
   }
 
   std::vector<std::string> GovernedFiles;
   for (llvm::StringRef File : AllFiles) {
     // A file is governed by this CDB if lookup for the file would find it.
     // Independent of whether it has an entry for that file or not.
     actOnAllParentDirectories(File, [&](PathRef Path) {
       if (DirectoryHasCDB.lookup(Path)) {
-        if (Path == Result.PI.SourceRoot)
+        if (pathEqual(Path, Result.PI.SourceRoot))
           // Make sure listeners always get a canonical path for the file.
           GovernedFiles.push_back(removeDots(File));
         // Stop as soon as we hit a CDB.
         return true;
       }
       return false;
     });
   }