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

[nridge]

Summary

This will be needed to support call hierarchy

[nridge]

(As per discussion in https://github.com/clangd/clangd/issues/162#issuecomment-648923948 onwards.)

[kadircet]

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?

[nridge]

In D83536#2143436, @kadircet wrote:

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

[kadircet]

In D83536#2144282, @nridge wrote:

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]

Rebase

[nridge]

Still need to add the flag.

[nridge]

Add the requested flag

[kadircet]

clang-tools-extra/clangd/index/Background.h

143 ↗

(On Diff #284262)

please set the default to false

clang-tools-extra/clangd/index/FileIndex.cpp

219 ↗

(On Diff #284262)

we don't need the option here right? as IsMainFileOnly flag should always be false for symbols inside headers(preamble).

421 ↗

(On Diff #284262)

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

119 ↗

(On Diff #284262)

please drop the default

[nridge]

Address review comments

[kadircet]

thanks, lgtm. mostly comments arounds tests, sorry for not looking at them before.

clang-tools-extra/clangd/index/FileIndex.h

156 ↗

(On Diff #284576)

again default to false.

clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp

192

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

627–633

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

709–710

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

[nridge]

Address review comments

[nridge]

Rebase

[kadircet]

thanks, LGTM!

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

630

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.

[nridge]

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

630

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]

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

630

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.

clang-tools-extra/clangd/index/SymbolCollector.cpp

bool SymbolCollector::handleDeclOccurrence(
   bool CollectRef = static_cast<bool>(Opts.RefFilter & toRefKind(Roles));
   bool IsOnlyRef =
       !(Roles & (static_cast<unsigned>(index::SymbolRole::Declaration) |
                  static_cast<unsigned>(index::SymbolRole::Definition)));
 
   if (IsOnlyRef && !CollectRef)
     return true;
 
-  // Do not store references to main-file symbols.
   // Unlike other fields, e.g. Symbols (which use spelling locations), we use
   // file locations for references (as it aligns the behavior of clangd's
   // AST-based xref).
   // FIXME: we should try to use the file locations for other fields.
-  if (CollectRef && !IsMainFileOnly && !isa<NamespaceDecl>(ND) &&
+  if (CollectRef && !isa<NamespaceDecl>(ND) &&
       (Opts.RefsInHeaders ||
        SM.getFileID(SM.getFileLoc(Loc)) == SM.getMainFileID()))
     DeclRefs[ND].emplace_back(SM.getFileLoc(Loc), Roles);
   // Don't continue indexing if this is a mere reference.
   if (IsOnlyRef)
     return true;
 
   // FIXME: ObjCPropertyDecl are not properly indexed here:

clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp

TEST_F(BackgroundIndexTest, IndexTwoFiles) {
   MemoryShardStorage MSS(Storage, CacheHits);
   OverlayCDB CDB(/*Base=*/nullptr);
   BackgroundIndex Idx(Context::empty(), FS, CDB,
                       [&](llvm::StringRef) { return &MSS; });
 
   tooling::CompileCommand Cmd;
   Cmd.Filename = testPath("root/A.cc");
   Cmd.Directory = testPath("root");
   Cmd.CommandLine = {"clang++", "-DA=1", testPath("root/A.cc")};
   CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
 
   ASSERT_TRUE(Idx.blockUntilIdleForTest());
   EXPECT_THAT(runFuzzyFind(Idx, ""),
               UnorderedElementsAre(AllOf(Named("common"), NumReferences(1U)),
                                    AllOf(Named("A_CC"), NumReferences(0U)),
-                                   AllOf(Named("g"), NumReferences(0U)),
+                                   AllOf(Named("g"), NumReferences(1U)),
                                    AllOf(Named("f_b"), Declared(),
                                          Not(Defined()), NumReferences(0U))));
 
   Cmd.Filename = testPath("root/B.cc");
   Cmd.CommandLine = {"clang++", Cmd.Filename};
   CDB.setCompileCommand(testPath("root/B.cc"), Cmd);
 
   ASSERT_TRUE(Idx.blockUntilIdleForTest());
   // B_CC is dropped as we don't collect symbols from A.h in this compilation.
   EXPECT_THAT(runFuzzyFind(Idx, ""),
               UnorderedElementsAre(AllOf(Named("common"), NumReferences(5U)),
                                    AllOf(Named("A_CC"), NumReferences(0U)),
-                                   AllOf(Named("g"), NumReferences(0U)),
+                                   AllOf(Named("g"), NumReferences(1U)),
                                    AllOf(Named("f_b"), Declared(), Defined(),
                                          NumReferences(1U))));
 
   auto Syms = runFuzzyFind(Idx, "common");
   EXPECT_THAT(Syms, UnorderedElementsAre(Named("common")));
   auto Common = *Syms.begin();
   EXPECT_THAT(getRefs(Idx, Common.ID),
               RefsAre({FileURI("unittest:///root/A.h"),

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

EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Foo").ID,
                                   HaveRanges(Main.ranges("foo")))));
   EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Bar").ID,
                                   HaveRanges(Main.ranges("bar")))));
   EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "func").ID,
                                   HaveRanges(Main.ranges("func")))));
   EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(Symbols, "NS").ID, _))));
   EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "MACRO").ID,
                                   HaveRanges(Main.ranges("macro")))));
-  // Symbols *only* in the main file (a, b, c, FUNC) had no refs collected.
-  auto MainSymbols =
-      TestTU::withHeaderCode(SymbolsOnlyInMainCode.code()).headerSymbols();
-  EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(MainSymbols, "a").ID, _))));
-  EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(MainSymbols, "b").ID, _))));
-  EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(MainSymbols, "c").ID, _))));
-  EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(MainSymbols, "FUNC").ID, _))));
+  // Symbols *only* in the main file (a, b, c) should have refs collected
+  // as well.
+  EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "a").ID, _)));
+  EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "b").ID, _)));
+  EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "c").ID, _)));
+  // However, references to main-file macros are not collected.
+  EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(Symbols, "FUNC").ID, _))));
 }
 
 TEST_F(SymbolCollectorTest, MacroRefInHeader) {
   Annotations Header(R"(
   #define $foo[[FOO]](X) (X + 1)
   #define $bar[[BAR]](X) (X + 2)
 
   // Macro defined multiple times.

 }
 
 TEST_F(SymbolCollectorTest, SpelledReferences) {
   struct {
     llvm::StringRef Header;
     llvm::StringRef Main;
     llvm::StringRef TargetSymbolName;
   } TestCases[] = {
       {
           R"cpp(
         struct Foo;
         #define MACRO Foo
       )cpp",
           R"cpp(
         struct $spelled[[Foo]] {
           $spelled[[Foo]]();
           ~$spelled[[Foo]]();
         };
         $spelled[[Foo]] Variable1;
         $implicit[[MACRO]] Variable2;
       )cpp",
           "Foo",
       },
       {
           R"cpp(
         class Foo {
         public:
           Foo() = default;
         };
       )cpp",
           R"cpp(
         void f() { Foo $implicit[[f]]; f = $spelled[[Foo]]();}
       )cpp",
           "Foo::Foo" /// constructor.
       },
   };
   CollectorOpts.RefFilter = RefKind::All;
   CollectorOpts.RefsInHeaders = false;
-  for (const auto& T : TestCases) {
+  for (const auto &T : TestCases) {
     Annotations Header(T.Header);
     Annotations Main(T.Main);
     // Reset the file system.
     InMemoryFileSystem = new llvm::vfs::InMemoryFileSystem;
     runSymbolCollector(Header.code(), Main.code());
 
     const auto SpelledRanges = Main.ranges("spelled");
     const auto ImplicitRanges = Main.ranges("implicit");
TEST_F(SymbolCollectorTest, HeaderAsMainFile) {
   CollectorOpts.RefFilter = RefKind::All;
   Annotations Header(R"(
   class $Foo[[Foo]] {};
 
   void $Func[[Func]]() {
     $Foo[[Foo]] fo;
   }
   )");
-  // The main file is normal .cpp file, we shouldn't collect any refs of symbols
-  // which are not declared in the preamble.
+  // We should collect refs to main-file symbols in all cases:
+
+  // 1. The main file is normal .cpp file.
   TestFileName = testPath("foo.cpp");
   runSymbolCollector("", Header.code());
-  EXPECT_THAT(Refs, UnorderedElementsAre());
+  EXPECT_THAT(Refs,
+              UnorderedElementsAre(Pair(findSymbol(Symbols, "Foo").ID,
+                                        HaveRanges(Header.ranges("Foo"))),
+                                   Pair(findSymbol(Symbols, "Func").ID,
+                                        HaveRanges(Header.ranges("Func")))));
 
-  // Run the .h file as main file, we should collect the refs.
+  // 2. Run the .h file as main file.
   TestFileName = testPath("foo.h");
   runSymbolCollector("", Header.code(),
                      /*ExtraArgs=*/{"-xobjective-c++-header"});
   EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Foo"), QName("Func")));
   EXPECT_THAT(Refs,
               UnorderedElementsAre(Pair(findSymbol(Symbols, "Foo").ID,
                                         HaveRanges(Header.ranges("Foo"))),
                                    Pair(findSymbol(Symbols, "Func").ID,
                                         HaveRanges(Header.ranges("Func")))));
 
-  // Run the .hh file as main file (without "-x c++-header"), we should collect
-  // the refs as well.
+  // 3. Run the .hh file as main file (without "-x c++-header").
   TestFileName = testPath("foo.hh");
   runSymbolCollector("", Header.code());
   EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Foo"), QName("Func")));
   EXPECT_THAT(Refs,
               UnorderedElementsAre(Pair(findSymbol(Symbols, "Foo").ID,
                                         HaveRanges(Header.ranges("Foo"))),
                                    Pair(findSymbol(Symbols, "Func").ID,
                                         HaveRanges(Header.ranges("Func")))));