Merged to release_90 in r367135.
On Fri, Jul 26, 2019 at 7:06 AM Sam McCall via cfe-commits <cfe-commits@lists.llvm.org> wrote: > > 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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
