Author: sammccall Date: Fri Jul 26 07:07:11 2019 New Revision: 367112 URL: http://llvm.org/viewvc/llvm-project?rev=367112&view=rev Log: [clangd] Fix background index not triggering on windows due to case mismatch.
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. Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D65320 Modified: clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.h Modified: clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp?rev=367112&r1=367111&r2=367112&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp (original) +++ clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp Fri Jul 26 07:07:11 2019 @@ -117,20 +117,41 @@ DirectoryBasedGlobalCompilationDatabase: 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}; - 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}; + // 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 = ""; + Entry.CDB = tooling::CompilationDatabase::loadFromDirectory(Dir, Error); + Entry.Path = Dir; + } + return R.first->second; } llvm::Optional<DirectoryBasedGlobalCompilationDatabase::CDBLookupResult> @@ -139,38 +160,41 @@ DirectoryBasedGlobalCompilationDatabase: 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; } @@ -200,9 +224,9 @@ void DirectoryBasedGlobalCompilationData 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); }); } } @@ -213,7 +237,7 @@ void DirectoryBasedGlobalCompilationData // 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. Modified: clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.h?rev=367112&r1=367111&r2=367112&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.h (original) +++ clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.h Fri Jul 26 07:07:11 2019 @@ -80,8 +80,13 @@ public: 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; @@ -98,12 +103,7 @@ private: 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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
