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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
