Page MenuHomePhabricator

[clangd] Index refs to main-file symbols as well
ClosedPublic

Authored by nridge on Jul 10 2020, 12:03 AM.

Details

Summary

This will be needed to support call hierarchy

Diff Detail

Event Timeline

nridge created this revision.Jul 10 2020, 12:03 AM

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 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).

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.

nridge planned changes to this revision.Aug 4 2020, 11:40 PM

Still need to add the flag.

nridge updated this revision to Diff 284262.Aug 9 2020, 11:56 PM

Add the requested flag

kadircet added inline comments.Aug 10 2020, 2:43 AM
clang-tools-extra/clangd/index/Background.h
131

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

nridge updated this revision to Diff 284576.Aug 10 2020, 10:06 PM
nridge marked 4 inline comments as done.

Address review comments

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
724–738

could you instead run twice ? once with mainfilerefs = false, and once with it set to true?

815

these seem to be some unrelated formatting changes, please revert before landing.

nridge updated this revision to Diff 285907.Aug 16 2020, 3:22 PM
nridge marked 4 inline comments as done.

Address review comments

kadircet accepted this revision.Aug 17 2020, 1:01 AM

thanks, LGTM!

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
727

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.

This revision is now accepted and ready to land.Aug 17 2020, 1:01 AM
nridge added inline comments.Aug 17 2020, 8:43 AM
clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
727

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
kadircet added inline comments.Aug 17 2020, 9:40 AM
clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
727

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.

This revision was automatically updated to reflect the committed changes.
nridge marked 2 inline comments as done.Aug 17 2020, 9:34 PM