[Clangd] Index main-file symbols (bug 39761)

[nridge]

This is a first attempt at a patch for this.

There are some cases this doesn't handle, which I was thinking of handling in follow-up commits (but we could do them here if desired):

• Entities in anonymous namespaces are still not indexed, There is a comment in SymbolCollector::shouldCollectSymbol() which says "Also we skip the symbols in anonymous namespace as the qualifier names of these symbols are like foo::<anonymous>::bar, which need a special handling." It's not clear to me how that should be handled, suggestions are welcome.
• Macros in main-files are still not indexed. I found that when I tried to index them, the index would start to contain all sorts of built-in macros like __INT_FAST16_MAX__, __DBL_MIN__, etc. I assume we'll want to exclude those, but it wasn't immediately clear to me how.

I ran clangd-indexer on the clangd codebase to see what this patch does to the size of the generated index: the increase in on-disk size was only 0.45%. Presumably that will increase once we start storing entities in anonymous namespaces. (I haven't run it on all of LLVM yet as it takes a very long time to run.)

[nridge]

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

67

I found I had to do this to get many WorkspaceSymbolsTest test cases to pass.

In the absence of this flag, the header files in affected test cases are indexed as C files, and later (when included via the .cpp file) as C++ files. USRs for functions are different in C mode than in C++ mode (in C++ mode the USR includes the function's signature), leading to different SymbolIDs and thus two copies of the function in the index.

Having the test case parse the header file in C++ mode seems reasonable, as it matches what happens in production (when you open a .h file in a C++ project in an editor, clangd does use a compilation command that includes -xc++, or rather -xc++-header (thanks to InterpolatingCompilationDatabase?)).

Not sure if there's a better way to do this - should MockCompilationDatabase be doing this automatically somehow?

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

535

I had to simplify the test case a bit because otherwise there were 11 symbols and UnorderedElementsAre() only supports 10 :) Not sure if there's a better thing to do here.

[ilya-biryukov]

Thanks for taking a look at this.

Symbols from the main file might require special handling as there are various percularities with them.
E.g. it would be weird to see just a single "main" function being returned in the workspace symbols, despite having multiple of those in the codebase. There's a whole can of worms when the names of the symbols are the same, but they live inside different C++ files in the anonymous namespace.
However, it should be ok if we collect them only to expose in the workspace symbols.
I think we should take a look at collecting the symbols from the anonymous namespace. Those should be important, since this is where almost all cpp-file local symbols should live (in a well-behaved C++ codebase).

It's not clear to me how that should be handled, suggestions are welcome.

Let's just try collecting them. This would work fine with symbols that are only inside a single file, but will result in symbols with the same name from multiple files being merged together, which is bad.

I found that when I tried to index them, the index would start to contain all sorts of built-in macros like INT_FAST16_MAX, DBL_MIN, etc. I assume we'll want to exclude those, but it wasn't immediately clear to me how

Those macros should come from a buffer with the name "<built-in>". there should be a way to detect those and get rid of them.

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

297–298

Why out parameter here? Just compute this in the calling function instead.

352

Just set CollectRef to false on symbols from the main files?

544–545

Please add a comment that we only collect main file symbols for workspace symbols.

[hokein]

In D55185#1316101, @nridge wrote:

This is a first attempt at a patch for this.

There are some cases this doesn't handle, which I was thinking of handling in follow-up commits (but we could do them here if desired):

• Entities in anonymous namespaces are still not indexed, There is a comment in SymbolCollector::shouldCollectSymbol() which says "Also we skip the symbols in anonymous namespace as the qualifier names of these symbols are like foo::<anonymous>::bar, which need a special handling." It's not clear to me how that should be handled, suggestions are welcome.

For code completion, we don't want them in the index, these kind of symbols can be offered from clang's AST (as anonymous namespace symbols typically are in main file). For workspace symbols, we want them in the index, they will be returned when users search foo::bar instead of foo::<anonymous>::bar I suppose.

So I think we can treat them as normal static symbols.

• Macros in main-files are still not indexed. I found that when I tried to index them, the index would start to contain all sorts of built-in macros like __INT_FAST16_MAX__, __DBL_MIN__, etc. I assume we'll want to exclude those, but it wasn't immediately clear to me how.

I think we should exclude builtin macros, MacroInfo provides a isBuiltinMacro function that could help you.

I ran clangd-indexer on the clangd codebase to see what this patch does to the size of the generated index: the increase in on-disk size was only 0.45%. Presumably that will increase once we start storing entities in anonymous namespaces. (I haven't run it on all of LLVM yet as it takes a very long time to run.)

AFAIK, the symbol (section) is not the largest part of index (we have 30K symbols), would be interesting to know the (index, memory) size (including symbols in anonymous namespace).

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

297–298

The comment is stale.

544–545

This introduces a new dependency IsMainFileSymbol, we need to check IsMainFileSymbol to make sure we set IndexedForCodeCompletion correctly. And we need to update all callsides of isIndexedForCodeCompletion.

How about

• expose a static method isDeclaredInMainFile(const NamedDecl, SourceManager SM) in SymbolCollector
• use this method in shouldCollectSymbol, and isIndexedForCodeCompletion

This would get rid of passing IsMainFileSymbol argument in shouldCollectSymbol, addDeclaration. The tradeoff is that we pay an extra cost of IsDeclaredInMainFile, but I think it is fine.

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

146

I think we need add a test for static symbols.

[nridge]

Thanks for the reviews!

In D55185#1316552, @ilya-biryukov wrote:

It's not clear to me how that should be handled, suggestions are welcome.

Let's just try collecting them. This would work fine with symbols that are only inside a single file, but will result in symbols with the same name from multiple files being merged together, which is bad.

Thanks for clarifying. I thought the comment was saying the name would require special handling.

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

544–545

This is the only place where we call isIndexedForCodeCompletion.

Given that, should we still make these changes?

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

535

I found the solution: UnorderedElementsAreArray().

[nridge]

Addressed review comments

[nridge]

In D55185#1316564, @hokein wrote:

AFAIK, the symbol (section) is not the largest part of index (we have 30K symbols), would be interesting to know the (index, memory) size (including symbols in anonymous namespace).

Do you have a suggestion for how to measure memory usage? Should I just look at the memory usage of the clangd process in top?

[hokein]

In D55185#1328011, @nridge wrote:

In D55185#1316564, @hokein wrote:

AFAIK, the symbol (section) is not the largest part of index (we have 30K symbols), would be interesting to know the (index, memory) size (including symbols in anonymous namespace).

Do you have a suggestion for how to measure memory usage? Should I just look at the memory usage of the clangd process in top?

You can use dexp(clangd/index/dex/dexp/) to load the index data, and it will print the memory usage of the index.

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

396

hmm, the isBuiltinMacro only detects the builtin macros which is not sufficient for our cases.

The current way seems a bit tricky. Could you dump the DefLoc to see the whole location information of __DBL_MIN_?
We don't record Predefined information in MacroInfo. I think we can do it by checking whether the FileID is a predefined file ID (using SM.getFileID(DefLoc) == PP->getPredefinesFileID()).

I think we can do this in a separate patch, just focus on the declarations in this patch, what do you think?

544

I think we can rephrase the comment more collectly -- we collect the main-file symbols, but we don't use them for code completion.

[nridge]

Address second round of review comments

[nridge]

In D55185#1328118, @hokein wrote:

In D55185#1328011, @nridge wrote:

In D55185#1316564, @hokein wrote:

AFAIK, the symbol (section) is not the largest part of index (we have 30K symbols), would be interesting to know the (index, memory) size (including symbols in anonymous namespace).

Do you have a suggestion for how to measure memory usage? Should I just look at the memory usage of the clangd process in top?

You can use dexp(clangd/index/dex/dexp/) to load the index data, and it will print the memory usage of the index.

Thanks!

So, on the clangd codebase, I got the following measurements for indexing with this patch compared to indexing without this patch:

• Size of index on disk increased by 3.2%
• Memory usage as reported by dexp increased by 3.6%

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

396

I found SourceManager::isWrittenInBuiltinFile() which seems to serve this purpose well.

[hokein]

! In D55185#1330328, @nridge wrote:

Thanks!

So, on the clangd codebase, I got the following measurements for indexing with this patch compared to indexing without this patch:

 
- Size of index on disk increased by 3.2%
- Memory usage as reported by `dexp` increased by 3.6%

Thanks for the investigation. Could you please rebase your patch? I'd like to try it on the whole llvm project (since it is too slow for you to run it on whole LLVM).

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

396

Can we postpone the macro case to another patch? I'm a bit nervous about macros...

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

345

Why ForCodeCompletion is true for ff? ff is a main file symbol.

644

Since we change the purpose of the test, I think the Header can be empty.

651

Maybe add a test case where anonymous namespace is inside a named namespace, like

namespace foo {
namespace {
class Foo {};
}
}

[nridge]

Address third round of review comments

[nridge]

In D55185#1331188, @hokein wrote:

Thanks for the investigation. Could you please rebase your patch? I'd like to try it on the whole llvm project (since it is too slow for you to run it on whole LLVM).

Rebased, thanks!

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

345

This code is interpreted as a header, so none of the symbols are main-file symbols.

[nridge]

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

396

Moved the macro part here: https://reviews.llvm.org/D55739

[hokein]

Thanks for the update.

Got some data on your patch, the increased size is larger than we expected, it seems that we have bugs in the patch, IIUC the references number should be unchanged.

Before (76M)

 ./bin/dexp symbols.dex
Loaded Dex from symbols.dex with estimated memory usage 277976448 bytes
  - number of symbols: 308624
  - number of refs: 5561489

After (89M)

 ./bin/dexp symbols-main-symbols.dex
Loaded Dex from symbols-main-symbols.dex with estimated memory usage 345706301 bytes
  - number of symbols: 440012
  - number of refs: 6022675

[nridge]

In D55185#1332829, @hokein wrote:

it seems that we have bugs in the patch, IIUC the references number should be unchanged.

Before (76M)

 ./bin/dexp symbols.dex
Loaded Dex from symbols.dex with estimated memory usage 277976448 bytes
  - number of symbols: 308624
  - number of refs: 5561489

After (89M)

 ./bin/dexp symbols-main-symbols.dex
Loaded Dex from symbols-main-symbols.dex with estimated memory usage 345706301 bytes
  - number of symbols: 440012
  - number of refs: 6022675

I am not able to reproduce this, at least not on the clangd codebase (I didn't try all of llvm).

Just to check: when you did this test, were you indexing the exact same code both times? I ask because, when I was looking into this, I initially made the mistake of indexing the same copy of the clangd source from which I built the indexer. In that case, the number of refs was slightly different, but that's expected because the source code being indexed is different. When I tried it again indexing the same source code both times, the number of refs was unchanged.

[hokein]

In D55185#1339243, @nridge wrote:

In D55185#1332829, @hokein wrote:

it seems that we have bugs in the patch, IIUC the references number should be unchanged.

Before (76M)

 ./bin/dexp symbols.dex
Loaded Dex from symbols.dex with estimated memory usage 277976448 bytes
  - number of symbols: 308624
  - number of refs: 5561489

After (89M)

 ./bin/dexp symbols-main-symbols.dex
Loaded Dex from symbols-main-symbols.dex with estimated memory usage 345706301 bytes
  - number of symbols: 440012
  - number of refs: 6022675

I am not able to reproduce this, at least not on the clangd codebase (I didn't try all of llvm).

Just to check: when you did this test, were you indexing the exact same code both times? I ask because, when I was looking into this, I initially made the mistake of indexing the same copy of the clangd source from which I built the indexer. In that case, the number of refs was slightly different, but that's expected because the source code being indexed is different. When I tried it again indexing the same source code both times, the number of refs was unchanged.

I did double check today, I can confirmed that both of them ran at the same code base. Didn't dig into it, my suspicion is that we collect header symbols in anonymous namespace with this patch, this maybe the reason that increases references.

[nridge]

Do not index symbols inside anonymous namespaces in header files.

[nridge]

In D55185#1345136, @hokein wrote:

I did double check today, I can confirmed that both of them ran at the same code base.

Thanks for checking.

Didn't dig into it, my suspicion is that we collect header symbols in anonymous namespace with this patch, this maybe the reason that increases references.

This is the only explanation that I can think of, too.

I modified the patch so we don't collect header symbols in anonymous namespaces. Could you try again with this patch, and see if it resolves the discrepancy?

[hokein]

I modified the patch so we don't collect header symbols in anonymous namespaces. Could you try again with this patch, and see if it resolves the discrepancy?

With the new patch, the result looks reasonable now (number of symbols: 405 K vs 309 K, dex usage: 320 MB vs 278 MB, index size: 82 MB vs 76 MB), the memory usage has 15% increase, which is a bit unfortunate, but we can optimize it.

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

342

I think we can optimize this, calling isInMainFile for each reference seems wasteful.

How about

if (IsOnlyRef && !CollectRef)
   return true;
bool IsMainFileSymbol = SM.isInMainFile(SM.getExpansionLoc(ND->getBeginLoc()));
if (!shouldCollectSymbol(*ND, *ASTCtx, Opts, IsMainFileSymbol))
   return true;
if (CollectRef && !isMainFileSymbol && ...)

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

27

The documentation needs update.

[nridge]

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

342

Heh, this is actually what I did in the first version of the patch, and then Ilya suggested I modify CollectRef instead.

Anyways, I changed it back now.

[nridge]

Addressed latest round of review comments

[hokein]

Thanks! The patch looks mostly good.

I think we'd need a new SymbolFlag for main-file symbols (allowing us to filter them out internally) as we are not quite interested in this kind of symbols internally, and the increase of the symbols may blow up our internal indexer, but this can be done in another patch.

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

119

Passing true to the function doesn't seem right to me. I think we might need to expose the code SM.isInMainFile(SM.getExpansionLoc(ND->getBeginLoc())); to a utility function, maybe a static method SymbolCollector::isDeclaredInMainFile.

[nridge]

Address latest review comment

[hokein]

looks good. Do you have commit access?

[nridge]

Thanks very much for the reviews!

In D55185#1350885, @hokein wrote:

looks good. Do you have commit access?

I do not. Could you commit it for me?

[sammccall]

Sorry to jump in late here.

The piece that seems to be missing is recording that a symbol is main-file only, which seems important for ranking - public symbols should be ranked above private ones.
(In Quality.h this is the distinction between AccessibleScope::GlobalScope and AccessibleScope::FileScope).

I think we want to add a new Symbol::SymbolFlag for this, I'd suggest FileLocal or so.

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

264

isWrittenInMainFile

(the names of these functions are terrible - for our purpose, isWrittenInMainFile is just faster)

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

45

Can I request making main-file symbol collection optional here? (on by default)

The cost/benefit tradeoff is different for a static index of a huge codebase, we may want to keep this off for our internal code index.

82

This doesn't make sense as part of the public SymbolCollector API.
I'd suggest just inlining it at its callsite.
(The test shouldn't really be calling it at all - I think the test is confused about its scope. But that's not new in this patch, so just inlining it there too seems ok for now)

[nridge]

In D55185#1352511, @sammccall wrote:

The piece that seems to be missing is recording that a symbol is main-file only, which seems important for ranking - public symbols should be ranked above private ones.
(In Quality.h this is the distinction between AccessibleScope::GlobalScope and AccessibleScope::FileScope).

Ranking for what? If you mean code completion, we are already excluding main-file symbols from code completion by not setting the IndexedForCodeCompletion flag.

[nridge]

Addressed Sam's comments, except for the SymbolFlag one as I do not understand it yet

[sammccall]

Thanks for the changes. LGTM, though I think we should follow up with the ranking stuff.

In D55185#1353242, @nridge wrote:

In D55185#1352511, @sammccall wrote:

The piece that seems to be missing is recording that a symbol is main-file only, which seems important for ranking - public symbols should be ranked above private ones.
(In Quality.h this is the distinction between AccessibleScope::GlobalScope and AccessibleScope::FileScope).

Ranking for what? If you mean code completion, we are already excluding main-file symbols from code completion by not setting the IndexedForCodeCompletion flag.

SymbolIndex::fuzzyFind provides ranking for non-completion searches.
This is used for workspaceSymbol at the moment and maybe other queries (the subtypes search we discussed on the mailing list?) in future.

Currently the scope isn't taken into account for non-completion queries, because historically only top-level global symbols were in the index. With members and now main-file symbols in the index, we should fix that.
I don't want to block this patch, so feel free to commit as is. I think we should follow up with:

• add the flag to Symbol to distinguish file-local symbols (please do this, but a separate patch is fine)
• set Scope accordingly in Quality.cpp, and adjust relevance scores for non-code-complete queries based on scope, in the opposide direction from CC queries (I'm happy to do this if you don't want to)

[nridge]

Add a new symbol flag to mark main-file symbols

[nridge]

In D55185#1353974, @sammccall wrote:

SymbolIndex::fuzzyFind provides ranking for non-completion searches.
This is used for workspaceSymbol at the moment and maybe other queries (the subtypes search we discussed on the mailing list?) in future.

Currently the scope isn't taken into account for non-completion queries, because historically only top-level global symbols were in the index. With members and now main-file symbols in the index, we should fix that.

Thanks, makes sense now.

I don't want to block this patch, so feel free to commit as is. I think we should follow up with:

• add the flag to Symbol to distinguish file-local symbols (please do this, but a separate patch is fine)

I've done this in the latest update to this patch.

• set Scope accordingly in Quality.cpp, and adjust relevance scores for non-code-complete queries based on scope, in the opposide direction from CC queries (I'm happy to do this if you don't want to)

I'm happy to give this a try. I think this one makes more sense in a separate patch.

As I'm not a committer, could you commit this patch for me? Thanks!

[nridge]

In D55185#1355426, @nridge wrote:

• set Scope accordingly in Quality.cpp, and adjust relevance scores for non-code-complete queries based on scope, in the opposide direction from CC queries (I'm happy to do this if you don't want to)

I'm happy to give this a try. I think this one makes more sense in a separate patch.

I did this in this patch.

[sammccall]

Thanks for adding the flag!

Merge.cpp also needs to handle what happens when different copies of the symbol have different value for the flag (--> it's not file local).

I'll address that and another last comment and land the patch.

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

351

just noticed this is a bit subtle: we want to know if this is *only* a main-file symbol, not just if it's ever declared in the main file. But the check works because D is the canonical decl (per the clang/Index infrastructure), which is the lexically first, and we see the headers before the main file. I'll add a comment.

[sammccall]

add comment about subtleties around canonical declarations and main-file check
invert sense of Symbol flag (FileLocal -> VisibleOutsideFile) so Merge does the right thing
test: add case for forward declarations (I think we do the wrong thing, fix later)
test: replace Matcher(true)/Matcher(false) with Matcher() and Not(Matcher())

[sammccall]

The forward-declaration-in-main-file case:
In principle, if the definition is missing, I think we should treat these differently (not index them, or treat them as visible-outside file) because they're almost certainly declared "properly" elsewhere.
@ilya-biryukov convinced me offline that this isn't a big deal in practice as forward declarations in main-files (and not subsequently defining the symbol) aren't that useful.
So going to avoid patching that, as there might be unintended consequences.

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

 #include "clang/Index/IndexSymbol.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
 #include "llvm/ADT/DenseMap.h"
 #include <functional>
 
 namespace clang {
 namespace clangd {
 
 /// \brief Collect declarations (symbols) from an AST.
 /// It collects most declarations except:
 /// - Implicit declarations
 /// - Anonymous declarations (anonymous enum/class/struct, etc)
 /// - Declarations in anonymous namespaces
 /// - Local declarations (in function bodies, blocks, etc)
 /// - Declarations in main files
 /// - Template specializations
 /// - Library-specific private declarations (e.g. private declaration generated
 /// by protobuf compiler)
 ///
 /// See also shouldCollectSymbol(...).
 ///
 /// Clients (e.g. clangd) can use SymbolCollector together with
 /// index::indexTopLevelDecls to retrieve all symbols when the source file is
 /// changed.
 class SymbolCollector : public index::IndexDataConsumer {
 public:
   struct Options {
     /// When symbol paths cannot be resolved to absolute paths (e.g. files in
     /// VFS that does not have absolute path), combine the fallback directory
     /// with symbols' paths to get absolute paths. This must be an absolute
     /// path.
     std::string FallbackDir;
     bool CollectIncludePath = false;
     /// If set, this is used to map symbol #include path to a potentially
     /// different #include path.
struct Options {
     bool CollectMacro = false;
     /// If this is set, only collect symbols/references from a file if
     /// `FileFilter(SM, FID)` is true. If not set, all files are indexed.
     std::function<bool(const SourceManager &, FileID)> FileFilter = nullptr;
   };
 
   SymbolCollector(Options Opts);
 
   /// Returns true is \p ND should be collected.
   static bool shouldCollectSymbol(const NamedDecl &ND, const ASTContext &ASTCtx,
                                   const Options &Opts);
 
   void initialize(ASTContext &Ctx) override;
 
   void setPreprocessor(std::shared_ptr<Preprocessor> PP) override {
     this->PP = std::move(PP);
   }
bool handleMacroOccurence(const IdentifierInfo *Name, const MacroInfo *MI,
                             SourceLocation Loc) override;
 
   SymbolSlab takeSymbols() { return std::move(Symbols).build(); }
   RefSlab takeRefs() { return std::move(Refs).build(); }
 
   void finish() override;
 
 private:
-  const Symbol *addDeclaration(const NamedDecl &, SymbolID);
+  const Symbol *addDeclaration(const NamedDecl &, SymbolID, bool IsMainFileSymbol);
   void addDefinition(const NamedDecl &, const Symbol &DeclSymbol);
 
   // All Symbols collected from the AST.
   SymbolSlab::Builder Symbols;
   // All refs collected from the AST.
   // Only symbols declared in preamble (from #include) and referenced from the
   // main file will be included.
   RefSlab::Builder Refs;

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

void SymbolCollector::initialize(ASTContext &Ctx) {
   ASTCtx = &Ctx;
   CompletionAllocator = std::make_shared<GlobalCodeCompletionAllocator>();
   CompletionTUInfo =
       llvm::make_unique<CodeCompletionTUInfo>(CompletionAllocator);
 }
 
 bool SymbolCollector::shouldCollectSymbol(const NamedDecl &ND,
                                           const ASTContext &ASTCtx,
                                           const Options &Opts) {
   if (ND.isImplicit())
     return false;
   // Skip anonymous declarations, e.g (anonymous enum/class/struct).
   if (ND.getDeclName().isEmpty())
     return false;
 
-  // FIXME: figure out a way to handle internal linkage symbols (e.g. static
-  // variables, function) defined in the .cc files. Also we skip the symbols
-  // in anonymous namespace as the qualifier names of these symbols are like
-  // `foo::<anonymous>::bar`, which need a special handling.
-  // In real world projects, we have a relatively large set of header files
-  // that define static variables (like "static const int A = 1;"), we still
-  // want to collect these symbols, although they cause potential ODR
-  // violations.
-  if (ND.isInAnonymousNamespace())
-    return false;
-
   // We want most things but not "local" symbols such as symbols inside
   // FunctionDecl, BlockDecl, ObjCMethodDecl and OMPDeclareReductionDecl.
   // FIXME: Need a matcher for ExportDecl in order to include symbols declared
   // within an export.
   const auto *DeclCtx = ND.getDeclContext();
   switch (DeclCtx->getDeclKind()) {
   case Decl::TranslationUnit:
   case Decl::Namespace:
default:
     // easier to cast.
     if (!isa<RecordDecl>(DeclCtx))
       return false;
   }
   if (explicitTemplateSpecialization<FunctionDecl>(ND) ||
       explicitTemplateSpecialization<CXXRecordDecl>(ND) ||
       explicitTemplateSpecialization<VarDecl>(ND))
     return false;
 
-  const auto &SM = ASTCtx.getSourceManager();
-  // Skip decls in the main file.
-  if (SM.isInMainFile(SM.getExpansionLoc(ND.getBeginLoc())))
-    return false;
   // Avoid indexing internal symbols in protobuf generated headers.
   if (isPrivateProtoDecl(ND))
     return false;
   return true;
 }
 
 // Always return true to continue indexing.
 bool SymbolCollector::handleDeclOccurence(
     const Decl *D, index::SymbolRoleSet Roles,
if (Opts.CountReferences &&
       SM.getFileID(SpellingLoc) == SM.getMainFileID())
     ReferencedDecls.insert(ND);
 
   bool CollectRef = static_cast<unsigned>(Opts.RefFilter) & Roles;
   bool IsOnlyRef =
       !(Roles & (static_cast<unsigned>(index::SymbolRole::Declaration) |
                  static_cast<unsigned>(index::SymbolRole::Definition)));
 
+  bool IsMainFileSymbol = SM.isInMainFile(SM.getExpansionLoc(ND->getBeginLoc()));
+  // Do not store references to main-file symbols.
+  if (IsMainFileSymbol)
+    CollectRef = false;
+
   if (IsOnlyRef && !CollectRef)
     return true;
+
   if (!shouldCollectSymbol(*ND, *ASTCtx, Opts))
     return true;
   if (CollectRef && !isa<NamespaceDecl>(ND) &&
       (Opts.RefsInHeaders || SM.getFileID(SpellingLoc) == SM.getMainFileID()))
     DeclRefs[ND].emplace_back(SpellingLoc, Roles);
   // Don't continue indexing if this is a mere reference.
   if (IsOnlyRef)
     return true;
 
   auto ID = getSymbolID(ND);
   if (!ID)
     return true;
 
   const NamedDecl &OriginalDecl = *cast<NamedDecl>(ASTNode.OrigD);
   const Symbol *BasicSymbol = Symbols.find(*ID);
   if (!BasicSymbol) // Regardless of role, ND is the canonical declaration.
-    BasicSymbol = addDeclaration(*ND, std::move(*ID));
+    BasicSymbol = addDeclaration(*ND, std::move(*ID), IsMainFileSymbol);
   else if (isPreferredDeclaration(OriginalDecl, Roles))
     // If OriginalDecl is preferred, replace the existing canonical
     // declaration (e.g. a class forward declaration). There should be at most
     // one duplicate as we expect to see only one preferred declaration per
     // TU, because in practice they are definitions.
-    BasicSymbol = addDeclaration(OriginalDecl, std::move(*ID));
+    BasicSymbol = addDeclaration(OriginalDecl, std::move(*ID), IsMainFileSymbol);
 
   if (Roles & static_cast<unsigned>(index::SymbolRole::Definition))
     addDefinition(OriginalDecl, *BasicSymbol);
   return true;
 }
 
 bool SymbolCollector::handleMacroOccurence(const IdentifierInfo *Name,
                                            const MacroInfo *MI,
                                            index::SymbolRoleSet Roles,
                                            SourceLocation Loc) {
   if (!Opts.CollectMacro)
     return true;
   assert(PP.get());
 
   const auto &SM = PP->getSourceManager();
   auto DefLoc = MI->getDefinitionLoc();
-  if (SM.isInMainFile(SM.getExpansionLoc(DefLoc)))
-    return true;
+  bool IsMainFileSymbol = SM.isInMainFile(SM.getExpansionLoc(DefLoc));
+  
   // Header guards are not interesting in index. Builtin macros don't have
   // useful locations and are not needed for code completions.
   if (MI->isUsedForHeaderGuard() || MI->isBuiltinMacro())
     return true;
 
+  // Also avoid storing predefined macros like __DBL_MIN__.
+  // These are not caught by MI->isBuiltinMacro(), but we can
+  // detect them by their definition not being in any file.
+  if (IsMainFileSymbol && SM.getFilename(DefLoc).empty())
+    return true;
+
   // Mark the macro as referenced if this is a reference coming from the main
   // file. The macro may not be an interesting symbol, but it's cheaper to check
   // at the end.
   if (Opts.CountReferences &&
       (Roles & static_cast<unsigned>(index::SymbolRole::Reference)) &&
       SM.getFileID(SM.getSpellingLoc(Loc)) == SM.getMainFileID())
     ReferencedMacros.insert(Name);
   // Don't continue indexing if this is a mere reference.
   // FIXME: remove macro with ID if it is undefined.
   if (!(Roles & static_cast<unsigned>(index::SymbolRole::Declaration) ||
         Roles & static_cast<unsigned>(index::SymbolRole::Definition)))
     return true;
 
   auto ID = getSymbolID(*Name, MI, SM);
   if (!ID)
     return true;
 
   // Only collect one instance in case there are multiple.
   if (Symbols.find(*ID) != nullptr)
     return true;
 
   Symbol S;
   S.ID = std::move(*ID);
   S.Name = Name->getName();
+  if (!IsMainFileSymbol)
     S.Flags |= Symbol::IndexedForCodeCompletion;
   S.SymInfo = index::getSymbolInfoForMacro(*MI);
   std::string FileURI;
   // FIXME: use the result to filter out symbols.
   shouldIndexFile(SM, SM.getFileID(Loc), Opts, &FilesToIndexCache);
   if (auto DeclLoc =
           getTokenLocation(DefLoc, SM, Opts, PP->getLangOpts(), FileURI))
     S.CanonicalDeclaration = *DeclLoc;
 
void SymbolCollector::finish() {
 
   ReferencedDecls.clear();
   ReferencedMacros.clear();
   DeclRefs.clear();
   FilesToIndexCache.clear();
 }
 
 const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND,
-                                              SymbolID ID) {
+                                              SymbolID ID,
+                                              bool IsMainFileSymbol) {
   auto &Ctx = ND.getASTContext();
   auto &SM = Ctx.getSourceManager();
 
   Symbol S;
   S.ID = std::move(ID);
   std::string QName = printQualifiedName(ND);
   std::tie(S.Scope, S.Name) = splitQualifiedName(QName);
   // FIXME: this returns foo:bar: for objective-C methods, we prefer only foo:
   // for consistency with CodeCompletionString and a clean name/signature split.
 
-  if (isIndexedForCodeCompletion(ND, Ctx))
+  // We only collect main-file symbols for "textDocument/workspaceSymbols".
+  if (!IsMainFileSymbol && isIndexedForCodeCompletion(ND, Ctx))
     S.Flags |= Symbol::IndexedForCodeCompletion;
   if (isImplementationDetail(&ND))
     S.Flags |= Symbol::ImplementationDetail;
   S.SymInfo = index::getSymbolInfo(&ND);
   std::string FileURI;
   auto Loc = findNameLoc(&ND);
   // FIXME: use the result to filter out symbols.
   shouldIndexFile(SM, SM.getFileID(Loc), Opts, &FilesToIndexCache);

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

TEST_F(BackgroundIndexTest, IndexTwoFiles) {
   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(Named("common"), Named("A_CC"),
+      UnorderedElementsAre(Named("common"), Named("A_CC"), Named("g"),
                            AllOf(Named("f_b"), Declared(), Not(Defined()))));
 
   Cmd.Filename = testPath("root/B.cc");
   Cmd.CommandLine = {"clang++", Cmd.Filename};
   CDB.setCompileCommand(testPath("root/A.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(Named("common"), Named("A_CC"),
+              UnorderedElementsAre(Named("common"), Named("A_CC"), Named("g"),
                                    AllOf(Named("f_b"), Declared(), Defined())));
 
   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"),
                        FileURI("unittest:///root/A.cc"),
EXPECT_THAT(
       UnorderedElementsAre(Named("common"), Named("A_CC"),
                            AllOf(Named("f_b"), Declared(), Not(Defined()))));
   for (const auto &Ref : *ShardHeader->Refs)
     EXPECT_THAT(Ref.second,
                 UnorderedElementsAre(FileURI("unittest:///root/A.h")));
 
   auto ShardSource = MSS.loadShard(testPath("root/A.cc"));
   EXPECT_NE(ShardSource, nullptr);
-  EXPECT_THAT(*ShardSource->Symbols, UnorderedElementsAre());
+  EXPECT_THAT(*ShardSource->Symbols, UnorderedElementsAre(Named("g")));
   EXPECT_THAT(*ShardSource->Refs, RefsAre({FileURI("unittest:///root/A.cc")}));
 }
 
 TEST_F(BackgroundIndexTest, DirectIncludesTest) {
   MockFSProvider FS;
   FS.Files[testPath("root/B.h")] = "";
   FS.Files[testPath("root/A.h")] = R"cpp(
       #include "B.h"

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

 }
 
 class WorkspaceSymbolsTest : public ::testing::Test {
 public:
   WorkspaceSymbolsTest()
       : Server(CDB, FSProvider, DiagConsumer, optsForTests()) {
     // Make sure the test root directory is created.
     FSProvider.Files[testPath("unused")] = "";
+    CDB.ExtraClangFlags = {"-xc++"};
   }
 
 protected:
   MockFSProvider FSProvider;
   MockCompilationDatabase CDB;
   IgnoreDiagnostics DiagConsumer;
   ClangdServer Server;
   int Limit = 0;
void addFile(StringRef FileName, StringRef Contents) {
     auto Path = testPath(FileName);
     FSProvider.Files[Path] = Contents;
     Server.addDocument(Path, Contents);
   }
 };
 
 } // namespace
 
-TEST_F(WorkspaceSymbolsTest, NoMacro) {
+TEST_F(WorkspaceSymbolsTest, Macros) {
   addFile("foo.cpp", R"cpp(
       #define MACRO X
       )cpp");
 
-  // Macros are not in the index.
-  EXPECT_THAT(getSymbols("macro"), IsEmpty());
+  // LSP's SymbolKind doesn't have a "Macro" kind, and
+  // indexSymbolKindToSymbolKind() currently maps macros
+  // to SymbolKind::String.
+  EXPECT_THAT(getSymbols("macro"),
+              ElementsAre(AllOf(QName("MACRO"), WithKind(SymbolKind::String))));
 }
 
 TEST_F(WorkspaceSymbolsTest, NoLocals) {
   addFile("foo.cpp", R"cpp(
       void test(int FirstParam, int SecondParam) {
         struct LocalClass {};
         int local_var;
       })cpp");
EXPECT_THAT(getSymbols("UnnamedStruct"),
                                 WithKind(SymbolKind::Variable))));
   EXPECT_THAT(getSymbols("InUnnamed"),
               ElementsAre(AllOf(QName("(anonymous struct)::InUnnamed"),
                                 WithKind(SymbolKind::Field))));
 }
 
 TEST_F(WorkspaceSymbolsTest, InMainFile) {
   addFile("foo.cpp", R"cpp(
-      int test() {
-      }
+      int test() {}
+      static test2() {}
       )cpp");
-  EXPECT_THAT(getSymbols("test"), IsEmpty());
+  EXPECT_THAT(getSymbols("test"), 
+              ElementsAre(QName("test"), QName("test2")));
 }
 
 TEST_F(WorkspaceSymbolsTest, Namespaces) {
   addFile("foo.h", R"cpp(
       namespace ans1 {
         int ai1;
       namespace ans2 {
         int ai2;
TEST_F(WorkspaceSymbolsTest, AnonymousNamespace) {
   addFile("foo.h", R"cpp(
       namespace {
       void test() {}
       }
       )cpp");
   addFile("foo.cpp", R"cpp(
       #include "foo.h"
       )cpp");
-  EXPECT_THAT(getSymbols("test"), IsEmpty());
+  EXPECT_THAT(getSymbols("test"), ElementsAre(QName("test")));
 }
 
 TEST_F(WorkspaceSymbolsTest, MultiFile) {
   addFile("foo.h", R"cpp(
       int foo() {
       }
       )cpp");
   addFile("foo2.h", R"cpp(

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

void build(StringRef HeaderCode, StringRef Code = "") {
     AST = File.build();
   }
 
   // build() must have been called.
   bool shouldCollect(StringRef Name, bool Qualified = true) {
     assert(AST.hasValue());
     return SymbolCollector::shouldCollectSymbol(
         Qualified ? findDecl(*AST, Name) : findUnqualifiedDecl(*AST, Name),
         AST->getASTContext(), SymbolCollector::Options());
   }
 
 protected:
   std::string HeaderName = "f.h";
   std::string FileName = "f.cpp";
   TestTU File;
   Optional<ParsedAST> AST;  // Initialized after build.
 };
 
 TEST_F(ShouldCollectSymbolTest, ShouldCollectSymbol) {
   build(R"(
     namespace nx {
     class X{};
     auto f() { int Local; } // auto ensures function body is parsed.
     struct { int x } var;
-    namespace { class InAnonymous {}; }
     }
   )",
-        "class InMain {};");
+        R"(
+    class InMain {};
+    namespace { class InAnonymous {}; }
+    static void g();
+  )");
   auto AST = File.build();
   EXPECT_TRUE(shouldCollect("nx"));
   EXPECT_TRUE(shouldCollect("nx::X"));
   EXPECT_TRUE(shouldCollect("nx::f"));
+  EXPECT_TRUE(shouldCollect("InMain"));
+  EXPECT_TRUE(shouldCollect("InAnonymous", /*Qualified=*/false));
+  EXPECT_TRUE(shouldCollect("g"));
 
-  EXPECT_FALSE(shouldCollect("InMain"));
   EXPECT_FALSE(shouldCollect("Local", /*Qualified=*/false));
-  EXPECT_FALSE(shouldCollect("InAnonymous", /*Qualified=*/false));
 }
 
 TEST_F(ShouldCollectSymbolTest, NoPrivateProtoSymbol) {
   HeaderName = "f.proto.h";
   build(
       R"(// Generated by the protocol buffer compiler.  DO NOT EDIT!
          namespace nx {
            class Top_Level {};
const std::string Header = R"(
     };
 
     void f1();
     inline void f2() {}
     static const int KInt = 2;
     const char* kStr = "123";
 
     namespace {
-    void ff() {} // ignore
+    void ff() {}
     }
 
     void f1() {}
 
     namespace foo {
     // Type alias
     typedef int int32;
     using int32_t = int32;
EXPECT_THAT(Symbols,
                    AllOf(QName("Foo::~Foo"), ForCodeCompletion(false)),
                    AllOf(QName("Foo::operator="), ForCodeCompletion(false)),
                    AllOf(QName("Foo::Nested"), ForCodeCompletion(false)),
                    AllOf(QName("Foo::Nested::f"), ForCodeCompletion(false)),
 
                    AllOf(QName("Friend"), ForCodeCompletion(true)),
                    AllOf(QName("f1"), ForCodeCompletion(true)),
                    AllOf(QName("f2"), ForCodeCompletion(true)),
+                   AllOf(QName("ff"), ForCodeCompletion(true)),
                    AllOf(QName("KInt"), ForCodeCompletion(true)),
                    AllOf(QName("kStr"), ForCodeCompletion(true)),
                    AllOf(QName("foo"), ForCodeCompletion(true)),
                    AllOf(QName("foo::bar"), ForCodeCompletion(true)),
                    AllOf(QName("foo::int32"), ForCodeCompletion(true)),
                    AllOf(QName("foo::int32_t"), ForCodeCompletion(true)),
                    AllOf(QName("foo::v1"), ForCodeCompletion(true)),
                    AllOf(QName("foo::bar::v2"), ForCodeCompletion(true)),

 o]]();
   )cpp");
   Annotations Main(R"cpp(
     int $xdef[[X]] = 42;
     class $clsdef[[Cls]] {};
     void $printdef[[print]]() {}
 
     // Declared/defined in main only.
-    int Y;
+    int $ydecl[[Y]];
   )cpp");
   runSymbolCollector(Header.code(), Main.code());
   EXPECT_THAT(
       Symbols,
       UnorderedElementsAre(
           AllOf(QName("X"), DeclRange(Header.range("xdecl")),
                 DefRange(Main.range("xdef"))),
           AllOf(QName("Cls"), DeclRange(Header.range("clsdecl")),
                 DefRange(Main.range("clsdef"))),
           AllOf(QName("print"), DeclRange(Header.range("printdecl")),
                 DefRange(Main.range("printdef"))),
           AllOf(QName("Z"), DeclRange(Header.range("zdecl"))),
-          AllOf(QName("foo"), DeclRange(Header.range("foodecl")))
+          AllOf(QName("foo"), DeclRange(Header.range("foodecl"))),
+          AllOf(QName("Y"), DeclRange(Main.range("ydecl")))
           ));
 }
 
 TEST_F(SymbolCollectorTest, Refs) {
   Annotations Header(R"(
   class $foo[[Foo]] {
   public:
     $foo[[Foo]]() {}
const std::string Header = R"(
     Y* y = nullptr;  // used in header doesn't count
     #define GLOBAL_Z(name) Z name;
   )";
   const std::string Main = R"(
     W* w = nullptr;
     W* w2 = nullptr; // only one usage counts
     X x();
     class V;
-    V* v = nullptr; // Used, but not eligible for indexing.
     class Y{}; // definition doesn't count as a reference
+    V* v = nullptr;
     GLOBAL_Z(z); // Not a reference to Z, we don't spell the type.
   )";
   CollectorOpts.CountReferences = true;
   runSymbolCollector(Header, Main);
   EXPECT_THAT(Symbols,
-              UnorderedElementsAre(AllOf(QName("W"), RefCount(1)),
-                                   AllOf(QName("X"), RefCount(1)),
-                                   AllOf(QName("Y"), RefCount(0)),
-                                   AllOf(QName("Z"), RefCount(0)), QName("y")));
+              UnorderedElementsAreArray(
+                {AllOf(QName("W"), RefCount(1)),
+                 AllOf(QName("Z"), RefCount(0)), 
+                 AllOf(QName("y"), RefCount(0)),
+                 AllOf(QName("z"), RefCount(0)),
+                 AllOf(QName("x"), RefCount(0)),
+                 AllOf(QName("w"), RefCount(0)),
+                 AllOf(QName("w2"), RefCount(0)),
+                 AllOf(QName("V"), RefCount(1)),
+                 AllOf(QName("v"), RefCount(0))}));
 }
 
 TEST_F(SymbolCollectorTest, SymbolRelativeNoFallback) {
   runSymbolCollector("class Foo {};", /*Main=*/"");
   EXPECT_THAT(Symbols, UnorderedElementsAre(
                            AllOf(QName("Foo"), DeclURI(TestHeaderURI))));
 }
 
Annotations Header(R"(
     #endif
   )");
   runSymbolCollector(Header.code(), /*Main=*/"", /*ExtraArgs=*/{"-DNAME=name"});
   EXPECT_THAT(Symbols, UnorderedElementsAre(AllOf(
                            QName("name"), DeclRange(Header.range("expansion")),
                            DeclURI(TestHeaderURI))));
 }
 
-TEST_F(SymbolCollectorTest, IgnoreSymbolsInMainFile) {
+TEST_F(SymbolCollectorTest, SymbolsInMainFile) {
   const std::string Header = R"(
     class Foo {};
     void f1();
     inline void f2() {}
   )";
   const std::string Main = R"(
     namespace {
-    void ff() {} // ignore
+    void ff() {}
     }
-    void main_f() {} // ignore
+    void main_f() {}
     void f1() {}
   )";
   runSymbolCollector(Header, Main);
   EXPECT_THAT(Symbols,
-              UnorderedElementsAre(QName("Foo"), QName("f1"), QName("f2")));
+              UnorderedElementsAre(QName("Foo"), QName("f1"), QName("f2"),
+                                   QName("ff"), QName("main_f")));
 }
 
 TEST_F(SymbolCollectorTest, ClassMembers) {
   const std::string Header = R"(
     class Foo {
       void f() {}
       void g();
       static void sf() {}
const std::string Main = R"(
     class C {
       friend ::X;
       friend class Y;
     };
   )";
   CollectorOpts.CountReferences = true;
   runSymbolCollector(Header, Main);
   EXPECT_THAT(Symbols, UnorderedElementsAre(AllOf(QName("X"), RefCount(1)),
-                                            AllOf(QName("Y"), RefCount(1))));
+                                            AllOf(QName("Y"), RefCount(1)),
+                                            AllOf(QName("C"), RefCount(0))));
 }
 
 TEST_F(SymbolCollectorTest, Origin) {
   CollectorOpts.Origin = SymbolOrigin::Static;
   runSymbolCollector("class Foo {};", /*Main=*/"");
   EXPECT_THAT(Symbols, UnorderedElementsAre(
                            Field(&Symbol::Origin, SymbolOrigin::Static)));
 }
 
 TEST_F(SymbolCollectorTest, CollectMacros) {
   CollectorOpts.CollectIncludePath = true;
   Annotations Header(R"(
     #define X 1
     #define $mac[[MAC]](x) int x
     #define $used[[USED]](y) float y;
 
     MAC(p);
   )");
-  const std::string Main = R"(
-    #define MAIN 1  // not indexed
+  Annotations Main(R"(
+    #define $main[[MAIN]] 1
     USED(t);
-  )";
+  )");
   CollectorOpts.CountReferences = true;
   CollectorOpts.CollectMacro = true;
-  runSymbolCollector(Header.code(), Main);
+  runSymbolCollector(Header.code(), Main.code());
   EXPECT_THAT(Symbols,
-              UnorderedElementsAre(QName("p"),
+              UnorderedElementsAre(QName("p"), QName("t"),
                                    AllOf(QName("X"), DeclURI(TestHeaderURI),
                                          IncludeHeader(TestHeaderURI)),
                                    AllOf(Labeled("MAC(x)"), RefCount(0),
                                          DeclRange(Header.range("mac"))),
                                    AllOf(Labeled("USED(y)"), RefCount(1),
-                                         DeclRange(Header.range("used")))));
+                                         DeclRange(Header.range("used"))),
+                                   AllOf(Labeled("MAIN"), RefCount(0),
+                                         DeclRange(Main.range("main")))));
 }
 
 TEST_F(SymbolCollectorTest, DeprecatedSymbols) {
   const std::string Header = R"(
     void TestClangc() __attribute__((deprecated("", "")));
     void TestClangd();
   )";
   runSymbolCollector(Header, /**/ "");