This will be needed to support call hierarchy
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
(As per discussion in https://github.com/clangd/clangd/issues/162#issuecomment-648923948 onwards.)
Thanks a lot for doing this Nathan and sorry for not replying with a concrete decision on the issue tracker, we were still discussing it internally, as 7% increase for allowing multi-level call hierarchy support (compared to ~10% initial cost for single-level support) seems quite high. Especially right before a new llvm release cut (which is due in 5 days).
I can see how this is needed for implementing the full protocol though (we've already cut on the multi-level callees support), so would you be so kind to hide this behind an option to symbol collector (CollectRefsInMainFile) and then introduce a flag to clangd for controlling it --collect-refs-in-main-file and plumb it to BackgroundIndex and FileIndex via ClangdServer?
I can do that.
Another thing we could consider, if the space increase is a concern, is to limit which references we store, e.g. to functions only (not variables or classes).
Yes, that's an option too, but I think putting it behind a flag is the safest for now, as we are cutting close to 11 release.
clang-tools-extra/clangd/index/Background.h | ||
---|---|---|
143 | please set the default to false | |
clang-tools-extra/clangd/index/FileIndex.cpp | ||
219 | we don't need the option here right? as IsMainFileOnly flag should always be false for symbols inside headers(preamble). | |
422 | i think we should rather put this option into FileIndex as a field (similar to UseDex) that way we could get rid of some plumbing | |
clang-tools-extra/clangd/index/FileIndex.h | ||
118 | please drop the default |
thanks, lgtm. mostly comments arounds tests, sorry for not looking at them before.
clang-tools-extra/clangd/index/FileIndex.h | ||
---|---|---|
156 | again default to false. | |
clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp | ||
195 | this requires it is own separate test, as this is not only testing for indexing two files case now. I would suggest a minimal test with one header and one cc ref, running background index twice to make sure both configurations work as expected. | |
clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp | ||
634–645 | could you instead run twice ? once with mainfilerefs = false, and once with it set to true? | |
722 | these seem to be some unrelated formatting changes, please revert before landing. |
thanks, LGTM!
clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp | ||
---|---|---|
637 | i don't think this is needed, am I missing something? IIRC, inmemoryfs should overwrite any existing files and even if it doesn't we are not really changing file contents in here. |
clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp | ||
---|---|---|
637 | I initially didn't have this, and got the following assertion in the second runSymbolCollector() call: ClangdTests: llvm/src/llvm/lib/Support/MemoryBuffer.cpp:48: void llvm::MemoryBuffer::init(const char *, const char *, bool): Assertion `(!RequiresNullTerminator || BufEnd[0] == 0) && "Buffer is not null terminated!"' failed. #0 0x00007f4242501c67 llvm::sys::PrintStackTrace(llvm::raw_ostream&) llvm/src/llvm/lib/Support/Unix/Signals.inc:563:11 #1 0x00007f4242501e09 PrintStackTraceSignalHandler(void*) llvm/src/llvm/lib/Support/Unix/Signals.inc:624:1 #2 0x00007f424250060b llvm::sys::RunSignalHandlers() llvm/src/llvm/lib/Support/Signals.cpp:67:5 #3 0x00007f424250251d SignalHandler(int) llvm/src/llvm/lib/Support/Unix/Signals.inc:405:1 #4 0x00007f42482a10e0 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x110e0) #5 0x00007f424157dfff raise /build/glibc-77giwP/glibc-2.24/signal/../sysdeps/unix/sysv/linux/raise.c:51:0 #6 0x00007f424157f42a abort /build/glibc-77giwP/glibc-2.24/stdlib/abort.c:91:0 #7 0x00007f4241576e67 __assert_fail_base /build/glibc-77giwP/glibc-2.24/assert/assert.c:92:0 #8 0x00007f4241576f12 (/lib/x86_64-linux-gnu/libc.so.6+0x2bf12) #9 0x00007f4242427350 llvm::MemoryBuffer::init(char const*, char const*, bool) llvm/src/llvm/lib/Support/MemoryBuffer.cpp:49:17 #10 0x00007f42424275b4 (anonymous namespace)::MemoryBufferMem<llvm::MemoryBuffer>::MemoryBufferMem(llvm::StringRef, bool) llvm/src/llvm/lib/Support/MemoryBuffer.cpp:89:3 #11 0x00007f4242427400 llvm::MemoryBuffer::getMemBuffer(llvm::StringRef, llvm::StringRef, bool) llvm/src/llvm/lib/Support/MemoryBuffer.cpp:115:7 #12 0x00007f424249ab20 llvm::vfs::detail::(anonymous namespace)::InMemoryFileAdaptor::getBuffer(llvm::Twine const&, long, bool, bool) llvm/src/llvm/lib/Support/VirtualFileSystem.cpp:615:12 #13 0x00007f4242b0ecf1 clang::FileManager::getBufferForFile(clang::FileEntry const*, bool, bool) llvm/src/clang/lib/Basic/FileManager.cpp:474:5 #14 0x00007f4242b52013 clang::SrcMgr::ContentCache::getBuffer(clang::DiagnosticsEngine&, clang::FileManager&, clang::SourceLocation, bool*) const llvm/src/clang/lib/Basic/SourceManager.cpp:172:8 #15 0x00007f4242fe64c0 clang::SourceManager::getBuffer(clang::FileID, clang::SourceLocation, bool*) const llvm/src/clang/include/clang/Basic/SourceManager.h:976:5 #16 0x00007f4242fe2425 clang::Preprocessor::EnterSourceFile(clang::FileID, clang::DirectoryLookup const*, clang::SourceLocation) llvm/src/clang/lib/Lex/PPLexerChange.cpp:77:29 #17 0x00007f424301c104 clang::Preprocessor::EnterMainSourceFile() llvm/src/clang/lib/Lex/Preprocessor.cpp:547:5 #18 0x00007f423d4a23fe clang::ParseAST(clang::Sema&, bool, bool) llvm/src/clang/lib/Parse/ParseAST.cpp:144:33 #19 0x00007f42466df752 clang::ASTFrontendAction::ExecuteAction() llvm/src/clang/lib/Frontend/FrontendAction.cpp:1059:1 #20 0x00007f42466df118 clang::FrontendAction::Execute() llvm/src/clang/lib/Frontend/FrontendAction.cpp:954:7 #21 0x00007f4246655ae4 clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) llvm/src/clang/lib/Frontend/CompilerInstance.cpp:984:23 #22 0x00007f42472a622a clang::tooling::FrontendActionFactory::runInvocation(std::shared_ptr<clang::CompilerInvocation>, clang::FileManager*, std::shared_ptr<clang::PCHContainerOperations>, clang::DiagnosticConsumer*) llvm/src/clang/lib/Tooling/Tooling.cpp:410:14 #23 0x00007f42472a60d4 clang::tooling::ToolInvocation::runInvocation(char const*, clang::driver::Compilation*, std::shared_ptr<clang::CompilerInvocation>, std::shared_ptr<clang::PCHContainerOperations>) llvm/src/clang/lib/Tooling/Tooling.cpp:385:3 #24 0x00007f42472a5015 clang::tooling::ToolInvocation::run() llvm/src/clang/lib/Tooling/Tooling.cpp:370:3 #25 0x0000000000bc916b clang::clangd::(anonymous namespace)::SymbolCollectorTest::runSymbolCollector(llvm::StringRef, llvm::StringRef, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&) llvm/src/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:283:15 #26 0x0000000000bda339 clang::clangd::(anonymous namespace)::SymbolCollectorTest_Refs_Test::TestBody() llvm/src/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:729:3 |
clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp | ||
---|---|---|
637 | ah makes sense. looks like we are calling MemoryBuffer:getMemBuffer, which would make the buffers unusable after runSymbolCollector exits. I must've been lucky enough to not overwrite the stack. could you put a comment mentioning the situation? then feel free to land. |
please set the default to false