[PATCH] D148088: [RFC][clangd] Move preamble index out of document open critical path

Kugan Vivekanandarajah via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed May 17 09:45:02 PDT 2023 

kuganv marked 2 inline comments as done.
kuganv added inline comments.


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:698-700
+      // While extending the life of FileMgr and VFS, StatCache should also be
+      // extended.
+      Ctx.setStatCache(Result->StatCache);
----------------
kadircet wrote:
> we use a "producingfs" when building the preamble, hence there shouldn't be any references to statcache inside the FM&VFS stored in the compiler instance here. have you noticed something to the contrary ?
> we use a "producingfs" when building the preamble, hence there shouldn't be any references to statcache inside the FM&VFS stored in the compiler instance here. have you noticed something to the contrary ?


In PrecompiledPreamble::Build, we use StatCacheFS for FileMgr creation. Shouldn’t we keep this Alive? Without this, I am seeing:

```
==1509807==ERROR: AddressSanitizer: heap-use-after-free on address 0x606000464c00 at pc 0x55f558fd7de2 bp 0x7f18a73ff010 sp 0x7f18a73ff008
READ of size 8 at 0x606000464c00 thread T48 (NodeServer.cpp1)
V[23:14:41.667] Ignored diagnostic. /data/users/kugan/fbsource/fbcode/synapse_dev/prod/NodeServer.cpp:2:2:Mandatory header <vector> not found in standard library!
I[23:14:41.669] Indexed c++20 standard library (incomplete due to errors): 0 symbols, 0 filtered
    #0 0x55f558fd7de1 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>::_M_data() const /opt/rh/gcc-toolset-12/root/usr/lib/gcc/x86_64-redhat-linux/12/../../../../include/c++/12/bits/basic_string.h:234:28
    #1 0x55f558fd7de1 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>::data() const /opt/rh/gcc-toolset-12/root/usr/lib/gcc/x86_64-redhat-linux/12/../../../../include/c++/12/bits/basic_string.h:2567:16
    #2 0x55f558fd7de1 in llvm::StringRef::StringRef(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&) /home/kugan/local/llvm-project/llvm/include/llvm/ADT/StringRef.h:101:18
    #3 0x55f558fd7de1 in clang::clangd::PreambleFileStatusCache::update(llvm::vfs::FileSystem const&, llvm::vfs::Status) /home/kugan/local/llvm-project/clang-tools-extra/clangd/FS.cpp:33:20
    #4 0x55f558fd925f in clang::clangd::PreambleFileStatusCache::getProducingFS(llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>)::CollectFS::status(llvm::Twine const&) /home/kugan/local/llvm-project/clang-tools-extra/clangd/FS.cpp:82:19
    #5 0x55f55917ddee in clang::clangd::(anonymous namespace)::TimerFS::status(llvm::Twine const&) /home/kugan/local/llvm-project/clang-tools-extra/clangd/Preamble.cpp:492:30
    #6 0x55f55670dcff in clang::FileSystemStatCache::get(llvm::StringRef, llvm::vfs::Status&, bool, std::unique_ptr<llvm::vfs::File, std::default_delete<llvm::vfs::File>>*, clang::FileSystemStatCache*, llvm::vfs::FileSystem&) /home/kugan/local/llvm-project/clang/lib/Basic/FileSystemStatCache.cpp:47:55
    #7 0x55f556701ab4 in clang::FileManager::getStatValue(llvm::StringRef, llvm::vfs::Status&, bool, std::unique_ptr<llvm::vfs::File, std::default_delete<llvm::vfs::File>>*) /home/kugan/local/llvm-project/clang/lib/Basic/FileManager.cpp:590:12
    #8 0x55f556700eed in clang::FileManager::getDirectoryRef(llvm::StringRef, bool) /home/kugan/local/llvm-project/clang/lib/Basic/FileManager.cpp:161:20
    #9 0x55f556701d3e in clang::FileManager::getDirectory(llvm::StringRef, bool) /home/kugan/local/llvm-project/clang/lib/Basic/FileManager.cpp:191:17
    #10 0x55f55929a800 in clang::clangd::getCanonicalPath[abi:cxx11](clang::FileEntryRef, clang::SourceManager const&) /home/kugan/local/llvm-project/clang-tools-extra/clangd/SourceCode.cpp:540:45
    #11 0x55f559627681 in clang::clangd::SymbolCollector::HeaderFileURICache::toURI[abi:cxx11](clang::FileEntryRef) /home/kugan/local/llvm-project/clang-tools-extra/clangd/index/SymbolCollector.cpp:211:24
    #12 0x55f55961926e in clang::clangd::SymbolCollector::getTokenLocation(clang::SourceLocation) /home/kugan/local/llvm-project/clang-tools-extra/clangd/index/SymbolCollector.cpp:425:36
    #13 0x55f5596233a9 in clang::clangd::SymbolCollector::handleMacroOccurrence(clang::IdentifierInfo const*, clang::MacroInfo const*, unsigned int, clang::SourceLocation) /home/kugan/local/llvm-project/clang-tools-extra/clangd/index/SymbolCollector.cpp:753:22
    #14 0x55f55c1c7e3d in indexPreprocessorMacro(clang::IdentifierInfo const*, clang::MacroInfo const*, clang::MacroDirective::Kind, clang::SourceLocation, clang::index::IndexDataConsumer&) /home/kugan/local/llvm-project/clang/lib/Index/IndexingAction.cpp:229:16
    #15 0x55f55c1c7e3d in indexPreprocessorMacros(clang::Preprocessor&, clang::index::IndexDataConsumer&) /home/kugan/local/llvm-project/clang/lib/Index/IndexingAction.cpp:236:7
    #16 0x55f55c1c8290 in clang::index::indexTopLevelDecls(clang::ASTContext&, clang::Preprocessor&, llvm::ArrayRef<clang::Decl const*>, clang::index::IndexDataConsumer&, clang::index::IndexingOptions) /home/kugan/local/llvm-project/clang/lib/Index/IndexingAction.cpp:283:5
    #17 0x55f55959d350 in clang::clangd::(anonymous namespace)::indexSymbols(clang::ASTContext&, clang::Preprocessor&, llvm::ArrayRef<clang::Decl*>, clang::clangd::MainFileMacros const*, clang::clangd::CanonicalIncludes const&, bool, llvm::StringRef, bool) /home/kugan/local/llvm-project/clang-tools-extra/clangd/index/FileIndex.cpp:82:3
    #18 0x55f55959f040 in clang::clangd::indexHeaderSymbols(llvm::StringRef, clang::ASTContext&, clang::Preprocessor&, clang::clangd::CanonicalIncludes const&) /home/kugan/local/llvm-project/clang-tools-extra/clangd/index/FileIndex.cpp:235:10
    #19 0x55f5595a8119 in clang::clangd::FileIndex::updatePreamble(llvm::StringRef, llvm::StringRef, clang::ASTContext&, clang::Preprocessor&, clang::clangd::CanonicalIncludes const&) /home/kugan/local/llvm-project/clang-tools-extra/clangd/index/FileIndex.cpp:464:7
```



================
Comment at: clang-tools-extra/clangd/Preamble.cpp:106-113
+    CI.setSema(nullptr);
+    CI.setASTConsumer(nullptr);
+    if (CI.getASTReader()) {
+      CI.getASTReader()->setDeserializationListener(nullptr);
+      /* This just sets consumer to null when DeserializationListener is null */
+      CI.getASTReader()->StartTranslationUnit(nullptr);
     }
----------------
kadircet wrote:
> kuganv wrote:
> > kadircet wrote:
> > > why are we triggering destructors for all of these objects eagerly? if this is deliberate to "fix" some issue, could you mention that in comments?
> > > why are we triggering destructors for all of these objects eagerly? if this is deliberate to "fix" some issue, could you mention that in comments?
> > 
> > Thanks a lot for the review.
> > If we don't destruct and set it to null, CapturedASTCtx will also have to keep instances such as ASTConsumer including other related callbacks such PreambleCallbacks. This was making the CapturedASTCtx interface and implementation complex.
> > If we don't destruct and set it to null, CapturedASTCtx will also have to keep instances such as ASTConsumer 
> 
> Sorry if I am being dense but I can't actually see any references to those objects in the structs we preseve. AFAICT, Sema, ASTConsumer and ASTReader are members of CompilerInstance, which we don't keep around. Can you point me towards any references to the rest of the objects you're setting to null in case I am missing any?
> 
> `ASTMutationListener` seems to be the only one that's relevant. It's indeed exposed through `ASTContext` and our indexing operations can trigger various callbacks to fire. Can you set only that one to nullptr and leave a comment explaining the situation?
> > If we don't destruct and set it to null, CapturedASTCtx will also have to keep instances such as ASTConsumer 
> 
> Sorry if I am being dense but I can't actually see any references to those objects in the structs we preseve. AFAICT, Sema, ASTConsumer and ASTReader are members of CompilerInstance, which we don't keep around. Can you point me towards any references to the rest of the objects you're setting to null in case I am missing any?
> 
> `ASTMutationListener` seems to be the only one that's relevant. It's indeed exposed through `ASTContext` and our indexing operations can trigger various callbacks to fire. Can you set only that one to nullptr and leave a comment explaining the situation?

Thanks for the review.  I will revise based on the feedback. 

In buildPreamble, we create CppFilePreambleCallbacks in stack to be used in PrecompiledPreamble::Build which becomes part of PrecompilePreambleConsumer. I think this this need to be reset. Probably this has to be done in a different way. If I dont reset, I am seeing crash in some cases. See:

```
85c0c79f in clang::ASTReader::DecodeIdentifierInfo(unsigned int) /home/kugan/local/llvm-project/clang/lib/Serialization/ASTReader.cpp:8695:32
    #1 0x558285c75266 in clang::ASTReader::readIdentifier(clang::serialization::ModuleFile&, llvm::SmallVector<unsigned long, 64u> const&, unsigned int&) /home/kugan/local/llvm-project/clang/include/clang/Seriali
zation/ASTReader.h:2126:12
    #2 0x558285c75266 in clang::ASTRecordReader::readIdentifier() /home/kugan/local/llvm-project/clang/include/clang/Serialization/ASTRecordReader.h:211:20
    #3 0x558285c75266 in clang::serialization::BasicReaderBase<clang::ASTRecordReader>::readDeclarationName() /home/kugan/local/llvm-project/build/tools/clang/include/clang/AST/AbstractBasicReader.inc:780:58
    #4 0x558285ce10bc in clang::ASTDeclReader::VisitNamedDecl(clang::NamedDecl*) /home/kugan/local/llvm-project/clang/lib/Serialization/ASTReaderDecl.cpp:694:26
    #5 0x558285ce10bc in clang::ASTDeclReader::VisitNamespaceDecl(clang::NamespaceDecl*) /home/kugan/local/llvm-project/clang/lib/Serialization/ASTReaderDecl.cpp:1785:3
    #6 0x558285cbc6e8 in clang::ASTDeclReader::Visit(clang::Decl*) /home/kugan/local/llvm-project/clang/lib/Serialization/ASTReaderDecl.cpp:543:37
    #7 0x558285d5a133 in clang::ASTReader::ReadDeclRecord(unsigned int) /home/kugan/local/llvm-project/clang/lib/Serialization/ASTReaderDecl.cpp:4052:10
    #8 0x558285c73677 in clang::ASTReader::GetDecl(unsigned int) /home/kugan/local/llvm-project/clang/lib/Serialization/ASTReader.cpp:7607:5
    #9 0x558285c73677 in clang::ASTReader::GetLocalDecl(clang::serialization::ModuleFile&, unsigned int) /home/kugan/local/llvm-project/clang/include/clang/Serialization/ASTReader.h:1913:12
    #10 0x558285bfd741 in clang::ASTReader::FindExternalLexicalDecls(clang::DeclContext const*, llvm::function_ref<bool (clang::Decl::Kind)>, llvm::SmallVectorImpl<clang::Decl*>&)::$_9::operator()(clang::serializ
ation::ModuleFile*, llvm::ArrayRef<llvm::support::detail::packed_endian_specific_integral<unsigned int, (llvm::support::endianness)2, 1ul, 1ul>>) const /home/kugan/local/llvm-project/clang/lib/Serialization/ASTRe
ader.cpp:7687:21
    #11 0x558285bfd741 in clang::ASTReader::FindExternalLexicalDecls(clang::DeclContext const*, llvm::function_ref<bool (clang::Decl::Kind)>, llvm::SmallVectorImpl<clang::Decl*>&) /home/kugan/local/llvm-project/c
lang/lib/Serialization/ASTReader.cpp:7697:7
    #12 0x55827e942de7 in clang::ExternalASTSource::FindExternalLexicalDecls(clang::DeclContext const*, llvm::SmallVectorImpl<clang::Decl*>&) /home/kugan/local/llvm-project/clang/include/clang/AST/ExternalASTSour
ce.h:185:5
    #13 0x55827e942de7 in clang::DeclContext::LoadLexicalDeclsFromExternalStorage() const /home/kugan/local/llvm-project/clang/lib/AST/DeclBase.cpp:1421:11
    #14 0x55827e94436b in clang::DeclContext::decls_begin() const /home/kugan/local/llvm-project/clang/lib/AST/DeclBase.cpp:1477:5
    #15 0x5582822787ab in clang::DeclContext::decls() const /home/kugan/local/llvm-project/clang/include/clang/AST/DeclBase.h:2188:48
    #16 0x5582822787ab in clang::clangd::indexHeaderSymbols(llvm::StringRef, clang::ASTContext&, clang::Preprocessor&, clang::clangd::CanonicalIncludes const&) /home/kugan/local/llvm-project/clang-tools-extra/cla
ngd/index/FileIndex.cpp:233:37
    #17 0x558282281be9 in clang::clangd::FileIndex::updatePreamble(llvm::StringRef, llvm::StringRef, clang::ASTContext&, clang::Preprocessor&, clang::clangd::CanonicalIncludes const&) /home/kugan/local/llvm-proje
ct/clang-tools-extra/clangd/index/FileIndex.cpp:464:7
```





Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148088/new/

https://reviews.llvm.org/D148088

More information about the cfe-commits mailing list