[clangd] Fix crash-bug in preamble indexing when using modules.

[adamcz]

Summary

When preamble contains #undef, indexing code finds the matching #define
and uses that during indexing. However, it would only look for local
definitions. If the macro was defined in a module, MacroInfo
would be nullptr and clangd would crash.

This change makes clangd ignore any #undef without a matching #define
inside the same TU.

The indexing of macros happens for preamble only, so then #undef must be
in the preamble, which is why we need two .h files in a test.

Note that clangd is currently not ready for module support, but this
brings us one step closer.

This was previously attempted in
4061d9e42cff621462931ac7df9666806c77a237, but had to be reverted due to
broken test. This version fixes that test-only bug by setting a custom module
cache path to avoid re-use of modules across test invocations.

[sammccall]

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

77

errs should probably go away

96

or just [&]{ deleteModuleCache(...); } - bind isn't unreadable here, but we use it so rarely that it almost feels obscure :-)

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

 
 TEST_F(SymbolCollectorTest, MacrosInHeaders) {
   CollectorOpts.CollectMacro = true;
   TestFileName = testPath("test.h");
   runSymbolCollector("", "#define X");
   EXPECT_THAT(Symbols,
               UnorderedElementsAre(AllOf(QName("X"), ForCodeCompletion(true))));
 }
+
+// Regression test for a crash-bug we used to have.
+TEST_F(SymbolCollectorTest, UndefOfModuleMacro) {
+  auto TU = TestTU::withCode(R"cpp(#include "bar.h")cpp");
+  TU.AdditionalFiles["bar.h"] = R"cpp(
+    #include "foo.h"
+    #undef X
+    )cpp";
+  TU.AdditionalFiles["foo.h"] = "#define X 1";
+  TU.AdditionalFiles["module.map"] = R"cpp(
+    module foo {
+     header "foo.h"
+     export *
+   }
+   )cpp";
+  TU.ExtraArgs.push_back("-fmodules");
+  TU.ExtraArgs.push_back("-fmodule-map-file=" + testPath("module.map"));
+  TU.OverlayRealFileSystemForModules = true;
+
+  TU.build();
+  // We mostly care about not crashing, but verify that we didn't insert garbage
+  // about X too.
+  EXPECT_THAT(TU.headerSymbols(), Not(Contains(QName("X"))));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/unittests/TestFS.h

 llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
 buildTestFS(llvm::StringMap<std::string> const &Files,
             llvm::StringMap<time_t> const &Timestamps = {});
 
 // A VFS provider that returns TestFSes containing a provided set of files.
 class MockFS : public ThreadsafeFS {
 public:
   IntrusiveRefCntPtr<llvm::vfs::FileSystem> viewImpl() const override {
-    return buildTestFS(Files, Timestamps);
+    auto MemFS = buildTestFS(Files, Timestamps);
+    if (!OverlayRealFileSystemForModules)
+      return MemFS;
+    llvm::IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem> OverlayFileSystem =
+        new llvm::vfs::OverlayFileSystem(llvm::vfs::getRealFileSystem());
+    OverlayFileSystem->pushOverlay(MemFS);
+    return OverlayFileSystem;
   }
 
   // If relative paths are used, they are resolved with testPath().
   llvm::StringMap<std::string> Files;
   llvm::StringMap<time_t> Timestamps;
+  // If true, real file system will be used as fallback for the in-memory one.
+  // This is useful for testing module support.
+  bool OverlayRealFileSystemForModules = false;
 };
 
 // A Compilation database that returns a fixed set of compile flags.
 class MockCompilationDatabase : public GlobalCompilationDatabase {
 public:
   /// If \p Directory is not empty, use that as the Directory field of the
   /// CompileCommand, and as project SourceRoot.
   ///

clang-tools-extra/clangd/unittests/TestTU.h

struct TestTU {
   llvm::Optional<std::string> ClangTidyChecks;
   llvm::Optional<std::string> ClangTidyWarningsAsErrors;
   // Index to use when building AST.
   const SymbolIndex *ExternalIndex = nullptr;
 
   // Simulate a header guard of the header (using an #import directive).
   bool ImplicitHeaderGuard = true;
 
+  // Whether to use overlay the TestFS over the real filesystem. This is
+  // required for use of implicit modules.where the module file is written to
+  // disk and later read back.
+  // FIXME: Change the way reading/writing modules work to allow us to keep them
+  // in memory across multiple clang invocations, at least in tests, to
+  // eliminate the need for real file system here.
+  // Please avoid using this for things other than implicit modules. The plan is
+  // to eliminate this option some day.
+  bool OverlayRealFileSystemForModules = false;
+
   // By default, build() will report Error diagnostics as GTest errors.
   // Suppress this behavior by adding an 'error-ok' comment to the code.
   ParsedAST build() const;
   std::shared_ptr<const PreambleData> preamble() const;
   ParseInputs inputs(MockFS &FS) const;
   SymbolSlab headerSymbols() const;
   RefSlab headerRefs() const;
   std::unique_ptr<SymbolIndex> index() const;

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

 #include "Diagnostics.h"
 #include "TestFS.h"
 #include "index/FileIndex.h"
 #include "index/MemIndex.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/Utils.h"
+#include "llvm/ADT/ScopeExit.h"
 
 namespace clang {
 namespace clangd {
 
 ParseInputs TestTU::inputs(MockFS &FS) const {
   std::string FullFilename = testPath(Filename),
               FullHeaderName = testPath(HeaderFilename),
               ImportThunk = testPath("import_thunk.h");
ParseInputs TestTU::inputs(MockFS &FS) const {
   }
   Argv.insert(Argv.end(), ExtraArgs.begin(), ExtraArgs.end());
   // Put the file name at the end -- this allows the extra arg (-xc++) to
   // override the language setting.
   Argv.push_back(FullFilename);
   Inputs.CompileCommand.Filename = FullFilename;
   Inputs.CompileCommand.Directory = testRoot();
   Inputs.Contents = Code;
+  if (OverlayRealFileSystemForModules)
+    FS.OverlayRealFileSystemForModules = true;
   Inputs.TFS = &FS;
   Inputs.Opts = ParseOptions();
   Inputs.Opts.BuildRecoveryAST = true;
   Inputs.Opts.PreserveRecoveryASTType = true;
   Inputs.Opts.ClangTidyOpts.Checks = ClangTidyChecks;
   Inputs.Opts.ClangTidyOpts.WarningsAsErrors = ClangTidyWarningsAsErrors;
   Inputs.Index = ExternalIndex;
   if (Inputs.Index)
     Inputs.Opts.SuggestMissingIncludes = true;
   return Inputs;
 }
 
+void initializeModuleCache(CompilerInvocation &CI) {
+  llvm::SmallString<128> ModuleCachePath;
+  ASSERT_FALSE(
+      llvm::sys::fs::createUniqueDirectory("module-cache", ModuleCachePath));
+  CI.getHeaderSearchOpts().ModuleCachePath = ModuleCachePath.c_str();
+  llvm::errs() << "MC: " << ModuleCachePath << "\n";
+  llvm::errs().flush();
+}
+
+void deleteModuleCache(const std::string ModuleCachePath) {
+  if (!ModuleCachePath.empty()) {
+    ASSERT_FALSE(llvm::sys::fs::remove_directories(ModuleCachePath));
+  }
+}
+
 std::shared_ptr<const PreambleData> TestTU::preamble() const {
   MockFS FS;
   auto Inputs = inputs(FS);
   IgnoreDiagnostics Diags;
   auto CI = buildCompilerInvocation(Inputs, Diags);
   assert(CI && "Failed to build compilation invocation.");
+  if (OverlayRealFileSystemForModules)
+    initializeModuleCache(*CI);
+  auto ModuleCacheDeleter = llvm::make_scope_exit(
+      std::bind(deleteModuleCache, CI->getHeaderSearchOpts().ModuleCachePath));
   return clang::clangd::buildPreamble(testPath(Filename), *CI, Inputs,
                                       /*StoreInMemory=*/true,
                                       /*PreambleCallback=*/nullptr);
 }
 
 ParsedAST TestTU::build() const {
   MockFS FS;
   auto Inputs = inputs(FS);
   StoreDiags Diags;
   auto CI = buildCompilerInvocation(Inputs, Diags);
   assert(CI && "Failed to build compilation invocation.");
+  if (OverlayRealFileSystemForModules)
+    initializeModuleCache(*CI);
+  auto ModuleCacheDeleter = llvm::make_scope_exit(
+      std::bind(deleteModuleCache, CI->getHeaderSearchOpts().ModuleCachePath));
+
   auto Preamble = clang::clangd::buildPreamble(testPath(Filename), *CI, Inputs,
                                                /*StoreInMemory=*/true,
                                                /*PreambleCallback=*/nullptr);
   auto AST = ParsedAST::build(testPath(Filename), Inputs, std::move(CI),
                               Diags.take(), Preamble);
   if (!AST.hasValue()) {
     ADD_FAILURE() << "Failed to build code:\n" << Code;
     llvm_unreachable("Failed to build TestTU!");

clang/lib/Index/IndexingAction.cpp

 
 static void indexTranslationUnit(ASTUnit &Unit, IndexingContext &IndexCtx) {
   Unit.visitLocalTopLevelDecls(&IndexCtx, topLevelDeclVisitor);
 }
 
 static void indexPreprocessorMacros(const Preprocessor &PP,
                                     IndexDataConsumer &DataConsumer) {
   for (const auto &M : PP.macros())
-    if (MacroDirective *MD = M.second.getLatest())
+    if (MacroDirective *MD = M.second.getLatest()) {
+      auto *MI = MD->getMacroInfo();
+      // When using modules, it may happen that we find #undef of a macro that
+      // was defined in another module. In such case, MI may be nullptr, since
+      // we only look for macro definitions in the current TU. In that case,
+      // there is nothing to index.
+      if (!MI)
+        continue;
+
       DataConsumer.handleMacroOccurrence(
           M.first, MD->getMacroInfo(),
           static_cast<unsigned>(index::SymbolRole::Definition),
           MD->getLocation());
     }
+}
 
 void index::indexASTUnit(ASTUnit &Unit, IndexDataConsumer &DataConsumer,
                          IndexingOptions Opts) {
   IndexingContext IndexCtx(Opts, DataConsumer);
   IndexCtx.setASTContext(Unit.getASTContext());
   DataConsumer.initialize(Unit.getASTContext());
   DataConsumer.setPreprocessor(Unit.getPreprocessorPtr());