[clangd] check for synthesized symbols when tracking include locations

[mizvekov]

This fixes #75115

In C mode with MSVC compatibility, when the assert macro is defined, as a workaround, static_assert is implicitly defined as well, if not already so, in order to work around a broken assert.h implementation.
This workaround was implemented in 8da0903

A synthesized symbol does not occur in source code, and therefore should not have valid source location, but this was not checked when inserting this into a symbol -> include file map.

The invalid FileID value is used for empty key representation in the include file hash table, so it's not valid to insert it.

[llvmbot]

@llvm/pr-subscribers-clangd

Author: Matheus Izvekov (mizvekov)

Changes

This fixes #75115

In C mode with MSVC compatibility, when the assert macro is defined, as a workaround, static_assert is implicitly defined as well, if not already so, in order to work around a broken assert.h implementation.
This workaround was implemented in 8da0903

A synthesized symbol does not occur in source code, and therefore should not have valid source location, but this was not checked when inserting this into a symbol -> include file map.

The invalid FileID value is used for empty key representation in the include file hash table, so it's not valid to insert it.

---

Full diff: https://github.com/llvm/llvm-project/pull/75128.diff

2 Files Affected:

• (modified) clang-tools-extra/clangd/index/SymbolCollector.cpp (+5-1)
• (modified) clang-tools-extra/clangd/test/GH75115.test (+7-4)

diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp b/clang-tools-extra/clangd/index/SymbolCollector.cpp
index aac6676a995fe..e9e76abb90a2d 100644
--- a/clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -821,7 +821,11 @@ void SymbolCollector::setIncludeLocation(const Symbol &S, SourceLocation DefLoc,
 
   // Use the expansion location to get the #include header since this is
   // where the symbol is exposed.
-  IncludeFiles[S.ID] = SM.getDecomposedExpansionLoc(DefLoc).first;
+  FileID FID = SM.getDecomposedExpansionLoc(DefLoc).first;
+  if (FID.isInvalid())
+    return;
+
+  IncludeFiles[S.ID] = FID;
 
   // We update providers for a symbol with each occurence, as SymbolCollector
   // might run while parsing, rather than at the end of a translation unit.
diff --git a/clang-tools-extra/clangd/test/GH75115.test b/clang-tools-extra/clangd/test/GH75115.test
index 030392f1d69b3..dc86f5b9a6cee 100644
--- a/clang-tools-extra/clangd/test/GH75115.test
+++ b/clang-tools-extra/clangd/test/GH75115.test
@@ -1,12 +1,15 @@
 // RUN: rm -rf %t.dir && mkdir -p %t.dir
 // RUN: echo '[{"directory": "%/t.dir", "command": "clang --target=x86_64-pc-windows-msvc -x c GH75115.test", "file": "GH75115.test"}]' > %t.dir/compile_commands.json
-// RUN: not --crash clangd -enable-config=0 --compile-commands-dir=%t.dir -check=%s 2>&1 | FileCheck -strict-whitespace %s
-
-// FIXME: Crashes
+// RUN: clangd -enable-config=0 --compile-commands-dir=%t.dir -check=%s 2>&1 | FileCheck -strict-whitespace %s
 
 // CHECK: Building preamble...
 // CHECK-NEXT: Built preamble
 // CHECK-NEXT: Indexing headers...
-// CHECK-NEXT: !KeyInfoT::isEqual(Val, EmptyKey) && !KeyInfoT::isEqual(Val, TombstoneKey) && "Empty/Tombstone value shouldn't be inserted into map!"
+// CHECK-NEXT: Building AST...
+// CHECK-NEXT: Indexing AST...
+// CHECK-NEXT: Building inlay hints
+// CHECK-NEXT: semantic highlighting
+// CHECK-NEXT: Testing features at each token
+// CHECK-NEXT: All checks completed, 0 errors
 
 #define assert

[kadircet]

Hi @mizvekov thanks for the fix, but I am not sure if this is at the right level. The way you're bailing out currently prevents clangd from indexing all implicit definitions, even if we have a hard-coded mapping for them (based on the symbol name).

Also the map you're preventing the insertion stores FileIDs as values, not keys, hence it isn't the place firing assertion failures. Can you instead move the logic to SymbolCollector::finish, which uses FileIDs to determine includers for objective-c symbols? we should only perform that logic if there's a valid FileID.

[mizvekov]

Hi @mizvekov thanks for the fix, but I am not sure if this is at the right level. The way you're bailing out currently prevents clangd from indexing all implicit definitions, even if we have a hard-coded mapping for them (based on the symbol name).

Also the map you're preventing the insertion stores FileIDs as values, not keys, hence it isn't the place firing assertion failures. Can you instead move the logic to SymbolCollector::finish, which uses FileIDs to determine includers for objective-c symbols? we should only perform that logic if there's a valid FileID.

I see. Though the situation you describe seems impossible at the present state of the implementation.

For standard library stuff, we have hardcoded std::move and std::remove, implemented here but those require some sort of user provided declaration somewhere in code, which will have a valid source location, and we indeed unit test that.

As an aside, I think in the above case we should honor the -freestanding flag, but currently we do not.

For the builtin-which-requires-include case, that is unimplemented, with a FIXME note:

llvm-project/clang-tools-extra/include-cleaner/lib/FindHeaders.cpp

Line 177 in 6d8fe3d

// FIXME: Use the header mapping for builtins with a known header.

I agree in principle to the change you suggested, even if presently non testable, but I still think it makes sense to skip inserting invalid FileIDs into the IncludeFiles map.

[mizvekov]

@kadircet ping

[mizvekov]

@kadircet gentle ping

[kadircet]

thanks and sorry for leaving this out there, I was busy with other things and then OOO.