llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clangd Author: John Porto (jpporto) <details> <summary>Changes</summary> StringMap insertion may cause elements to be rehased, or the underlying storage to be reallocated, so it is generatelly unsafe to take pointers to StringMap "values". This in turn caused clangd to fail to load its on disk cache as the code depended on pointer equality (and immutability) to work. After this change, clangd will successfully re-open its index for large projects (e.g., LLVM itself), thus improving the developer experience. --- Full diff: https://github.com/llvm/llvm-project/pull/169339.diff 2 Files Affected: - (modified) clang-tools-extra/clangd/GlobalCompilationDatabase.cpp (+22-10) - (modified) clang-tools-extra/clangd/GlobalCompilationDatabase.h (+1-1) ``````````diff diff --git a/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp b/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp index c6afd0bc07cbd..43e27b3f21d1a 100644 --- a/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp +++ b/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp @@ -398,8 +398,12 @@ DirectoryBasedGlobalCompilationDatabase::getDirectoryCaches( Ret.reserve(Dirs.size()); std::lock_guard<std::mutex> Lock(DirCachesMutex); - for (unsigned I = 0; I < Dirs.size(); ++I) - Ret.push_back(&DirCaches.try_emplace(FoldedDirs[I], Dirs[I]).first->second); + for (unsigned I = 0; I < Dirs.size(); ++I) { + std::unique_ptr<DirectoryCache> &DC = DirCaches[FoldedDirs[I]]; + if (!DC) + DC = std::make_unique<DirectoryCache>(Dirs[I]); + Ret.push_back(DC.get()); + } return Ret; } @@ -571,7 +575,7 @@ class DirectoryBasedGlobalCompilationDatabase::BroadcastThread::Filter { enum { Unknown, Missing, TargetCDB, OtherCDB } State = Unknown; DirInfo *Parent = nullptr; }; - llvm::StringMap<DirInfo> Dirs; + llvm::StringMap<std::unique_ptr<DirInfo>> Dirs; // A search path starts at a directory, and either includes ancestors or not. using SearchPath = llvm::PointerIntPair<DirInfo *, 1>; @@ -583,16 +587,18 @@ class DirectoryBasedGlobalCompilationDatabase::BroadcastThread::Filter { DirInfo *Child = nullptr; actOnAllParentDirectories(FilePath, [&](llvm::StringRef Dir) { auto &Info = Dirs[Dir]; + if (!Info) + Info = std::make_unique<DirInfo>(); // If this is the first iteration, then this node is the overall result. if (!Leaf) - Leaf = &Info; + Leaf = Info.get(); // Fill in the parent link from the previous iteration to this parent. if (Child) - Child->Parent = &Info; + Child->Parent = Info.get(); // Keep walking, whether we inserted or not, if parent link is missing. // (If it's present, parent links must be present up to the root, so stop) - Child = &Info; - return Info.Parent != nullptr; + Child = Info.get(); + return Info->Parent != nullptr; }); return Leaf; } @@ -609,7 +615,7 @@ class DirectoryBasedGlobalCompilationDatabase::BroadcastThread::Filter { DirValues.reserve(Dirs.size()); for (auto &E : Dirs) { DirKeys.push_back(E.first()); - DirValues.push_back(&E.second); + DirValues.push_back(E.second.get()); } // Also look up the cache entry for the CDB we're broadcasting. @@ -677,7 +683,10 @@ class DirectoryBasedGlobalCompilationDatabase::BroadcastThread::Filter { std::vector<SearchPath> SearchPaths(AllFiles.size()); for (unsigned I = 0; I < AllFiles.size(); ++I) { if (Parent.Opts.CompileCommandsDir) { // FIXME: unify with config - SearchPaths[I].setPointer(&Dirs[*Parent.Opts.CompileCommandsDir]); + std::unique_ptr<DirInfo> &Dir = Dirs[*Parent.Opts.CompileCommandsDir]; + if (!Dir) + Dir = std::make_unique<DirInfo>(); + SearchPaths[I].setPointer(Dir.get()); continue; } if (ExitEarly()) // loading config may be slow @@ -693,7 +702,10 @@ class DirectoryBasedGlobalCompilationDatabase::BroadcastThread::Filter { SearchPaths[I].setPointer(addParents(AllFiles[I])); break; case Config::CDBSearchSpec::FixedDir: - SearchPaths[I].setPointer(&Dirs[*Spec.FixedCDBPath]); + std::unique_ptr<DirInfo> &Dir = Dirs[*Spec.FixedCDBPath]; + if (!Dir) + Dir = std::make_unique<DirInfo>(); + SearchPaths[I].setPointer(Dir.get()); break; } } diff --git a/clang-tools-extra/clangd/GlobalCompilationDatabase.h b/clang-tools-extra/clangd/GlobalCompilationDatabase.h index 1d636d73664be..2a8e3821c4596 100644 --- a/clang-tools-extra/clangd/GlobalCompilationDatabase.h +++ b/clang-tools-extra/clangd/GlobalCompilationDatabase.h @@ -143,7 +143,7 @@ class DirectoryBasedGlobalCompilationDatabase class DirectoryCache; // Keyed by possibly-case-folded directory path. // We can hand out pointers as they're stable and entries are never removed. - mutable llvm::StringMap<DirectoryCache> DirCaches; + mutable llvm::StringMap<std::unique_ptr<DirectoryCache>> DirCaches; mutable std::mutex DirCachesMutex; std::vector<DirectoryCache *> `````````` </details> https://github.com/llvm/llvm-project/pull/169339 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
