[clangd] Check if macro is already in the IdentifierTable before loading it

[qdelacru]

Summary

Having nested macros in the C code could cause clangd to fail an assert in clang::Preprocessor::setLoadedMacroDirective() and crash.

#0 0x00000000007acd79 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /qdelacru/llvm-project/llvm/lib/Support/Unix/Signals.inc:565:22
#1 0x00000000007ace30 PrintStackTraceSignalHandler(void*) /qdelacru/llvm-project/llvm/lib/Support/Unix/Signals.inc:632:1
#2 0x00000000007aaded llvm::sys::RunSignalHandlers() /qdelacru/llvm-project/llvm/lib/Support/Signals.cpp:76:20
#3 0x00000000007ac7c1 SignalHandler(int) /qdelacru/llvm-project/llvm/lib/Support/Unix/Signals.inc:407:1
#4 0x00007f096604db20 __restore_rt (/lib64/libpthread.so.0+0x12b20)
#5 0x00007f0964b307ff raise (/lib64/libc.so.6+0x377ff)
#6 0x00007f0964b1ac35 abort (/lib64/libc.so.6+0x21c35)
#7 0x00007f0964b1ab09 _nl_load_domain.cold.0 (/lib64/libc.so.6+0x21b09)
#8 0x00007f0964b28de6 (/lib64/libc.so.6+0x2fde6)
#9 0x0000000001004d1a clang::Preprocessor::setLoadedMacroDirective(clang::IdentifierInfo*, clang::MacroDirective*, clang::MacroDirective*) /qdelacru/llvm-project/clang/lib/Lex/PPMacroExpansion.cpp:116:5
#10 0x00000000032287b5 clang::ASTReader::resolvePendingMacro(clang::IdentifierInfo*, clang::ASTReader::PendingMacroInfo const&) /qdelacru/llvm-project/clang/lib/Serialization/ASTReader.cpp:2219:31
#11 0x000000000324f130 clang::ASTReader::finishPendingActions() /qdelacru/llvm-project/clang/lib/Serialization/ASTReader.cpp:9249:7
#12 0x000000000325d15e clang::ASTReader::FinishedDeserializing() /qdelacru/llvm-project/clang/lib/Serialization/ASTReader.cpp:11575:5
#13 0x000000000093c64b clang::ExternalASTSource::Deserializing::~Deserializing() /qdelacru/llvm-project/clang/include/clang/AST/ExternalASTSource.h:89:5
#14 0x000000000324a74f clang::ASTReader::get(llvm::StringRef) /qdelacru/llvm-project/clang/lib/Serialization/ASTReader.cpp:8038:10
#15 0x00000000011c5708 clang::clangd::(anonymous namespace)::loadMainFilePreambleMacros(clang::Preprocessor const&, clang::clangd::PreambleData const&) /qdelacru/llvm-project/clang-tools-extra/clangd/CodeComplete.cpp:1113:44
#16 0x00000000011c5de0 clang::clangd::(anonymous namespace)::semaCodeComplete(std::unique_ptr<clang::CodeCompleteConsumer, std::default_delete<clang::CodeCompleteConsumer> >, clang::CodeCompleteOptions const&, clang::clangd::(anonymous namespace)::SemaCompleteInput const&, clang::clangd::IncludeStructure*) /qdelacru/llvm-project/clang-tools-extra/clangd/CodeComplete.cpp:1190:3
#17 0x00000000011c703b clang::clangd::(anonymous namespace)::CodeCompleteFlow::run(clang::clangd::(anonymous namespace)::SemaCompleteInput const&) && /qdelacru/llvm-project/clang-tools-extra/clangd/CodeComplete.cpp:1395:21
#18 0x00000000011ca87a clang::clangd::codeComplete(llvm::StringRef, clang::clangd::Position, clang::clangd::PreambleData const*, clang::clangd::ParseInputs const&, clang::clangd::CodeCompleteOptions, clang::clangd::SpeculativeFuzzyFind*) /qdelacru/llvm-project/clang-tools-extra/clangd/CodeComplete.cpp:1865:78
#19 0x000000000119c351 clang::clangd::ClangdServer::codeComplete(llvm::StringRef, clang::clangd::Position, clang::clangd::CodeCompleteOptions const&, llvm::unique_function<void (llvm::Expected<clang::clangd::CodeCompleteResult>)>)::'lambda'(llvm::Expected<clang::clangd::InputsAndPreamble>)::operator()(llvm::Expected<clang::clangd::InputsAndPreamble>) /qdelacru/llvm-project/clang-tools-extra/clangd/ClangdServer.cpp:377:61
#20 0x00000000011a821a void llvm::detail::UniqueFunctionBase<void, llvm::Expected<clang::clangd::InputsAndPreamble> >::CallImpl<clang::clangd::ClangdServer::codeComplete(llvm::StringRef, clang::clangd::Position, clang::clangd::CodeCompleteOptions const&, llvm::unique_function<void (llvm::Expected<clang::clangd::CodeCompleteResult>)>)::'lambda'(llvm::Expected<clang::clangd::InputsAndPreamble>)>(void*, llvm::Expected<clang::clangd::InputsAndPreamble>&) /qdelacru/llvm-project/llvm/include/llvm/ADT/FunctionExtras.h:204:16
#21 0x0000000001554403 llvm::unique_function<void (llvm::Expected<clang::clangd::InputsAndPreamble>)>::operator()(llvm::Expected<cl

An example of the code that causes the assert failure:

#define ECHO(X) X
#define ECHO2(X) ECHO(X)
...

During code completion in clangd, the macros will be loaded in loadMainFilePreambleMacros() by iterating over the macro names and calling PreambleIdentifiers->get(). Since these macro names are store in a StringSet (has StringMap underlying container), the order of the iterator is not guaranteed to be same as the order seen in the source code.

When clangd is trying to resolve nested macros it sometimes attempts to load them out of order which causes a macro to be stored twice. In the example above, ECHO2 macro gets resolved first, but since it uses another macro that has not been resolved it will try to resolve/store that as well. Now there are two MacroDirectives stored in the Preprocessor, ECHO and ECHO2. When clangd tries to load the next macro, ECHO, the preprocessor fails an assert in clang::Preprocessor::setLoadedMacroDirective() because there is already a MacroDirective stored for that macro name.

In this diff, I check if the macro is already inside the IdentifierTable and if it is skip it so that it is not resolved twice.

[kadircet]

oh wow thanks! would you mind adding a test case like:

TEST(CompletionTest, NoCrashDueToMacroOrdering) {
  EXPECT_THAT(completions(R"cpp(
    #define ECHO(X) X
    #define ECHO2(X) ECHO(X)
    int finish_preamble = E^CHO(2);)cpp")
                  .Completions,
              UnorderedElementsAre(Labeled("ECHO"), Labeled("ECHO2")));
}

into clang-tools-extra/clangd/unittest/CodeCompleteTests.cpp ?

[qdelacru]

Added suggested new test case

[kadircet]

please provide an email address (for attribution) if i should land this for you.

[qdelacru]

Yes I need help landing this, please use [e-mail]. Thanks!

clang-tools-extra/clangd/CodeComplete.cpp

 void loadMainFilePreambleMacros(const Preprocessor &PP,
                                 const PreambleData &Preamble) {
   // The ExternalPreprocessorSource has our macros, if we know where to look.
   // We can read all the macros using PreambleMacros->ReadDefinedMacros(),
   // but this includes transitively included files, so may deserialize a lot.
   ExternalPreprocessorSource *PreambleMacros = PP.getExternalSource();
   // As we have the names of the macros, we can look up their IdentifierInfo
   // and then use this to load just the macros we want.
+  const auto &ITable = PP.getIdentifierTable();
   IdentifierInfoLookup *PreambleIdentifiers =
-      PP.getIdentifierTable().getExternalIdentifierLookup();
+      ITable.getExternalIdentifierLookup();
+
   if (!PreambleIdentifiers || !PreambleMacros)
     return;
-  for (const auto &MacroName : Preamble.Macros.Names)
+  for (const auto &MacroName : Preamble.Macros.Names) {
+    if (ITable.find(MacroName.getKey()) != ITable.end())
+      continue;
     if (auto *II = PreambleIdentifiers->get(MacroName.getKey()))
       if (II->isOutOfDate())
         PreambleMacros->updateOutOfDateIdentifier(*II);
   }
+}
 
 // Invokes Sema code completion on a file.
 // If \p Includes is set, it will be updated based on the compiler invocation.
 bool semaCodeComplete(std::unique_ptr<CodeCompleteConsumer> Consumer,
                       const clang::CodeCompleteOptions &Options,
                       const SemaCompleteInput &Input,
                       IncludeStructure *Includes = nullptr) {
   trace::Span Tracer("Sema completion");

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

EXPECT_THAT(
                      Kind(CompletionItemKind::Constructor))));
   EXPECT_THAT(
       completions(Context + "Container c = Cont^<int>()", {}, Opts).Completions,
       Contains(AllOf(Labeled("Container<typename T>(int Size)"),
                      SnippetSuffix(""),
                      Kind(CompletionItemKind::Constructor))));
 }
 
+TEST(CompletionTest, NoCrashDueToMacroOrdering) {
+  EXPECT_THAT(completions(R"cpp(
+    #define ECHO(X) X
+    #define ECHO2(X) ECHO(X)
+    int finish_preamble = EC^HO(2);)cpp")
+                  .Completions,
+              UnorderedElementsAre(Labeled("ECHO(X)"), Labeled("ECHO2(X)")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang